Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compiler behavior when generating code for override that is ambiguous to the runtime #45453

Closed
gafter opened this issue Jun 25, 2020 · 3 comments
Assignees
Labels
Area-Compilers Bug Feature - Covariant Returns Permit an override to return a more specific reference type
Milestone

Comments

@gafter
Copy link
Member

gafter commented Jun 25, 2020

Along with the implementation of covariant returns, we have a bug fix for the code generation strategy when we generate metadata for an override of a method that might otherwise be ambiguous to the runtime. Specifically, in a case such as

class Base<T>
{
    public virtual void M(ref T t) { }
    public virtual void M(out int t) { t = 1; }
}
class Derived : Base<int>
{
    public override void M(ref int t) { }
}

where the two methods would both be inferred as candidates for the overridden method by the runtime, the compiler currently warns

warning CS1957: Member 'Derived.M(ref int)' overrides 'Base<int>.M(ref int)'. There are multiple override candidates at run-time. It is implementation dependent which method will be called.

However, the compiler can produce a “methodimpl” (aka .override) entry in metadata to instruct the runtime precisely which method is overridden, thereby resolving the ambiguity.

This is a change in behavior, and though it is a bug fix it could result in a change in behavior that is unwanted by some clients. In addition, despite being required to by the specification the runtime does not always obey the table entries in metadata (see dotnet/runtime#38119). We are expecting the runtime bug to be fixed in .net 5.

The question is: under what conditions should the compiler

  1. emit methodimpl (.override) table entries in metadata so that the metadata is no longer ambiguous.
  2. produce a wanrning that the behavior might be ambiguous at runtime.

With regards to (1), note that because the metadata differs from before, and because there is a known bug in some runtimes, it is possible that there exist examples where a program behavior changes from correct to incorrect if we add the methodimpl.

The options that we are considering are:

  1. Produce code-gen fix unconditionally. Stop producing the warning.
    Pros: Simplest product code (already implemented in PR).
    Cons: May change behavior on older runtimes (correct to incorrect or vice versa) without warning.
  2. Produce code-gen fix if runtime known to support it. Produce warning if runtime not known to support it.
    Pros: Most compatible for older runtimes, most correct for newer runtimes.
    Cons: More code in the product.
  3. Produce code-gen fix unconditionally. Always produce the warning.
    Pros: No runtime-version-specific branches in the compiler.
    Cons: May change behavior on older runtimes (correct to incorrect or vice versa) with no change in the warning
    Produces incorrect (false positive) warnings for newer runtimes.
  4. Produce code-gen fix unconditionally. Produce warning if runtime not known to support it.
    Pros: Somewhat simpler product code (relative to 2).
    Cons: May change behavior on older runtimes (correct to incorrect or vice versa) with no change in the warning
@gafter gafter added Bug Area-Compilers Feature - Covariant Returns Permit an override to return a more specific reference type labels Jun 25, 2020
@gafter gafter added this to the 16.8 milestone Jun 25, 2020
@gafter gafter self-assigned this Jun 25, 2020
@gafter
Copy link
Member Author

gafter commented Jun 25, 2020

See also #44067

@gafter
Copy link
Member Author

gafter commented Jun 25, 2020

We decided to go with option 2. We can possibly enhance the warning to suggest moving to .net 5.

gafter pushed a commit to gafter/roslyn that referenced this issue Jun 26, 2020
@gafter gafter added 4 - In Review A fix for the issue is submitted for review. and removed 3 - Working labels Jun 26, 2020
gafter pushed a commit that referenced this issue Jul 7, 2020
* Implement and test retargeting symbols for covariant returns.
* A PE symbol with multiple methodimpl entries is ignored as an explicit override for language covariant returns.
* Test binary compatibility scenarios with overrides inserted into the hierarchy.
* Test a scenario with duplicate requirements for a methodimpl entry
* A methodimpl may be required anytime the runtime and language disagree about the overridden method.
  (As a side-effect, this fixes a number of previously-believed-unfixable bugs regarding the mismatch between C# and CIL)
* Emit a call to the least derived override in the hierarchy with the correct return type.
* Test covariant returns in expression trees.
* Test capturing a covariant method in a delegate creation.
* Test consumption and override of covariantly overridden methods, properties, and indexers from VB.
* Require that the target runtime supports covariant returns when the feature is used.
* Adjust tests for new special member.
* Test compile-time behavior of nullable variance in covariant returns.
* Produce RequireMethodImplToRemainInEffectAttribute on covariant overrides.
* Eliminate CS1957 by generating correct code.
  Fixes #44067
* Cache `PEMethodSymbol.ExplicitlyOverriddenClassMethod`
  Closes #44068
* Move some PROTOTYPE comments to issues.
  Relates to #44206, #44207, #44208, #44209
* Remove virtual `ExplicitlyOverriddenClassMethod` from MethodSymbol.
  Instead provide (and test) leaf APIs to be used directly by `OverriddenOrHiddenMembersHelpers`.
* Enhance the covariant return tests per PR review.
* Rename `RequireMethodImplToRemainInEffectAttribute` to `PreserveBaseOverridesAttribute` per API review
* Document breaking change
* Changes per review comments
  - Revert behavior of PEMethodSymbol.IsOverride (true even if we cannot report a unique overridden method)
  - Add runtime test for behavior of methodimpl in ambiguous scenarios (reported 
    dotnet/runtime#38119)
* Modify how overrides are computed for PE symbols as requested in code review.
* Condition WRN_MultipleRuntimeOverrideMatches on whether runtime has working methodimpl
  Relates to dotnet/runtime#38119
  Fixes #45453
* Test `SourceMethodSymbol.RequiresExplicitOverride` in ambiguous scenarios.
* Preserve historical test for methodimpl generation as a fallback.
@gafter
Copy link
Member Author

gafter commented Jul 7, 2020

Fixed in #44025

@gafter gafter closed this as completed Jul 7, 2020
@gafter gafter removed the 4 - In Review A fix for the issue is submitted for review. label Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Feature - Covariant Returns Permit an override to return a more specific reference type
Projects
None yet
Development

No branches or pull requests

1 participant