Dynamic setters are now shared between types

[MrJul]

Following up on #65, this PR makes dynamic setter methods shared between all types.

My first try at implementing this kept creating the setters type in PropertyAssignmentEmitter, but this posed several problems:

• The setters type must be "top level enough" to be accessible by all compiled types. Thus it can't be a sub type of the current type (potentially nested private). Defining one directly at the assembly level seemed wrong since only the compiler caller knows where it wants the types to be created.
• It was hard to define when to call CreateType (effectively building the type for SRE) on the setters type, since it needs to be shared between several compilation calls.

Instead, this implementation requires the caller to define the setters type itself and pass it to the compiler. The caller can define the type as it sees fit and controls its sharing between calls and its final creation. This is very similar to what Avalonia already does for its extra generated XAML types (XamlIlHelpers, XamlILTrampolines, etc.).

The only downside is that this type can be seen as an implementation detail: another non-IL backend emitter might not need it. I think the pros outweigh the cons, and that a potential other implementation might still want to use this type to define its setters.

After this PR, Avalonia.Themes.Fluent without fonts goes from 815 KB (337 setters) to 774 KB (19 setters).

[kekekeks]

Defining one directly at the assembly level seemed wrong since only the compiler caller knows where it wants the types to be created.

We could provide a callback, so it shouldn't be a problem.

It was hard to define when to call CreateType (effectively building the type for SRE) on the setters type, since it needs to be shared between several compilation calls.

Setter sharing is only required in Cecil mode, with SRE we are free to call CreateType after a particular XAML file is finished.

I haven't read the code yet, but how do you handle "XAML references a private nested type" scenario?

[MrJul]

We could provide a callback, so it shouldn't be a problem.

I thought about it, but it doesn't solve the lifetime problem as one might want the setters to be shared between all compiler calls, some calls only, not at all... The callback can of course provide the type, but then the compiler has no idea when it should build it. Nothing unsolvable (it was my first approach after all), I felt it was cleaner and easier for the caller to control the setters type. Happy to discuss and change if needed :)

Note that I'm talking about XamlX as a general purpose compiler here since it's very generic (great work by the way, when I started looking at the code, I really didn't expect XamlX to be so nicely separated!), not for Avalonia specific cases, which are well defined and scoped.

I haven't read the code yet, but how do you handle "XAML references a private nested type" scenario?

I don't, sorry if I wasn't clear, the "nested private type" I was talking referred to a setter being generated first in a private class (closure), then being incorrectly reused from another type. This doesn't happen here since the caller is responsible for providing an accessible type that will hold the setters.

In general since you mention referencing private types from XAML files, I don't think XamlX does any visibility check, whether that's using private types or private members?
For example:

public class C { private string P { get; set; } }

<C xmlns='test' P='foo' />

compiles, but fails at runtime with attempt by method 'Populate' to access method 'C.set_P(System.String)' failed.

[kekekeks]

This doesn't happen here since the caller is responsible for providing an accessible type that will hold the setters.

Consider the following scenario:

class MyControl : UserControl
{
    private object Foo {...} // avalonia property
}

<UserControl x:Class="MyControl" Foo="{DynamicResource Bar}" />

For the user control itself the Foo property is accessible, but it's not accessible for a separate class where dynamic setters are cached. Same with private nested types.

[kekekeks]

but then the compiler has no idea when it should build it

CreateType is a no-op for Cecil backend (or any backend that would emit code ahead of time), it's only required for System.Reflection.Emit backend where we don't need setter reuse anyway. Even if we did reuse types, we'd still need to call CreateType before consuming any compilation results.
Providing a pre-created type is completely fine too, don't worry about it

[MrJul]

For the user control itself the Foo property is accessible, but it's not accessible for a separate class where dynamic setters are cached. Same with private nested types.

Good one! Indeed that would break currently.
I'll add tests covering this case and rework the PR completely since one shared type obviously isn't a solution.

CreateType is a no-op for Cecil backend (or any backend that would emit code ahead of time), it's only required for System.Reflection.Emit backend where we don't need setter reuse anyway. Even if we did reuse types, we'd still need to call CreateType before consuming any compilation results.

I know about CreateType only being useful for SRE (Avalonia doesn't bother to call it when using Cecil), but setters reuse already happens even for SRE, eg.:

<Grid>
  <Border Background="{StaticResource Foo}" />
  <Border Background="{StaticResource Foo}" />
</Grid>

One compilation, twice the same setter. Anyways, point well understood :) Since I'm reworking this I'll add some options/callbacks so the emitter and caller can work together to determine whether a cache is needed and which type should be used.

[MrJul]

Pass 3!

A new interface IXamlDynamicSetterContainerProvider has been added, which provides the type into which dynamic setters are generated for a given property and context.

The default implementation of this interface, DefaultXamlDynamicSetterContainerProvider, can accept a type used to share the setters between different compiled types. If a shared type isn't specified (which is the default) or if the property to set isn't publicly accessible, there's no sharing: the current type is used and a private method is generated.

In Avalonia, the Cecil compiler can define a shared setters type before compiling and pass it to the DefaultXamlDynamicSetterContainerProvider. SRE can simply use the default since setters sharing isn't needed here.

[kekekeks]

I'm terribly sorry for my inactivity, it was caused by covid and then recent events. Hopefully I would either merge this or provide feedback in a week or two.

[MrJul]

I'm terribly sorry for my inactivity, it was caused by covid and then recent events. Hopefully I would either merge this or provide feedback in a week or two.

No worries, take your time :) Thank you for letting me know.

[MrJul]

I just rebased this PR onto master and resolved conflicts, if you happen to have some time to review :)

[kekekeks]

LGTM in general, would be nice to have a test or two for effectively private methods not being placed to the shared cache.

[MrJul]

LGTM in general, would be nice to have a test or two for effectively private methods not being placed to the shared cache.

I've added the requested tests. This involved a bit of refactoring to the CompilerTestBase class to be able to create type builders in the derived test classes without depending on Cecil or SRE.

Note that the private property case is only enabled on Cecil, since we need to extend the root type to access the property, which can't be done with SRE.

I've also added a new commit to handle protected properties in the same way as the private ones.

[maxkatz6]

@kekekeks can it be merged?