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

Frozen collection tests fail on NativeAOT #78046

Closed
VSadov opened this issue Nov 8, 2022 · 13 comments · Fixed by #78005
Closed

Frozen collection tests fail on NativeAOT #78046

VSadov opened this issue Nov 8, 2022 · 13 comments · Fixed by #78005
Labels
area-NativeAOT-coreclr area-System.Collections blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'

Comments

@VSadov
Copy link
Member

VSadov commented Nov 8, 2022

Failures in #77934

Either the tests should be made compatible with NativeAOT (preferrable, if possible), or disabled in the NativeAOT test runs.

The typical failure looks like:

[FAIL] System.Collections.Frozen.Tests.FrozenDictionary_Generic_Tests_string_string_Ordinal.LookupItems_AllItemsFoundAsExpected(size: 1, comparer: null, specifySameComparer: False)
System.NotSupportedException : 'Xunit.Sdk.AssertEqualityComparer`1[System.Collections.Generic.HashSet`1[System.Collections.Generic.KeyValuePair`2[System.String, System.String]]].CompareTypedSets[System.Collections.Generic.KeyValuePair`2[System.String, System.String]](System.Collections.IEnumerable,System.Collections.IEnumerable)' is missing native code. MethodInfo.MakeGenericMethod() is not compatible with AOT compilation. Inspect and fix AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativeaot-compatibility
   at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x30
   at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.MakeGenericMethod(Type[]) + 0x1c0
   at System.Collections.Frozen.Tests.FrozenDictionary_Generic_Tests`2.LookupItems_AllItemsFoundAsExpected(Int32 size, IEqualityComparer`1 comparer, Boolean specifySameComparer) + 0x280
   at System.Collections.Immutable!<BaseAddress>+0x83e170
   at System.Reflection.DynamicInvokeInfo.Invoke(Object, IntPtr, Object[], BinderBundle, Boolean) + 0x14c

Something is using MethodInfo.MakeGenericMethod() and it is not an AOT-friendly pattern.

Report

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
0 0 0
@VSadov VSadov added the blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' label Nov 8, 2022
@VSadov
Copy link
Member Author

VSadov commented Nov 8, 2022

@stephentoub - should the tests be disabled on NativeAOT or this is supposed to work?

@stephentoub
Copy link
Member

stephentoub commented Nov 8, 2022

Something is using MethodInfo.MakeGenericMethod() and it is not an AOT-friendly pattern.

I assume it's xunit itself. We can disable the tests for now until we can come up with an AOT-friendlier pattern that doesn't significantly increase the test complexity (if you have suggestions, happy to hear them).

The tests are implemented in a generic base class, e.g.

public abstract class FrozenSet_Generic_Tests<T> : ISet_Generic_Tests<T>

which is then derived from to run those tests with specific types/configurations, e.g.
public abstract class FrozenSet_Generic_Tests_string : FrozenSet_Generic_Tests<string>
{
protected override string CreateT(int seed)
{
int stringLength = seed % 10 + 5;
Random rand = new Random(seed);
byte[] bytes1 = new byte[stringLength];
rand.NextBytes(bytes1);
return Convert.ToBase64String(bytes1);
}
}
public class FrozenSet_Generic_Tests_string_Default : FrozenSet_Generic_Tests_string
{
protected override IEqualityComparer<string> GetIEqualityComparer() => EqualityComparer<string>.Default;
}
public class FrozenSet_Generic_Tests_string_Ordinal : FrozenSet_Generic_Tests_string
{
protected override IEqualityComparer<string> GetIEqualityComparer() => StringComparer.Ordinal;
}

and this particular test is using a MemberData to a specific enumerable method:
[Theory]
[MemberData(nameof(LookupItems_AllItemsFoundAsExpected_MemberData))]
public void LookupItems_AllItemsFoundAsExpected(int size, IEqualityComparer<T> comparer, bool specifySameComparer)

@VSadov
Copy link
Member Author

VSadov commented Nov 8, 2022

I think we should disable for now - since it blocks clean CI for NativeAOT libraries tests.
@LakshanF did we have issues like this before when enabling tests for NativeAOT? Are there known solutions?

@jkotas
Copy link
Member

jkotas commented Nov 8, 2022

we have issues like this before when enabling tests for NativeAOT? Are there known solutions?

Yes, we have hit this xunit problem several times. The workaround is to add the dynamically created generic instantiations to .rd.xml. For example: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Drawing.Common/tests/default.rd.xml

@VSadov
Copy link
Member Author

VSadov commented Nov 8, 2022

So what are the dynamically instantiated types here?

I see:

FrozenSet_Generic_Tests<string>
FrozenSet_Generic_Tests<ulong>
FrozenSet_Generic_Tests<int>
FrozenSet_Generic_Tests<SimpleClass>

is that all?

@VSadov
Copy link
Member Author

VSadov commented Nov 8, 2022

it actually complains about

Xunit.Sdk.AssertEqualityComparer`1[System.Collections.Generic.HashSet`1[System.Collections.Generic.KeyValuePair`2[System.String, System.String]]].CompareTypedSets[System.Collections.Generic.KeyValuePair`2[System.String, System.String]](System.Collections.IEnumerable,System.Collections.IEnumerable)

How that should look in rd.xml format?

@VSadov
Copy link
Member Author

VSadov commented Nov 8, 2022

