Roslyn-analyzers: CA1065 raised in property getters

[AlexanderTaeschner]

Analyzer package

Microsoft.CodeAnalysis.FxCopAnalyzers

Package Version

Example: v2.9.4 (Latest)

Diagnostic ID

CA1065

Repro steps

With .Net Core 3 Preview 8 and Visual Studio 16.3 Preview 2 Community installed I get errors whenever a NotSupportedException or InvalidOperationException is raised in a property getter (the same code did not produce warning with Preview 7, 16.3 Preview 1), e.g.:
public class ProxyStream : Stream { ... public override bool CanSeek => throw new NotSupportedException(); ... }

Expected behavior

No warning.

Actual behavior

warning CA1065: get_CanSeek creates an exception of type Exception, an exception type that should not be raised in a property. If this exception instance might be raised, use a different exception type, convert this property into a method, or change this property's logic so that it no longer raises an exception.

[Zenexer]

Seeing the same thing for InvalidOperationException.

[Zenexer]

InvalidOperationException and NotSupportedException are both in the allowed list: https://github.com/dotnet/roslyn-analyzers/blob/c9d6579f31fd4ad45f3d23187189657343b395a4/src/Microsoft.CodeQuality.Analyzers/Core/ApiDesignGuidelines/DoNotRaiseExceptionsInUnexpectedLocations.cs#L170

[AlexanderTaeschner]

The test case CSharpPropertyNoDiagnostics in DoNotRaiseExceptionsInUnexpectedLocationsTests which test for this allowed case even succeeds on my machine, but the packaged version 2.9.4 produces the warnings mentioned above (both for InvalidOperationException and NotSupportedException).

[Zenexer]

Here's another scenario that fails. Initially I thought it was specific to this scenario until I saw the existing issue. I assume it has the same underlying cause, but I figured it was worth mentioning.

public static DirectoryInfo ResourceDirectory => _resourceDirectory ?? throw GetUninitializedException();  // CA1065

private static InvalidOperationException GetUninitializedException()
    => new InvalidOperationException("Not initialized");

I haven't really debugged it, but it looks like ((IThrowOperation)operationContext.Operation).Exception?.Type here always indicates Exception now, for some reason:

https://github.com/dotnet/roslyn-analyzers/blob/c9d6579f31fd4ad45f3d23187189657343b395a4/src/Microsoft.CodeQuality.Analyzers/Core/ApiDesignGuidelines/DoNotRaiseExceptionsInUnexpectedLocations.cs#L99

Could this be a regression in Roslyn, rather than the analyzer?

[mavasani]

@Zenexer @AlexanderTaeschner Thanks for the repro and the detailed explanation. The last comment does suggest that it might be an underlying breaking change in Roslyn, given the analyzer has not changed for quite a while now. I will investigate further, but do let us know if you find out the root cause sooner.

[rtaylor72]

I also was caught off guard with this one. I do not see an issue with NotImplementedException being in abstract classes to make sure the coder implements in their concrete class. Also, this is auto generated commonly by Visual Studio for stubs so you may see more push on this. Can you also add NotImplementedException to the list?

[Zenexer]

@rtaylor72 Typically, you should probably be declaring members as abstract if you want to force implementation in subclasses. You probably shouldn't be encountering many cases where it makes sense to throw NotImplementedException in an abstract class.

That being said, there is a bug here: there are lists of allowed exception types, and NotImplementedException is already in most of them. The lists aren’t taking effect because the type is being interpreted as a generic Exception for some reason. That's probably what caused the behavior you observed in https://github.com/MicrosoftDocs/visualstudio-docs/issues/3809.

That being said, there may be places it doesn't make sense to throw a NotImplementedException. Abstract classes are generally on that list, but I don't think the analyzer takes that into account at the moment. I'm pretty sure what you're seeing is a bug.

[rtaylor72]

This was from some old code I resurrected for a Blazor presentation. For some reason I was thinking "abstract" could not be on properties at the time or I just ran with the generated code when I was abstracting it. @Zenexer, I agree with you that those should be set to abstract so that validation happens at compile time. Thanks You!

[mavasani]

Tagging @gafter @333fred. This analyzer breaking change seems to be the side effect of
https://github.com/dotnet/roslyn/blob/a5418f7398495db4ea858f0c9efd9b13541ff478/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs#L395. For example, unit test change here: https://github.com/dotnet/roslyn/pull/37052/files/9a9a0c72c629e6b4d2c5fd00a49ab338bc8176fd#diff-b466aea1f2ec7f7d0d4d45b702409b17R135

@gafter Changed the bound tree structure and hence the IOperation tree structure for IThrowOperation to follow the ECMA spec to have an implicit conversion from thrown exception type to System.Exception so that IThrowOperation.Exception.Type is always System.Exception. This would likely break almost all existing analyzers operating on IThrowOperation.

@333fred Is it possible to handle to change the operation factory to not expose this IConversionOperation wrapping the thrown expression? Or the analyzers need to be updated to handle this case?

[mavasani]

I have fixed the analyzer side for now. In case we see any more reports on this compiler breaking change, we can consider a change in the operation factory.