SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
784 stars 226 forks source link

Fix S1128 FP: documentation tags with dotnet build command #3633

Open pavel-mikula-sonarsource opened 4 years ago

pavel-mikula-sonarsource commented 4 years ago

History

Issue with documentation tags was originally reported and fixed in #2694 with a simple reproducer:

using System; // Compliant, used by cref.

namespace SonarAnalyzer.Experiments.CSharp
{
    public enum S1128
    {
        /// <summary><see cref="DateTime"/></summary>
        DateTimeValue,
    }
}

Reproducible scenario

As @mgasparel found out in his comment this issue became reproducible again when:

Not reproducible scenario

This is NOT reproducible with the same setup inside Visual Studio. The FP is not reported. It also doesn't reproduce in our UTs from in-memory solution.

Observations

It seems that c.Node.DescendantTrivia() does return all trivias for CompilationUnitSyntax under Visual Studio environment. And return empty collection under dotnet build context. Weird.

It also seems that it returns the trivias also under dotnet build context when explicitly parametrized with descentIntoChildren argument: c.Node.DescendantTrivia(x => true) but from a quick test, it seems that these comments doesn't have HasStructure flag set.

To make it more interesting, it seems that taking DescendantTrivia() from some nested node (like the EnumDeclarationSyntax) can return all trivias with their structure parsed and available.

There might be something environment/context dependent in Roslyn behavior (from some of it's versions). Guess would be that Roslyn doesn't parse the travias for dotnet build for performance reasons.

Measurement

Way to debug this:

andrei-epure-sonarsource commented 3 years ago

This also is reported to happen via the Azure Pipeline MSBuild task - see community thread.

andrei-epure-sonarsource commented 2 years ago

Another report: https://community.sonarsource.com/t/s1128-raised-when-namespace-is-referenced-by-type-used-in-comment/63148

killergege commented 2 years ago

We have the same false positive with .NET 6 using Azure pipeline tasks (SonarCloudAnalyze@1 / DotNetCoreCLI@2 / SonarCloudPrepare@1) and SonarCloud. In our case, this is a custom exception. Project's ImplicitUsings is enabled, there is no Global usings.

SonarLint (connected mode) for VS2022 doesn't report the issue, only Azure.

LadislavMargai commented 1 year ago

This issue is quite old (2 years). After running the dotnet build CLI command for .NET 6 project + SonarAnalyzer.CSharp 8.41 NuGet package I can still reproduce the issue.

Is there any estimate when it could be fixed?

pavel-mikula-sonarsource commented 1 year ago

Hi,

we have no specific ETA to fix this, unfortunately. It might get picked up during a hardening sprint, just as with any other issue.

markusschaber commented 1 year ago

The same here, running sonarqube in a locally hosted gitlab instance.

martin-strecker-sonarsource commented 1 year ago

Probably caused by documentation mode set to none during the compilation. This is the same blocker as discovered by https://github.com/SonarSource/sonar-dotnet/pull/7700#issue-1825988946 and described in https://github.com/dotnet/roslyn/issues/41640 I didn't investigate this, though but it seems plausible.

sebastien-marichal commented 2 months ago

I was able to reproduce the error following the steps to reproduce. When updating the SonarAnalyzer.CSharp package in the sonar-repro project to the latest version (9.28.0.94264), the issue is no longer raised.

Should we keep it open?

pavel-mikula-sonarsource commented 2 months ago

@sebastien-marichal That sounds suspicious, as the root of the problem comes from the way Roslyn parses stuff => analyzer update should not change anything.