chore: Preserve generated *ViewModel constructors, properties

[jonpryor]

Context: unoplatform/uno@559b1f7

While running the uno.chefs app on macOS under NativeAOT:

dotnet publish -c Release -r osx-x64 -f net10.0-desktop -p:TargetFrameworkOverride=net10.0-desktop -bl \
  Chefs/Chefs.csproj \
  -p:SelfContained=true -p:PublishAot=true -p:IsAotCompatible=true -p:UseSkiaRendering=true \
  -p:IlcGenerateMapFile=true -p:IlcGenerateMstatFile=true -p:IlcGenerateDgmlFile=true -p:IlcGenerateMetadataLog=true \
  -p:EmitCompilerGeneratedFiles=true -p:CompilerGeneratedFilesOutputPath=`pwd`/_gen
Chefs/bin/Release/net10.0-desktop/osx-x64/publish/Chefs

Console output would contain the following warnings:

fail: Uno.UI.Dispatching.NativeDispatcher[0]
      NativeDispatcher unhandled exception
      System.InvalidOperationException: A suitable constructor for type 'Chefs.Presentation.ShellViewModel' could not be located. Ensure the type is concrete and services are registered for all parameters of a public constructor.

With a local patch in Uno.dll, we see that the above
InvalidOperationException is from
Microsoft.Extensions.DependencyInjection:

fail: Uno.UI.Dispatching.NativeDispatcher[0]
      NativeDispatcher unhandled exception
      System.InvalidOperationException: A suitable constructor for type 'Chefs.Presentation.ShellViewModel' could not be located. Ensure the type is concrete and services are registered for all parameters of a public constructor.
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateConstructorCallSite(ResultCache, ServiceIdentifier, Type, CallSiteChain) + 0x3e9
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateExact(ServiceDescriptor, ServiceIdentifier, CallSiteChain, Int32) + 0x181
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.TryCreateExact(ServiceIdentifier, CallSiteChain) + 0x14f
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.CreateCallSite(ServiceIdentifier serviceIdentifier, CallSiteChain callSiteChain) + 0xc9
         at Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteFactory.GetCallSite(ServiceIdentifier, CallSiteChain) + 0x92
         at Microsoft.Extensions.DependencyInjection.ServiceProvider.CreateServiceAccessor(ServiceIdentifier serviceIdentifier) + 0x68
         at System.Collections.Concurrent.ConcurrentDictionary`2.GetOrAdd(TKey, Func`2) + 0xd2
         at Microsoft.Extensions.DependencyInjection.ServiceProvider.GetService(ServiceIdentifier, ServiceProviderEngineScope) + 0x39
         at Uno.Extensions.Navigation.Navigators.ControlNavigator.<>c__DisplayClass8_0.<<CreateViewModel>b__0>d.MoveNext() + 0x80

The problem is that trimming removed the ShellViewModel constructor,
along with other members.

Question: why were these members removed?

Answer 1: because the members weren't used, via static analysis.
This is the case here, as there is no new ShellViewModel() within
uno.chefs or its generated code.

Answer 2: Even when "Answer 1" isn't true, the linker not removing
a member doesn't mean that the member can be accessed through
Reflection! Reflection requires a different set of data.

We thus need to convince the trimmer to not only preserve the members,
but also to allow the members to be used through Reflection.

From unoplatform/uno@559b1f78:

The two predominant ways to do this are via the custom attributes:

• DynamicallyAccessedMembersAttribute
• DynamicDependencyAttribute

At this point we know we have e.g. ShellModel (user-written code),
and ShellViewModel (from src/Uno.Extensions.Reactive.Generator).

How are these types associated with each other at runtime?

They're associated with each other at runtime via a dictionary
generated by BindableViewModelMappingGenerator:

// generated code
namespace Chefs;
partial class ReactiveViewModelMappings {
  public static readonly IDictionary<Type, Type> ViewModelMappings = new Dictionary<Type, Type> {
    { typeof(ShellModel), typeof(ShellViewModel) },
    // …
  };
}

Use of IViewRegistry.FindViewByModel<T>() allows finding the latter
from the former, e.g. views.FindViewByModel<ShellModel>() returns a
MappedViewMap with MappedViewMap.MappedViewModel being
ShellViewModel:

Console.WriteLine(views.FindByViewModel<ShellModel>());
// MappedViewMap { View = , ViewSelector = , ViewModel = Chefs.Presentation.ShellModel, Data = , ResultData = , ViewAttributes = , MappedViewModel = Chefs.Presentation.ShellViewModel }

To preserve the ShellViewModel members, where do we put
[DynamicallyAccessedMembers] or [DynamicDependency]?

