search

dotnet / android

.NET for Android provides open-source bindings of the Android SDK for use with .NET managed languages such as C#
MIT License
1.93k stars 532 forks source link

Enable `DefaultValueAttributeSupport` in partial trim mode. #9525

Closed matouskozak closed 2 days ago

matouskozak commented 2 days ago

Currently, users can experience runtime crashes related to the use of DefaultValue attributes (taking type and string) without any trim warnings (see https://github.com/dotnet/runtime/issues/109724). This is because the trim warnings are disable in partial trim mode and this use-case of DefaultValue attributes is not trim compatible.

Workaround around this is to enable _DefaultValueAttributeSupport to prevent customer apps from crashing without trim warnings. App will still crash in TrimMode="full" but user would get warned during compile time.

jonathanpeppers commented 2 days ago

Should we just add Sven's minimal repro:

To a new test in a new System.Xml folder here:

matouskozak commented 2 days ago

Should we just add Sven's minimal repro:

To a new test in a new System.Xml folder here:

Thanks, added in https://github.com/dotnet/android/pull/9525/commits/14cfd74a6f924109abf13894e5106db44ada51a5. What do you think?

jonathanpeppers commented 2 days ago

It looks like a projitems file needed an update, I pushed this change.

We used to share the files between Xamarin & .NET; it could be refactored in the future.

matouskozak commented 2 days ago

The failures are related:

System.InvalidOperationException : XmlTypeReflectionError, System.XmlTests.C
----> System.ArgumentException : RuntimeInstanceNotAllowed

at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(TypeModel , String , ImportContext , String , XmlAttributes , Boolean , Boolean , RecursionLimiter )
   at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(TypeModel , String , ImportContext , String , XmlAttributes , RecursionLimiter )
   at System.Xml.Serialization.XmlReflectionImporter.ImportElement(TypeModel , XmlRootAttribute , String , RecursionLimiter )
   at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(Type , XmlRootAttribute , String )
   at System.Xml.Serialization.XmlSerializer..ctor(Type type, String defaultNamespace)
   at System.Xml.Serialization.XmlSerializer..ctor(Type type)
   at System.XmlTests.XmlSerializerTest.TrimmingDefaultValueAttribute()
   at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object , BindingFlags )
--ArgumentException
   at System.ComponentModel.DefaultValueAttribute.get_Value()
   at System.Xml.Serialization.XmlAttributes..ctor(ICustomAttributeProvider )
   at System.Xml.Serialization.XmlReflectionImporter.GetAttributes(MemberInfo )
   at System.Xml.Serialization.XmlReflectionImporter.InitializeStructMembers(StructMapping , StructModel , Boolean , String , RecursionLimiter )
   at System.Xml.Serialization.XmlReflectionImporter.ImportStructLikeMapping(StructModel , String , Boolean , XmlAttributes , RecursionLimiter )
   at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(TypeModel , String , ImportContext , String , XmlAttributes , Boolean , Boolean , RecursionLimiter )
jonathanpeppers commented 2 days ago

It passed in Debug mode, which was broken before?

image

This app has TrimMode=full:

https://github.com/dotnet/android/blob/a1fab8bfdbaa2baec7abd8dd07a08969e0b49e38/tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Mono.Android.NET-Tests.csproj#L37-L39

Can the test check a feature flag at runtime (AppContext.TryGetSwitch), and ignore in some cases? Ideally we'd get a green test in Debug and the rest ignore.

matouskozak commented 2 days ago

It passed in Debug mode, which was broken before?

image

This app has TrimMode=full:

https://github.com/dotnet/android/blob/a1fab8bfdbaa2baec7abd8dd07a08969e0b49e38/tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Mono.Android.NET-Tests.csproj#L37-L39

Can the test check a feature flag at runtime (AppContext.TryGetSwitch), and ignore in some cases? Ideally we'd get a green test in Debug and the rest ignore.

No, it was the release (related to illink) that was broken but the "fix" is only for TrimMode="partial" so if the test are running in full trim mode then they will fail. We can either extend the "fix" to all trim modes or just add the _DefaultValueAttributeSupportto https://github.com/dotnet/android/blob/a1fab8bfdbaa2baec7abd8dd07a08969e0b49e38/tests/Mono.Android-Tests/Runtime-Microsoft.Android.Sdk/Mono.Android.NET-Tests.csproj#L40-L41

Note: I limited the "fix" to partial mode because that's where no trim warnings are shown and yet it failed. In full mode the warnings will show potential issues so that users can add approriate feature switches if needed.

jonathanpeppers commented 2 days ago

Let me add an extra test run, that runs the tests with TrimMode=partial.

jonathanpeppers commented 2 days ago

I think it's working now:

image

I reran, because there are some other random, http failures.