Avoid assert in emit layer when intercepted call implicitly captures …

…receiver to temp (#74509)
dotnet · Aug 5, 2024 · 90da6fa · 90da6fa
1 parent db9621c
commit 90da6fa
Show file tree

Hide file tree

Showing 3 changed files with 493 additions and 2 deletions.
diff --git a/docs/features/interceptors.md b/docs/features/interceptors.md
@@ -179,6 +179,37 @@ This allows generator authors to avoid *polluting lookup* with interceptors, hel
 
 We may also want to consider adjusting behavior of `[EditorBrowsable]` to work in the same compilation.
 
+### Struct receiver capture
+
+An interceptor whose `this` parameter takes a struct by-reference can generally be used to intercept a struct instance method call, assuming the methods are compatible per [Signature matching](#signature-matching). This includes situations where the receiver must be implicitly captured to temp before the invocation, even if such capture is not permitted when the interceptor is called directly. See also [12.8.9.3 Extension method invocations](https://github.com/dotnet/csharpstandard/blob/standard-v7/standard/expressions.md#12893-extension-method-invocations) in the standard.
+
+
+```cs
+using System.Runtime.CompilerServices;
+
+struct S
+{
+    public void Original() { }
+}
+
+static class Program
+{
+    public static void Main()
+    {
+        new S().Original(); // L1: interception is valid, no errors.
+        new S().Interceptor(); // error CS1510: A ref or out value must be an assignable variable
+    }
+}
+
+static class D
+{
+    [InterceptsLocation(1, "..(refers to call to 'Original()' at L1)")]
+    public static void Interceptor(this ref S s)
+}
+```
+
+The reason we permit implicit receiver capture for the above intercepted call is: we want intercepting to be possible even when the interceptor author doesn't own the original receiver type. If we didn't do this, then intercepting `Original()` in the above example would only be possible by adding instance members to `struct S`.
+
 ### Editor experience
 
 Interceptors are treated like a post-compilation step in this design. Diagnostics are given for misuse of interceptors, but some diagnostics are only given in the command-line build and not in the IDE. There is limited traceability in the editor for which calls in a compilation are actually being intercepted. If this feature is brought forward past the experimental stage, this limitation will need to be re-examined.

diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
@@ -138,6 +138,7 @@ private void InterceptCallAndAdjustArguments(
             ref BoundExpression? receiverOpt,
             ref ImmutableArray<BoundExpression> arguments,
             ref ImmutableArray<RefKind> argumentRefKindsOpt,
+            ref ArrayBuilder<LocalSymbol> temps,
             bool invokedAsExtensionMethod,
             Syntax.SimpleNameSyntax? nameSyntax)
         {
@@ -281,11 +282,26 @@ private void InterceptCallAndAdjustArguments(
                     || (!receiverOpt.Type.IsReferenceType && interceptor.Parameters[0].Type.IsReferenceType));
                 receiverOpt = MakeConversionNode(receiverOpt, interceptor.Parameters[0].Type, @checked: false, markAsChecked: true);
 
+                var thisRefKind = methodThisParameter.RefKind;
+                // Instance call receivers can be implicitly captured to temps in the emit layer, but not static call arguments
+                // Therefore we may need to explicitly store the receiver to temp here.
+                if (thisRefKind != RefKind.None
+                    && !Binder.HasHome(
+                        receiverOpt,
+                        thisRefKind == RefKind.Ref ? Binder.AddressKind.Writeable : Binder.AddressKind.ReadOnlyStrict,
+                        _factory.CurrentFunction,
+                        peVerifyCompatEnabled: false,
+                        stackLocalsOpt: null))
+                {
+                    var receiverTemp = _factory.StoreToTemp(receiverOpt, out var assignmentToTemp);
+                    temps.Add(receiverTemp.LocalSymbol);
+                    receiverOpt = _factory.Sequence(locals: [], sideEffects: [assignmentToTemp], receiverTemp);
+                }
+
                 arguments = arguments.Insert(0, receiverOpt);
                 receiverOpt = null;
 
                 // CodeGenerator.EmitArguments requires that we have a fully-filled-out argumentRefKindsOpt for any ref/in/out arguments.
-                var thisRefKind = methodThisParameter.RefKind;
                 if (argumentRefKindsOpt.IsDefault && thisRefKind != RefKind.None)
                 {
                     argumentRefKindsOpt = method.Parameters.SelectAsArray(static param => param.RefKind);
@@ -401,7 +417,7 @@ BoundExpression visitArgumentsAndFinishRewrite(BoundCall node, BoundExpression?
                     ref temps,
                     invokedAsExtensionMethod);
 
-                InterceptCallAndAdjustArguments(ref method, ref rewrittenReceiver, ref rewrittenArguments, ref argRefKindsOpt, invokedAsExtensionMethod, node.InterceptableNameSyntax);
+                InterceptCallAndAdjustArguments(ref method, ref rewrittenReceiver, ref rewrittenArguments, ref argRefKindsOpt, ref temps, invokedAsExtensionMethod, node.InterceptableNameSyntax);
 
                 if (Instrument)
                 {