-
-
Notifications
You must be signed in to change notification settings - Fork 757
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
@@ -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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Location.kt
Outdated
Show resolved
Hide resolved
detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Location.kt
Outdated
Show resolved
Hide resolved
detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Location.kt
Outdated
Show resolved
Hide resolved
detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Location.kt
Outdated
Show resolved
Hide resolved
* 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>
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.