-
Notifications
You must be signed in to change notification settings - Fork 14
Use LineSpan.Path instead of SourceTree?.FilePath when trimming Location #163
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
base: main
Are you sure you want to change the base?
Conversation
If the Location is created without referencing the SourceTree, the previous implementation of GetLocationTrimmed lost the file path resulting in diagnostic messages without clickable files.
| return Location.Create(location.SourceTree?.FilePath ?? string.Empty, location.SourceSpan, location.GetLineSpan().Span); | ||
| var lineSpan = location.GetLineSpan(); | ||
|
|
||
| return Location.Create(lineSpan.Path ?? string.Empty, location.SourceSpan, lineSpan.Span); |
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.
The fact that diagnostics are not clickable is due to a known issue with Roslyn not supporting incremental Location values. I'm curious about your fix though, does LineSpan.Path return something different compared to SourceTree.FilePath and does that address the issue in question?
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.
The problem arrises when I use Location.Create before instantiating the EquatableDiagnostic, because Location.Create leaves SourceTree set to null and thus the filePath is not propagated through GetLocationTrimmed.
Eg:
new EquatableDiagnostic(
diagnosticDescriptor,
Location.Create(
filePath,
new TextSpan(startIndex, endIndex.HasValue ? endIndex.Value - startIndex : 0),
new LinePositionSpan(
GetLinePosition(startIndex, fileContent),
GetLinePosition(endIndex ?? startIndex, fileContent)
)
)
)the filePath i provide gets wiped out because it is stored in the span and not in the SourceTree (which is null)
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.
I see, does this address the issue of diagnostics not being clickable though? I'm not sure that it does, because fundamentally the issue is with Location.Create producing external locations that are not clickable.
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.
Not sure what you mean by "clickable". The important part for me is that the path I provide is visible in the terminal output. It's probably the VSCode terminal that makes the location clickable for me.
Are you talking about the diagnostic issues not showing up inline in the IDE editor?
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.
I was referring to this issue: dotnet/runtime#92509. Generally speaking, diagnostics pointing to locations created via Location.Create suffer from issues such as them not being suppressible.
If the Location is created without referencing the SourceTree, the previous implementation of GetLocationTrimmed lost the file path resulting in diagnostic messages without clickable file paths.
This might not be relevant for this project (yet), but I'm copying the code for my own SourceGenerator and ran into this issue when adding diagnostics for AdditionalFiles where created the location using
Location.Createinstead of getting it from syntax.PS: I'm pretty new to Roslyn, so I don't know whether the lineSpan always have the path, but the implementation of
Location.GetDebuggerDisplayseems to indicate that it usually does.