Partial fix for the negative virtual static method tests #17

trylek · 2021-05-11T00:20:22Z

UnimplementedAbstractMethodOnConcreteClass - I have added logic
to verify that all SVMs are implemented once a class is fully loaded.
That required me to propagate a new flag around limiting the class
load level for methods, otherwise we were hitting stack overflows
in many of the tests. I have recycled a pre-existing HRESULT ID which
is probably not what we want, I just didn't want to churn the codebase
before hearing from you what is the right way to proceed here.
UnimplementedAbstractMethodOnAbstractClass - the one remaining
failing case is 'ConstraintCheckShouldFail' - I guess that corresponds
to what you alluded to in mentioning that we're not properly checking
class constraints yet.
MultipleMethodImplRecordsSameMethodDecl - I added an extra parameter
to the SVM resolution logic that traverses all the MethodImpls and
throws when hitting a duplicate.

The remaining failures involve the following negative tests:

InterfaceVariance (pending my offline feedback)
VarianceSafety (pending my offline feedback)
UnimplementedAbstractMethodOnAbstractClass / ConstraintCheckShouldFail

Thanks

Tomas

trylek · 2021-05-11T00:31:00Z

src/coreclr/vm/methodtable.cpp

+            for (MethodIterator it(pInterfaceMT); it.IsValid(); it.Next())
+            {
+                MethodDesc *pMD = it.GetMethodDesc();
+                if (!ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* checkDuplicates */ TRUE))


I should probably add some checks here to just check the SVMs, will do in the next iteration along with your PR feedback.

In fact, is that something we should add to the negative tests, like some test that defines a (non-virtual) static method on an interface and verifies that the method doesn't need to be overridden in a class that implements the interface? I'm not really sure, it sounds quite artificial. On the positive side, should we just check that pMD is static and virtual as a pre-requisite to running the ResolveVirtualStaticMethod check?

How about just check for pMD being static and virtual, and that's probably enough for here.

You're right though, the current testbed doesn't add instance methods to any of the static interfaces, and this would likely break if that wasn't done. I'm kinda hoping to write that sort of test in C#, as it will be far easier to do.

trylek · 2021-05-11T00:32:47Z

src/coreclr/vm/methodtable.cpp

+                if (!ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* checkDuplicates */ TRUE))
+                {
+                    IMDInternalImport* pInternalImport = GetModule()->GetMDImport();
+                    GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, GetCl(), pMD->GetName(), IDS_CLASSLOAD_STATICVIRTUAL);


This is the ID I recycled - it basically shows all the information that's needed but its wording is misleading as it complains about the method being static virtual instead of missing implementation.

Please fix before checkin, but yes new string ids are annoying.

davidwrighton

This looks generally good. I only had a few comments.

davidwrighton · 2021-05-11T18:54:34Z

src/coreclr/vm/methodtable.cpp

+            for (MethodIterator it(pInterfaceMT); it.IsValid(); it.Next())
+            {
+                MethodDesc *pMD = it.GetMethodDesc();
+                if (!ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* checkDuplicates */ TRUE))


davidwrighton · 2021-05-11T18:55:26Z

src/coreclr/vm/methodtable.cpp

+                if (!ResolveVirtualStaticMethod(pInterfaceMT, pMD, /* allowNullResult */ TRUE, /* checkDuplicates */ TRUE))
+                {
+                    IMDInternalImport* pInternalImport = GetModule()->GetMDImport();
+                    GetModule()->GetAssembly()->ThrowTypeLoadException(pInternalImport, GetCl(), pMD->GetName(), IDS_CLASSLOAD_STATICVIRTUAL);


Please fix before checkin, but yes new string ids are annoying.

davidwrighton · 2021-05-11T19:02:19Z

src/coreclr/vm/methodtable.cpp

@@ -5659,6 +5659,11 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited,  const
            }
            else
            {
+                if (HasVirtualStaticMethods() && !IsAbstract() && !IsSharedByGenericInstantiations())


This deserves at least a TODO describing how to avoid doing this work for anything in System.Private.CoreLib. Maintaining this work as a pay-for-play feature requires that we are very careful making any changes in this part of the system.

davidwrighton · 2021-05-11T22:02:27Z

@trylek, I have a pair of PR's that address the variance concerns.

trylek · 2021-05-11T22:24:01Z

Thanks David for the update. I believe I have addressed all your PR feedback so please take another look when you have a chance.

davidwrighton · 2021-05-12T01:22:30Z

Please add the check for static and virtual as requested above, and fix the merge conflict.

1) UnimplementedAbstractMethodOnConcreteClass - I have added logic to verify that all SVMs are implemented once a class is fully loaded. That required me to propagate a new flag around limiting the class load level for methods, otherwise we were hitting stack overflows in many of the tests. I have recycled a pre-existing HRESULT ID which is probably not what we want, I just didn't want to churn the codebase before hearing from you what is the right way to proceed here. 2) UnimplementedAbstractMethodOnAbstractClass - the one remaining failing case is 'ConstraintCheckShouldFail' - I guess that corresponds to what you alluded to in mentioning that we're not properly checking class constraints yet. 3) MultipleMethodImplRecordsSameMethodDecl - I added an extra parameter to the SVM resolution logic that traverses all the MethodImpls and throws when hitting a duplicate. The remaining failures involve the following negative tests: InterfaceVariance (pending my offline feedback) VarianceSafety (pending my offline feedback) UnimplementedAbstractMethodOnAbstractClass / ConstraintCheckShouldFail Thanks Tomas

