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

Consider type declaration order in MethodImpls #111998

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

MichalStrehovsky
Copy link
Member

This fixes the Loader\classloader\InterfaceFolding\Ambiguous test.

For the following program in pseudo-C# (test is in IL since this is not valid C#):

B<int, int> b = new B<int, int>();
if (!(((I<int>)b).Foo() != "B::Foo2"))
{
    B<string, string> b2 = new B<string, string>();
    if (!(((I<string>)b2).Foo() != "B::Foo2"))
    {
        Console.WriteLine("Pass");
        return 100;
    }
}

Console.WriteLine("Failed!");
return -1;

interface I<T>
{
    string Foo();
}

class A<U> : I<U>
{
    string I<U>.Foo() => "A::Foo";
}

internal class B<V, W> : A<V>, I<W>
{
    string I<W>.Foo() => "B::Foo1";
    string I<V>.Foo() => "B::Foo2";
}

We incorrectly resolve the interface call into B::Foo1 because I<int>.Foo could be implemented by both Foo1 and Foo2 methods and we just pick whatever we see first. Per ECMA-335: "If there are multiple methods for the same interface method (i.e. with different generic type parameters), place them in the list in type declaration order of the associated interfaces."

This implements the tie break.

Cc @dotnet/ilc-contrib

This fixes the Loader\classloader\InterfaceFolding\Ambiguous test.

For the following program in pseudo-C# (test is in IL since this is not valid C#):

```csharp
B<int, int> b = new B<int, int>();
if (!(((I<int>)b).Foo() != "B::Foo2"))
{
    B<string, string> b2 = new B<string, string>();
    if (!(((I<string>)b2).Foo() != "B::Foo2"))
    {
        Console.WriteLine("Pass");
        return 100;
    }
}

Console.WriteLine("Failed!");
return -1;

interface I<T>
{
    string Foo();
}

class A<U> : I<U>
{
    string I<U>.Foo() => "A::Foo";
}

internal class B<V, W> : A<V>, I<W>
{
    string I<W>.Foo() => "B::Foo1";
    string I<V>.Foo() => "B::Foo2";
}

```

We incorrectly resolve the interface call into `B::Foo1` because `I<int>.Foo` could be implemented by both `Foo1` and `Foo2` methods and we just pick whatever we see first. Per ECMA-335: "If there are multiple methods for the same interface method (i.e. with different generic type parameters), place them in the list in type declaration order of the associated interfaces."

This implements the tie break.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs:280

  • The new method FindInterfaceImplFromDeclFromMethodImpls is introduced without corresponding test cases. Ensure that this method is covered by tests.
private static MethodDesc FindInterfaceImplFromDeclFromMethodImpls(MetadataType type, MethodDesc decl)
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

The crash in https://helix.dot.net/api/2019-06-17/jobs/cbb9e7e4-8a5d-4034-831f-f689a4eb08c9/workitems/Microsoft.Extensions.Hosting.WindowsServices.Tests/console is a crash on shutdown. Going to assume it's another instance of the UCRT by-design bug #108640.

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok. I don't like taking changes here, but I don't see anything wrong with this.

@MichalStrehovsky MichalStrehovsky merged commit adb2af2 into dotnet:main Feb 6, 2025
114 of 116 checks passed
@MichalStrehovsky MichalStrehovsky deleted the methodimplorder branch February 6, 2025 06:07
grendello added a commit to grendello/runtime that referenced this pull request Feb 6, 2025
* main: (23 commits)
  add important remarks to NrbfDecoder (dotnet#111286)
  docs: fix spelling grammar and missing words in clr-code-guide.md (dotnet#112222)
  Consider type declaration order in MethodImpls (dotnet#111998)
  Add a feature flag to not use GVM in Linq Select (dotnet#109978)
  [cDAC] Implement ISOSDacInterface::GetMethodDescPtrFromIp (dotnet#110755)
  Restructure JSImport/JSExport generators to share more code and utilize more Microsoft.Interop.SourceGeneration shared code (dotnet#107769)
  Add more detailed explanations to control-flow RegexOpcode values (dotnet#112170)
  Add repo-specific condition to labeling workflows (dotnet#112169)
  Fix bad assembly when a nested exported type is marked via link.xml (dotnet#107945)
  Make `CalculateAssemblyAction` virtual. (dotnet#112154)
  JIT: Enable reusing profile-aware DFS trees between phases (dotnet#112198)
  Add support for LDAPTLS_CACERTDIR \ TrustedCertificateDirectory (dotnet#111877)
  JIT: Support custom `ClassLayout` instances with GC pointers in them (dotnet#112064)
  Factor positive lookaheads better into find optimizations (dotnet#112107)
  Add ImmutableCollectionsMarshal.AsMemory (dotnet#112177)
  [mono] ILStrip write directly to the output filestream (dotnet#112142)
  Allow the NativeAOT runtime pack to be specified as the ILC runtime package (dotnet#111876)
  JIT: some reworking for conditional escape analysis (dotnet#112194)
  Replace HELPER_METHOD_FRAME with DynamicHelperFrame in patchpoints (dotnet#112025)
  [Android] Decouple runtime initialization and entry point execution for Android sample (dotnet#111742)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants