Interceptors: handle duplicates, additional signature validation, etc.

docs/features/interceptors.md

@@ -78,6 +78,7 @@ namespace System.Runtime.CompilerServices
 `[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".
 
 #### File paths
 
@@ -111,17 +112,17 @@ Interceptors cannot have type parameters or be declared in generic types at any
 
 ### Signature matching
 
-PROTOTYPE(ic): It is suggested to permit nullability differences and other comparable differences. Perhaps we can revisit the matching requirements of "partial methods" and imitate them here.
-
 When a call is intercepted, the interceptor and interceptable methods must meet the signature matching requirements detailed below:
 - When an interceptable instance method is compared to a classic extension method, we use the extension method in reduced form for comparison. The extension method parameter with the `this` modifier is compared to the instance method `this` parameter.
-- The returns and parameters, including the `this` parameter, must have the same ref kinds and types, except that reference types with oblivious nullability can match either annotated or unannotated reference types.
+- The returns and parameters, including the `this` parameter, must have the same ref kinds and types.
+- A warning is reported instead of an error if a type difference is found where the types are not distinct to the runtime. For example, `object` and `dynamic`.
+- No warning or error is reported for a *safe* nullability difference, such as when the interceptable method accepts a `string` parameter, and the interceptor accepts a `string?` parameter.
 - Method names and parameter names are not required to match.
 - Parameter default values are not required to match. When intercepting, default values on the interceptor method are ignored.
 - `params` modifiers are not required to match.
 - `scoped` modifiers and `[UnscopedRef]` must be equivalent.
 - In general, attributes which normally affect the behavior of the call site, such as `[CallerLineNumber]` are ignored on the interceptor of an intercepted call.
-  - The only exception to this is when the attribute affects "capabilities" of the method in a way that affects safety, such as with `[UnscopedRef]`. In this case, attributes are required to match across interceptable and interceptor methods.
+  - The only exception to this is when the attribute affects "capabilities" of the method in a way that affects safety, such as with `[UnscopedRef]`. Such attributes are required to match across interceptable and interceptor methods.
 
 Arity does not need to match between intercepted and interceptor methods. In other words, it is permitted to intercept a generic method with a non-generic interceptor.
 
@@ -133,7 +134,7 @@ If an `[InterceptsLocation]` attribute is found in the compilation which does no
 
 ### Interceptor accessibility
 
-An interceptor must be accessible at the location where interception is occurring. PROTOTYPE(ic): This enforcement is not yet implemented.
+An interceptor must be accessible at the location where interception is occurring.
 
 An interceptor contained in a file-local type is permitted to intercept a call in another file, even though the interceptor is not normally *visible* at the call site.
 
@@ -157,7 +158,6 @@ During the binding phase, `InterceptsLocationAttribute` usages are decoded and t
 - intercepted file-path and location
 - attribute location
 - attributed method symbol
-PROTOTYPE(ic): the exact collection used to collect the attribute usages, and the exact way it is used, are not finalized. The main concern is to ensure we can scale to large numbers of interceptors without issue, and that we can report diagnostics for duplicate interception of the same location in a deterministic way.
 
 At this time, diagnostics are reported for the following conditions:
 - problems specific to the attributed interceptor method itself, for example, that it is not an ordinary method.
@@ -172,4 +172,5 @@ During the lowering phase, when a given `BoundCall` is lowered:
 
 At this time, diagnostics are reported for the following conditions:
 - incompatibility between the interceptor and interceptable methods, for example, in their signatures.
-- *duplicate* `[InterceptsLocation]`, that is, multiple interceptors which intercept the same call. PROTOTYPE(ic): not yet implemented.
+- *duplicate* `[InterceptsLocation]`, that is, multiple interceptors which intercept the same call.
+- interceptor is not accessible at the call site.

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -7523,15 +7523,24 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_InterceptorCharacterOutOfRange" xml:space="preserve">
     <value>The given line is '{0}' characters long, which is fewer than the provided character number '{1}'.</value>
   </data>
+  <data name="ERR_InterceptorLineCharacterMustBePositive" xml:space="preserve">
+    <value>Line and character numbers provided to InterceptsLocationAttribute must be positive.</value>
+  </data>
   <data name="ERR_InterceptorPositionBadToken" xml:space="preserve">
     <value>The provided line and character number does not refer to an interceptable method name, but rather to token '{0}'.</value>
   </data>
   <data name="ERR_InterceptorMustReferToStartOfTokenPosition" xml:space="preserve">
-    <value>The provided character number does not refer to the start of method name token '{0}'. Consider using character number '{1}' instead.</value>
+    <value>The provided line and character number does not refer to the start of token '{0}'. Did you mean to use line '{1}' and character '{2}'?</value>
   </data>
   <data name="ERR_InterceptorSignatureMismatch" xml:space="preserve">
     <value>Cannot intercept method '{0}' with interceptor '{1}' because the signatures do not match.</value>
   </data>
+  <data name="WRN_InterceptorSignatureMismatch" xml:space="preserve">
+    <value>Intercepting a call to '{0}' with interceptor '{1}', but the signatures do not match.</value>
+  </data>
+  <data name="WRN_InterceptorSignatureMismatch_Title" xml:space="preserve">
+    <value>Signatures of interceptable and interceptor methods do not match.</value>
+  </data>
   <data name="ERR_InterceptableMethodMustBeOrdinary" xml:space="preserve">
     <value>An interceptable method must be an ordinary member method.</value>
   </data>
@@ -7553,10 +7562,31 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
   <data name="ERR_InterceptorNonUniquePath" xml:space="preserve">
     <value>Cannot intercept a call in file with path '{0}' because multiple files in the compilation have this path.</value>
   </data>
+  <data name="ERR_DuplicateInterceptor" xml:space="preserve">
+    <value>The indicated call is intercepted multiple times.</value>
+  </data>
+  <data name="ERR_InterceptorNotAccessible" xml:space="preserve">
+    <value>Cannot intercept call with '{0}' because it is not accessible within '{1}'.</value>
+  </data>
+  <data name="ERR_InterceptorScopedMismatch" xml:space="preserve">
+    <value>Cannot intercept call to '{0}' with '{1}' because of a difference in 'scoped' modifiers or '[UnscopedRef]' attributes.</value>
+  </data>
   <data name="ERR_ConstantValueOfTypeExpected" xml:space="preserve">
     <value>A constant value of type '{0}' is expected</value>
   </data>
   <data name="ERR_UnsupportedPrimaryConstructorParameterCapturingRefAny" xml:space="preserve">
     <value>Cannot use primary constructor parameter of type '{0}' inside an instance member</value>
   </data>
+  <data name="WRN_NullabilityMismatchInParameterTypeOnInterceptor" xml:space="preserve">
+    <value>Nullability of reference types in type of parameter '{0}' doesn't match interceptable method '{1}'.</value>
+  </data>
+  <data name="WRN_NullabilityMismatchInParameterTypeOnInterceptor_Title" xml:space="preserve">
+    <value>Nullability of reference types in type of parameter doesn't match interceptable method.</value>
+  </data>
+  <data name="WRN_NullabilityMismatchInReturnTypeOnInterceptor" xml:space="preserve">
+    <value>Nullability of reference types in return type doesn't match interceptable method '{0}'.</value>
+  </data>
+  <data name="WRN_NullabilityMismatchInReturnTypeOnInterceptor_Title" xml:space="preserve">
+    <value>Nullability of reference types in return type doesn't match interceptable method.</value>
+  </data>
 </root>

src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs

@@ -2247,44 +2247,61 @@ internal void AddModuleInitializerMethod(MethodSymbol method)
             LazyInitializer.EnsureInitialized(ref _moduleInitializerMethods).Add(method);
         }
 
-        private ConcurrentSet<(InterceptsLocationAttributeData, MethodSymbol)>? _interceptions;
+        // NB: the 'Many' case for these dictionary values means there are duplicates. An error is reported for this after binding.
+        private ConcurrentDictionary<(string FilePath, int Line, int Character), OneOrMany<(Location AttributeLocation, MethodSymbol Interceptor)>>? _interceptions;
 
-        internal void AddInterception(InterceptsLocationAttributeData location, MethodSymbol interceptor)
+        internal void AddInterception(string filePath, int line, int character, Location attributeLocation, MethodSymbol interceptor)
         {
             Debug.Assert(!_declarationDiagnosticsFrozen);
-            LazyInitializer.EnsureInitialized(ref _interceptions).Add((location, interceptor));
+
+            var dictionary = LazyInitializer.EnsureInitialized(ref _interceptions);
+            dictionary.AddOrUpdate((filePath, line, character),
+                addValueFactory: static (key, newValue) => OneOrMany.Create(newValue),
+                updateValueFactory: static (key, existingValues, newValue) =>
+                {
+                    // AddInterception can be called when attributes are decoded on a symbol, which can happen for the same symbol concurrently.
+                    // If something else has already added the interceptor denoted by a given `[InterceptsLocation]`, we want to drop it.
+                    // Since the collection is almost always length 1, a simple foreach is adequate for detecting this.
+                    foreach (var (attributeLocation, interceptor) in existingValues)
+                    {
+                        if (attributeLocation == newValue.AttributeLocation && interceptor.Equals(newValue.Interceptor, TypeCompareKind.ConsiderEverything))
+                        {
+                            return existingValues;
+                        }
+                    }
+                    return existingValues.Add(newValue);
+                },
+                // Explicit tuple element names are needed here so that the names unify when this is an extension method call (netstandard2.0).
+                factoryArgument: (AttributeLocation: attributeLocation, Interceptor: interceptor));
         }
 
-        internal (InterceptsLocationAttributeData data, MethodSymbol interceptor)? GetInterceptor(Location? callLocation)
+        internal (Location AttributeLocation, MethodSymbol Interceptor)? TryGetInterceptor(Location? callLocation, BindingDiagnosticBag diagnostics)
         {
             if (_interceptions is null || callLocation is null)
             {
                 return null;
             }
 
-            var sourceTree = callLocation.SourceTree;
-            Debug.Assert(sourceTree is not null);
             var callLineColumn = callLocation.GetLineSpan().Span.Start;
-            foreach (var (interceptsLocation, interceptor) in _interceptions)
+            Debug.Assert(callLocation.SourceTree is not null);
+            var key = (callLocation.SourceTree.FilePath, callLineColumn.Line, callLineColumn.Character);
+
+            if (_interceptions.TryGetValue(key, out var interceptionsAtAGivenLocation))
             {
-                if (interceptsLocation.FilePath == sourceTree.FilePath
-                    && interceptsLocation.Line == callLineColumn.Line
-                    && interceptsLocation.Character == callLineColumn.Character)
+                if (interceptionsAtAGivenLocation is [var oneInterception])
                 {
-                    return (interceptsLocation, interceptor);
+                    return oneInterception;
                 }
+
+                // We don't normally reach this branch in batch compilation.
+                // One scenario where we may reach this is when validating used assemblies, which performs lowering of method bodies even if declaration errors would be reported.
+                // See 'CSharpCompilation.GetCompleteSetOfUsedAssemblies'.
+                diagnostics.Add(ErrorCode.ERR_ModuleEmitFailure, callLocation, this.SourceModule.Name, new LocalizableResourceString(nameof(CSharpResources.ERR_DuplicateInterceptor), CodeAnalysisResources.ResourceManager, typeof(CodeAnalysisResources)));
             }
 
             return null;
         }
 
-        private void BuildInterceptionsMap()
-        {
-            // PROTOTYPE(ic): build a map where we can quickly lookup with a location and get a symbol.
-            // At this time, should report any duplicate interception diagnostics.
-            // NB: the attribute which appears lexically first wins a tie. Subsequent attributes referring to same location result in errors.
-        }
-
         #endregion
 
         #region Binding
@@ -3244,6 +3261,8 @@ internal override bool CompileMethods(
             bool hasDeclarationErrors = !FilterAndAppendDiagnostics(diagnostics, GetDiagnostics(CompilationStage.Declare, true, cancellationToken), excludeDiagnostics, cancellationToken);
             excludeDiagnostics?.Free();
 
+            hasDeclarationErrors |= CheckDuplicateInterceptions(diagnostics);
+
             // TODO (tomat): NoPIA:
             // EmbeddedSymbolManager.MarkAllDeferredSymbolsAsReferenced(this)
 
@@ -3275,8 +3294,6 @@ internal override bool CompileMethods(
                     return false;
                 }
 
-                BuildInterceptionsMap();
-
                 // Perform initial bind of method bodies in spite of earlier errors. This is the same
                 // behavior as when calling GetDiagnostics()
 
@@ -3379,12 +3396,40 @@ public override void VisitNamedType(NamedTypeSymbol symbol)
             }
         }
 
+        /// <returns><see langword="true"/> if file types are present in files with duplicate file paths. Otherwise, <see langword="false" />.</returns>
         private bool CheckDuplicateFilePaths(DiagnosticBag diagnostics)
         {
             var visitor = new DuplicateFilePathsVisitor(diagnostics);
             return visitor.CheckDuplicateFilePathsAndFree(SyntaxTrees, GlobalNamespace);
         }
 
+        /// <returns><see langword="true"/> if duplicate interceptors are present in the compilation. Otherwise, <see langword="false" />.</returns>
+        private bool CheckDuplicateInterceptions(DiagnosticBag diagnostics)
+        {
+            if (_interceptions is null)
+            {
+                return false;
+            }
+
+            bool anyDuplicates = false;
+            foreach ((_, OneOrMany<(Location, MethodSymbol)> interceptionsOfAGivenLocation) in _interceptions)
+            {
+                Debug.Assert(interceptionsOfAGivenLocation.Count != 0);
+                if (interceptionsOfAGivenLocation.Count == 1)
+                {
+                    continue;
+                }
+
+                anyDuplicates = true;
+                foreach (var (attributeLocation, _) in interceptionsOfAGivenLocation)
+                {
+                    diagnostics.Add(ErrorCode.ERR_DuplicateInterceptor, attributeLocation);
+                }
+            }
+
+            return anyDuplicates;
+        }
+
         private void GenerateModuleInitializer(PEModuleBuilder moduleBeingBuilt, DiagnosticBag methodBodyDiagnosticBag)
         {
             Debug.Assert(_declarationDiagnosticsFrozen);

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -2209,6 +2209,14 @@ internal enum ErrorCode
         ERR_InterceptorFilePathCannotBeNull = 27013,
         ERR_InterceptorNameNotInvoked = 27014,
         ERR_InterceptorNonUniquePath = 27015,
+        ERR_DuplicateInterceptor = 27016,
+        WRN_InterceptorSignatureMismatch = 27017,
+        ERR_InterceptorNotAccessible = 27018,
+        ERR_InterceptorScopedMismatch = 27019,
+        ERR_InterceptorLineCharacterMustBePositive = 27020,
+        WRN_NullabilityMismatchInReturnTypeOnInterceptor = 27021,
+        WRN_NullabilityMismatchInParameterTypeOnInterceptor = 27022,
+
         #endregion
 
         // Note: you will need to do the following after adding warnings:

src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs

@@ -80,6 +80,8 @@ static ErrorFacts()
             nullableWarnings.Add(GetId(ErrorCode.WRN_ParameterDisallowsNull));
             nullableWarnings.Add(GetId(ErrorCode.WRN_ParameterNotNullIfNotNull));
             nullableWarnings.Add(GetId(ErrorCode.WRN_ReturnNotNullIfNotNull));
+            nullableWarnings.Add(GetId(ErrorCode.WRN_NullabilityMismatchInReturnTypeOnInterceptor));
+            nullableWarnings.Add(GetId(ErrorCode.WRN_NullabilityMismatchInParameterTypeOnInterceptor));
 
             NullableWarnings = nullableWarnings.ToImmutable();
         }
@@ -531,6 +533,9 @@ internal static int GetWarningLevel(ErrorCode code)
                 case ErrorCode.WRN_CapturedPrimaryConstructorParameterPassedToBase:
                 case ErrorCode.WRN_UnreadPrimaryConstructorParameter:
                 case ErrorCode.WRN_CallNotInterceptable:
+                case ErrorCode.WRN_InterceptorSignatureMismatch:
+                case ErrorCode.WRN_NullabilityMismatchInReturnTypeOnInterceptor:
+                case ErrorCode.WRN_NullabilityMismatchInParameterTypeOnInterceptor:
                     return 1;
                 default:
                     return 0;
@@ -584,6 +589,12 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
                 case ErrorCode.ERR_InterceptorSignatureMismatch:
                 case ErrorCode.ERR_InterceptorMustHaveMatchingThisParameter:
                 case ErrorCode.ERR_InterceptorMustNotHaveThisParameter:
+                case ErrorCode.ERR_DuplicateInterceptor:
+                case ErrorCode.WRN_InterceptorSignatureMismatch:
+                case ErrorCode.ERR_InterceptorNotAccessible:
+                case ErrorCode.ERR_InterceptorScopedMismatch:
+                case ErrorCode.WRN_NullabilityMismatchInReturnTypeOnInterceptor:
+                case ErrorCode.WRN_NullabilityMismatchInParameterTypeOnInterceptor:
                     // Update src\EditorFeatures\CSharp\LanguageServer\CSharpLspBuildOnlyDiagnostics.cs
                     // whenever new values are added here.
                     return true;
@@ -2326,6 +2337,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
                 case ErrorCode.ERR_InterceptorFilePathCannotBeNull:
                 case ErrorCode.ERR_InterceptorNameNotInvoked:
                 case ErrorCode.ERR_InterceptorNonUniquePath:
+                case ErrorCode.ERR_InterceptorLineCharacterMustBePositive:
                 case ErrorCode.ERR_ConstantValueOfTypeExpected:
                 case ErrorCode.ERR_UnsupportedPrimaryConstructorParameterCapturingRefAny:
                     return false;