Skip to content

Reduce generated invocations to FindNameScope #15370

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 15, 2024
Merged

Reduce generated invocations to FindNameScope #15370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/tools/Avalonia.Generators/NameGenerator/InitializeComponentCodeGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

namespace Avalonia.Generators.NameGenerator;

internal class InitializeComponentCodeGenerator: ICodeGenerator
internal class InitializeComponentCodeGenerator : ICodeGenerator
{
private readonly bool _diagnosticsAreConnected;
private const string AttachDevToolsCodeBlock = @"
Expand All @@ -29,11 +29,20 @@ public string GenerateCode(string className, string nameSpace, IXamlType xamlTyp
{
var properties = new List<string>();
var initializations = new List<string>();
const string thisFindNameScopeVariable = " var __thisNameScope__ = this.FindNameScope();";
bool hasNames = false;
foreach (var resolvedName in names)
{
if (!hasNames)
{
initializations.Add(thisFindNameScopeVariable);
}

var (typeName, name, fieldModifier) = resolvedName;
properties.Add($" {fieldModifier} {typeName} {name};");
initializations.Add($" {name} = this.FindNameScope()?.Find<{typeName}>(\"{name}\");");
initializations.Add($" {name} = __thisNameScope__?.Find<{typeName}>(\"{name}\");");

hasNames = true;
}

var attachDevTools = _diagnosticsAreConnected && IsWindow(xamlType);
Expand Down Expand Up @@ -68,7 +77,7 @@ public void InitializeComponent(bool loadXaml = true{(attachDevTools ? ", bool a
}}
";
}

private static bool IsWindow(IXamlType xamlType)
{
var type = xamlType;
Expand Down
3 changes: 2 additions & 1 deletion ...lonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/AttachedProps.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
var __thisNameScope__ = this.FindNameScope();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the prefix and suffic double underscores following some convention I'm unaware of? Otherwise, naming conventions are important and this probably shouldn't have been done. thisNameScope would be fine IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My intention was to avoid any potential conflicts, hence the uncommon naming. This is still only a generated local so it won't ever leak outside, and thus barely matters imo

Copy link
Member

Choose a reason for hiding this comment

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

Chance of conflicts is pretty low, but not zero.

With IL code generation we usually generate some obscure names like "!CompiledXaml" that can't be really compiled via C#. It's not possible with source generators though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking this is a local variable so why even worry about it. But there is risk of conflicts with control names. That said, we probably should have a standard for this. I have seen two underscore prefixes used for generated variables but never as suffixes too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Accessing instance members with this would solve the problem, I think. No special naming would be needed. Probably static members would have to be qualified with the current type name as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jp2masa I had the same thought after my comment above and was going to check it later to make sure. But if you are thinking the same that's probably a better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why we're arguing over a generated local's name that has already been merged. This is too much mental effort for something that literally doesn't impact users.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it was merged awful quickly. I try to read through what I can just to be aware. I agree it's low impact and can be changed later as/if required. I wouldn't shut down discussion like this though, long-term it's probably useful especially as we add more generated code.

Copy link
Member

Choose a reason for hiding this comment

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

Since a local/parameter always has priority, imo it's usually best in source generators to make sure member references are correctly qualified, and leave the local/parameters "unescaped".
e.g. this.Property = someLocal; instead of Property = __someLocal__

I agree with Robloo, there's no arguing here, just discussion that could help future PRs.

UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
}
}
}
3 changes: 2 additions & 1 deletion ...tors.Tests/InitializeComponent/GeneratedInitializeComponent/AttachedPropsWithDevTools.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ namespace Sample.App
}
#endif

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
}
}
}
3 changes: 2 additions & 1 deletion ...enerators.Tests/InitializeComponent/GeneratedInitializeComponent/ControlWithoutWindow.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
}
}
}
7 changes: 4 additions & 3 deletions ...onia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/CustomControls.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

ClrNamespaceRoutedViewHost = this.FindNameScope()?.Find<global::Avalonia.ReactiveUI.RoutedViewHost>("ClrNamespaceRoutedViewHost");
UriRoutedViewHost = this.FindNameScope()?.Find<global::Avalonia.ReactiveUI.RoutedViewHost>("UriRoutedViewHost");
UserNameTextBox = this.FindNameScope()?.Find<global::Controls.CustomTextBox>("UserNameTextBox");
var __thisNameScope__ = this.FindNameScope();
ClrNamespaceRoutedViewHost = __thisNameScope__?.Find<global::Avalonia.ReactiveUI.RoutedViewHost>("ClrNamespaceRoutedViewHost");
UriRoutedViewHost = __thisNameScope__?.Find<global::Avalonia.ReactiveUI.RoutedViewHost>("UriRoutedViewHost");
UserNameTextBox = __thisNameScope__?.Find<global::Controls.CustomTextBox>("UserNameTextBox");
}
}
}
5 changes: 3 additions & 2 deletions ...lonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/DataTemplates.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
NamedListBox = this.FindNameScope()?.Find<global::Avalonia.Controls.ListBox>("NamedListBox");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
NamedListBox = __thisNameScope__?.Find<global::Avalonia.Controls.ListBox>("NamedListBox");
}
}
}
13 changes: 7 additions & 6 deletions ...lonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/FieldModifier.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

FirstNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("FirstNameTextBox");
LastNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("LastNameTextBox");
PasswordTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
ConfirmPasswordTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("ConfirmPasswordTextBox");
SignUpButton = this.FindNameScope()?.Find<global::Avalonia.Controls.Button>("SignUpButton");
RegisterButton = this.FindNameScope()?.Find<global::Avalonia.Controls.Button>("RegisterButton");
var __thisNameScope__ = this.FindNameScope();
FirstNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("FirstNameTextBox");
LastNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("LastNameTextBox");
PasswordTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
ConfirmPasswordTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("ConfirmPasswordTextBox");
SignUpButton = __thisNameScope__?.Find<global::Avalonia.Controls.Button>("SignUpButton");
RegisterButton = __thisNameScope__?.Find<global::Avalonia.Controls.Button>("RegisterButton");
}
}
}
3 changes: 2 additions & 1 deletion ...alonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/NamedControl.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
}
}
}
7 changes: 4 additions & 3 deletions ...lonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/NamedControls.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
PasswordTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
SignUpButton = this.FindNameScope()?.Find<global::Avalonia.Controls.Button>("SignUpButton");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
PasswordTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
SignUpButton = __thisNameScope__?.Find<global::Avalonia.Controls.Button>("SignUpButton");
}
}
}
21 changes: 11 additions & 10 deletions ...Avalonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/SignUpView.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,17 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Controls.CustomTextBox>("UserNameTextBox");
UserNameValidation = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBlock>("UserNameValidation");
PasswordTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
PasswordValidation = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBlock>("PasswordValidation");
AwesomeListView = this.FindNameScope()?.Find<global::Avalonia.Controls.ListBox>("AwesomeListView");
ConfirmPasswordTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("ConfirmPasswordTextBox");
ConfirmPasswordValidation = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBlock>("ConfirmPasswordValidation");
SignUpButtonDescription = this.FindNameScope()?.Find<global::Avalonia.Controls.Documents.Run>("SignUpButtonDescription");
SignUpButton = this.FindNameScope()?.Find<global::Avalonia.Controls.Button>("SignUpButton");
CompoundValidation = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBlock>("CompoundValidation");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Controls.CustomTextBox>("UserNameTextBox");
UserNameValidation = __thisNameScope__?.Find<global::Avalonia.Controls.TextBlock>("UserNameValidation");
PasswordTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
PasswordValidation = __thisNameScope__?.Find<global::Avalonia.Controls.TextBlock>("PasswordValidation");
AwesomeListView = __thisNameScope__?.Find<global::Avalonia.Controls.ListBox>("AwesomeListView");
ConfirmPasswordTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("ConfirmPasswordTextBox");
ConfirmPasswordValidation = __thisNameScope__?.Find<global::Avalonia.Controls.TextBlock>("ConfirmPasswordValidation");
SignUpButtonDescription = __thisNameScope__?.Find<global::Avalonia.Controls.Documents.Run>("SignUpButtonDescription");
SignUpButton = __thisNameScope__?.Find<global::Avalonia.Controls.Button>("SignUpButton");
CompoundValidation = __thisNameScope__?.Find<global::Avalonia.Controls.TextBlock>("CompoundValidation");
}
}
}
3 changes: 2 additions & 1 deletion ...lonia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/xNamedControl.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
}
}
}
7 changes: 4 additions & 3 deletions ...onia.Generators.Tests/InitializeComponent/GeneratedInitializeComponent/xNamedControls.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ namespace Sample.App
AvaloniaXamlLoader.Load(this);
}

UserNameTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
PasswordTextBox = this.FindNameScope()?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
SignUpButton = this.FindNameScope()?.Find<global::Avalonia.Controls.Button>("SignUpButton");
var __thisNameScope__ = this.FindNameScope();
UserNameTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("UserNameTextBox");
PasswordTextBox = __thisNameScope__?.Find<global::Avalonia.Controls.TextBox>("PasswordTextBox");
SignUpButton = __thisNameScope__?.Find<global::Avalonia.Controls.Button>("SignUpButton");
}
}
}