From 19b21fbb9d2079bff97af72c36925fac7fb1efc4 Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Tue, 23 Jul 2024 11:47:23 -0700 Subject: [PATCH 1/2] Avoid assert in emit layer when intercepted call implicitly captures receiver to temp --- docs/features/interceptors.md | 31 ++ .../LocalRewriter/LocalRewriter_Call.cs | 20 +- .../Semantic/Semantics/InterceptorsTests.cs | 329 ++++++++++++++++++ 3 files changed, 378 insertions(+), 2 deletions(-) diff --git a/docs/features/interceptors.md b/docs/features/interceptors.md index f93209462b994..72d340e44641e 100644 --- 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 index 8c1b126d3081f..82fe7099eb900 100644 --- 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 arguments, ref ImmutableArray argumentRefKindsOpt, + ref ArrayBuilder 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) { diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs index 91fa4ec491db2..1ee07d173965c 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs @@ -7513,4 +7513,333 @@ static class Interceptors var method = model.GetInterceptorMethod(node); Assert.Equal($"void Interceptors.M1(this {refKind}S s)", method.ToTestDisplayString()); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void ReceiverCapturedToTemp_StructRvalueReceiver() + { + var source = CSharpTestSource.Parse(""" + using System; + + public struct S + { + void M() => Console.WriteLine(0); + + public static void Main() + { + new S().M(); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Last(); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + public static class C + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this ref S s) => Console.WriteLine(1); + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.Main", """ + { + // Code size 16 (0x10) + .maxstack 1 + .locals init (S V_0) + IL_0000: ldloca.s V_0 + IL_0002: initobj "S" + IL_0008: ldloca.s V_0 + IL_000a: call "void C.M1(ref S)" + IL_000f: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void C.M1(this ref S s)", method.ToTestDisplayString()); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void ReceiverCapturedToTemp_StructInReceiver() + { + // Implicitly capture receiver to temp in 's.M()' because target method needs a writable reference. + var source = CSharpTestSource.Parse(""" + using System; + + public struct S + { + void M() => Console.WriteLine(0); + + public static void Main() + { + M0(new S()); + } + + static void M0(in S s) + { + s.M(); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Last(); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + public static class C + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this ref S s) => Console.WriteLine(1); + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M0", """ + { + // Code size 15 (0xf) + .maxstack 1 + .locals init (S V_0) + IL_0000: ldarg.0 + IL_0001: ldobj "S" + IL_0006: stloc.0 + IL_0007: ldloca.s V_0 + IL_0009: call "void C.M1(ref S)" + IL_000e: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void C.M1(this ref S s)", method.ToTestDisplayString()); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void ReceiverNotCapturedToTemp_StructRefReceiver() + { + var source = CSharpTestSource.Parse(""" + using System; + + public struct S + { + void M() => Console.WriteLine(0); + + public static void Main() + { + S s = default; + M0(ref s); + } + + static void M0(ref S s) + { + s.M(); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Last(); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + public static class C + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this ref S s) => Console.WriteLine(1); + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M0", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "void C.M1(ref S)" + IL_0006: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void C.M1(this ref S s)", method.ToTestDisplayString()); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void ReceiverNotCapturedToTemp_StructReadonlyMethod() + { + var source = CSharpTestSource.Parse(""" + using System; + + public struct S + { + readonly void M() => Console.WriteLine(0); + + public static void Main() + { + M0(new S()); + } + + static void M0(in S s) + { + s.M(); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Last(); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + public static class C + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this in S s) => Console.WriteLine(1); + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M0", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "void C.M1(in S)" + IL_0006: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void C.M1(this in S s)", method.ToTestDisplayString()); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void ReceiverNotCapturedToTemp_StructLvalueReceiver() + { + var source = CSharpTestSource.Parse(""" + using System; + + public struct S + { + void M() => Console.WriteLine(0); + + public static void Main() + { + M0(new S()); + } + + static void M0(S s) + { + s.M(); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Last(); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + public static class C + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this ref S s) => Console.WriteLine(1); + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.M0", """ + { + // Code size 8 (0x8) + .maxstack 1 + IL_0000: ldarga.s V_0 + IL_0002: call "void C.M1(ref S)" + IL_0007: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void C.M1(this ref S s)", method.ToTestDisplayString()); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void ReceiverNotCapturedToTemp_ByValueParameter() + { + var source = CSharpTestSource.Parse(""" + using System; + + public class C + { + void M() => Console.WriteLine(0); + + public static void Main() + { + new C().M(); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Last(); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + public static class SC + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this C c) => Console.WriteLine(1); + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "1"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("C.Main", """ + { + // Code size 11 (0xb) + .maxstack 1 + IL_0000: newobj "C..ctor()" + IL_0005: call "void SC.M1(C)" + IL_000a: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void SC.M1(this C c)", method.ToTestDisplayString()); + } } From 1ef090fcf7d7e2565d786455e833f15b97fdbcfb Mon Sep 17 00:00:00 2001 From: Rikki Gibson Date: Mon, 29 Jul 2024 15:12:32 -0700 Subject: [PATCH 2/2] Add suggested tests --- .../Semantic/Semantics/InterceptorsTests.cs | 115 ++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs index 1ee07d173965c..41187c8c9f6de 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs @@ -7842,4 +7842,119 @@ .maxstack 1 var method = model.GetInterceptorMethod(node); Assert.Equal("void SC.M1(this C c)", method.ToTestDisplayString()); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void Receiver_RefReturn_NotCapturedToTemp() + { + var source = CSharpTestSource.Parse(""" + using System; + + public struct S + { + public int F; + + static S s; + static ref S RS() => ref s; + + void M() => throw null!; + + public static void Main() + { + RS().F = 1; + Console.Write(RS().F); + RS().M(); + Console.Write(RS().F); + } + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source); + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Single(i => i.ToString() == "RS().M()"); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + public static class SC + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static void M1(this ref S s) => s.F = 2; + } + """, "Interceptors.cs", RegularWithInterceptors); + + var verifier = CompileAndVerify([source, interceptors, s_attributesTree], expectedOutput: "12"); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("S.Main", """ + { + // Code size 52 (0x34) + .maxstack 2 + IL_0000: call "ref S S.RS()" + IL_0005: ldc.i4.1 + IL_0006: stfld "int S.F" + IL_000b: call "ref S S.RS()" + IL_0010: ldfld "int S.F" + IL_0015: call "void System.Console.Write(int)" + IL_001a: call "ref S S.RS()" + IL_001f: call "void SC.M1(ref S)" + IL_0024: call "ref S S.RS()" + IL_0029: ldfld "int S.F" + IL_002e: call "void System.Console.Write(int)" + IL_0033: ret + } + """); + + comp = (CSharpCompilation)verifier.Compilation; + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + Assert.Equal("void SC.M1(this ref S s)", method.ToTestDisplayString()); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/71657")] + public void CannotReturnRefToImplicitTemp() + { + var source = CSharpTestSource.Parse(""" + using System; + using System.Diagnostics.CodeAnalysis; + + public ref struct S + { + static Span Test() + { + return new S().M(); + } + + [UnscopedRef] + public Span M() => default; + } + """, "Program.cs", RegularWithInterceptors); + + var comp = CreateCompilation(source, targetFramework: TargetFramework.Net90); + comp.VerifyEmitDiagnostics( + // Program.cs(8,16): error CS8156: An expression cannot be used in this context because it may not be passed or returned by reference + // return new S().M(); + Diagnostic(ErrorCode.ERR_RefReturnLvalueExpected, "new S()").WithLocation(8, 16)); + + var model = comp.GetSemanticModel(source); + var node = source.GetRoot().DescendantNodes().OfType().Single(i => i.ToString() == "new S().M()"); + var locationSpecifier = model.GetInterceptableLocation(node)!; + + var interceptors = CSharpTestSource.Parse($$""" + using System; + + static class D + { + {{locationSpecifier.GetInterceptsLocationAttributeSyntax()}} + public static Span M(this ref S s) => default; + } + """, "Interceptors.cs", RegularWithInterceptors); + + comp = CreateCompilation([source, interceptors, s_attributesTree], targetFramework: TargetFramework.Net90); + comp.VerifyEmitDiagnostics( + // Program.cs(8,16): error CS8156: An expression cannot be used in this context because it may not be passed or returned by reference + // return new S().M(); + Diagnostic(ErrorCode.ERR_RefReturnLvalueExpected, "new S()").WithLocation(8, 16)); + + model = comp.GetSemanticModel(source); + var method = model.GetInterceptorMethod(node); + AssertEx.Equal("System.Span D.M(this ref S s)", method.ToTestDisplayString()); + } }