dotnet / razor

Compiler and tooling experience for Razor ASP.NET Core apps in Visual Studio, Visual Studio for Mac, and VS Code.
https://asp.net
MIT License
491 stars 190 forks source link

Razor files: Generated code misses #line hidden for using directives #10375

Open martin-strecker-sonarsource opened 4 months ago

martin-strecker-sonarsource commented 4 months ago

Version Used:

.Net 9 preview SDK version 9.0.100-preview.3.24204.13

Steps to Reproduce:

  1. Create a Blazor App
  2. Enable <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles> in the csproj
  3. Add BugRepro.razor
@using System.Net.Http
@using static Microsoft.AspNetCore.Components.Web.RenderMode

The actual generated code looks like so:

// ...
#nullable restore
#line (1,2)-(2,1) "C:\Projects\sonar-scanner-vsts-test\src\BlazorApp1\Components\BugRepro.razor"
using System.Net.Http

#nullable disable
    ;
#nullable restore
#line (2,2)-(2,61) "C:\Projects\sonar-scanner-vsts-test\src\BlazorApp1\Components\BugRepro.razor"
using static Microsoft.AspNetCore.Components.Web.RenderMode

#line default
#line hidden
#nullable disable
    ;
    #nullable restore
    public partial class BugRepro : global::Microsoft.AspNetCore.Components.ComponentBase
    #nullable disable
// ...

After the using System.Net.Http the #line default #line hidden directives are missing. The expected generated code should look like so:

#nullable restore
#line (1,2)-(2,1) "C:\Projects\sonar-scanner-vsts-test\src\BlazorApp1\Components\BugRepro.razor"
using System.Net.Http

#line default
#line hidden
#nullable disable
    ;
#nullable restore
#line (2,2)-(2,61) "C:\Projects\sonar-scanner-vsts-test\src\BlazorApp1\Components\BugRepro.razor"
using static Microsoft.AspNetCore.Components.Web.RenderMode

#line default
#line hidden
#nullable disable
    ;
    #nullable restore
    public partial class BugRepro : global::Microsoft.AspNetCore.Components.ComponentBase
    #nullable disable
// ...

Why is this a problem:

Because of the missing #line default #line hidden, the ; is mapped back to the razor file at a position two lines below the using. This line might be outside the number of lines of the razor files. This causes this error https://community.sonarsource.com/t/java-lang-illegalargumentexception-line-575-is-out-of-range-for-file-xxx-file-has-386-lines/108740/17 in our analyzer.

See https://github.com/SonarSource/sonar-dotnet/issues/9288 for more details and https://github.com/SonarSource/sonar-dotnet/pull/9289 for two unit tests. One shows the problematic mapping and the other one shows that adding #line default #line hidden would fix the problem.

davidwengier commented 4 months ago

The #line default #line hidden pair were deliberately removed in https://github.com/dotnet/razor/pull/9991 so adding them back hopefully is a last resort, or it will regress tooling scenarios. I suspect this broke with the combination of that PR, and https://github.com/dotnet/razor/pull/9949, combined.

Presumably this only happens if the Razor file has an @using that is within 2 lines of the end of the .razor file, right @martin-strecker-sonarsource? I'm somewhat surprised thats common enough to impact you, though I guess its possible people are running into this as they type in a brand new .razor file, if the first thing they do is add using statements.

johanbenschop commented 4 months ago

This issue impacts us on SonarCloud, and yes, it's the _Imports.razor file for us that only has 11 @using lines. Adding a couple of new lines followed by a commented-out line solved the error from Sonar for us.

This razor file is used for a part of our application using Blazor, as followed from the official docs. I think this would be rather common.

costin-zaharia-sonarsource commented 4 months ago

The problem is reproducible also when using dotnet 8.0.5.

davidwengier commented 4 months ago

Oh yes, I didn't think of the _Imports file, that makes sense. Thanks.

costin-zaharia-sonarsource commented 4 months ago

I think the problem starts with the namespace declaration.

The generated syntax tree for:

@using System
@using System.Collections.Generic

is:

#pragma checksum "C:\_Imports.razor" "{ff1816ec-aa5e-4d10-87f7-6f4963833460}" "395205bceb8baabf9dd9a6d172f3f922d3bf2479"
// <auto-generated/>
#pragma warning disable 1591
namespace ASP.TestCases
{
...

If I call .GetLocation().GetMappedLineSpan() for the ASP SyntaxToken (first part of the namespace), the result is a span with the interval (3,10)-(3,13).

I would have expected that the namespace declaration would be treated as generated code and not be mapped. Since it's not part of the user code, any line reported is wrong. In this specific case, it happens that line 4 (0-based index) does not exist in the file.

davidwengier commented 4 months ago

@costin-zaharia-sonarsource In that example with the namespace, does the mapped span come back with [HasMappedPath](https://sourceroslyn.io/Microsoft.CodeAnalysis/R/2689bf9173f268a5.html) equal to true? That would be surprising. It's an odd API, but Roslyn will happily return the original span when calling GetMappedLineSpan(), but the HasMappedPath should return false on those occasions.

costin-zaharia-sonarsource commented 4 months ago

Hi @davidwengier, great question. I've missed that. The HasMappedPath is indeed false. Sorry for the red herring.

The problem is indeed the semicolon.

martin-strecker-sonarsource commented 4 months ago

Presumably, this only happens if the Razor file has an @using that is within 2 lines of the end of the .razor file

The problem is that for the ; token of the first using, token.GetLocation().GetMappedLineSpan() points to some arbitrary location in the original file (two lines below the @using). There is no mapping from the ; in the generated file back to the original file, and as such, the return value of token.GetLocation().GetMappedLineSpan() should indicate this. Adding #line default #line hidden is one option to fix this.

One problem with enhanced line directives is that they lack proper support for "range," as described, e.g., here: https://github.com/dotnet/roslyn/issues/69248#issuecomment-1749952541. You chose to separate the original and generated code by new lines. In the case of @using, the ; misses the required annotation to turn off the mapping.

davidwengier commented 4 months ago

@martin-strecker-sonarsource Thanks, thats a very informative link. This line in particular:

The #line directive does not specify where the user written expression ends.

This surprises me. Logically speaking I would have thought that the changes introduced in #10380 would solve this, as based on this diff for example:

image

I would have hoped that Roslyn would know that the newline and ; are not mapped, given the end range on the line mapping. I guess it matches the behaviour of a regular #line directive though.

martin-strecker-sonarsource commented 4 months ago

I would have hoped that Roslyn would know that the newline and ; are not mapped

I'm not sure about it and it needs to be tested. The actual mapping is quite complicated (due to support for debugger sequence points) and described here: https://github.com/dotnet/csharplang/blob/8bb3c4ca0180db2b9dc3573d91bee60da2ec530c/proposals/csharp-10.0/enhanced-line-directives.md

Can you test in https://github.com/dotnet/razor/pull/10380 if token.GetLocation().GetMappedLineSpan() (where token is the ; token) returns something that indicates that the ; is not part of the original file?

I can do such a test in https://github.com/SonarSource/sonar-dotnet/pull/9289 if that helps.

costin-zaharia-sonarsource commented 3 months ago

I've found one more case that might be related to this.

When @page directive is used, the generated code looks like this:

...
#line default
#line hidden
#nullable disable
    ;
    [global::Microsoft.AspNetCore.Components.RouteAttribute(
    // language=Route,Component
#nullable restore
#line (1,7)-(1,15) "C:\src\work\sonar-dotnet\sonar-dotnet-shared-library\src\test\resources\RazorProtobufImporter\WebProject\Cases.razor"
"/cases"

#line default
#line hidden
#nullable disable
    )]
...

and the "/cases" token is mapped back to the user file. To me it looks to be a very similar issue.

If you prefer I can open a separate issue for this.

jjonescz commented 3 months ago

I would have hoped that Roslyn would know that the newline and ; are not mapped

I'm not sure about it and it needs to be tested. The actual mapping is quite complicated (due to support for debugger sequence points)

Sounds correct - the linked PR won't fix this issue. I've added a test - https://github.com/dotnet/razor/pull/10380#discussion_r1615996407.

In this case I guess a workaround (for analyzer authors) could be to filter out these mappings which point to lines that are greater than the following mappings since it should be impossible to have an actual overlapping code like that. For example:

#nullable restore
#line (1,2)-(1,23) "Shared/Component1.razor"
using System.Net.Http // maps to line 1

#nullable disable
    ; // maps to line 3 👈 can filter this out since there's a mapping to line 2 below!
#nullable restore
#line (2,2)-(2,61) "Shared/Component1.razor" // maps to line 2
using static Microsoft.AspNetCore.Components.Web.RenderMode

#line default
#line hidden
#nullable disable
    ; // doesn't map anywhere thanks to the `#line hidden`