IndexOutOfRangeException in UseConcreteTypeAnalyzer (CA1859)

[mavasani]

Analyzer

Diagnostic ID: CA1859: Use concrete types when possible for improved performance

Analyzer source

SDK: Built-in CA analyzers in .NET 8 SDK or later

Version: SDK 8.0.0-preview.23364.2

AD0001 analyzer exception diagnostic

Exception seen while working on Roslyn.sln

Severity	Code	Description	Project	File	Line	Suppression State
Error	AD0001	Analyzer 'Microsoft.NetCore.Analyzers.Performance.UseConcreteTypeAnalyzer' threw an exception of type 'System.IndexOutOfRangeException' with message 'Index was outside the bounds of the array.'.
Exception occurred with following context:
Compilation: Microsoft.CodeAnalysis.Workspaces
IOperation: Invocation
SyntaxTree: C:\roslyn\src\Workspaces\Core\Portable\Storage\SQLite\v2\SQLitePersistentStorage_Threading.cs
SyntaxNode: vt.Item1() [InvocationExpressionSyntax]@[3489..3499) (59,16)-(59,26)

System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at Analyzer.Utilities.AnalyzerOptionsExtensions.TryGetSyntaxTreeForOption(ISymbol symbol, SyntaxTree& tree)
   at Microsoft.NetCore.Analyzers.Performance.UseConcreteTypeAnalyzer.<>c__DisplayClass9_1.<Initialize>b__4(ISymbol symbol)
   at Microsoft.NetCore.Analyzers.Performance.UseConcreteTypeAnalyzer.Collector.HandleInvocation(IInvocationOperation op)
   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: CA1859	Microsoft.CodeAnalysis.Workspaces (netstandard2.0)	C:\roslyn\src\Workspaces\Core\Portable\Microsoft.CodeAnalysis.Workspaces.csproj	1	Active

[geeknoid]

This looks like a bug in Roslyn:

The HandleInvocation function here calls the CanUpgrade function (here or here or here). This invokes the delegate created here. This finally ends up calling MatchesConfiguredVisibility in the infrastructure, all with straightforward arguments. Then inside this function eventually calls TryGetSyntaxTreeForOption which is where things head south and trigger the array-of-bounds access.

Can this be reassigned to someone that can look at the behavior inside this infrastructure code where the crash actually occurs?

[mavasani]

Thanks @geeknoid, I’ll investigate

[Youssef1313]

Probably MatchesConfiguredVisibility is designed to be only called for symbols from source, not from metadata? So I guess what could be happening is HandleInvocation calls CanUpgrade(op.TargetMethod) and then the TargetMethod is something from metadata, which might end up being something without locations? Or will it be a bug in the compiler if we have a symbol with zero locations?

[Youssef1313]

I think my hypothesis is partially ^[1] correct. The crashing IInvocationOperation in roslyn is invoking a delegate. So TargetMethod is a SynthesizedDelegateInvokeMethod which has no locations.

https://github.com/dotnet/roslyn/blob/00c15ab5fb430bf19c29bd19fa907b7c95e66765/src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedDelegateSymbol.cs#L218-L222

^[1] The wrong part about my previous comment is source vs metadata. Both should have locations, but some synthesized symbols like SynthesizedDelegateInvokeMethod don't.

[mavasani]

Probably MatchesConfiguredVisibility is designed to be only called for symbols from source, not from metadata? So I guess what could be happening is HandleInvocation calls CanUpgrade(op.TargetMethod) and then the TargetMethod is something from metadata, which might end up being something without locations? Or will it be a bug in the compiler if we have a symbol with zero locations?

Thanks @Youssef1313 - this seems to be the reason. Fetching options for a specific syntax tree should only be invoked on symbols we know are from source. Specifically, MatchesConfiguredVisibility is only designed to be called for the symbols exposed off the symbol analysis contexts, which are guaranteed to be source symbols. It should not be invoked for other symbols.