There are two plausible places:

1. On ModelAttribute and IModel<T>
2. Within BindableViewModelMappingGenerator-generated code.

(1) ModelAttribute and IModel<T> are viable locations because
ViewModelGenTool_* emits partial types for e.g. ShellModel:

// generated code
[Model(typeof(ShellViewModel))]
partial record ShellModel : IModel<ShellViewModel> {
}

While this works, it also means that using [Model]/IModel<T>
without MVUX would result in app size increases.

Which leaves (2): update BindableViewModelMappingGenerator so that
a codepath uses [DynamicallyAccessedMembers].

Update BindableViewModelMappingGenerator to instead emit code
similar to:

namespace Chefs;
static partial class ReactiveViewModelMappings
{
  private const DynamicallyAccessedMemberTypes Types =
      DynamicallyAccessedMemberTypes.PublicConstructors
    | DynamicallyAccessedMemberTypes.PublicProperties
    ;

  private static void AddMapping(
    Dictionary<Type, Type> dict,
    [DynamicallyAccessedMembers(Types)] Type key,
    [DynamicallyAccessedMembers(Types)] Type value)
    => dict.Add(key, value);

  public static readonly IDictionary<Type, Type> ViewModelMappings;

  static ReactiveViewModelMappings()
  {
    var mappings = new Dictionary<Type, Type>();
    AddMapping(mappings, typeof(Chefs.Presentation.ShellModel), typeof(global::Chefs.Presentation.ShellViewModel));
    // …
    ViewModelMappings = mappings;
  }
}

The introduction of ReactiveViewModelMappings.AddMapping() allows
a place to provide [DynamicallyAccessedMembers], which in turn
allows the trimmer to preserve constructors and properties on
ShellViewModel, which fixes the InvalidOperationException.

Note that properties must also be preserved, as if only constructors
are preserved we will see this failure message:

fail: Uno.UI.DataBinding.BindingPropertyHelper[0]
      The [Pages] property getter does not exist on type [Chefs.Presentation.WelcomeViewModel]

[jonpryor]

Given the investigation, what are possible fixes?

A fix is the original approach in this PR: update ModelAttribute.Bindable/IModel<T> to use [DynamicallyAccessedMembers] to preserve constructors.

An alternate approach may be to update the pattern used "downstream" in uno.chefs, so that instead of a Dictionary<Type, Type> being constructed and passed around, there is instead a set of method invocations:

foo.AddViewModelMapping(typeof(ShellModel), typeof(ShellViewModel));
// …

AddViewModelMapping() could then have Type parameters which use [DynamicallyAccessedMembers], allowing NativeAOT to "see" that things should be preserved.

Unanswered question: what is the real API for this, how are the apps and generators updated to use it, etc.

Possible answer: we could retain the existing "API" of ReactiveViewModelMappings.ViewModelMappings, but turn the field into a property which performs the above AddViewModelMapping() ("somehow") before returning a runtime dictionary?

[jonpryor]

Aside: this is dead code:

uno.extensions/src/Uno.Extensions.Navigation.UI/Navigators/ControlNavigator.cs

Lines 244 to 257 in c0d699a

	// Attempt to create view model using reflection
	try
	{
	var ctr = mapping.ViewModel.GetNavigationConstructor(navigator!, Region.Services!, out var args);
	if (ctr is not null)
	{
	return ctr.Invoke(args);
	}
	}
	catch
	{
	if (Logger.IsEnabled(LogLevel.Information)) Logger.LogInformationMessage("ViewModel not included in RouteMap, and unable to instance using Activator instead of ServiceProvider");
	}
	return default;

It's dead code because services.GetService(mapping!.ViewModel); never returns null; it throws!

uno.extensions/src/Uno.Extensions.Navigation.UI/Navigators/ControlNavigator.cs

Line 238 in c0d699a

var created = services.GetService(mapping!.ViewModel);

This is in fact the unhanded exception I'm seeing, because services.GetService(…) throws. (We never hit mapping.ViewModel.GetNavigationConstructor(…)!)

[jonpryor]

Building upon alternate approaches, another alternate approach -- which maintains API and ABI -- is to update BindableViewModelMappingGenerator.cs to emit code which involves [DynamicallyAccessedMembers]:

