Don't check safe-to-escape of receiver arguments to readonly members

[RikkiGibson]

Fixes #35146 but we'll need to create a new issue to track fixing escape analysis on properties where one accessor is readonly and the other is not.

[RikkiGibson]

These tests are a bit messed up--bad names and copy/pastes. I'll fix them up.

[tannergooding]

This is the case you were telling me about, where we can't easily determine which accessor we are using?

So an indexer that takes a Span<T> (for multi-dimensional indexing) won't work with this change and will need to be handled in the future, correct (just talking about the getter, since the setter needs to be mutable in this case)?

[RikkiGibson]

Yeah if it looks like this[Span<T> span] { readonly get; set; } we will be pessimistic and say the span could escape illegally even though we should be able to tell that only the getter is called.

[RikkiGibson]

The purpose of this test is more to explicitly illustrate the existing behavior. Strictly speaking we might be able to exclude such properties from escape analysis but it doesn't seem to present much value.

[gafter]

Is there a proposed update to a specification? Or is this something already specified? Is there something over on csharplang that is a reference for this change?

[tannergooding]

This is functionality that already works for methods of readonly struct. It does not currently work for readonly members of a mutable struct, so I logged #35146

[gafter]

Where would I find a spec for the behavior implemented here? Or the related functionality for readonly struct for that matter?

[tannergooding]

I would expect it to be in either https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.2/span-safety.md or https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.2/readonly-ref.md, if anywhere.

However, I did not immediately see a section detailing a ref struct passed to an instance method of another ref struct and how the escaping rules apply if that instance method is itself considered readonly (which in 7.2 would have only applied to readonly ref struct, but in 8.0 also applies to readonly members of ref struct).

[gafter]

I would prefer a specification for the proposed language change (even if that means also writing spec language for stuff already done, such as readonly structs) to be reviewed before I would feel comfortable reviewing its implementation.

[agocke]

I agree with @gafter. This isn't bug-fix level, we need a spec proposal first.

[RikkiGibson]

It seems like the currently shipped behavior on escape analysis for arguments to readonly struct methods isn't specified in the span-safety or readonly-ref docs. It seems better to put in span-safety.

How about I propose a change to the span safety spec as the next step? Please let me know if there's anything I should look at to onboard to that or if there's some other way this should get done.

[RikkiGibson]

After some closer study of the span safety spec I've identified the following relevant clauses in the Language Constraints section.

In a method invocation, the following constraints apply:

• If there is a ref or out argument of a ref struct type (including the receiver), with safe-to-escape E1, then
• no argument (including the receiver) may have a narrower safe-to-escape than E1.

It seems like these constraints are what's checked by CheckInvocationArgMixing. The spec language excludes ref readonly/in arguments. Since the receiver argument is passed by ref readonly to a readonly method or accessor, there's no need to check its safe-to-escape and this PR brings us closer to adherence to the spec.

I'm going to update the implementation to quote the spec and ask you to take another look.

[gafter]

[333fred]

Talked with @RikkiGibson offline. We think there might be a forward compat scenario where you can call one of these members from a C# 7 assembly in VS 2019, but not in 2017, so he's going to add more tests to verify behavior and adjust as necessary.

[333fred]

Done review pass (commit 5)

[RikkiGibson]

We decided that this is an uncommon enough scenario that we're comfortable asking people to simply update their compiler version to make it work.