Add IMethodSymbol.IsReadOnly to public API

[RikkiGibson]

This proposes adding the following public API to IMethodSymbol:

/// <summary>
/// Indicates whether the method is readonly, i.e.
/// whether 'this' is 'ref readonly' in the scope of the method.
/// </summary>
bool IsReadOnly { get; }

Note that once the last prototype comments are addressed, the check methodSymbol.IsReadOnly will be basically equivalent to methodSymbol.ThisParameter.RefKind == RefKind.In.

[333fred]

@RikkiGibson tests are failing.

[333fred]

I don't see any tests for these APIs. Do we already have them?

[333fred]

Done review pass (commit 1)

[RikkiGibson]

F:\workspace\_work\1\s\src\Compilers\CSharp\Portable\Symbols\Metadata\PE\PEMethodSymbol.cs(1043,32): error CS0507: 'PEMethodSymbol.IsDeclaredReadOnly': cannot change access modifiers when overriding 'public' inherited member 'MethodSymbol.IsDeclaredReadOnly' [F:\workspace\_work\1\s\src\Compilers\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.csproj]

Weird, it seems like this error should have come up right away locally. Will fix.

[RikkiGibson]

I don't see any tests for these APIs. Do we already have them?

The implementations via MethodSymbol are tested already. Maybe I should add one that goes specifically through SemanticModel APIs?

[333fred]

The implementations via MethodSymbol are tested already. Maybe I should add one that goes specifically through SemanticModel APIs?

This would probably be good to test. We have a number of public and internal APIs already that are implemented differently, if we ever change the internal version it'd be good to verify the public ones as well.

[333fred]

LGTM (commit 5) with the namespace change revert and Chuck's suggestion of ExceptionUtilities.Unreachable

[gafter]

I don’t think we need two APIs here. The user should only (usually) care about effective. If they care about the syntax that was used they can look at the syntax. I recommend IsReadonly to tell if it is effectively readonly for any reason. (We don’t have DeclaredPrivate to distinguish a member that has the private modifier, versus only private because it was declred in a class versus an interface) #Resolved

[RikkiGibson]

How does this affect SymbolDisplayVisitor? Should it just always display a readonly keyword for an effectively readonly method?

[RikkiGibson]

This can be deferred until after the feature gets merged, especially because as the PR description notes, "effective readonly" can be checked via methodSymbol.ThisParameter.RefKind

[RikkiGibson]

The public API has been simplified down to IMethodSymbol.IsReadOnly. This won't have a negative impact on symbol display--we've decided that if there a few scenarios where 'readonly' is shown for an "effectively readonly" metadata symbol where the keyword wasn't explicitly provided in source, that's OK.

[gafter]

Please make this API work for reduced extension methods.

[333fred]

LGTM (commit 12)

[RikkiGibson]

@dotnet/roslyn-compiler Could I get a second sign-off please?

[gafter]