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

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

Open
martin-strecker-sonarsource opened this issue May 16, 2024 · 12 comments
Labels
area-compiler Umbrella for all compiler issues untriaged

Comments

@martin-strecker-sonarsource

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 SonarSource/sonar-dotnet#9288 for more details and SonarSource/sonar-dotnet#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.

@phil-allen-msft phil-allen-msft added the area-compiler Umbrella for all compiler issues label May 16, 2024
@davidwengier
Copy link
Contributor

The #line default #line hidden pair were deliberately removed in #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 #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
Copy link

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

The problem is reproducible also when using dotnet 8.0.5.

@davidwengier
Copy link
Contributor

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

@costin-zaharia-sonarsource
Copy link

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
Copy link
Contributor

@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
Copy link

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
Copy link
Author

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: dotnet/roslyn#69248 (comment). 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
Copy link
Contributor

@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
Copy link
Author

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 #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 SonarSource/sonar-dotnet#9289 if that helps.

@costin-zaharia-sonarsource

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
Copy link
Contributor

jjonescz commented May 27, 2024

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 - #10380 (comment).

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`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-compiler Umbrella for all compiler issues untriaged
Projects
None yet
Development

No branches or pull requests

6 participants