…tnet#52769) Transition to GC Unsafe mode on every MONO_RT_EXTERNAL_ONLY function in reflection.c In particular, fix mono_reflection_type_from_name which is used in https://github.com/xamarin/xamarin-android/blob/681887ebdbd192ce7ce1cd02221d4939599ba762/src/monodroid/jni/embedded-assemblies.cc#L350 Fixes stack traces like ``` 05-14 08:06:12.848 31274 31274 F DEBUG : #00 pc 00000b99 [vdso] (__kernel_vsyscall+9) 05-14 08:06:12.848 31274 31274 F DEBUG : #1 pc 0005ad68 /apex/com.android.runtime/lib/bionic/libc.so (syscall+40) (BuildId: 6e3a0180fa6637b68c0d181c343e6806) 05-14 08:06:12.848 31274 31274 F DEBUG : #2 pc 00076511 /apex/com.android.runtime/lib/bionic/libc.so (abort+209) (BuildId: 6e3a0180fa6637b68c0d181c343e6806) 05-14 08:06:12.848 31274 31274 F DEBUG : #3 pc 0002afcd /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::MonodroidRuntime::mono_log_handler(char const*, char const*, char const*, int, void*)+141) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1) 05-14 08:06:12.848 31274 31274 F DEBUG : #4 pc 00112c5d /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (eglib_log_adapter+141) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #5 pc 00020fdf /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (monoeg_g_logv+175) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #6 pc 0002113a /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (monoeg_g_log+42) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #7 pc 00128892 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_threads_transition_do_blocking+258) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #8 pc 0012a406 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_threads_enter_gc_safe_region_unbalanced_with_info+134) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #9 pc 0012a27e /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_threads_enter_gc_safe_region_internal+46) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #10 pc 000799a7 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_loader_lock+71) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #11 pc 000447a1 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_create_from_typedef+129) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #12 pc 0003c073 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_get_checked+99) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #13 pc 0003cc0f /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_from_name_checked_aux+735) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #14 pc 00037989 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_class_from_name_checked+73) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #15 pc 000cc5f4 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_get_type_internal+132) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #16 pc 000c9bce /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_get_type_with_rootimage+126) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #17 pc 000ca204 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (_mono_reflection_get_type_from_info+292) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #18 pc 000ca06e /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_type_from_name_checked+334) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #19 pc 000c9f01 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonosgen-2.0.so (mono_reflection_type_from_name+49) (BuildId: b67e93dd750dafdd6f65f408b021b6a3a74868ac) 05-14 08:06:12.849 31274 31274 F DEBUG : #20 pc 0001b40b /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::EmbeddedAssemblies::typemap_java_to_managed(char const*)+427) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1) 05-14 08:06:12.849 31274 31274 F DEBUG : #21 pc 0001b551 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::EmbeddedAssemblies::typemap_java_to_managed(_MonoString*)+113) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1) 05-14 08:06:12.849 31274 31274 F DEBUG : #22 pc 000211a7 /data/app/~~rMrkpKmVPaBpM5jKb8fPAg==/com.microsoft.maui-JfRo8RWSDJaNtJuBa0y7_Q==/lib/x86/libmonodroid.so (xamarin::android::internal::MonodroidRuntime::typemap_java_to_managed(_MonoString*)+39) (BuildId: 9726f32ad5f8fa5e7c5762baf2f6e3294da41cc1) ```