diff --git a/src/Uno.Extensions.Reactive.Generator/Bindables/BindableViewModelMappingGenerator.cs b/src/Uno.Extensions.Reactive.Generator/Bindables/BindableViewModelMappingGenerator.cs
index c71c55905..da99c8bf1 100644
--- a/src/Uno.Extensions.Reactive.Generator/Bindables/BindableViewModelMappingGenerator.cs
+++ b/src/Uno.Extensions.Reactive.Generator/Bindables/BindableViewModelMappingGenerator.cs
@@ -33,16 +33,30 @@ internal class BindableViewModelMappingGenerator : ICodeGenTool
 				{this.GetCodeGenAttribute()}
 				public static partial class ReactiveViewModelMappings
 				{{
+					private const global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes Types =
+						  global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors
+						| global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties
+						;
+					private static global::System.Collections.Generic.KeyValuePair<global::System.Type, global::System.Type> E(
+						[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type key,
+						[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type value)
+						=> new global::System.Collections.Generic.KeyValuePair<global::System.Type, global::System.Type>(key, value);
 					/// <summary>
 					/// Gets a mapping from a declared view model type (key) to its bindable counterpart type (value) generated by the feeds platform.
 					/// </summary>
 					/// <remarks>
 					/// This can be used in navigation engine to work only with the declared type while the navigation takes care to use the bindable friendly version.
 					/// </remarks>
-					public static readonly global::System.Collections.Generic.IDictionary<global::System.Type, global::System.Type> ViewModelMappings = new global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type>
+					public static readonly global::System.Collections.Generic.IDictionary<global::System.Type, global::System.Type> ViewModelMappings;
+					
+					static ReactiveViewModelMappings()
 					{{
-						{_bindableVMs.Select(kvp => $"{{ typeof({kvp.Key}), typeof({kvp.Value}) }},").Align(6)}
-					}};
+						var mappings = new global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type>();
+						var collection = (global::System.Collections.Generic.ICollection<
+							global::System.Collections.Generic.KeyValuePair<global::System.Type, global::System.Type>>) mappings;
+						{_bindableVMs.Select(kvp => $"collection.Add(E(typeof({kvp.Key}), typeof({kvp.Value})));").Align(6)}
+						ViewModelMappings = mappings;
+					}}
 				}}
 			}}".Align(0);
 

Resulting C# code is:

namespace Chefs
{
	[global::System.CodeDom.Compiler.GeneratedCodeAttribute("BindableViewModelMappingGenerator", "1")]
	public static partial class ReactiveViewModelMappings
	{
		private const global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes Types =
			  global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicConstructors
			| global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicProperties
			;
		private static global::System.Collections.Generic.KeyValuePair<global::System.Type, global::System.Type> E(
			[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type key,
			[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type value)
			=> new global::System.Collections.Generic.KeyValuePair<global::System.Type, global::System.Type>(key, value);
		/// <summary>
		/// Gets a mapping from a declared view model type (key) to its bindable counterpart type (value) generated by the feeds platform.
		/// </summary>
		/// <remarks>
		/// This can be used in navigation engine to work only with the declared type while the navigation takes care to use the bindable friendly version.
		/// </remarks>
		public static readonly global::System.Collections.Generic.IDictionary<global::System.Type, global::System.Type> ViewModelMappings;

		static ReactiveViewModelMappings()
		{
			var mappings = new global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type>();
			var collection = (global::System.Collections.Generic.ICollection<
				global::System.Collections.Generic.KeyValuePair<global::System.Type, global::System.Type>>) mappings;
			collection.Add(E(typeof(Chefs.Presentation.CookbookDetailModel), typeof(global::Chefs.Presentation.CookbookDetailViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.CreateUpdateCookbookModel), typeof(global::Chefs.Presentation.CreateUpdateCookbookViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.FavoriteRecipesModel), typeof(global::Chefs.Presentation.FavoriteRecipesViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.FilterModel), typeof(global::Chefs.Presentation.FilterViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.GenericDialogModel), typeof(global::Chefs.Presentation.GenericDialogViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.HomeModel), typeof(global::Chefs.Presentation.HomeViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.LiveCookingModel), typeof(global::Chefs.Presentation.LiveCookingViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.LoginModel), typeof(global::Chefs.Presentation.LoginViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.MainModel), typeof(global::Chefs.Presentation.MainViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.MapModel), typeof(global::Chefs.Presentation.MapViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.NotificationsModel), typeof(global::Chefs.Presentation.NotificationsViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.ProfileModel), typeof(global::Chefs.Presentation.ProfileViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.RecipeDetailsModel), typeof(global::Chefs.Presentation.RecipeDetailsViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.RegistrationModel), typeof(global::Chefs.Presentation.RegistrationViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.SearchModel), typeof(global::Chefs.Presentation.SearchViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.SettingsModel), typeof(global::Chefs.Presentation.SettingsViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.ShellModel), typeof(global::Chefs.Presentation.ShellViewModel)));
			collection.Add(E(typeof(Chefs.Presentation.WelcomeModel), typeof(global::Chefs.Presentation.WelcomeViewModel)));
			ViewModelMappings = mappings;
		}
	}
}

The change is to use ICollection<T>.Add(T) instead of Dictionary<TKey, TValue>.Add(TKey, Value), which allows us to introduce a E(Type, Type) method with the appropriate [DynamicallyAccessedMembers] information that NativeAOT needs.

This allows things to work without the original ModelAttribute.Bindable/IModel<T> changes.

What I'm still uncertain about is whether it's the correct fix.

[jonpryor]

The benefit to using IDictionary<KeyValuePair<Type, Type>>.Add(KeyValuePair<Type, Type>) is that no new API surface area is introduced.

The downside is that it should (technically) be slower, as its going through an interface invocation instead of hitting Dictionary<Type, Type> directly.

We could change what the generator produces to still not add new API while avoiding interface invocations.

Another approach would be to add new API:

namespace Uno.Extensions.Reactive;

public class ModelToViewModelMapping : IDictionary<Type, Type> {
    public void Add([DAM(…)] Type key, [DAM(…)] Type value) => …
}

which would allow minimal changes to the generator -- it would emit new ModelToViewMapping() instead of new Dictionary<Type, Type> -- while adding new API surface area. I'm not sure that this is a good tradeoff, and will break any code that assumes that ViewModelMappings is a Dictionary<Type, Type>.

---

We thus have 2-3 approaches to preserving constructors on Uno.Extensions.Reactive.Generator-generated types:

1. Update ModelAttribute.Bindable and IModel<T>, as was originally done in this PR.
2. Update Uno.Extensions.Reactive.Generator output so that .ViewModelMappings generation goes through methods which are appropriately annotated with [DynamicallyAccessedMembers]
3. (2) but the "methods appropriately annotated" are public API.

Given these three approaches, and given that it's Uno.Extensions.Reactive.Generator-generated types which are in play, my personal preference is (1).

[dr1rrb]

Building upon alternate approaches, another alternate approach -- which maintains API and ABI -- is to update BindableViewModelMappingGenerator.cs to emit code which involves [DynamicallyAccessedMembers]:

../..

What I'm still uncertain about is whether it's the correct fix.

Yes definitely the best approach IMO. That dictionary is generated for the navigation to be able to know which VM to create when the user attempts to navigate to a given model type.

About the Add and the perf impact : negligible in that case. But I think a simpler code that would be enough could be:

