Analysis results paths are pointing to generated code instead of the Razor files

[costin-zaharia-sonarsource]

For the diagnostics reported in .razor files, the locations are pointing to the generated files instead of the actual razor files.

E.g. when a diagnostic is reported on the IncrementCount identifier from the following code (Counter.razor file)

<button class="btn btn-primary" @onclick="IncrementCount">Click me</button>

The Sarif report contains:

          "locations": [
            {
              "resultFile": {
                "uri": "Microsoft.NET.Sdk.Razor.SourceGenerators/Microsoft.NET.Sdk.Razor.SourceGenerators.RazorSourceGenerator/Components_Pages_Counter_razor.g.cs",

You can find here a reproducer. It contains already the generated Sarif report (BlazorWebApp\BuildOutput.sarif)

When working on custom code analyzers and development tools in general, it is very useful to have access to the original location of the syntax. For sonar-dotnet analyzers, we need the correct locations to be able to import and display the issues on SonarQube and SonarCloud.

We tried to do the mapping ourselves (by using GetMappedLineSpan) but the API offers limited functionality, and we often encounter issues like #69248. This has also unexpected behavior in the IDE like dotnet/razor#9308 or dotnet/razor#9108.

@davidwengier explained here that:

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.

tl;dr

Would it be possible to change paths for the diagnostics in razor files to point to the original files instead of the generated ones?

[chsienki]

@jaredpar I'm not super familiar with the Sarif stuff, but I would assume it's just reporting the Location of the SyntaxTree? I don't see how we could report the 'original location' without somehow adding to the SyntaxTree where it 'came from', which would be up to the individual generators to report.

It doesn't seem too out there but would need new roslyn APIs to support this.

[chsienki]

@costin-zaharia-sonarsource We're working on fixing the GetMappedLineSpan issues in an upcoming release. With those fixed, would you still need to use the sarif paths?

[davidwengier]

I don't think I follow this request. It seems like maybe the ask is for Roslyn to change GetLocation to honour #line mappings, but only for Razor generated files?

Also, it seems to me like if the GetLocation() call in the analyzer returns a location in the .razor file then that would just mean running into dotnet/razor#9308 again.

I don't see how we could report the 'original location' without somehow adding to the SyntaxTree where it 'came from'

Isn't this exactly what #line mappings are for, and the generator is emitting them, and there is a Roslyn API to return them.

Seems like either the analyzer should report locations in the generated C# file, and let the user deal with that (it's non-trivial to get a dotnet build of a Razor project to produce errors from the C# compiler in the generated C# file, and I don't think people complain much?), or report the mapped locations using GetMappedLineSpan, and then tooling can fix/workaround dotnet/razor#9308 so that squiggles appear correct (work for which is already underway I believe)

[costin-zaharia-sonarsource]

We're working on fixing the GetMappedLineSpan issues in an upcoming release. With those fixed, would you still need to use the sarif paths?

Hi @chsienki, that's great, thanks for working on this. If the GetMappedLineSpan returns the right locations, it will fix a couple of issues on our side. We will be able to display the right location in SQ, and SC and improve the precision for the highlighting we show on the server.

Do you have maybe an issue we can track? #69248 seems to be closed.

At the same time, since we import our data from the Sarif reports, we will still have to report on the actual .razor/.cshtml files instead of the generated ones. So we can correctly map the diagnostics to the code, on the SQ\SC server side.

Using manual mapping is not a big problem for us (if the returned paths are correct), but, if I understand correctly from @davidwengier in this comment, this works against the compiler design and might be a source of issues and unexpected behavior. My understanding is that issues like dotnet/razor#9308 and dotnet/razor#9108 are caused by the fact that our diagnostics point to the .razor files instead of the generated ones. Please correct me if I misunderstood.

[costin-zaharia-sonarsource]

@davidwengier

I don't think I follow this request. It seems like maybe the ask is for Roslyn to change GetLocation to honour #line mappings, but only for Razor generated files?

No, this request is about having in SARIF report the paths to the .razor/.cshtml files instead of the generated one.

Also, it seems to me like if the GetLocation() call in the analyzer returns a location in the .razor file then that would just mean running into dotnet/razor#9308 again.

This is my concern, that by doing manual mappings we are not using the compiler API in the way it was designed.

[jaredpar]

@mavasani

Looking at this today it seems that when emitting the file.uri property the SARIF logger is using the original location not the mapped location. That is contrary to how we display the diagnostic during build where we use the mapped span. Essentially:

• command line build: diagnostics point to our .razor files as they use mapped spans
• SARIF file: diagnostics point to our .g.cs files as they use the raw spans

Is this a deliberate decision or something we can change?

[jaredpar]

In discussion with @mavasani it appears that it's an oversight that we can fix here.

@RikkiGibson can you try and get this in for 17.9? It should be a pretty straight forward change.

[RikkiGibson]

Resolved in #71454