Interceptors: handle duplicates, additional signature validation, etc. #67786
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
RikkiGibson
changed the title
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Symbols/Wrapped/WrappedParameterSymbol.cs
Outdated
Show resolved
Hide resolved
| 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) |
There was a problem hiding this comment.
I am wondering if it would make sense to call this in SourceModuleSymbol.ForceComplete, for example, after we find this.GlobalNamespace.HasComplete(CompletionPart.MembersCompleted). At this point I think we should have reported declaration diagnostics for all members, and we would know if there are duplicate interceptors for a location.
|
|
||
| // original signature: | ||
| // public TValue ConcurrentDictionary<TKey, TValue>.AddOrUpdate<TArg>(TKey key, Func<TKey,TArg,TValue> addValueFactory, Func<TKey,TValue,TArg,TValue> updateValueFactory, TArg factoryArgument); | ||
| public static TValue AddOrUpdate<TKey, TValue, TArg>( |
There was a problem hiding this comment.
Other extensions in this file polyfill methods for netstandard2.0 in a similar way. I wanted to be consistent with them.
| // 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))); |
There was a problem hiding this comment.
It seems unexpected to me for a TryGet() method to report diagnostics. (What if there are multiple calls?) If we have one specific caller where reporting a diagnostic makes the sense (and it looks like we only have one caller currently), would it make sense to simply return the OneOrMany to the caller and have the caller report the diagnostic?
There was a problem hiding this comment.
Detecting the problematic condition at the call site would require returning additional information. I think it's fairly common in the compiler to report errors associated with something we are trying to get, for example, with use site diagnostics.
src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Call.cs
Outdated
Show resolved
Hide resolved
| considerReturnType: true, | ||
| considerTypeConstraints: false, | ||
| considerCallingConvention: false, | ||
| considerRefKindDifferences: true, | ||
| typeComparison: TypeCompareKind.ObliviousNullableModifierMatchesAny); | ||
| considerArity: false, | ||
| typeComparison: TypeCompareKind.AllNullableIgnoreOptions | TypeCompareKind.IgnoreTupleNames); |
There was a problem hiding this comment.
We are checking for unsafe nullability differences separately. I decided to just not report on safe nullability differences.
When nullable differences are detected by signatures comparing not-equal like this it's a pain, because the differences might be dynamic/object stuff, or it might be string?/string stuff, which we might want to group separately, so that #nullable disable warnings disables the latter and not the former.
This is why the following "bug" occurs for partial methods. (warning is not reported in SharpLab, you'll have to paste into a new project in VS to reproduce.)
#nullable enable annotations
#nullable disable warnings
public partial class C
{
// implementation accepting "less" than definition is unsafe, and we don't warn here with '#nullable disable warnings'
public partial void M1(string? s);
public partial void M1(string s) { }
// implementation accepting "more" than definition is safe, but we warn here with '#nullable disable warnings'
public partial void M2(string s);
public partial void M2(string? s) { } // warning CS8826: Partial method declarations 'void C.M2(string s)' and 'void C.M2(string? s)' have signature differences.
}There was a problem hiding this comment.
Discussed a bit offline. At this point, permitting "safe" variance without warnings seems like the right thing for nullability and scoped. I feel like erroring on object/dynamic differences is right here and ignoring tuple name differences is right. It feels a tiny bit arbitrary sometimes...but open to feedback on some of these decisions.
|
|
||
| // PROTOTYPE(ic): test a valid case with large numbers of interceptors. (EndToEndTests?) |
There was a problem hiding this comment.
Not yet. I should have noted that some of these were moved to the test plan, but not checked off yet.
| @@ -3244,6 +3261,8 @@ internal override StrongNameKeys StrongNameKeys | |||
| bool hasDeclarationErrors = !FilterAndAppendDiagnostics(diagnostics, GetDiagnostics(CompilationStage.Declare, true, cancellationToken), excludeDiagnostics, cancellationToken); | |||
| excludeDiagnostics?.Free(); | |||
|
|
|||
| hasDeclarationErrors |= CheckDuplicateInterceptions(diagnostics); | |||
There was a problem hiding this comment.
From offline discussion, maybe this should be moved into CompileMethodBodies and maybe CheckDuplicateFilePaths as well.
Then the ERR_ModuleEmitFailure reported in TryGetInterceptor becomes unnecessary.
That's okay as a PROTOTYPE
There was a problem hiding this comment.
Also consider adding tests for duplicate interceptions and duplicate file paths in a compilation for ref-only assembly (showing whether or not we ran those checks).
There was a problem hiding this comment.
This comment also seems relevant.
Basically, the search is for a consistent place to report duplicates at the end of the declaration phase. This place would be appropriate for both duplicate file-local types and for interceptors.
Related to test plan #67421
Addresses a number of prototype comments and outstanding issues. interceptors.md has been updated to reflect what has generally been done in this PR.