perf: ImplementedRoutedEvents evaluated at compile-time via a source generator

[Youssef1313]

GitHub Issue (If applicable): closes #6879, closes #4288

PR Type

What kind of change does this PR introduce?

• Performance

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

• [ ] Docs have been added/updated which fit documentation template (for bug fixes / features)
• [ ] Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
• [ ] Validated PR Screenshots Compare Test Run results.
• [ ] Contains NO breaking changes
• [ ] Associated with an issue (GitHub or internal) and uses the automatic close keywords.
• [ ] Commits must be following the Conventional Commits specification.

Other information

Internal Issue (If applicable):

[gitpod-io[bot]]

[Youssef1313]

@jeromelaban This is ready for review now.

[Youssef1313]

Would be nice to find a way to test this automatically. Bugs or regressions in this code could potentially become very expensive and difficult to diagnose.

In what way would you like to see the tests? My ideas are:

• Source-generated tests that are persisted to disk for every control?
• Tests that show and assert against the generated code for various test cases? Are there any particular test cases you're interested in?

[Youssef1313]

@carldebilly Can you check the way I'm testing this? Obviously the current tests are not enough, I'll expand them, just asking about the how you like the approach :)

Note to self: I need to handle nested classes case. Wondering if generators like native ctor supports this? @carldebilly do you know?

[Youssef1313]

Pinging @carldebilly @jeromelaban for review.

[jeromelaban]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[jeromelaban]

I'm a bit bothered by the introduction of an override specific to Uno on controls. If we ever need to change the signature, it'll be troublesome and would be a breaking change.

An intermediate change would be to use the generator for types inside of Uno only, and make the virtual method private protected.

As for making it available to external types, maybe with a registry similar to the one defined with bindable metadata, or by using a syntax like this in the generated type:

private static readonly RoutedEventFlag __internal_implemented_routed_event = 
   ControlHelper.RegisterImplementedRoutedEvents(...RoutedEventFlag...);

The value would serve nothing but allow the execution in the .cctor without having to define one, and the control helper would avoid making the Control API surface wider.

Also, we're already caching the result in a dictionary, that would repurpose it.

[Youssef1313]

An intermediate change would be to use the generator for types inside of Uno only, and make the virtual method private protected.

That would be a breaking change too since there is already an existing method protected static GetImplementedRoutedEvents on Control (which we will change and make private protected. Do you mean you're okay with this as "one-time" breaking change, but want to avoid potential breaking changes later? or there shouldn't be breaking changes here at all?

As for the ControlHelper, this class seems to be used for testing only currently. Did you mean to create a new one outside of the test assembly and store the RoutedEventFlag in a dictionary via Register.... method?

[Youssef1313]

• I made it private protected, not sure if that's enough @jeromelaban
• CI is failing with: Removed method Uno.UI.Xaml.RoutedEventFlag Windows.UI.Xaml.Controls.Control.GetImplementedRoutedEvents(System.Type type) not found in ignore set., but I added that to PackageDiffIgnore 😕

[jeromelaban]

@Youssef1313 You added it to the version block that is not the latest available (3.10.7). It's going to change soon, so it'll have to be updated again anyways :)

For the private protected, that's not enough. The generator will try to be executed in apps, which won't have access to the base for the generated method. I think we should go the helper route, so all types can benefit from it, not just the internal types.

[Youssef1313]

The generator will try to be executed in apps, which won't have access to the base for the generated method.

This shouldn't happen since I have:

				if (!getImplementedRoutedEventsSymbol.ContainingAssembly.IsSameAssemblyOrHasFriendAccessTo(context.Compilation.Assembly))
				{
					return;
				}

---

I think we should go the helper route, so all types can benefit from it, not just the internal types.

I need some more info on that. The only ControlHelper I found is used in RuntimeTests only.

[jeromelaban]

The generator will try to be executed in apps, which won't have access to the base for the generated method.

This shouldn't happen since I have:

				if (!getImplementedRoutedEventsSymbol.ContainingAssembly.IsSameAssemblyOrHasFriendAccessTo(context.Compilation.Assembly))
				{
					return;
				}

I think we should go the helper route, so all types can benefit from it, not just the internal types.

I need some more info on that. The only ControlHelper I found is used in RuntimeTests only.

Ok got it for the friends assembly validation. For the control helper, you'll need to create it. It's just a simple class that forwards the access to the internal method that provides access to the GetImplementedRoutedEvents implementation.

[MartinZikmund]

@Youssef1313 sorry, missed this one and made a duplicate issue (#8411). Is there a chance you will get to this soon or should I pick it up? Important note - as part of my implementation of BringIntoView support I noticed the OnBringIntoView virtual method is not on Control but on UIElement (along with a few other "handler" methods) - so this will need to be taken into account.

[Youssef1313]

@jeromelaban @MartinZikmund Can you take a look please? Thanks!

[carldebilly]

@Youssef1313 I NEVER thought this feature would be that complicated to implement. Anyway, it seems ready to merge :-)

[MartinZikmund]

Letssss goooooo! 🚀🚀🚀

[MartinZikmund]

@Youssef1313 can you please try rebasing this? It seems there are some conflicts and build errors 👀

[jeromelaban]

/azp run

[azure-pipelines[bot]]

Azure Pipelines successfully started running 2 pipeline(s).

[carldebilly]

Finally merged... that was a long road. :-)