* Initial implementation for contract customization fix build errors Move converter rooting to DefaultJsonTypeInfoResolver so that it can be used standalone Fix ConfigurationList.IsReadOnly Minor refactorings (#1) * Makes the following changes: * Move singleton initialization for DefaultTypeInfoResolver behind a static property. * Consolidate JsonSerializerContext & IJsonTypeInfoResolver values to a single field. * Move reflection fallback logic away from JsonSerializerContext and into JsonSerializerOptions * Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs * remove testing of removed field Simplify the JsonTypeInfo.CreateObject implemenetation (#2) * Simplify the JsonTypeInfo.CreateObject implemenetation * Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs * Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com> Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com> Tests and fixes for JsonTypeInfoKind.None TypeInfo type mismatch tests Allow setting NumberHandling on JsonTypeInfoKind.None test resolver returning wrong type of options JsonTypeInfo/JsonPropertyInfo mutability tests rename test file Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver (#3) * Move default converter rooting responsibility behind DefaultJsonTypeInfoResolver * address feedback Add simple test for using JsonTypeInfo<T> with APIs directly taking it fix and tests for untyped/typed CreateObject uncomment test cases, remove todo More tests and tiny fixes Add a JsonTypeInfoResolver.Combine test for JsonSerializerContext (#4) * Fix JsonTypeInfoResolver.Combine for JsonSerializerContext * Break up failing test Fix simple scenarios for combining contexts (#6) * Fix simple scenarios for combining contexts * feedback JsonSerializerContext combine test with different camel casing Remove unneeded virtual calls & branching when accessing Get & Set delegates (#7) JsonPropertyInfo tests everything minus ShouldSerialize & NumberHandling Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs throw InvalidOperationException rather than ArgumentNullException for source gen when PropertyInfo.Name is assigned through JsonPropertyInfoValues tests for duplicated property names and JsonPropertyInfo.NumberHandling Add tests for NumberHandling and failing tests for ShouldSerialize disable the failing test and add extra checks disable remainder of the failing ShouldSerialize tests, fix working one Fix ShouldSerialize and IgnoreCondition interop Add failing tests for CreateObject + parametrized constructors Fix CreateObject support for JsonConstructor types (#10) * Fix CreateObject support for JsonConstructor types * address feedback Make contexts more combinator friendly (#9) * Make contexts more combinator friendly * remove converter cache * redesign test to account for JsonConstructorAttribute * Combine unit tests * address feedback * Add acceptance tests for DataContract attributes & Specified pattern (#11) * Add private field serialization acceptance test (#13) * tests, PR feedback (#14) * PR feedback and extra tests * Shorten class name, remove incorrect check (not true for polimorphic cases) * Make parameter matching for custom properties map property Name with parameter (#16) * Test static initialization with JsonTypeInfo (#17) * Fix test failures and proper fix this time (#18) * Fix test failures and proper fix this time * reinstate ActiveIssueAttribute * PR feedback and adjust couple of tests which don't set TypeInfoResolver * fix IAsyncEnumerable tests * Lock JsonSerializerOptions in JsonTypeInfo.EnsureConfigured() Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com> Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>

trylek requested a review from davidwrighton May 11, 2021 00:20

trylek changed the title ~~Partial fix for the negative virtual static methods~~ Partial fix for the negative virtual static method tests May 11, 2021

trylek commented May 11, 2021

View reviewed changes

davidwrighton reviewed May 11, 2021

View reviewed changes

trylek added 3 commits May 12, 2021 12:47

Address David's PR feedback

3536ed4

Add virtual static check to SVM resolution logic

d49f3b0

trylek force-pushed the MethodImplChecks branch from b65d2ef to d49f3b0 Compare May 12, 2021 10:48

davidwrighton merged commit 5a8a973 into davidwrighton:feature/staticvirtualinterfaces May 12, 2021

trylek deleted the MethodImplChecks branch January 10, 2022 19:40

github-actions bot locked and limited conversation to collaborators Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial fix for the negative virtual static method tests #17

Partial fix for the negative virtual static method tests #17

trylek commented May 11, 2021

trylek May 11, 2021

davidwrighton May 11, 2021

trylek May 11, 2021

davidwrighton May 11, 2021

trylek May 11, 2021

davidwrighton May 11, 2021

davidwrighton left a comment

davidwrighton May 11, 2021

davidwrighton May 11, 2021

davidwrighton May 11, 2021

davidwrighton commented May 11, 2021

trylek commented May 11, 2021

davidwrighton commented May 12, 2021

Partial fix for the negative virtual static method tests #17

Partial fix for the negative virtual static method tests #17

Conversation

trylek commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwrighton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidwrighton commented May 11, 2021

trylek commented May 11, 2021

davidwrighton commented May 12, 2021