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

Roslyn analyzers that report diagnostics for razor files don't get squiggles #9308

Closed
vsfeedback opened this issue Sep 22, 2023 · 9 comments
Closed
Assignees
Labels
author: migration bot 🤖 The issue was created by a issue mover bot. The author may not be the actual author
Milestone

Comments

@vsfeedback
Copy link

This issue has been moved from a ticket on Developer Community.


If an issue is reported for a .razor or .cshtml file. The squiggly line does not appear for the location inside VS, nor do the reported issue details show up on hover over.


Original Comments

Feedback Bot on 9/3/2023, 07:15 PM:

(private comment, text removed)

Feedback Bot on 9/4/2023, 03:14 AM:

(private comment, text removed)

Wenwen Fan [MSFT] on 9/18/2023, 07:36 PM:

(private comment, text removed)

Čaba Šagi on 9/19/2023, 08:06 AM:

(private comment, text removed)

Wenwen Fan [MSFT] on 9/18/2023, 07:42 PM:

(private comment, text removed)

Wenwen Fan [MSFT] on 9/19/2023, 06:40 PM:

(private comment, text removed)


Original Solutions

(no solutions)

@ghost ghost added untriaged author: migration bot 🤖 The issue was created by a issue mover bot. The author may not be the actual author labels Sep 22, 2023
@ryzngard
Copy link
Contributor

https://github.com/csaba-sagi-sonarsource/VSRepro has a repro. Clicking error list takes to correct page but the squiggle is missing. This is specifically for analyzers

@davidwengier
Copy link
Member

davidwengier commented Sep 22, 2023

I don't think 3rd party analyzers on the generated code is a generally supported scenario. Certainly code fixes won't work at all.

Also, I'm not surprised line mappings don't work, as they are not very sophisticated without the Razor compiler moving to enhanced line pragmas. I would expect that if a diagnostic is reported on the unmapped location, it probably has a better chance of working, as we will correctly map it to the .razor file, and we have more source mapping information than the Roslyn APIs can see. I don't think Roslyn's internal analyzers generally call GetMappedSpan() when creating diagnostics.

Certainly in the example repo, just reporting the diagnostic on identifierName.GetLocation() should have us correctly mapping that location to the right spot in the Razor file.

FYI @csaba-sagi-sonarsource

@csaba-sagi-sonarsource
Copy link

@davidwengier

You can find more information about why we do the mapping ourselves here. In this comment you will also have a reproducer that will show you how it misbehaves if we don't do the mapping ourselves.

@davidwengier
Copy link
Member

I'm not sure I follow. You've pointed to a comment as the reason for using the mapped location, because otherwise "without the mapping, the reported location will point to the generated file." but I'm suggesting that's exactly what you should be doing: reporting diagnostics for the generated file.

@csaba-sagi-sonarsource
Copy link

@davidwengier,
Yes, that behaviour would be good inside visual studio, since there is some sort of "background magic" where the diagnostic is mapped back to the razor file even though the issue was reported on the generated file.
However, we also have a separate product that is not part of Visual Studio. How we transfer the reported issue from the Roslyn analysis to our product is through the SARIF report I mentioned in the comment. If we don't do the mapping and report on the generated file, the SARIF report will have the location from the generated file, and we will not show the correct locations in our product.

@davidwengier
Copy link
Member

Ahh thanks for that clarification, that's the bit I missed.

@davidwengier davidwengier changed the title Issue details and highlithing missing in VS2022 for .razor and .cshtml files Roslyn analyzers that report diagnostics for razor files don't get squiggles Sep 22, 2023
@davidwengier
Copy link
Member

Not sure where this is falling down, but can think of two possibilities:

  • If Roslyn is returning these to us when we request diagnostics for the .g.cs file, then we need some extension to the LSP diagnostic types, as they only support a Range, with no document information. Essentially, when asking for diagnostics on a .g.cs file, we expect to only get them. If we get already mapped locations, we'll try to map them, and that would fail which would cause this behaviour.
  • If Roslyn isn't returning these to us, then we'd have to make a request to Roslyn for diagnostics on the .razor file itself, which we don't do.

Either way we need to design a solution properly if we want to support this scenario.

@ryzngard
Copy link
Contributor

If Roslyn isn't returning these to us, then we'd have to make a request to Roslyn for diagnostics on the .razor file itself, which we don't do.

Some Sunday night speculation after driving 5 hours in the rain:

In my head this is what I thought was happening. Roslyn supports reporting diagnostics on misc files, but I'm guessing it's not tested for misc files that have an LSP editor + backing server. In all honesty, the best solution might be to let the editor handle this. The diagnostic itself in the error window is out of our control, and afaik any extension can add to it. We don't own drawing squiggles, we just report diagnostics with a range. It looks like the range is correct on this diagnostic because you can click on the error list and navigate correctly. I think this is backed by the below comment:

Yes, that behaviour would be good inside visual studio, since there is some sort of "background magic" where the diagnostic is mapped back to the razor file even though the issue was reported on the generated file.

If we do want to support analyzers mapping correctly from .g.cs files to Razor, that's a different topic. It sounds like that's being worked around manually right now.

TLDR: The current bug is a bug on the editor (I think, haven't tested) and anything more is a feature request style issue. Then we consider what #9308 (comment) covers

@ryzngard
Copy link
Contributor

ryzngard commented Jan 9, 2024

This is fixed by Roslyn in dotnet/roslyn#71454 and will ship in 17.9

The recommended way of reporting errors in analyzers is to use the generated .g.cs file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author: migration bot 🤖 The issue was created by a issue mover bot. The author may not be the actual author
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants