Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Binding SG] Enable usage of private fields and properties in SetBinding path #23810

Merged

Conversation

jkurdek
Copy link
Member

@jkurdek jkurdek commented Jul 24, 2024

Description of Change

The existing implementation of the SetBinding method cannot utilize private properties or fields in the getter, as the generated code is created in a separate class, making it unable to access these private members.

This PR addresses this limitation by introducing the capability to handle inaccessible fields and properties in the binding path through the generation of unsafe accessors.


Private Fields

private Button _button = new Button();

public MainPage()
{
   EntryB.SetBinding(Entry.TextProperty, static (MainPage mp) => mp._button.Text);
}
[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_button")]
private static extern ref Button GetUnsafeField<id>_button(MainPage source);

Private Properties

private Button Button {get; set;}

public MainPage()
{
   EntryB.SetBinding(Entry.TextProperty, static (MainPage mp) => mp.Button.Text);
}
[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "get_Button")]
private static extern Button GetUnsafeProperty<id>Button(MainPage source);

[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "set_Button")]
private static extern void SetUnsafeProperty<id>Button(MainPage source, Button value);

Issues Fixed

Depends on #23671
Fixes #23535

Limitations

This PR does NOT add support for usage of nested private types. This issue is tracked here: #23534. Currently unsafe accessors cannot be applied to private types. This may change with: dotnet/runtime#90081

@jkurdek jkurdek self-assigned this Jul 24, 2024
@jkurdek jkurdek force-pushed the binding-sg/enable-private-fields-properties branch from 47be94a to d607ba5 Compare July 31, 2024 10:46
@jkurdek jkurdek marked this pull request as ready for review July 31, 2024 10:46
@jkurdek jkurdek requested a review from a team as a code owner July 31, 2024 10:46
@samhouts samhouts added the area-xaml XAML, CSS, Triggers, Behaviors label Jul 31, 2024
Copy link
Member

@simonrozsival simonrozsival left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very promising! I left a few comments.

src/Controls/src/BindingSourceGen/BindingCodeWriter.cs Outdated Show resolved Hide resolved
Comment on lines 52 to 53
[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "set_{{propertyName}}")]
private static extern void SetUnsafeProperty{{id}}{{propertyName}}({{containingType}} source, {{memberType}} value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen when the property doesn't have a setter? I would expect the compilation of the generated code to fail in that case. Let's cover that with a unit test + ideally let's not generate the unsafe property setter at all when there isn't a setter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compilation will not fail. UnsafeAccessor can be used to modify a value of readonly fields / properties.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when the property doesn't have a setter, there won't be any set_{{propertyName}} method 🤔 would that only crash at runtime if the unsafe setter method is called?

Copy link
Member Author

src/Controls/src/BindingSourceGen/Setter.cs Show resolved Hide resolved
src/Controls/src/BindingSourceGen/GeneratorDataModels.cs Outdated Show resolved Hide resolved
src/Controls/src/BindingSourceGen/Setter.cs Outdated Show resolved Hide resolved
simonrozsival
simonrozsival previously approved these changes Aug 13, 2024

namespace Microsoft.Maui.Controls.BindingSourceGen;

public static class BindingCodeWriter
{
private static readonly string NewLine = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "\r\n" : "\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symbol 'Environment' is banned for use by analyzers: Analyzers should not read their settings directly from environment variables(RS1035)

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I was worried that we had to rely on [UnsafeAccessor] for this.

But reviewing the code example:

private Button _button = new Button();

public MainPage()
{
   SomeControl.SetBinding(Entry.TextProperty, static (MainPage mp) => mp._button.Text);
}

Without this change, you would have to do this:

private Button _button = new Button();

public Button Button => _button;

public MainPage()
{
   SomeControl.SetBinding(Entry.TextProperty, static (MainPage mp) => mp.Button.Text);
}

From a developer's perspective, they wouldn't understand why they would have to do this. So, I think we have to support private members?

Reviewing the code, there are several tests, etc. Looks good to me. 👍

@simonrozsival
Copy link
Member

All failing tests are unrelated.

@mattleibow mattleibow merged commit 4e6a927 into dotnet:net9.0 Aug 21, 2024
115 of 117 checks passed
@samhouts samhouts added the fixed-in-net9.0-nightly This may be available in a nightly release! label Aug 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors fixed-in-net9.0-nightly This may be available in a nightly release!
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants