diff --git a/docs/features/interceptors.md b/docs/features/interceptors.md index 6335788b1fc33..675eb8642ac1d 100644 --- a/docs/features/interceptors.md +++ b/docs/features/interceptors.md @@ -19,7 +19,6 @@ c.InterceptableMethod(1); // prints "interceptable 1" class C { - [Interceptable] public void InterceptableMethod(int param) { Console.WriteLine($"interceptable {param}"); @@ -47,49 +46,6 @@ static class D ## Detailed design [design]: #detailed-design -### InterceptableAttribute - -A method can indicate that its calls can be *intercepted* by including `[Interceptable]` on its declaration. - -PROTOTYPE(ic): For now, if a call is intercepted to a method which lacks this attribute, a warning is reported, and interception still occurs. This may be changed to an error in the future. - -```cs -namespace System.Runtime.CompilerServices -{ - [AttributeUsage(AttributeTargets.Method)] - public sealed class InterceptableAttribute : Attribute { } -} -``` - -#### PROTOTYPE(ic): Use an assembly attribute instead? -The main reason we want something like `[Interceptable]` is so that users can better reason about *which calls might be getting intercepted or not*. We don't think there's a meaningful *security* concern which is addressed by `[Interceptable]`--if something untrusted has the ability to add source files to your build, taking away the ability for them to intercept certain calls doesn't really make a difference. - -It's been suggested that `[Interceptable]` on method declarations is the wrong point of control. Some SG authors will have good reason to want to intercept a call to a method from a library they don't own--for example, EF wanting to intercept calls to IQueryable extensions. It shouldn't be necessary for those authors to have to request the library authors to add an attribute before they can start doing it. - -One option to address this is to remove `[Interceptable]` from the design and not provide any mechanism for limiting which calls can be intercepted or not. - -Another option would be to expose an assembly attribute instead. - -```cs -namespace System.Runtime.CompilerServices -{ - /// If present, indicates the set of types whose methods can have their calls intercepted in the current compilation. - /// When this attribute is not present, methods on any type can have their calls intercepted. - [AttributeUsage(AttributeTargets.Assembly, AllowMultiple = false)] - public sealed class AllowInterception(params Type[] types) : Attribute { } -} - -[assembly: AllowInterception(typeof(EndpointRouteBuilderExtensions))] -``` - -In order to intercept a classic extension method call, the extension type must be provided to this attribute, not the `this` parameter type. - -We specify `AllowMultiple = false` here to imply that the attribute is supposed to be hand-authored by the user. It's not meant for generators to use this to *opt themselves in* to being able to intercept things--it's for the user to state once and comprehensively which things can be intercepted. - -In such a scheme, it's not clear how often users would actually want to go to the trouble of writing the attribute--particularly since the process of writing and maintaining it would simply be to run source generators, review the "not interceptable" errors, and add the related types to this attribute. Is that useful to users in practice? - -Another alternative here might be to use `AllowMultiple = true`, require in practice that generators produce these attributes (i.e. disallow intercepting in the current compilation if this attribute isn't used in it), and let users search for usages of the attribute to get a sense of which calls may be getting intercepted in their project. - ### InterceptsLocationAttribute A method indicates that it is an *interceptor* by adding one or more `[InterceptsLocation]` attributes. These attributes refer to the source locations of the calls it intercepts. @@ -104,10 +60,12 @@ namespace System.Runtime.CompilerServices } ``` +Any "ordinary method" (i.e. with `MethodKind.Ordinary`) can have its calls intercepted. + `[InterceptsLocation]` attributes included in source are emitted to the resulting assembly, just like other custom attributes. PROTOTYPE(ic): We may want to recognize `file class InterceptsLocationAttribute` as a valid declaration of the attribute, to allow generators to bring the attribute in without conflicting with other generators which may also be bringing the attribute in. See open question in [User opt-in](#user-opt-in). -https://github.com/dotnet/roslyn/issues/67079 is a bug which causes file-local source declarations of well-known attributes to be generally treated as known. When that bug is fixed, we may want to single out one or both of `InterceptableAttribute` and `InterceptsLocationAttribute` as "recognized, even though they are file-local". +https://github.com/dotnet/roslyn/issues/67079 is a bug which causes file-local source declarations of well-known attributes to be generally treated as known. When that bug is fixed, we may want to single out `InterceptsLocationAttribute` as "recognized, even though they are file-local". #### File paths @@ -146,7 +104,6 @@ using System.Runtime.CompilerServices; class C { - [Interceptable] public static void InterceptableMethod(T1 t) => throw null!; } @@ -160,7 +117,7 @@ static class Program static class D { - [InterceptsLocation("Program.cs", 13, 11)] + [InterceptsLocation("Program.cs", 12, 11)] public static void Interceptor1(object s) => throw null!; } ``` diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs index a3e3da05595f0..9d164bd43ab0f 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs @@ -154,13 +154,6 @@ private void InterceptCallAndAdjustArguments( Debug.Assert(interceptableLocation != null); Debug.Assert(interceptor.Arity == 0); - if (!method.IsInterceptable) - { - // PROTOTYPE(ic): Eventually we may want this to be an error. - // For now it's convenient to just warn so we can experiment with intercepting APIs that haven't yet been marked. - this._diagnostics.Add(ErrorCode.WRN_CallNotInterceptable, attributeLocation, method); - } - var containingMethod = this._factory.CurrentFunction; Debug.Assert(containingMethod is not null); diff --git a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs index 9b2b2d905ec96..48e7d6d79c23b 100644 --- a/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs +++ b/src/Compilers/CSharp/Portable/Symbols/Source/SourceMethodSymbolWithAttributes.cs @@ -597,14 +597,6 @@ private void DecodeWellKnownAttributeAppliedToMethod(ref DecodeWellKnownAttribut diagnostics.Add(ErrorCode.ERR_UnscopedRefAttributeUnsupportedMemberTarget, arguments.AttributeSyntaxOpt.Location); } } - else if (attribute.IsTargetAttribute(this, AttributeDescription.InterceptableAttribute)) - { - if (MethodKind != MethodKind.Ordinary) - { - diagnostics.Add(ErrorCode.ERR_InterceptableMethodMustBeOrdinary, arguments.AttributeSyntaxOpt.Location); - } - arguments.GetOrCreateData().HasInterceptableAttribute = true; - } else if (attribute.IsTargetAttribute(this, AttributeDescription.InterceptsLocationAttribute)) { DecodeInterceptsLocationAttribute(arguments); diff --git a/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs b/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs index a3b7f061d8674..00c14652a8532 100644 --- a/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs +++ b/src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs @@ -26,37 +26,6 @@ public InterceptsLocationAttribute(string filePath, int line, int character) } """, "attributes.cs"); - [Fact] - public void IsInterceptable() - { - var source = """ - using System.Runtime.CompilerServices; - using System; - - class C - { - - public static void InterceptableMethod() { Console.Write("interceptable"); } - - public static void NotInterceptable() { Console.Write("not interceptable"); } - } - """; - - var verifier = CompileAndVerify(new[] { (source, "Program.cs"), s_attributesSource }, symbolValidator: verify, sourceSymbolValidator: verify); - verifier.VerifyDiagnostics(); - - void verify(ModuleSymbol module) - { - var method = module.GlobalNamespace.GetMember("C.InterceptableMethod"); - Assert.True(method.IsInterceptable); - Assert.Equal(MethodKind.Ordinary, method.MethodKind); - - method = module.GlobalNamespace.GetMember("C.NotInterceptable"); - Assert.False(method.IsInterceptable); - Assert.Equal(MethodKind.Ordinary, method.MethodKind); - } - } - [Fact] public void SelfInterception() { @@ -144,7 +113,6 @@ public void Accessibility_02() { // An interceptor declared within a file-local type can intercept a call even if the call site can't normally refer to the file-local type. var source1 = """ - using System.Runtime.CompilerServices; using System; class C @@ -165,7 +133,7 @@ public static void Main() file class D { - [InterceptsLocation("Program.cs", 11, 9)] + [InterceptsLocation("Program.cs", 10, 9)] public static void Interceptor1() { Console.Write("interceptor 1"); } } """; @@ -177,6 +145,7 @@ file class D [Fact] public void FileLocalAttributeDefinitions_01() { + // Treat a file-local declaration of InterceptsLocationAttribute as a well-known attribute within the declaring compilation. var source = """ using System; using System.Runtime.CompilerServices; @@ -185,7 +154,6 @@ public void FileLocalAttributeDefinitions_01() class C { - public static void M() => throw null!; } @@ -200,7 +168,6 @@ public static void M() namespace System.Runtime.CompilerServices { - file class InterceptableAttribute : Attribute { } file class InterceptsLocationAttribute : Attribute { public InterceptsLocationAttribute(string filePath, int line, int character) { } @@ -212,65 +179,62 @@ public InterceptsLocationAttribute(string filePath, int line, int character) { } verifier.VerifyDiagnostics(); } + /// + /// File-local InterceptsLocationAttribute from another compilation is not considered to *duplicate* an interception, even if it is inherited. + /// See also . + /// [Fact] public void FileLocalAttributeDefinitions_02() { - var source0 = """ + var source1 = """ using System.Runtime.CompilerServices; + using System; + + var c = new C(); + c.M(); public class C { + public void M() => Console.Write(1); - public static void M() => throw null!; + [InterceptsLocation("Program.cs", 5, 3)] + public virtual void Interceptor() => throw null!; } - + namespace System.Runtime.CompilerServices { - file class InterceptableAttribute : Attribute { } + [AttributeUsage(AttributeTargets.Method, AllowMultiple = true, Inherited = true)] + file sealed class InterceptsLocationAttribute : Attribute + { + public InterceptsLocationAttribute(string filePath, int line, int character) + { + } + } } """; - var source1 = """ - using System; - using System.Runtime.CompilerServices; + // Inherited attribute on 'override void Interceptor' from other compilation doesn't cause a call in this compilation to be intercepted. + var source2 = """ - C.M(); - static class D - { - [InterceptsLocation("File1.cs", 4, 3)] - public static void M() - { - Console.Write(1); - } - } + // leading blank lines for alignment with the call in the other compilation. + var d = new D(); + d.M(); - namespace System.Runtime.CompilerServices + class D : C { - file class InterceptsLocationAttribute : Attribute - { - public InterceptsLocationAttribute(string filePath, int line, int character) { } - } + public override void Interceptor() => throw null!; } """; - var verifier = CompileAndVerify(new[] { (source0, "File0.cs"), (source1, "File1.cs") }, expectedOutput: "1"); - verifier.VerifyDiagnostics(); - - var comp0 = CreateCompilation((source0, "File0.cs")); - comp0.VerifyEmitDiagnostics(); + var comp1 = CreateCompilation((source1, "Program.cs")); + comp1.VerifyEmitDiagnostics(); - var verifier1 = CompileAndVerify((source1, "File1.cs"), new[] { comp0.ToMetadataReference() }, expectedOutput: "1"); - verifier1.VerifyDiagnostics(); + var comp2Verifier = CompileAndVerify((source2, "Program.cs"), references: new[] { comp1.ToMetadataReference() }, expectedOutput: "1"); + comp2Verifier.VerifyDiagnostics(); - // PROTOTYPE(ic): https://github.com/dotnet/roslyn/issues/67079 - // We are generally treating file-local definitions in source as matching the names of well-known attributes. - // Once the type is emitted to metadata and read back in, we no longer recognize it as the same attribute due to name mangling. - var verifier1_1 = CompileAndVerify((source1, "File1.cs"), new[] { comp0.EmitToImageReference() }, expectedOutput: "1"); - verifier1_1.VerifyDiagnostics( - // File1.cs(8,6): warning CS27000: Call to 'C.M()' is intercepted, but the method is not marked with 'System.Runtime.CompilerServices.InterceptableAttribute'. - // [InterceptsLocation("File1.cs", 4, 3)] - Diagnostic(ErrorCode.WRN_CallNotInterceptable, @"InterceptsLocation(""File1.cs"", 4, 3)").WithArguments("C.M()").WithLocation(8, 6)); + comp2Verifier = CompileAndVerify((source2, "Program.cs"), references: new[] { comp1.EmitToImageReference() }, expectedOutput: "1"); + comp2Verifier.VerifyDiagnostics(); } [Fact] @@ -898,7 +862,7 @@ public static class D comp0.VerifyEmitDiagnostics(); var source1 = """ - using System.Runtime.CompilerServices; + using System; class C1 @@ -1087,11 +1051,7 @@ static class D } """; var verifier = CompileAndVerify(new[] { (source, "Program.cs"), s_attributesSource }, expectedOutput: "12"); - // PROTOTYPE(ic): drop warning when InterceptableAttribute is removed from the design. verifier.VerifyDiagnostics( - // Program.cs(16,6): warning CS27000: Call to 'Action.Invoke()' is intercepted, but the method is not marked with 'System.Runtime.CompilerServices.InterceptableAttribute'. - // [InterceptsLocation("Program.cs", 10, 9)] - Diagnostic(ErrorCode.WRN_CallNotInterceptable, @"InterceptsLocation(""Program.cs"", 10, 9)").WithArguments("System.Action.Invoke()").WithLocation(16, 6) ); } @@ -1445,7 +1405,7 @@ static class D public void InterceptableFromMetadata() { var source1 = """ - using System.Runtime.CompilerServices; + using System; public class C @@ -1525,84 +1485,6 @@ public static string Prop ); } - [Fact] - public void Interceptable_BadMethodKind() - { - var source = """ - using System.Runtime.CompilerServices; - using System; - - interface I1 { } - class C : I1 { } - - static class Program - { - public static void Main() - { - var lambda = [Interceptable] (string param) => { }; // 1 - - InterceptableMethod("call site"); - - [Interceptable] // 2 - static void InterceptableMethod(string param) { Console.Write("interceptable " + param); } - } - - public static string Prop - { - [Interceptable] // 3 - set { } - } - } - """; - var comp = CreateCompilation(new[] { (source, "Program.cs"), s_attributesSource }); - comp.VerifyDiagnostics( - // Program.cs(11,23): error CS27008: An interceptable method must be an ordinary member method. - // var lambda = [Interceptable] (string param) => { }; // 1 - Diagnostic(ErrorCode.ERR_InterceptableMethodMustBeOrdinary, "Interceptable").WithLocation(11, 23), - // Program.cs(15,10): error CS27008: An interceptable method must be an ordinary member method. - // [Interceptable] // 2 - Diagnostic(ErrorCode.ERR_InterceptableMethodMustBeOrdinary, "Interceptable").WithLocation(15, 10), - // Program.cs(21,10): error CS27008: An interceptable method must be an ordinary member method. - // [Interceptable] // 3 - Diagnostic(ErrorCode.ERR_InterceptableMethodMustBeOrdinary, "Interceptable").WithLocation(21, 10) - ); - } - - [Fact] - public void CallNotInterceptable() - { - var source = """ - using System.Runtime.CompilerServices; - using System; - - class C - { - public C InterceptableMethod(string param) { Console.Write("interceptable " + param); return this; } - } - - static class Program - { - public static void Main() - { - var c = new C(); - c.InterceptableMethod("call site"); - } - } - - static class D - { - [InterceptsLocation("Program.cs", 14, 11)] - public static C Interceptor1(this C c, string param) { Console.Write("interceptor " + param); return c; } - } - """; - var verifier = CompileAndVerify(new[] { (source, "Program.cs"), s_attributesSource }, expectedOutput: "interceptor call site"); - verifier.VerifyDiagnostics( - // Program.cs(20,6): warning CS27000: Call to 'C.InterceptableMethod(string)' is intercepted, but the method is not marked with 'System.Runtime.CompilerServices.InterceptableAttribute'. - // [InterceptsLocation("Program.cs", 14, 11)] - Diagnostic(ErrorCode.WRN_CallNotInterceptable, @"InterceptsLocation(""Program.cs"", 14, 11)").WithArguments("C.InterceptableMethod(string)").WithLocation(20, 6) - ); - } - [Fact] public void InterceptorCannotBeGeneric_01() { @@ -3397,42 +3279,6 @@ class D verifier.VerifyDiagnostics(); } - [Fact] - public void InterceptableExplicitImplementation() - { - var source = """ - using System.Runtime.CompilerServices; - using System; - - var c = new C(); - ((I)c).M(); - - interface I - { - void M(); - } - - class C : I - { - [Interceptable] // 1 - void I.M() => throw null!; - } - - static class D - { - [InterceptsLocation("Program.cs", 5, 8)] - public static void Interceptor(this I i) => throw null!; - } - """; - - var comp = CreateCompilation(new[] { (source, "Program.cs"), s_attributesSource }); - comp.VerifyEmitDiagnostics( - // Program.cs(14,6): error CS27008: An interceptable method must be an ordinary member method. - // [Interceptable] // 1 - Diagnostic(ErrorCode.ERR_InterceptableMethodMustBeOrdinary, "Interceptable").WithLocation(14, 6) - ); - } - [Fact] public void InterceptorExtern() {