Fix S4487 FP: recognize usages of private fields in partial Blazor components on Rider

naefp commented 8 months ago

UPDATE: Issue blocked until Rider and Omnisharp include Razor in the Compilation. See this comment for more details.

Description

When declaring and setting a private field in a razor.cs file, but only accessing it in the related razor file, sonar marks it as not used. Sonar should check the related razor file for S4487 as well.

Repro steps

Create a Blazor component with two files:

MyBlazorComponent.razor

<div>
  <p>@_myPrivateField</p>
</div>

MyBlazorComponent.razor.cs

public partial class MyBlazorComponent
{
  private string _myPrivateField; // Sonar marks this field as not used

  void Some()
  {
    _myPrivateField = "hi";
  }
}

Expected behavior

Recognize that the field actually is being used in razor file.

Actual behavior

False positive for S4487.

Known workarounds

Mark every occurrence as false positive, or disable rule csharpsquid:S4487 for **/*.razor.cs completely.

Probably not using code in separate *razor.cs file but declare the private field in the razor file itself. Did not test that.

Related information

C# 12
Rider 2023.3.2
.Net 8
SonarLint plugin 10.2.1.77304 connected to SonarCloud
Windows 11

cristian-ambrosini-sonarsource commented 8 months ago

Hi @naefp, I'm investigating this issue as well. The implementation is shared between S4487 and S1144 so the issue is probably linked to #8537

cristian-ambrosini-sonarsource commented 8 months ago

I confirm this as a FP. To provide additional context, the issue does not reproduce in the following:

SL4VS
local analysis run with SonarQube
analyzer UTs

naefp commented 7 months ago

It seems like S2933 (Fields that are only assigned in the constructor should be "readonly") is affected as well.

MyBlazorComponent.razor

<SomeComponant @bind-Value="@_myPrivateField " />

MyBlazorComponent.razor.cs

public partial class MyBlazorComponent
{
  private string _myPrivateField = "Some intial value"; // Sonar want to make this 'readonly'
}

sebastien-marichal commented 7 months ago

As for #8537, the issue seems to be related to SonarLint only. Since it is not yet able to analyze Razor generated files, it is not able to see the field usage in the component. We have an internal ticket to tackle the support of Razor in SonarLint which should fix this issue.

antonioaversa commented 3 months ago

TL;DR: issue blocked until Omnisharp starts supporting Razor compilation

We have reproduced and debugged live the issue from the IDE with the following setups:

Rider 2024.1.2 (Roslyn 4.10.0-3.24216.12) and Visual Studio 17.10.1 (Roslyn 4.10.0-3.24270.2)
notice that the Roslyn version is recent enough to include changes around razor files generation and location mapping
latest version of SonarLint 10.6.2.78685 for Rider
latest version of SonarLint 8.0.0.92083 for Visual Studio
a simple Blazor project with a private field declared in the cs part of a component (inheriting from ComponentBase) and used in the razor part of the component
a Debugger.Launch in the rule

The difference between Visual Studio and Rider, even after recent changes around Razor file generation from Microsoft, is that Rider uses Omnisharp, which deal with projects containing razor files differently.

As of the time of our testing, what we observed is that the Compilation does not contain any razor generated file, which are present when building in Visual Studio, with extension .ide.g.cs. As an example, for the simple Blazor project mentioned above, only 5 Syntax Trees are visible in the Compilation when in Rider, as opposed to the 14 files of the Compilation in Visual Studio.

Given that the original razor files are not even additional files in the Compilation, there is currently no way to distinguish between a false positive case induced by a missing partial class, produced by lack of razor generation, and a true positive case, where all the partial classes are available.

This primarily happens:

in the context of SonarLint, where the analysis is run by the IDE (and not via dotnet build)
and in the context of Rider, where Omnisharp is used

Another context where the issue may happen is dotnet format.

We could disable the rule in the presence of partial classes that inherit from BaseComponent. That, however, would introduce false negatives in CI as well as in Visual Studio, where generated files are included in the Compilation and the rule works as expected.

Therefore, given the limited scope of the issue, we are not going to change the rule, and instead we are going to wait for Rider to expose generated files in the Compilation.

SonarSource / sonar-dotnet