Fix overload resolution to handle binary compat in the face of covariant returns#46367
Conversation
b489625 to
5e688c2
Compare
Relates to dotnet#46361
Adjust overload resolution to handle explicit overrides that skip inheritance steps in binary compat scenarios Fixes dotnet#45798
99c20b1 to
d466493
Compare
| } | ||
| "; | ||
| var compilation = CreateCovariantCompilation(program, options: TestOptions.DebugExe, references: new[] { ref0, ref1b, ref2 }); | ||
| // compilation.VerifyDiagnostics(); |
There was a problem hiding this comment.
is comment intentional? #Resolved
There was a problem hiding this comment.
| if (TypeSymbol.Equals(current.ContainingType, overridden.ContainingType, TypeCompareKind.ConsiderEverything2)) | ||
| for (TypeSymbol b = derivedType.BaseTypeNoUseSiteDiagnostics; (object)b != null; b = b.BaseTypeNoUseSiteDiagnostics) | ||
| { | ||
| if (b.Equals(baseType, TypeCompareKind.ConsiderEverything)) |
There was a problem hiding this comment.
ConsiderEverything [](start = 55, length = 18)
Why ConsiderEverything? #ByDesign
There was a problem hiding this comment.
Because methods in a method group are gathered from the inheritance hierarchy of the receiver, and therefore the receiver types must be related by precise inheritance.
In reply to: 461758371 [](ancestors = 461758371)
There was a problem hiding this comment.
Because methods in a method group are gathered from the inheritance hierarchy of the receiver, and therefore the receiver types must be related by precise inheritance.
This component is not responsible to gather candidates and doesn't control how it is done. I think we should not make any assumptions about what could be in the set. I think that Either we should come up with a test demonstrating that usage of ConsiderEverything is significant for correctness, otherwise we should ignore all options, which will be more robust approach in the log run.
In reply to: 461827656 [](ancestors = 461827656,461758371)
There was a problem hiding this comment.
This code isn't making an assumption. It is checking it. This parallels the existing behavior of overload resolution (see line 949 of the old code - an exact check). If you don't understand how this is modeling the language specification please let me know and I will explain in detail.
Hint: This is part of implementing the strategy described in a big block comment starting at line 719. All of this code depends on the fact that we are implementing that strategy cooperatively with the code that gathers the overload candidates. The code before this PR and the code in this PR depend on that handshake. This PR adheres to that model rather than relaxing it as you suggest.
In reply to: 461938115 [](ancestors = 461938115,461827656,461758371)
| { | ||
| return false; | ||
| } | ||
| private static bool IsBaseClass(TypeSymbol derivedType, TypeSymbol baseType) |
There was a problem hiding this comment.
IsBaseClass [](start = 28, length = 11)
It looks like this is a dupe of TypeSymbol.IsDerivedFrom method. #Closed
There was a problem hiding this comment.
It is similar. It is closer to ConversionBase.IsBaseClass.
In reply to: 461762763 [](ancestors = 461762763)
There was a problem hiding this comment.
It is similar. It is closer to ConversionBase.IsBaseClass.
It is quite possible I am missing something, but based on a quick glance at the code in both methods, I see no meaningful difference. Please, point me to a unit-test that would demonstrate an observable difference between using this method and using TypeSymbol.IsDerivedFrom method in IsMoreDerivedOverride, or add a test like that. Otherwise, I believe there is no benefit in cloning existing helpers and duplicating code in the product, and we should simply switch to using TypeSymbol.IsDerivedFrom.
In reply to: 461831483 [](ancestors = 461831483,461762763)
There was a problem hiding this comment.
This code is specialized to avoid gathering use site diagnostics and to perform an exact type check. If you don't mind the extra work of computing and discarding use-site diagnostics I will reuse that.
In reply to: 461944824 [](ancestors = 461944824,461831483,461762763)
|
|
||
| // Don't search beyond the overridden member. | ||
| if (TypeSymbol.Equals(current.ContainingType, overridden.ContainingType, TypeCompareKind.ConsiderEverything2)) | ||
| for (TypeSymbol b = derivedType.BaseTypeNoUseSiteDiagnostics; (object)b != null; b = b.BaseTypeNoUseSiteDiagnostics) |
There was a problem hiding this comment.
BaseTypeNoUseSiteDiagnostics [](start = 44, length = 28)
It is not clear why are we ignoring use-site diagnostics. It looks like problems in inheritance chain can affect the result. In cases like this we collect use-site diagnostics. #Closed
There was a problem hiding this comment.
We have always ignored use site diagnostics when determining if one method is a more specific override than another in overload resolution. This is not a change from existing compiler behavior. Use-site diagnostics would have been reported when gathering the candidates from the hierarchy of the receiver.
In reply to: 461764045 [](ancestors = 461764045)
There was a problem hiding this comment.
We have always ignored use site diagnostics when determining if one method is a more specific override than another in overload resolution.
If that is the case, please point to the specific place in this component where use-site diagnostics is ignored when we traverse bases.
Use-site diagnostics would have been reported when gathering the candidates from the hierarchy of the receiver.
This component is not responsible to gather candidates and doesn't control how it is done. It can be called with any set of candidates as we more and more often build the set of candidates without relying on regular name binding from syntax.
In reply to: 461828701 [](ancestors = 461828701,461764045)
There was a problem hiding this comment.
Note, that we have a Used Assembly References feature in the pipe line that depends on us always collecting use-site diagnostics every time there is a slightest chance that presence of the errors affects the analysis result in any way. We are quite tolerant to duplicate use-site errors reporting, but not as tolerant to missing recording an assembly "usage". I think it would be more robust long term if we simply collect the diagnostics here without trying to be too smart.
In reply to: 461940796 [](ancestors = 461940796,461828701,461764045)
There was a problem hiding this comment.
Those use site diagnostics have been reported when the candidates were collected. I understand that you would prefer that overload resolution now be enhanced to be able to return use site diagnostics when computing the relationship between methods in the method group. It did not do so before when traversing the inheritance hierarchy (when computing a method's overridden method) and I am not willing to do that in the context of this PR. Please open a separate PR for that suggested work item. You might also want to worry about OverriddenOrHiddenMembersResult.GetOverriddenMember() which was the source of the traversal data in the previous form of this code and which did not gather use-site diagnostics. I do not think it needs to, but you are welcome to follow up on that outside this PR.
In reply to: 461942977 [](ancestors = 461942977,461940796,461828701,461764045)
There was a problem hiding this comment.
If that is the case, please point to the specific place in this component where use-site diagnostics is ignored when we traverse bases.
Original version of this code, line 941, (use-site diagnostics arising when traversing bases in order to compute the overridden member are discarded).
In reply to: 461940796 [](ancestors = 461940796,461828701,461764045)
| // methods ultimately override the same original method. This addresses issues in binary compat | ||
| // scenarios where the override chain may skip some steps. | ||
| // See https://github.com/dotnet/roslyn/issues/45798 for an example. | ||
| return moreDerivedOverride.GetLeastOverriddenMember(accessingTypeOpt: null).OriginalDefinition == |
There was a problem hiding this comment.
OriginalDefinition [](start = 88, length = 18)
Comparing original definitions feels fragile. Methods in generic types that are constructed with different type arguments cannot override the same method, even when the overridden methods correspond to the same original definition. #ByDesign
There was a problem hiding this comment.
We've already verified that the methods' containing types are in a precise inheritance relationship.
In reply to: 461770335 [](ancestors = 461770335)
There was a problem hiding this comment.
We've already verified that the methods' containing types are in a precise inheritance relationship.
Where and how, is that the other place I made a comment about the TypeCompareKind used?
In reply to: 461838954 [](ancestors = 461838954,461770335)
There was a problem hiding this comment.
In general, I do not believe taking shortcuts like that buy us anything, they do not simplify code, do not make it easer to understand and maintain. I believe that it would be more robust if we don't take them without a real benefit that can be demonstrated/measured.
In reply to: 461938985 [](ancestors = 461938985,461838954,461770335)
There was a problem hiding this comment.
Where and how, is that the other place I made a comment about the TypeCompareKind used?
The where is on line 931 (934 in iteration 4). This preserves the behavior of this code for existing scenarios not involving covariant returns. I do not intend to modify or relax the behavior of this code for existing scenarios, so this code is as intended.
This is the inner loop of overload resolution, so performance-affecting shortcuts are important. You are welcome to try and see how much things degrade without them after this PR is integrated.
In reply to: 461948173 [](ancestors = 461948173,461938985,461838954,461770335)
| const int limit = start * 4; | ||
|
|
||
| for (int count = start; count <= limit && diagnostics.IsEmpty; count += step) | ||
| int count; |
There was a problem hiding this comment.
int count; [](start = 12, length = 10)
It is not clear what motivated this change, it looks like it didn't change behavior of the method. #Closed
| } | ||
|
|
||
| [ConditionalFact(typeof(ClrOnly), typeof(NoIOperationValidation), Reason = "https://github.com/dotnet/roslyn/issues/29428")] | ||
| [ConditionalFact(typeof(ClrOnly), typeof(NoIOperationValidation), Reason = "https://github.com/dotnet/roslyn/issues/29428", AlwaysSkip = "https://github.com/dotnet/roslyn/issues/46361")] |
There was a problem hiding this comment.
#46361 [](start = 146, length = 45)
Do other branches experience the same issue? Otherwise, we should treat this failure as possibly related to the changes around covariant returns (perhaps even in this PR), i.e. compiler is possibly consuming more stack than before, which brings this test close to the failure point. #Closed
There was a problem hiding this comment.
Yes, this test is flaky on other branches. It doesn't run the test on a fresh thread.
In reply to: 461776977 [](ancestors = 461776977)
| public static bool IsCoreClr => !IsDesktop; | ||
| public static bool IsCoreClrUnix => IsCoreClr && IsUnix; | ||
| public static bool IsMonoOrCoreClr => IsMono || IsCoreClr; | ||
| public static bool RuntimeSupportsCovariantReturnsOfClasses => Type.GetType("System.Runtime.CompilerServices.RuntimeFeature")?.GetField("CovariantReturnsOfClasses") != null; |
There was a problem hiding this comment.
RuntimeSupportsCovariantReturnsOfClasses [](start = 27, length = 40)
This feels wrong. It shouldn't matter in which runtime a test is executed. A test should be able any Target framework for a compilation and only the actual references should determine whether covariant returns are supported within the compilation. #Closed
There was a problem hiding this comment.
These are testing runtime conditions. Running covariant tests does depend on having a runtime that supports them.
In reply to: 461778705 [](ancestors = 461778705)
There was a problem hiding this comment.
Running covariant tests does depend on having a runtime that supports them.
Executing compiled code that utilizes covariant return feature - yes. Everything else, shape of symbols, result of emit, diagnostics, etc. doesn't depend
In reply to: 461836199 [](ancestors = 461836199,461778705)
There was a problem hiding this comment.
Right, which is why tests that are designed to test other things are not conditional.
In reply to: 461946462 [](ancestors = 461946462,461836199,461778705)
| return Type.GetType("System.Runtime.CompilerServices.RuntimeFeature")?.GetField("CovariantReturnsOfClasses") == null; | ||
| } | ||
| } | ||
| public override bool ShouldSkip => !ExecutionConditionUtil.RuntimeSupportsCovariantReturnsOfClasses; |
There was a problem hiding this comment.
ShouldSkip [](start = 29, length = 10)
Same comment. #Closed
There was a problem hiding this comment.
This is for conditioning tests that should only be run on runtimes that can handle covariant returns because their point is to verify the runtime behavior of the compiler-generated code.
In reply to: 461779636 [](ancestors = 461779636)
| } | ||
|
|
||
| [ConditionalFact(typeof(CovarantReturnRuntimeOnly))] | ||
| [ConditionalFact(typeof(CovariantReturnRuntimeOnly))] |
There was a problem hiding this comment.
[ConditionalFact(typeof(CovariantReturnRuntimeOnly))] [](start = 8, length = 53)
It is quite possible I am missing the motivation for conditionally disabling the tests. If it is just to avoid the failure because we are unable to execute the code in proc on some platforms, Default Interface Implementation tests solve this problem by conditionally passing null as expectedOutput to CompileAndVerify. This way we get as much verification as possible on each platform. #Closed
There was a problem hiding this comment.
Doing so here would simply duplicate tests already in the symbol tests. These tests are conditionally enabled because they are specifically for verifying the runtime behavior of the compiler-generated code. Compile-time behavior is tested in the symbol tests.
In reply to: 461782992 [](ancestors = 461782992)
|
Done with review pass (iteration 2) #Closed |
|
@jcouv @AlekseyTs I have responded to your comments. Do you have any others? |
|
Done with review pass (iteration 3) #Closed |
|
@jcouv @AlekseyTs I have responded to your comments. Do you have any others? |
… a more derived overide.
…t/roslyn into covariant-returns-45798
| else if (containingTypeMapOpt == null) | ||
| { | ||
| if (MemberGroupContainsOverride(members, member)) | ||
| if (MemberGroupContainsMoreDerivedOverride(members, member, checkOverrideContainingType: true, ref useSiteDiagnostics)) |
There was a problem hiding this comment.
Notes to self from chat with Neal:
The spec says that when collecting candidates for a method group, we should only keep the definition (and discard the overrides). But for practical reasons, the compiler instead keeps the most derived override instead (and we fix it up when emitting).
Previously, when comparing two candidates, we would see if one overrides the other (by looking up the overridden member chain). If they do, we keep the more derived one. But that doesn't work well in multi-assembly scenarios with particular build sequences (two overrides may not belong to the same chain).
Now, when comparing two candidates, we look all the way up their overridden member chains to find the definition, and if the definitions match, then we keep the more derived one (by looking at containing types).
* upstream/master: (304 commits) Tweak diagnostics to account for records (dotnet#46341) Diagnose precedence inversion in a warning wave (dotnet#46239) Remove PROTOTYPE tag (dotnet#45965) Only run a single pass of NullableWalker per-member (dotnet#46402) Fix crash, and offer "declare as nullable" for tuple fields (dotnet#46437) Simplify contract for RunWithShutdownBlockAsync Fix optprof plugin input check if EditorAdaptersFactoryService gives us a null buffer Cannot assign maybe-null value to TNotNull variable (dotnet#41445) Fix overload resolution to handle binary compat in the face of covariant returns (dotnet#46367) Same failure on Linux Skip some tests on Mac Added search option for inline parameter name hints Spelling tweak docs Improve comment Improve the "not exhaustive" diagnostic in the presence of a when clause. (dotnet#46143) PR feedback Use record keyword to display records (dotnet#46338) remove test that aserts .NET Standard should be prefered over .NET Framework ...
Fixes #45798