[Trimming] Add new API to create TypedBindings from expression by simonrozsival · Pull Request #19995 · dotnet/maui

simonrozsival · 2024-01-19T08:24:42Z

Description of Change

TypedBinding is a great replacement for the reflection-based Binding class. Unfortunately, it can only be used through XamlC. This PR adds a new generic overload of the SetBinding extension method which will allow developers to use the typed binding from C#/F#.

I recommend reviewing the PR commit-by-commit.

Note: This PR depends on #20415 (support for relative binding sources).

Expressions

In this PR, I'm introducing a way to replace Binding with TypedBinding through a new SetBinding<TSource, TProperty> extension method. The extension method expects a getter expression instead of the string path. This has several advantages:

faster bindings (no reflection)
code completion in IDE
trimming-friendly, works well with NativeAOT

The extension method parses the expression and it uses it to automatically generate the handlers array the same way XamlC generates it. In most cases, the getter expression can be used to generate the setter as well. The only case where this is not possible to compile the setter is when the last part of the setter is a custom indexer (we don't have access to the setter method). In this case, the developer needs to pass the setter Action manually.

The "compiled" getters and setters are almost as fast as explicit getters and setters would be. This change doesn't increase the size of the final app bundle too much either: the .ipa build using NativeAOT was roughly 1.25% bigger than with just the reflection-based bindings.

Performance implications

Based on running a few benchmarks (BindingBenchmarker, modified TypedBindingBenchmarker), constructing TypedBindings from expressions is slightly slower than parsing the string path and the "compilation" also allocates more memory. I need to collect more data.

TODO:

Gather and present the performance data
Is there an impact on app startup?

Follow-ups

There are still many places where we use new Binding("...") and SetBinding("...") in the MAUI codebase. These can be migrated in some follow-up PR. There's no need to do the migration right now in this PR (it's quite big already).
- ControlGallery
- Controls.Sample
- Controls.Sample.UITests
- Windows specific code
- Tizen specific code
- Unit tests (Controls)
- Examples in XML docs
ShellSearchResultsRenderer (iOS) and ShellSearchViewAdapter (Android) default template can't be migrated to typed binding and it won't work with typed binding. That's the (only) reason why I can't remove any trimming/AOT warnings in tests. We will need to disable it using a feature switch for native AOT in some follow-up PR.
Make the SetBinding<TSource, TProperty> method public + update PublicAPI.Unshipped.txt
- I wasn't able to update PublicAPI.Unshipped.txt using VSCode or manually for some reason.

Issues Fixed

Contributes to #19397
Closes #19912

simonrozsival · 2024-01-22T14:49:44Z

/cc @StephaneDelcroix @jonathanpeppers @vitek-karas

jsuarezruiz

D:\a\_work\1\s\src\Compatibility\Core\src\Android\Renderers\ListViewAdapter.cs(739,5): error IL2026: Using member 'Microsoft.Maui.Controls.BindableObjectExtensions.SetBinding(BindableObject, BindableProperty, String, BindingMode, IValueConverter, String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. The use of string-based binding is not trimming safe. Use the expression-based SetBinding overloads instead. [D:\a\_work\1\s\src\Compatibility\Core\src\Compatibility.csproj::TargetFramework=net9.0-android]

simonrozsival · 2024-02-02T13:01:28Z

/cc @jonathanpeppers this is ready for review now

jonathanpeppers · 2024-02-02T17:00:38Z

src/Controls/src/Core/Shell/BaseShellItem.cs

+				if (bo is MenuItem)
+				{
+					imageBinding = TypedBindingFactory.Create(static (MenuItem item) => item.IconImageSource);
+					labelBinding = TypedBindingFactory.Create(static (MenuItem item) => item.Text);
+				}
+				else if (bo is BaseShellItem)
+				{
+					imageBinding = TypedBindingFactory.Create(static (BaseShellItem item) => item.FlyoutIcon);
+					labelBinding = TypedBindingFactory.Create(static (BaseShellItem item) => item.Title);
+				}
+				else
+				{
+					throw new InvalidOperationException("Invalid type for flyout item cell.");
+				}


Are there any other types that need to work here besides MenuItem and BaseShellItem? What was the exception you would have gotten previously for using the wrong type inside a flyout?

Or did previously you got no error, and the bindings just "didn't work"?

src/Controls/tests/Core.UnitTests/TypedBindingFromExpressionUnitTests.cs

jonathanpeppers · 2024-02-02T17:12:00Z

src/Controls/src/Core/TypedBindingFactory.cs

