PreviewFeatures - Handle an error case where GetOverriddenMembers() returns null for an indexer

[tannergooding]

Customer Impact

The Requires Preview Features Analyzer (CA2252) encounters an unhandled exception in some scenarios when null symbols are evaluated. This was discovered while @tannergooding was consuming preview features in the TerraFX project and an indexer property was evaluated.

Severity Code Description Project File Line Suppression State Detail Description
Error AD0001 Analyzer 'Microsoft.NetCore.Analyzers.Runtime.DetectPreviewFeatureAnalyzer' threw an exception of type 'System.ArgumentNullException' with message 'Value cannot be null.
Parameter name: key'.
Exception occurred with following context:
Compilation: TerraFX.Samples.DirectX
IOperation: PropertyReference
SyntaxTree: C:\Users\tagoo\Source\repos\terrafx.interop.windows\samples\DirectX\D3D12\HelloTriangle12.cs
SyntaxNode: psoDesc.RTVFormats[0] [ElementAccessExpressionSyntax]@[5006..5027) (110,12)-(110,33)

Fix

Defensive code was added to bail from symbol analysis if the symbol is null.

Testing

Additional test coverage was added for indexer properties. Another test gap of ref T properties was identified during this testing, and another test was added for that scenario; the test passed without the fix herein. The fix was also validated against the TerraFX project scenarios.

Risks

Very low. Addition of defensive code to prevent an unhandled exception. The code path could never have resulted in a diagnostic being reported for a null symbol, so behavior is otherwise unchanged.

[tannergooding]

It's not quite clear why the type is marked IsOverriden but GetOverriddenMembers returns null, it seems like possibly a deeper issue in dotnet/roslyn.

In particular, Roslyn has this logic for IsOverridden

        // Has to be metadata virtual and cannot be a destructor.  
        // Must either lack the newslot flag or be an explicit override (i.e. via the MethodImpl table).
        //
        // The IsExplicitClassOverride case is based on LangImporter::DefineMethodImplementations in the native compiler.
        // ECMA-335 
        // 10.3.1 Introducing a virtual method
        // If the definition is not marked newslot, the definition creates a new virtual method only 
        // if there is not virtual method of the same name and signature inherited from a base class.
        //
        // This means that a virtual method without NewSlot flag in a type that doesn't have a base
        // is a new virtual method and doesn't override anything.
        public override bool IsOverride =>
            !this._containingType.IsInterface &&
            this.IsMetadataVirtual() && !this.IsDestructor &&
            ((!this.IsMetadataNewSlot() && (object)_containingType.BaseTypeNoUseSiteDiagnostics != null) || this.IsExplicitClassOverride);

and a virtual indexer emits:

.method public hidebysig specialname newslot virtual 
        instance int32 get_Item (
            int32 index
        ) cil managed 

while an overriding indexer emits:

.method public hidebysig specialname virtual 
        instance int32 get_Item (
            int32 index
        ) cil managed 

So everything looks like it should line up here.

[tannergooding]

Just noting that ref T Property isn't a bug case here; I initially thought it might have been however and there was no coverage for it.

[tannergooding]

CC. @buyaa-n

[buyaa-n]

LGTM, Isn't it need to be fixed in 6.0? Then please target release/6.0.1xx
CC @jeffhandley for approval

[tannergooding]

Then please target release/6.0.1xx

Does this not have the same general backport policies as dotnet/runtime?

[jmarolf]

Does this not have the same general backport policies as dotnet/runtime?

I assume you would need Tactics approval @tannergooding. main is .NET 7. merging into release/6.0.1xx will auto-merge to main. There is no need to do dual checkins

[jeffhandley]

I'll send it for tactics approval tomorrow. I already gave Steve the heads-up it would be coming.

[tannergooding]

Retargeted against release/6.0.1xx

[jeffhandley]

Tactics approved via email