Migrate AvaloniaNameSourceGenerator to IIncrementalGenerator #19216
Conversation
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
| var optionsProvider = pair.Right.Right; | ||
| var filePath = text.Path; | ||
|
|
||
| if (!(filePath.EndsWith(".xaml", StringComparison.OrdinalIgnoreCase) || |
There was a problem hiding this comment.
We should probably add some compiler-visible MSBuild metadata for XAML items that are going to be compiled (right now the actual check is the item being AvaloniaResource with the plan to have it restricted to AvaloniaXaml). It should be available before CoreCompile target, so we can pass that info into the analyzer.
There was a problem hiding this comment.
There is an extra metadata check already.
We read build_metadata.AdditionalFiles.SourceItemGroup metadata in the same file below, and for XAML files we set it as AvaloniaXaml.
See
There was a problem hiding this comment.
We had to add this metadata to avoid conflicts with MAUI XAML that also uses generators
src/tools/Avalonia.Generators/NameGenerator/AvaloniaNameIncrementalGenerator.cs
Outdated
Show resolved
Hide resolved
| }) | ||
| .Where(tuple => tuple.textContent is not null); | ||
|
|
||
| var generatorInput = xamlFiles.Combine(context.CompilationProvider); |
There was a problem hiding this comment.
IIRC passing CSharpCompilation to RegisterSourceOutput will trigger generator to run on any change in the entire project, which kinda defeats the purpose of generator being incremental.
There was a problem hiding this comment.
Possibly, but we need compilation provider, to have access to type system. Caching it might potentially lead to invalid state.
There can be a point in parsing XAML and reading x:Name metadata without type information, but it would be much more involving refactoring of the compiler.
There was a problem hiding this comment.
AFAIK, the CompilationProvider updates on every keystroke in source, so a Combine will rerun the latter portion of the pipeline on every keystroke (the entire RegisterSourceOutput in this case). This can be somewhat mitigated with frequent checking of the CancellationToken to bail out.
Normally, I gather type information from symbols during the transform part of ForAttributeWithMetadataName into an equatable model, but this is an AdditionalTextsProvider-based generator.
You could create another intermediate stage where you parse the XAML and return an equatable model containing unresolved types (fully-qualified names and ones that need additional resolution, such as a using with a group of namespaces). Then the next stage of the pipeline with CompilationProvider only needs to resolve types. Though in a large project, especially with automation, there could be thousands of named controls to resolve on keystroke. Tough problem.
There was a problem hiding this comment.
Pushed changes to only update compilation instance when any dependency assembly was added or changed.
There was a problem hiding this comment.
@stevemonaco just noticed your comment.
Yes, I tried to make XamlX work with unresolved types. Where I created a read-only type system containing pre-computed referenced assemblies. And treat any type as fake type, resolved after parsing on separated step.
I eventually gave up, when realized issues with conflicting XmlnsDefinition. Single XAML namespace can be mapped on multiple CLR namespaces, and XamlX can't delay this resolution until later.
I wasn't planning to refactor XamlX while making this PR.
There was a problem hiding this comment.
Actually, tried a bit different approach with parsing and resolving XML types first, without compilation info.
And then another step with up-to-date compilation combined used to resolve actual types.
Need to understand performance difference though.
…on should be reused between dependency changes
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
src/tools/Avalonia.Generators/NameGenerator/AvaloniaNameIncrementalGenerator.cs
Outdated
Show resolved
Hide resolved
src/tools/Avalonia.Generators/NameGenerator/AvaloniaNameIncrementalGenerator.cs
Outdated
Show resolved
Hide resolved
src/tools/Avalonia.Generators/NameGenerator/AvaloniaNameIncrementalGenerator.cs
Show resolved
Hide resolved
src/tools/Avalonia.Generators/NameGenerator/AvaloniaNameIncrementalGenerator.cs
Show resolved
Hide resolved
|
You can test this PR using the following package version. |
| var genericTypeName = typeAgs.Count == 0 | ||
| ? $"global::{typeName}" | ||
| : $"global::{typeName}<{string.Join(", ", typeAgs.Select(arg => $"global::{arg}"))}>"; | ||
| return new ResolvedName(genericTypeName, name, fieldModifier); | ||
| } | ||
|
|
||
| IXamlAstNode IXamlAstVisitor.Visit(IXamlAstNode node) |
There was a problem hiding this comment.
Can we pass a cancellation token to this class and check it here in Visit to avoid continuing to parse files unnecessary?
| } | ||
|
|
||
| IXamlAstNode IXamlAstVisitor.Visit(IXamlAstNode node) |
There was a problem hiding this comment.
Can we pass a cancellation token to this class and check it here in Visit to avoid continuing to parse files unnecessary?
| { | ||
| var resolvedNames = new List<ResolvedName>(); | ||
| foreach (var xmlName in xmlView.XmlNames) | ||
| { |
There was a problem hiding this comment.
The cancellation token should probably be checked in this loop. (If done the catch block below should ignore OperationCanceledException)
What does the pull request do?
It was a bit long overdue.
Breaking changes
It raises minimal supported Visual Studio / .NET SDK version (only for source generators, not affecting anything else).