AD0001 'SonarAnalyzer.Rules.CSharp.SymbolicExecutionRunner' threw an exception of type 'System.NullReferenceException'

AraHaan commented 6 months ago

Description

https://github.com/Elskom/IDisposableGenerator/security/code-scanning On this page it displays proof that the analyzer reports nullref exceptions when analyzing the code in this repository. Locally everything seems to run just fine though.

Repro steps

The issue happens with the code in https://github.com/Elskom/IDisposableGenerator, for some reason it fails to happen locally, just in the github security scanning phase does this problem occur.

Known workarounds

No known workaround yet.

Related information

C#/VB.NET Plugins version: latest
Visual Studio version: latest preview Visual Studio.
MSBuild / dotnet version: latest preview from Visual Studio and the .NET SDK
SonarScanner for .NET version (if used): latest preview .NET SDK (9.0.100 preview I believe)
Operating System: Windows 11 latest beta channel build.

pavel-mikula-sonarsource commented 6 months ago

Hi @AraHaan,

Your first link seems to be private and we can't see any details.

AraHaan commented 6 months ago

It seems this also causes the bug to happen as well on my ref assemblies in https://github.com/Elskom/runtime/ when cloned locally and opened inside of Visual Studio 2022 as well.

As for the stack traces on the original link they fail in a similar way with this content:

Analyzer 'SonarAnalyzer.Rules.CSharp.SymbolicExecutionRunner' threw an exception of type 'System.NullReferenceException' with message 'Object reference not set to an instance of an object.'.
Exception occurred with following context:
Compilation: srcassembly.dll
SyntaxTree: IDisposableGeneratorTests.VisualBasic.cs
SyntaxNode: [Fact] public async Task TestGeneratingDisposableOwnsVisualBasic ... [MethodDeclarationSyntax]@[2632..3999) (94,4)-(141,9) System.NullReferenceException: Object reference not set to an instance of an object.
at lambda_method183(Closure , Object , SyntaxTree , String , CancellationToken , ReportDiagnostic& )
at StyleCop.Analyzers.Lightup.SyntaxTreeOptionsProviderWrapper.TryGetDiagnosticValue(SyntaxTree tree, String diagnosticId, CancellationToken cancellationToken, ReportDiagnostic& severity)
at SonarAnalyzer.Rules.SymbolicExecutionRunnerBase.IsEnabled(SonarSyntaxNodeReportingContext context, DiagnosticDescriptor descriptor)
at SonarAnalyzer.Rules.SymbolicExecutionRunnerBase.<>c__DisplayClass19_0.b__0(KeyValuePair2 x) at System.Linq.Enumerable.WhereEnumerableIterator1.MoveNext()
at System.Linq.Lookup2.Create(IEnumerable1 source, Func2 keySelector, IEqualityComparer1 comparer)
at System.Linq.GroupedEnumerable2.GetEnumerator() at System.Linq.Enumerable.SelectEnumerableIterator2.MoveNext()
at System.Linq.Enumerable.WhereEnumerableIterator1.ToArray() at System.Linq.Enumerable.ToArray[TSource](IEnumerable1 source)
at SonarAnalyzer.Rules.SymbolicExecutionRunnerBase.AnalyzeRoslyn(SonarAnalysisContext analysisContext, SonarSyntaxNodeReportingContext nodeContext, ISymbol symbol)
at SonarAnalyzer.Rules.SymbolicExecutionRunnerBase.Analyze(SonarAnalysisContext analysisContext, SonarSyntaxNodeReportingContext nodeContext, ISymbol symbol)
at SonarAnalyzer.Rules.SymbolicExecutionRunnerBase.Analyze(SonarAnalysisContext analysisContext, SonarSyntaxNodeReportingContext nodeContext)
at SonarAnalyzer.Rules.CSharp.SymbolicExecutionRunner.<>c__DisplayClass9_0.b__1(SonarSyntaxNodeReportingContext c)
at SonarAnalyzer.AnalysisContext.SonarCompilationStartAnalysisContext.<>c__DisplayClass10_01.<RegisterNodeAction>b__0(SyntaxNodeAnalysisContext x) at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.<>c__621.b__62_0(ValueTuple2 data) at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock[TArg](DiagnosticAnalyzer analyzer, Action1 analyze, TArg argument, Nullable`1 info) Suppress the following diagnostics to disable this analyzer: S1944, S2053, S2222, S2259, S2583, S2589, S3329, S3655, S3900, S3949, S3966, S4158, S5773

pavel-mikula-sonarsource commented 6 months ago

Thank you @AraHaan, that's an interesting stack trace.

Some context for whoever will pick this up: This is the interesting line:

at SonarAnalyzer.Rules.SymbolicExecutionRunnerBase.IsEnabled(SonarSyntaxNodeReportingContext context, DiagnosticDescriptor descriptor)

where we invoke our own shim of SyntaxTreeOptionsProvider.

The Roslyn version used in that scenario returns something that we don't expect.

pavel-mikula-sonarsource commented 6 months ago

@AraHaan What exact VS version do you use?

Can you please put this line anywhere in that project

#error version

and write us what version of Roslyn does it report in the produced error message?

AraHaan commented 6 months ago

IDE: Microsoft Visual Studio Enterprise 2022 (64-bit) - Preview Version 17.10.0 Preview 1.0 Compiler: Compiler version: '4.10.0-1.24081.13 (fa72fa61)'. Language version: preview.

martin-strecker-sonarsource commented 6 months ago

Here is what I tried so far: This is the method we shim in the version provided by @AraHaan (4.10.0-1.24081.13 (fa72fa61))

https://github.com/dotnet/roslyn/blob/fa72fa61b0d822ea8a3fbeb96f668340419ab5cd/src/Compilers/Core/Portable/Compilation/SyntaxTreeOptionsProvider.cs#L21

That file has stayed the same for four years. The method signature is public abstract bool TryGetDiagnosticValue(SyntaxTree tree, string diagnosticId, CancellationToken cancellationToken, out ReportDiagnostic severity);

The NRE happens, according to the stack trace above, in lambda_method183(Closure , Object , SyntaxTree , String , CancellationToken , ReportDiagnostic& ) which corresponds to this call https://github.com/SonarSource/sonar-dotnet/blob/c0d6a6125867f05835c809345de6548e0a10e063/analyzers/src/SonarAnalyzer.CFG/ShimLayer/SyntaxTreeOptionsProviderWrapper.cs#L55

The NRE happens inside the generated code

This is where the call happens (note: this code is no longer in the latest master since #8792 but moved somewhere else): https://github.com/SonarSource/sonar-dotnet/blob/90253746e6a5da59e1e30ecfd0b1b6bc22a712c2/analyzers/src/SonarAnalyzer.Common/Rules/SymbolicExecutionRunnerBase.cs#L57-L58 Note: There is a wrapper returned, even if context.Compilation.Options is null in line 57.

The following arguments are passed to lambda_method183(Closure , Object , SyntaxTree , String , CancellationToken , ReportDiagnostic& ):

Closure: I am not precisely sure why there is a closure, but it is probably not relevant anyway
Object: This is misleadingly called "node" in the wrapper. It corresponds to "context.Compilation.Options"; this one might be null. It is the "this" parameter of the instance method call in Expression.Call(instance. If it is null, that call raises an NRE.

https://github.com/SonarSource/sonar-dotnet/blob/c0d6a6125867f05835c809345de6548e0a10e063/analyzers/src/SonarAnalyzer.CFG/ShimLayer/LightupHelpers.cs#L645-L664

SyntaxTree: This one is unlikely to be null or cause an NRE because there is only a cast in the wrapper code that does not cause NREs, and in the call itself ArgumentNullException is to be expected
String: diagnosticId unlikely as well for the same reasons as SyntaxTree
CancellationToken: Value type -> no NRE
ReportDiagnostic&: the out parameter. Value type -> no NRE

The most likely cause is that context.Compilation.Options is null. I cloned the project and opened it in VS. The NRE wasn't raised there. This is most likely due to the preview version of Roslyn used by @AraHaan.

We could:

Add a null check for context.Compilation.Options. We will not be able to bring this under test, though, and my analysis above might be incorrect.
Report the issue in Roslyn, but we do not have evidence and a re-producer for them.
The problem appears in a preview version of Roslyn. We may wait until we can re-produce in a release version.

I looked at the Roslyn repo for reports of some similar problems, but couldn't find any. @pavel-mikula-sonarsource What do you think?

pavel-mikula-sonarsource commented 6 months ago

SyntaxTreeOptionsProviderWrapper.TryGetDiagnosticValueAccessor is delegate type that returns the lambda expression => that's where the closure is.

options begin null sounds like the most likely situation indeed.

I'd suggest to install the preview version. If it reproduces, we should be able to confirm where is it coming from. Then we can ask Roslyn if it is intentional and add nullcheck to remove the NRE.

AraHaan commented 6 months ago

I also identified that the nullref also happens here as well:

And roslyn also throws an exception in a few code fix providers to use "primary constructor" because it cannot change the fact that this.KeepOpen is a source generated property. That problem I am also going to report to the roslyn team for them to consider a way to have it check if the codefix provider would throw before it produces the diagnostic so it would not produce said diagnostic as the codefix would be impossible to make. I will ask a roslyn dev on discord with the link to the martin's message and ask them if I should file a bug report with what is currently known at the time for further inspection into the issue as well.

pavel-mikula-sonarsource commented 4 weeks ago

We'll close this one, as we're not able to reproduce it. It was likely related to the Preview version, and we haven't heard about a similar issue since it was created.

Thank you @AraHaan for reporting it.

SonarSource / sonar-dotnet