Fix S2234 Bug: AD0001 is thrown due to referencing a location outside of the current compilation

[rjgotten]

Description

Warning	AD0001	Analyzer 'SonarAnalyzer.Rules.CSharp.ParametersCorrectOrder' threw an exception of type 'System.ArgumentException' with message 'Reported diagnostic 'S2234' has a source location in file '[[redacted-project]]\Primitives\DateTimeRange.cs', which is not part of the compilation being analyzed.
Parameter name: diagnostic'.
Exception occurred with following context:
Compilation: [[redacted-project]].Tests
SyntaxTree: [[redacted-project]].Tests\Primitives\DateTimeRangeTests.cs
SyntaxNode: new DateTimeRange(end, start) [ObjectCreationExpressionSyntax]@[1395..1424) (37,21)-(37,50)

System.ArgumentException: Reported diagnostic 'S2234' has a source location in file '[[redacted-project]]\Primitives\DateTimeRange.cs', which is not part of the compilation being analyzed.
Parameter name: diagnostic
   at Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalysisContextHelpers.VerifyDiagnosticLocationInCompilation(String id, Location location, Compilation compilation)
   at Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalysisContextHelpers.VerifyDiagnosticLocationsInCompilation(Diagnostic diagnostic, Compilation compilation)
   at Microsoft.CodeAnalysis.Diagnostics.DiagnosticAnalysisContextHelpers.VerifyArguments(Diagnostic diagnostic, Compilation compilation, Func`3 isSupportedDiagnostic, CancellationToken cancellationToken)
   at Microsoft.CodeAnalysis.Diagnostics.SyntaxNodeAnalysisContext.ReportDiagnostic(Diagnostic diagnostic)
   at SonarAnalyzer.AnalysisContext.SonarReportingContextBase`1.ReportIssueCore(Diagnostic diagnostic)
   at SonarAnalyzer.Rules.ParametersCorrectOrderBase`1.<Initialize>b__6_0(SonarSyntaxNodeReportingContext c)
   at SonarAnalyzer.AnalysisContext.SonarCompilationStartAnalysisContext.<>c__DisplayClass10_0`1.<RegisterNodeAction>b__0(SyntaxNodeAnalysisContext x)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__49`1.<ExecuteSyntaxNodeAction>b__49_0(ValueTuple`2 data)
   at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action`1 analyze, TArg argument, Nullable`1 info, CancellationToken cancellationToken)
-----

Suppress the following diagnostics to disable this analyzer: S2234	[[redacted-project]].Tests.csproj	[[redacted-project]].Tests	1

Repro steps

Impossible to provide as an inline code sample or as a sample project.
Issue occurs with a complex setup where the class being analyzed ends up not being part of the current partial compilation unit handled by Roslyn. This can likely only be accurately reproduced with the full solution which I am not at liberty to share access to.

This worked with SonarLint v7.4.0.80741 and SonarC# v9.12.0.78982.
It broke with SonarLint v7.5.0.82292 and SonarC# v9.15.0.81779.

So at least you have a bisection window.

Known workarounds

• Suppress the rule wholesale via .editorconfig; situational suppressing via pragma or attribute does not work.
• Downgrade SonarLint v7.5.0.82292 back to prior version v7.4.0.80741, which shipped with SonarC# v9.12.0.78982 and didn't exhibit this issue.

Related information

• C#/VB.NET Plugins version: v9.15.0.81779 according to what shipped with SonarLint 7.5.0.82292
• Visual Studio version: VS 2022 v17.8.5
• MSBuild / dotnet version: dotnet SDK 8.0.11
• SonarScanner for .NET version (if used): N/A
• Operating System: Windows 11

[sebastien-marichal]

Hello @rjgotten,

Thank you for reporting this issue.
Unfortunately, it is very difficult to reproduce your issue without a reproducer. It seems to happen in a very peculiar scenario.

So I must ask, is there a chance you could provide a very simplistic version of your issue?
It does not need to be a one-liner.

If this is really not possible, I would need you to describe your issue in more detail.
How the method is defined? Is the usage and the definition in the same project and/or solution? Is the method static? Number of arguments, etc...
Please try to provide as much detail as you can so I can make a reproducer on my side.

Thank you.

[rjgotten]

@sebastien-marichal
The usage is in the same solution, but not in the same project.
The class in question on which it's raised, is our DateTimeRange - which is just a basic start/end range implementation over DateTime along with some methods to return number of days or passages of midnight inbetween.

The AD0001 is raised on a violation of S2234 inside the *.Tests project that sits alongside the domain models project.
Specifically, it's raised where we intentionally pass the start and end parameter for this class out of order, to test that the constructor throws an ArgumentException for this case.

We actually specifically suppress S2234 from being raised via pragmas.
But that only suppresses the result. Not the actual analysis. So the rule still crashes.

[Test]
public void Constructor_InvertedRange_Throws()
{
  var start = DateTime.Parse("2021-01-01T10:00", CultureInfo.InvariantCulture);
  var end   = DateTime.Parse("2021-01-01T11:00", CultureInfo.InvariantCulture);

  Assert.Multiple(() => 
  {
    #pragma warning disable S2234 // Parameters should be passed in the correct order
    Assert.That(() => new DateTimeRange(start, end), Throws.Nothing);
    Assert.That(() => new DateTimeRange(end, start), Throws.ArgumentException);
    #pragma warning restore S2234 // Parameters should be passed in the correct order
  });
}

[sebastien-marichal]

Hello @rjgotten,

I am still not able to reproduce your issue.
However, we might have identified a potential bug that could be the origin of your issue.

Unfortunately, without a proper reproducer, we won't be able to confirm the issue is fixed.

I will add this issue to the backlog to fix the bug.

---

When reporting an issue (see here) we make sure the primary location is part of the compilation, if not we do not raise the issue due to potential AD0001. We should probably do the same for the secondary locations as well.

[rjgotten]

When reporting an issue (see here) we make sure the primary location is part of the compilation, if not we do not raise the issue due to potential AD0001. We should probably do the same for the secondary locations as well.

... huh. Lightbulb moment 💡
I think I actually suggested this as the point of failure before, a few years ago when this rule broke in similar ways.
What are the odds...

Have one suggestion though:
Maybe instead of not raising the rule violation at all, just leave the offending secondary location out?
Afaik Roslyn analyzers only need the primary location to report back correctly - the secondaries are purely supplementary. They could be used by other tooling, of course. But even other tooling would probably prefer a lesser-documented violation than a swallowed violation, no? 😉

[sebastien-marichal]

To make it more explicit, when a secondary location is not part of the compilation, we have two possible solutions:

• drop the diagnostic, as for primary locations
• drop only secondary locations that are not part of the compilation, as suggested by @rjgotten