		static ReactiveViewModelMappings()
		{
			var mappings = new global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type>();
			Add(mappings, typeof(Chefs.Presentation.CookbookDetailModel), typeof(global::Chefs.Presentation.CookbookDetailViewModel));
			Add(mappings, typeof(Chefs.Presentation.CreateUpdateCookbookModel), typeof(global::Chefs.Presentation.CreateUpdateCookbookViewModel));

			ViewModelMappings = mappings;
		}
		
private static void Add(
	global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type> mappings,
	[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type key,
	[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type value)
		=> mappings.Add(key, value);

100% retro compatible.

[dr1rrb]

Building upon alternate approaches, another alternate approach -- which maintains API and ABI -- is to update BindableViewModelMappingGenerator.cs to emit code which involves [DynamicallyAccessedMembers]:
../..
What I'm still uncertain about is whether it's the correct fix.

Yes definitely the best approach IMO. That dictionary is generated for the navigation to be able to know which VM to create when the user attempts to navigate to a given model type.

About the Add and the perf impact : negligible in that case. But I think a simpler code that would be enough could be:

		static ReactiveViewModelMappings()
		{
			var mappings = new global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type>();
			Add(mappings, typeof(Chefs.Presentation.CookbookDetailModel), typeof(global::Chefs.Presentation.CookbookDetailViewModel));
			Add(mappings, typeof(Chefs.Presentation.CreateUpdateCookbookModel), typeof(global::Chefs.Presentation.CreateUpdateCookbookViewModel));

			ViewModelMappings = mappings;
		}
		
private static void Add(
	global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type> mappings,
	[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type key,
	[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type value)
		=> mappings.Add(key, value);

100% retro compatible.

Actually thinking again about it, I think the [DAM] should be set only on the value:

private static void Add(
	global::System.Collections.Generic.Dictionary<global::System.Type, global::System.Type> mappings,
	global::System.Type key,
	[global::System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers(Types)] global::System.Type value)
		=> mappings.Add(key, value);

We are not supposed to create dynamically instances of the model itself (i.e. the key). They are created without releflection in the generated ctor of the VM ^^