I am trying:

    <Assembly Name="xunit.core">
      <Type Name="Xunit.Sdk.AssertEqualityComparer`1[[System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib], [System.String, System.Private.CoreLib]], System.Private.CoreLib]]">
        <Method Name="CompareTypedSets" Dynamic="Required All">
          <GenericArgument Name="System.Collections.Generic.KeyValuePair`2[[System.String, System.Private.CoreLib], [System.String, System.Private.CoreLib]], System.Private.CoreLib" />
        </Method>
      </Type>
    </Assembly>

That gives me an error at compile time:

Unhandled exception. Internal.TypeSystem.TypeSystemException+TypeLoadException: Failed to load type ' ' from assembly 'xunit.core, Version=2.4.2.0, Culture=neutral, PublicKeyToken=8d05b1bb7a6fdb6c'

@MichalStrehovsky
Copy link
Member

Failed to load type ' ' from assembly

This is because of a space after comma (not sure if the one between generic arguments or the assembly name qualifier, but I hit this bug). Delete all spaces after commas.

RD.XML is (mis)using the SerString format parser that we have in the compiler. It's not a particularly good parser because it was supposed to be temporary (just like RD.XML), but AFAIK S.R.Metadata didn't deliver a parser that we're waiting for here:

// TODO: This file is pretty much a line-by-line port of C++ code to parse CA type name strings from NUTC.
// It's a stopgap solution.
// This should be replaced with type name parser in System.Reflection.Metadata once it starts shipping.

@VSadov
Copy link
Member Author

VSadov commented Nov 9, 2022

Delete all spaces after commas.

That helped!!

It looks like there are some more methods that need rd directives, but now I think, I will just keep adding them until tests start passing.

@VSadov
Copy link
Member Author

VSadov commented Nov 9, 2022

Hmm, but the runtime errors do not go away.
For example I have this in rd.xml

    <Assembly Name="xunit.core">
      <Type Name="Xunit.Sdk.AssertEqualityComparer`1[[System.Collections.Generic.HashSet`1[[System.Collections.Generic.KeyValuePair`2[[System.UInt64, System.Private.CoreLib],[System.UInt64, System.Private.CoreLib]],System.Private.CoreLib]],System.Private.CoreLib]]" Dynamic="Required All">
        <Method Name="CompareTypedSets" Dynamic="Required All">
          <GenericArgument Name="System.Collections.Generic.KeyValuePair`2[[System.UInt64,System.Private.CoreLib],[System.UInt64,System.Private.CoreLib]],System.Private.CoreLib" />
        </Method>
      </Type>
    </Assembly>

and that is accepted at compile time, but I still get the following at run time.

  [FAIL] System.Collections.Frozen.Tests.FrozenDictionary_Generic_Tests_ulong_ulong.LookupItems_AllItemsFoundAsExpected(size: 1, comparer: null, specifySameComparer: False)
  System.NotSupportedException : 'Xunit.Sdk.AssertEqualityComparer`1[System.Collections.Generic.HashSet`1[System.Collections.Generic.KeyValuePair`2[System.UInt64, System.UInt64]]].CompareTypedSets[System.Collections.Generic.KeyValuePair`2[System.UInt64, System.UInt64]](System.Collections.IEnumerable,System.Collections.IEnumerable)' is missing native code. MethodInfo.MakeGenericMethod() is not compatible with AOT compilation. Inspect and fix AOT related warnings that were generated when the app was published. For more information see https://aka.ms/nativeaot-compatibility
     at System.Reflection.Runtime.MethodInfos.RuntimeNamedMethodInfo`1.GetUncachedMethodInvoker(RuntimeTypeInfo[], MemberInfo) + 0x2f

@VSadov
Copy link
Member Author

VSadov commented Nov 9, 2022

It looks correct. Did I miss a space or a comma somewhere? What does it want?

@MichalStrehovsky
Copy link
Member

It looks correct. Did I miss a space or a comma somewhere? What does it want?

It's possible it's AssertEqualityComparer in a different assembly. Because this is xunit and of course the type is triplicated. You're trying in assembly xunit.core, but I think this is xunit.assert (or less likely xunit.execution.dotnet).

I looked at the stack trace myself and the stack is:

 	System.Collections.Immutable.Tests.exe!S_P_CoreLib_System_Reflection_Runtime_MethodInfos_RuntimeNamedMethodInfo_1<S_P_CoreLib_System_Reflection_Runtime_MethodInfos_NativeFormat_NativeFormatMethodCommon>__MakeGenericMethod() Line 150	Unknown
 	System.Collections.Immutable.Tests.exe!xunit_assert_Xunit_Sdk_AssertEqualityComparer_1<System___Canon>__CheckIfSetsAreEqual() Line 316	Unknown
 	System.Collections.Immutable.Tests.exe!xunit_assert_Xunit_Sdk_AssertEqualityComparer_1<System___Canon>__Equals_0() Line 111	Unknown
 	System.Collections.Immutable.Tests.exe!xunit_assert_Xunit_Assert__Equal_2<System___Canon>() Line 91	Unknown
>	System.Collections.Immutable.Tests.exe!System_Collections_Immutable_Tests_System_Collections_Frozen_Tests_FrozenDictionary_Generic_Tests_2<System___Canon__System___Canon>__LookupItems_AllItemsFoundAsExpected() Line 196	Unknown

This is my favorite part of xunit where it will just go and to figure out some crazy way to compare the things passed to it. I always wonder if that's the notion of equality the test was really intending.

@VSadov
Copy link
Member Author

VSadov commented Nov 9, 2022

Yes the assembly should be xunit.assert

@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr area-System.Collections blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms'
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants