Add tests for `GetInterfaceMap` on static interface methods #90518

hamarb123 · 2023-08-14T12:39:21Z

Fixes #90351.
Includes the currently single failing case from the comment #90351 (comment) - therefore I propose that all (/ or at least most) of the combinations are useful since they actually found a bug.
This probably can't be merged until it's fixed, or maybe that just needs to be disabled for now, but that's alright lol.

The failing case is the case which checks C2Implicit<string>'s implementations of I1<string>.G<T>. Previously, until recently when some other unrelated (to this) changes to static interface methods were made, many more cases were broken.

Related failure issues, which have had their tests disabled:

cc @jkotas

src/libraries/System.Runtime/tests/System/Type/TypeTests.cs

ghost · 2023-08-14T19:07:05Z

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Adds a the tests for #90351.
Includes the currently single failing case from the comment #90351 (comment) - therefore I propose that all (/ or at least most) of the combinations are useful since they actually found a bug.
This probably can't be merged until it's fixed, or maybe that just needs to be disabled for now, but that's alright lol.

The failing case is the case which checks C2Implicit<string>'s implementations of I1<string>.G<T>. Previously, until recently when some other unrelated (to this) changes to static interface methods were made, many more cases were broken.

cc @jkotas

Author:	hamarb123
Assignees:	-
Labels:	`area-System.Reflection`, `community-contribution`, `needs-area-label`
Milestone:	-

jkotas · 2023-08-14T19:12:17Z

maybe that just needs to be disabled for now, but that's alright lol.

I would recommend disabling the test against a new issue.

hamarb123 · 2023-08-14T23:40:34Z

I would recommend disabling the test against a new issue.

Is there a proper way to do this, or do I just need to skip the test manually? I ask because it's using an interesting setup, where it returns an IEnumerable<object[]> using yield as data for a [Theory]; presumably there's no way to do an [ActiveIssue] for just 1 of those cases?

jkotas · 2023-08-15T00:16:47Z

Comment out the failing case and add // [ActiveIssue("https://github.com/dotnet/runtime/issues/12345")] in front of it.

hamarb123 · 2023-08-21T11:04:03Z

I also need to exclude some tests on mono by the looks of it (mono and coreclr seem to behave differently in some of the cases?), I will add an [ActiveIssue] for that when I make an issue for that too.

• Would be good to test these anyway • Added to see if the failures on mono are with static interface methods only, or to do with default implementation methods

• Haven't opened an issue for this yet - will follow up with an issue & commit to add the issue link

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.DUPLICATE_EXTENTS_DATA.cs

src/libraries/System.Runtime/tests/System/Type/TypeTests.cs

trylek

Looks great to me, thank you!

hamarb123 · 2023-08-23T21:57:26Z

If there is nothing else to do, please flip the PR from Draft to Ready to review.

I haven't opened the other issue yet, if you're alright with merging it without that issue then feel free, or if you wanted to open it that would be great since I'm struggling to run mono locally and therefore write a useful reproduction for the issue with specifics of which cases fail and what happens instead and what's expected.

jkotas · 2023-08-23T22:03:04Z

For the tracking issue, it is fine to just link to the test that is disabled. You do not need to go the extra mile to extract the disabled test into a small repro.

jkotas · 2023-08-24T00:16:41Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-08-24T00:17:02Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2023-08-25T05:06:35Z

Native aot compiler fails on the new tests with:

Process terminated. Assertion failed.
   at Internal.TypeSystem.MetadataVirtualMethodAlgorithm.ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc interfaceMethod, MetadataType currentType) in /_/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs:line 615
   at ILCompiler.LazyGenericsSupport.GraphBuilder.LookForVirtualOverrides(EcmaMethod method) in /_/src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs:line 129
   at ILCompiler.LazyGenericsSupport.GraphBuilder..ctor(EcmaModule assembly) in /_/src/coreclr/tools/Common/Compiler/GenericCycleDetection/GraphBuilder.cs:line 62
   at ILCompiler.LazyGenericsSupport.CycleInfoHashtable.CreateValueFromKey(EcmaModule key) in /_/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs:line 52
   at Internal.TypeSystem.LockFreeReaderHashtable`2.CreateValueAndEnsureValueIsInTable(TKey key) in /_/src/coreclr/tools/Common/TypeSystem/Common/Utilities/LockFreeReaderHashtable.cs:line 562
   at ILCompiler.LazyGenericsSupport.GenericCycleDetector.FormsCycle(TypeSystemEntity entity, ModuleCycleInfo& cycleInfo) in /_/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs:line 194
   at ILCompiler.LazyGenericsSupport.GenericCycleDetector.DetectCycle(TypeSystemEntity owner, TypeSystemEntity referent) in /_/src/coreclr/tools/Common/Compiler/GenericCycleDetection/ModuleCycleInfo.cs:line 230
   at Internal.IL.ILImporter.GetMethodEntrypoint(MethodDesc method) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs:line 257
   at Internal.IL.ILImporter.ImportCall(ILOpcode opcode, Int32 token) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs:line 642
   at Internal.IL.ILImporter.ImportBasicBlock(BasicBlock basicBlock) in /_/src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs:line 424
   at Internal.IL.ILImporter.ImportBasicBlocks() in /_/src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs:line 308
   at Internal.IL.ILImporter.Import() in /_/src/coreclr/tools/aot/ILCompiler.Compiler/IL/ILImporter.Scanner.cs:line 166
   at ILCompiler.ILScanner.CompileSingleMethod(ScannedMethodNode methodCodeNodeNeedingCode) in /_/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/ILScanner.cs:line 122
   at System.Threading.Tasks.Parallel.<>c__DisplayClass19_0`2.<ForWorker>b__1(RangeWorker& currentWorker, Int64 timeout, Boolean& replicationDelegateYieldedBeforeCompletion)
   at System.Threading.Tasks.TaskReplicator.Replica.Execute()
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

I am going to push a fix for this crash.

jkotas · 2023-08-25T05:15:16Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-08-25T05:15:30Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas

Thank you!

hamarb123 added 2 commits August 14, 2023 22:30

Add unit tests for dotnet#90351

6e99cd3

Merge remote-tracking branch 'upstream' into main5

fa8237e

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 14, 2023

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 14, 2023

hamarb123 commented Aug 14, 2023

View reviewed changes

src/libraries/System.Runtime/tests/System/Type/TypeTests.cs Show resolved Hide resolved

This was referenced Aug 14, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

[wasm] AOT: System.Text.Json.Tests crashing inexplicably #89984

Closed

jkotas added the area-System.Reflection label Aug 14, 2023

jkotas removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Aug 14, 2023

steveharter requested review from steveharter and jkotas August 15, 2023 19:39

hamarb123 mentioned this pull request Aug 20, 2023

Exception on MakeGenericMethod called on a static interface method from GetInterfaceMap's TargetMethods for a default interface implementation #90863

Closed

Add [ActiveIssue] to skip over failing test case

7dbb3ea

hamarb123 added 3 commits August 22, 2023 05:55

Add some more DIM test cases

ff2e9b9

• Would be good to test these anyway • Added to see if the failures on mono are with static interface methods only, or to do with default implementation methods

Fix added DIM tests

23381f2

Disable some of the tests for mono

3475a75

• Haven't opened an issue for this yet - will follow up with an issue & commit to add the issue link

hamarb123 commented Aug 22, 2023

View reviewed changes

src/libraries/Common/src/Interop/Windows/Kernel32/Interop.DUPLICATE_EXTENTS_DATA.cs Outdated Show resolved Hide resolved

hamarb123 force-pushed the main5 branch from 3fd0e0a to 3475a75 Compare August 22, 2023 08:59

Merge remote-tracking branch 'upstream/main' into main5

b479fe5

hamarb123 marked this pull request as draft August 22, 2023 09:03

jkotas reviewed Aug 23, 2023

View reviewed changes

src/libraries/System.Runtime/tests/System/Type/TypeTests.cs Outdated Show resolved Hide resolved

jkotas mentioned this pull request Aug 23, 2023

InterfaceMap returns non-GenericMethodDefinition for static abstract interface generic methods on the target type #74132

Closed

jkotas requested a review from trylek August 23, 2023 01:56

trylek approved these changes Aug 23, 2023

View reviewed changes

jkotas mentioned this pull request Aug 23, 2023

Add regression test for GitHub issue #74132 #91009

Closed

hamarb123 marked this pull request as ready for review August 23, 2023 21:57

build-analysis bot mentioned this pull request Aug 23, 2023

[8.0] [wasm] Build failure: dotnet-install: Failed to check the disk space #91025

Closed

Add test number

8ad8d6c

Fix native AOT compiler crash

a4b5eaf

jkotas requested a review from MichalStrehovsky as a code owner August 25, 2023 05:14

jkotas approved these changes Aug 25, 2023

View reviewed changes

jkotas merged commit 17bae6c into dotnet:main Aug 25, 2023

ghost locked as resolved and limited conversation to collaborators Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for `GetInterfaceMap` on static interface methods #90518

Add tests for `GetInterfaceMap` on static interface methods #90518

hamarb123 commented Aug 14, 2023 •

edited

Loading

ghost commented Aug 14, 2023

jkotas commented Aug 14, 2023

hamarb123 commented Aug 14, 2023

jkotas commented Aug 15, 2023

hamarb123 commented Aug 21, 2023 •

edited

Loading

trylek left a comment

hamarb123 commented Aug 23, 2023 •

edited

Loading

jkotas commented Aug 23, 2023 •

edited

Loading

jkotas commented Aug 24, 2023

azure-pipelines bot commented Aug 24, 2023

jkotas commented Aug 25, 2023

jkotas commented Aug 25, 2023

azure-pipelines bot commented Aug 25, 2023

jkotas left a comment

Add tests for GetInterfaceMap on static interface methods #90518

Add tests for GetInterfaceMap on static interface methods #90518

Conversation

hamarb123 commented Aug 14, 2023 • edited Loading

ghost commented Aug 14, 2023

jkotas commented Aug 14, 2023

hamarb123 commented Aug 14, 2023

jkotas commented Aug 15, 2023

hamarb123 commented Aug 21, 2023 • edited Loading

trylek left a comment

Choose a reason for hiding this comment

hamarb123 commented Aug 23, 2023 • edited Loading

jkotas commented Aug 23, 2023 • edited Loading

jkotas commented Aug 24, 2023

azure-pipelines bot commented Aug 24, 2023

jkotas commented Aug 25, 2023

jkotas commented Aug 25, 2023

azure-pipelines bot commented Aug 25, 2023

jkotas left a comment

Choose a reason for hiding this comment

Add tests for `GetInterfaceMap` on static interface methods #90518

Add tests for `GetInterfaceMap` on static interface methods #90518

hamarb123 commented Aug 14, 2023 •

edited

Loading

hamarb123 commented Aug 21, 2023 •

edited

Loading

hamarb123 commented Aug 23, 2023 •

edited

Loading

jkotas commented Aug 23, 2023 •

edited

Loading