Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't allow invalid Source Locations #7030

Merged
merged 5 commits into from Mar 17, 2024
Merged

Don't allow invalid Source Locations #7030

merged 5 commits into from Mar 17, 2024

Conversation

BraisGabin
Copy link
Member

Fix #7024

A line or column value less than 1 is not valid. Detekt didn't check this but sarif those so we were generating invalid sarif.

Other way to solve the issue is to move this code to the sarif serializer but I think that it's more correct to fix it in our own code and don't allow to point to a source location that doesn't exist.

@BraisGabin BraisGabin added the pick request Marker for PRs that should be ported to the 1.0 release branch label Mar 8, 2024
@BraisGabin BraisGabin added this to the 2.0.0 milestone Mar 8, 2024
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 83.79%. Comparing base (9cb4976) to head (8f4457e).
Report is 17 commits behind head on main.

Files Patch % Lines
...kotlin/io/gitlab/arturbosch/detekt/api/Location.kt 50.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7030   +/-   ##
=========================================
  Coverage     83.78%   83.79%           
- Complexity     3929     3938    +9     
=========================================
  Files           578      578           
  Lines         12099    12123   +24     
  Branches       2505     2514    +9     
=========================================
+ Hits          10137    10158   +21     
+ Misses          710      709    -1     
- Partials       1252     1256    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -33,9 +33,9 @@ class Location(
*/
fun from(element: PsiElement, offset: Int = 0): Location {
val start = startLineAndColumn(element, offset)
val sourceLocation = SourceLocation(start.line, start.column)
val sourceLocation = SourceLocation(start.line.coerceAtLeast(1), start.column.coerceAtLeast(1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't coercing values mask underlying problems rule or core logic? I don't think we should do that here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about the new implementation? If a file is empty then DiagnosticUtils.getLineAndColumnInPsiFile doesn't provide a correct value. You can see it here:

    @NotNull
    public static LineAndColumn offsetToLineAndColumn(@Nullable Document document, int offset) {
        if (document != null && document.getTextLength() != 0) {
            int lineNumber = document.getLineNumber(offset);
            int lineStartOffset = document.getLineStartOffset(lineNumber);
            int column = offset - lineStartOffset;
            int lineEndOffset = document.getLineEndOffset(lineNumber);
            CharSequence lineContent = document.getCharsSequence().subSequence(lineStartOffset, lineEndOffset);
            return new LineAndColumn(lineNumber + 1, column + 1, lineContent.toString());
        } else {
            return new LineAndColumn(-1, offset, (String)null);
        }
    }

So in that case I return null. And then the default value that we have for the cases where we don't get the line/column returns a valid line/column.

I don't think I should fix this at the rule level because any rule that reports an empty file would crash.

@3flex 3flex enabled auto-merge (squash) March 17, 2024 06:16
@3flex 3flex merged commit d08f8a2 into main Mar 17, 2024
20 of 21 checks passed
@3flex 3flex deleted the fix-7024 branch March 17, 2024 06:33
cortinico pushed a commit that referenced this pull request Mar 23, 2024
* Improve tests

* Don't add invalid values on SourceLocation

* Don't hide problems with coerce

* Fix wording in assertion messages

---------

Co-authored-by: Matthew Haughton <3flex@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api core pick request Marker for PRs that should be ported to the 1.0 release branch rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid SARIF on EmptyKtFile
3 participants