+
+namespace Microsoft.Maui.Controls
+{
+	internal static class TypedBindingFactory


Looking at the code in here, it supports:

ParameterExpression

UnaryExpression

MemberExpression

MethodCallExpression

BinaryExpression

Does this need to be exhaustive yet? Should we at least put a <summary> comment of which expressions would work?

It seems like we'd need to implement them all if this was able to be used by customers -- which, isn't the case yet.

I think it's perfectly OK to support only a subset and throw if the provided expression is too complex. ASP.NET uses similar trick and also implements only a limited subset of the expression tree support. For example https://github.com/dotnet/aspnetcore/blob/c2a9c205aa65af4031e0fd6471c2aa89bde095c0/src/Mvc/Mvc.ViewFeatures/src/ExpressionMetadataProvider.cs#L72C59-L72C94.

I focused only on mirroring what customers can do with binding paths - accessing members, arrays, and indexers (as described in https://learn.microsoft.com/en-us/dotnet/maui/fundamentals/data-binding/binding-path). From those 5 expression types, it also only supports a subset of them (for example unary operator - just Convert, AsType, method call - just special name methods get_Indexer, get_Item), but it's all that's necessary to support the expressions that are similar to the string paths in classic bindings. As @StephaneDelcroix told me, the subset of bindings I implemented is very close to one of the earlier expression-based APIs that Xamarin.Forms once had (https://github.com/xamarin/Xamarin.Forms/blob/5.0.0/Xamarin.Forms.Core/Binding.cs#L332).

You're definitely right that the documentation part is lacking. I'm assuming we'll need to update the official documentation if this feature goes in later, but I can definitely add that <summary> in this PR. Thanks for the suggestion.

jonathanpeppers · 2024-02-02T17:14:27Z

src/Controls/src/Core/TemplatedItemsList.cs

 				groupProxy.HeaderContent = _itemsView.CreateDefault(ListProxy.ProxiedEnumerable);
 				groupProxy.HeaderContent.BindingContext = groupProxy;
-				groupProxy.HeaderContent.SetBinding(TextCell.TextProperty, "Name");
+				groupProxy.HeaderContent.SetBinding(TextCell.TextProperty, static (TemplatedItemsList<TView, TItem> list) => list.Name);


We are using BannedApiAnalyzers for a few things in this repo, do we need to also ban SetBinding(BindableProperty,string)? As the typed binding used here seems like it could be better for both performance and trimming/nativeaot.

Yes, I think that would be a good thing to do, but maybe in a follow-up PR that removes the remaining usages of this API in the app (mostly Windows and Tizen specific code). In fact, I think we should ban using the Binding class altogether.

We should also add [RequiresUnreferencedCode] to the Binding class so that customers get trimming warnings if they use it in their apps.

We should also add [RequiresUnreferencedCode] to the Binding class so that customers get trimming warnings if they use it in their apps.

Won't we have to do this - to "fix" all the warnings - anyway?

@vitek-karas we don't need it to fix all the warnings we get when we build the dotnet new maui app. Binding internally uses BindingExpression which has trimming-unsafe code so we get a warning anyway. I think we should add it so that when customers build their apps where they use Binding, the warnings appear in their codebase and not in the MAUI assembly.

I think we should add it so that when customers build their apps where they use Binding, the warnings appear in their codebase and not in the MAUI assembly.

That was exactly the reason I had in mind :-). Thanks.

ivanpovazan · 2024-02-05T14:08:52Z

Just for my understanding (I am still getting familiar with the context), regarding:

Unfortunately, it can only be used through XamlC and only in some limited cases where it can replace the old Binding (x:DataType annotation + no explicit source).

Would the changes in this PR enable us to improve XamlC codegen - covering more cases and making better use of TypedBindings as well, or it is purely to provide adequate API for using TypedBindings programatically?

src/Controls/tests/Core.UnitTests/TypedBindingFromExpressionUnitTests.cs

simonrozsival · 2024-02-05T15:56:14Z

Would the changes in this PR enable us to improve XamlC codegen - covering more cases and making better use of TypedBindings as well, or it is purely to provide adequate API for using TypedBindings programatically?

Yes, we should be able to compile a lot more bindings into typed bindings. I'll update the description to make it clear.

src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs

rmarinho · 2024-02-12T01:52:04Z

/rebase

rmarinho · 2024-02-12T01:52:20Z

/rebase

…d bindings

This reverts commit 662bb3d.

This reverts commit d280060.

simonrozsival · 2024-02-14T11:09:08Z

Closing this prototype for now. We might revive it later.

simonrozsival changed the title ~~[Trimming] Allow creating TypedBinding using an expression~~ [Trimming] Allow creating TypedBinding from expressions Jan 19, 2024

simonrozsival added the perf/app-size Application Size / Trimming (sub: perf) label Jan 19, 2024

simonrozsival mentioned this pull request Jan 23, 2024

Add support for RelativeBindingSource to TypedBinding #20101

Closed

jsuarezruiz assigned StephaneDelcroix Jan 26, 2024

jsuarezruiz reviewed Jan 26, 2024

View reviewed changes

github-actions bot force-pushed the typed-binding-from-expression branch from fe8e866 to f5a5859 Compare January 29, 2024 10:56

rmarinho deleted the branch dotnet:net9.0 January 31, 2024 11:32

rmarinho closed this Jan 31, 2024

simonrozsival deleted the typed-binding-from-expression branch January 31, 2024 12:41

simonrozsival restored the typed-binding-from-expression branch January 31, 2024 12:41

simonrozsival reopened this Jan 31, 2024

simonrozsival force-pushed the typed-binding-from-expression branch from f5a5859 to d3465b7 Compare February 1, 2024 17:18

simonrozsival changed the title ~~[Trimming] Allow creating TypedBinding from expressions~~ [Trimming] Improve TypedBindings Feb 1, 2024

simonrozsival force-pushed the typed-binding-from-expression branch from 9c4799d to 3a929ed Compare February 2, 2024 12:52

simonrozsival marked this pull request as ready for review February 2, 2024 13:00

simonrozsival requested a review from a team as a code owner February 2, 2024 13:00

simonrozsival requested review from StephaneDelcroix, jsuarezruiz and mattleibow February 2, 2024 13:00

simonrozsival mentioned this pull request Feb 2, 2024

[X] Compile bindings with explicit source #20266

Closed

jsuarezruiz requested review from jsuarezruiz and removed request for jsuarezruiz February 2, 2024 13:06

jonathanpeppers reviewed Feb 2, 2024

View reviewed changes

ivanpovazan reviewed Feb 5, 2024

View reviewed changes

src/Controls/tests/Core.UnitTests/TypedBindingFromExpressionUnitTests.cs Outdated Show resolved Hide resolved

simonrozsival commented Feb 5, 2024

View reviewed changes

src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs Outdated Show resolved Hide resolved

simonrozsival force-pushed the typed-binding-from-expression branch from b2f2c8b to 7d7f60b Compare February 6, 2024 08:17

simonrozsival marked this pull request as draft February 7, 2024 10:55

simonrozsival changed the title ~~[Trimming] Improve TypedBindings~~ [Trimming] Add new API to create TypedBindings from expression Feb 7, 2024

simonrozsival mentioned this pull request Feb 9, 2024

[iOS] Resolving Trimming Warnings for dotnet new maui #19397

Closed

19 tasks

simonrozsival linked an issue Feb 9, 2024 that may be closed by this pull request

[Proposal] Add a new public API to allow defining TypedBinding in code #19912

Closed

simonrozsival added 11 commits February 12, 2024 01:52

Implement RelativeBindingSource support for TypedBinding

32668dd

Add factory for creating TypedBinding from Expression

799b384

Add tests for relative binding source support

aa603b5

Add tests for creating TypedBinding from Expression

3be0a0c

Migrate internal uses of path-based bindings to expression-based type…

46e9ad1

…d bindings

Merge TypedBinding tests

14fa834

Add doc comment

4f4132f

Optimizations

2f500ff

Test cleanup

8206eaa

Revert "Implement RelativeBindingSource support for TypedBinding"

67ea2ff

This reverts commit 662bb3d.

Revert "Add tests for relative binding source support"

a1be504

This reverts commit d280060.

github-actions bot force-pushed the typed-binding-from-expression branch from 938264d to a1be504 Compare February 12, 2024 01:52

simonrozsival mentioned this pull request Feb 14, 2024

[Proposal] Compiling typed bindings using source generators #20574

Closed

simonrozsival closed this Feb 14, 2024

simonrozsival mentioned this pull request Feb 20, 2024

[Trimming] Use typed bindings internally #20567

Merged

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

Eilon added the area-publishing Issues with the app packaging/publishing process (ipk/apk/msix/trimming) label May 10, 2024

Conversation

simonrozsival commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Expressions

Performance implications

Follow-ups

Issues Fixed

Uh oh!

simonrozsival commented Jan 22, 2024

Uh oh!

jsuarezruiz left a comment

Choose a reason for hiding this comment

Uh oh!

simonrozsival commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanpovazan commented Feb 5, 2024

Uh oh!

Uh oh!

simonrozsival commented Feb 5, 2024

Uh oh!

Uh oh!

rmarinho commented Feb 12, 2024

Uh oh!

rmarinho commented Feb 12, 2024

Uh oh!

simonrozsival commented Feb 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

simonrozsival commented Jan 19, 2024 •

edited

Loading

simonrozsival commented Feb 2, 2024 •

edited

Loading