MVVM Toolkit vNext: source generators #3873

Sergio0694 · 2021-03-22T12:36:23Z

(🦙) Testing something for Discord Preview, please ignore this text at the top for now while we test it. Thanks! This is going to be great feature in the Toolkit, can't wait! (🦙)

Contributes to #3872

PR Type

What kind of change does this PR introduce?

Feature
Performance improvements

Changelog

Changes discussed/tracked in the linked issue.

PR Checklist

Please check if your PR fulfills the following requirements:

Tested code with current supported SDKs
Pull Request has been submitted to the documentation repository instructions. Link:
Sample in sample app has been added / updated (for bug fixes / features)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
Tests for the changes have been added (for bug fixes / features) (if applicable)
Header has been added to all new source files (run build/UpdateHeaders.bat)
Contains NO breaking changes

ghost · 2021-03-22T12:36:27Z

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Youssef1313 · 2021-06-19T06:12:00Z

Microsoft.Toolkit.Mvvm.SourceGenerators/Extensions/MemberDeclarationSyntaxExtensions.cs

+
+            if (index != -1)
+            {
+                return memberDeclaration.WithModifiers(memberDeclaration.Modifiers.RemoveAt(index));


This may not preserve trivia if the removed modifier has any. No idea if that's okay, but wanted to note.

Ah, good catch. Yeah that's fine, the generation step doesn't care about trivia at all (also to make things simpler). When we actually generate code at the end we just call NormalizeWhitespace() anyway that will fix everything 🙂

Youssef1313 · 2021-06-19T06:17:47Z

Microsoft.Toolkit.Mvvm.SourceGenerators/Input/ICommandGenerator.cs

+        {
+            string propertyName = methodSymbol.Name;
+
+            if (SymbolEqualityComparer.Default.Equals(methodSymbol.ReturnType, context.Compilation.GetTypeByMetadataName("System.Threading.Tasks.Task")!) &&


Why a suppression is needed? Equals parameters are null annotated

Ah, you're right, thanks! Fixed in e97a60b.
I think I had originally added that suppression due to missing annotations with some older preview version of the Roslyn NuGet packages, and tghen forgot to go back and remove that here after updating the references 😄

Improved type safety, more AOT-friendly, removed Unsafe.As<T> hack

Doing this enables the JIT to devirtualize function calls through the property

Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>

Added more info and remarks about field name conversion

michael-hawker · 2021-07-22T08:59:29Z

Thanks @Sergio0694 looks good to me. I've been playing with them already, never going to go back! 🦙❤

@XAML-Knight think you'll find this interesting too. Feel free to give it a quick go around.

XAML-Knight

Impressive check-in - I found a couple of small items.

XAML-Knight · 2021-07-22T22:08:21Z

Microsoft.Toolkit.Mvvm.SourceGenerators/ComponentModel/ObservableRecipientGenerator.cs

+            [NotNullWhen(false)] out DiagnosticDescriptor? descriptor)
+        {
+            INamedTypeSymbol
+                observableRecipientSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.Toolkit.Mvvm.ComponentModel.ObservableRecipient")!,


I'd recommend continuing to pass in the FullName of the Type, versus relying upon magic strings, where possible

"where possible" is the key phrase here, of course :)

XAML-Knight · 2021-07-22T22:08:53Z

Microsoft.Toolkit.Mvvm.SourceGenerators/ComponentModel/ObservableRecipientGenerator.cs

+                }
+            }
+
+            INamedTypeSymbol observableValidatorSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.Toolkit.Mvvm.ComponentModel.ObservableValidator")!;


Same as above, recommend sending in FullName of the Type

XAML-Knight · 2021-07-22T22:09:05Z

...enerators/ComponentModel/ObservableValidatorValidateAllPropertiesGenerator.SyntaxReceiver.cs

+            {
+                if (context.Node is ClassDeclarationSyntax classDeclaration &&
+                    context.SemanticModel.GetDeclaredSymbol(classDeclaration) is INamedTypeSymbol { IsGenericType: false } classSymbol &&
+                    context.SemanticModel.Compilation.GetTypeByMetadataName("Microsoft.Toolkit.Mvvm.ComponentModel.ObservableValidator") is INamedTypeSymbol validatorSymbol &&


Same as above, recommend sending in FullName of the Type

XAML-Knight · 2021-07-22T22:09:22Z

...it.Mvvm.SourceGenerators/ComponentModel/ObservableValidatorValidateAllPropertiesGenerator.cs

+            }
+
+            // Get the symbol for the ValidationAttribute type
+            INamedTypeSymbol validationSymbol = context.Compilation.GetTypeByMetadataName("System.ComponentModel.DataAnnotations.ValidationAttribute")!;


Same as above, recommend sending in FullName of the Type

XAML-Knight · 2021-07-22T22:20:44Z

...oft.Toolkit.Mvvm.SourceGenerators/Messaging/IMessengerRegisterAllGenerator.SyntaxReceiver.cs

+            {
+                if (context.Node is ClassDeclarationSyntax classDeclaration &&
+                    context.SemanticModel.GetDeclaredSymbol(classDeclaration) is INamedTypeSymbol { IsGenericType: false } classSymbol &&
+                    context.SemanticModel.Compilation.GetTypeByMetadataName("Microsoft.Toolkit.Mvvm.Messaging.IRecipient`1") is INamedTypeSymbol iRecipientSymbol &&


I can see why you're just passing in a string value, here

ghost · 2021-07-23T13:06:06Z

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

Adding the type name of the generator is not necessary since Visual Studio already groups generated sources for each individual generator, which also automatically avoids conflicts

Sergio0694 · 2021-07-23T13:21:45Z

Hey @XAML-Knight, thank you for having a look, glad you liked the PR! 😄

Let me give some context as to why I was using those hardcoded strings instead of using typeof(T) before, and in fact in 9283f2c I've removed those usages altogether and just switched to hardcoded strings everywhere for consistency. I've also talked with a Roslyn engineer to confirm this was a commonly used pattern on their end as well, for good measure 🙂

The short version of why those usages you've highlighted in your review comments weren't using typeof(T) is that those types were simply not available, so I couldn't have used typeof(T) even if I had wanted to (which I did, initially). Note that the MVVM Toolkit generator does not have a dependency on the MVVM Toolkit, so it can't see any of those types directly. The ones I was using typeof(T) for were types that I had manually linked to the generator project to enable that, which I've also removed now. The reason why typeof(T) is technically not entirely correct is that you're essentially loading a type that's not the same type as the one actually used in consuming projects. Source generators run at build time and they're a totally separate assembly that's only loaded by Roslyn when building a project (so they're not referenced by consuming projects directly either), and because of this they can't access runtime information from consuming projects such as Type instances - the consuming project is just not an assembly that's loaded into memory and that is being executed at all at that point. This is the reason why generators only work with syntax trees and semantic information (symbols), as those are the only type of information that is available at that point in the toolchain. Even when we were using typeof(T) before to simplify things, we were actually not referencing the same type that consumers would've used, but just loading a Type instance from a source copy of that types that happened to be in the compiled in the generator assembly, which also happened to match the one actually present in the MVVM Toolkit and therefore using the same fully qualified name when final consumers would actually use it at runtime. But it was not technically the same type. Now instead we're just matching the actual well known type name, which is what Roslyn also does frequently as well (eg. see here).

Hope this helps! 😄

XAML-Knight · 2021-07-23T00:06:36Z

Microsoft.Toolkit.Mvvm.SourceGenerators/ComponentModel/ObservableRecipientGenerator.cs

+            [NotNullWhen(false)] out DiagnosticDescriptor? descriptor)
+        {
+            INamedTypeSymbol
+                observableRecipientSymbol = context.Compilation.GetTypeByMetadataName("Microsoft.Toolkit.Mvvm.ComponentModel.ObservableRecipient")!,


"where possible" is the key phrase here, of course :)

XAML-Knight · 2021-07-23T21:14:35Z

Thanks for the detailed explanation, @Sergio0694

PR Approved

Sergio0694 · 2021-07-23T21:29:15Z

@XAML-Knight No worries, glad I could help! And yeah for sure, I did see that "where possible", just figured it would be useful to share some additional context in here instead of just saying "yeah it's not possible here", either for you in case you weren't super familiar with source generators yet (as they're pretty new, or more like, still in preview themselves ahah), as well as others possibly reading through this PR. Glad everything makes sense, thank you for the review! 🙌

Sergio0694 added feature 💡 nuget 📦 optimization ☄ Performance or memory usage improvements .NET Components which are .NET based (non UWP specific) mvvm-toolkit 🧰 Issues/PRs for the Microsoft.Toolkit.Mvvm package labels Mar 22, 2021

ghost requested review from michael-hawker, azchohfi, Kyaa-dost and Rosuavio March 22, 2021 12:36

michael-hawker added this to the 7.1 milestone Mar 22, 2021

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch 2 times, most recently from 618d250 to f9987b3 Compare March 23, 2021 19:44

Sergio0694 mentioned this pull request Mar 25, 2021

MVVM: Clearing the event handlers for ObservableObject #3889

Closed

Sergio0694 linked an issue Mar 25, 2021 that may be closed by this pull request

MVVM: Clearing the event handlers for ObservableObject #3889

Closed

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch 5 times, most recently from bc79e56 to ef8cbb0 Compare March 31, 2021 11:44

Sergio0694 mentioned this pull request Mar 31, 2021

Added solution filter for .NET projects #3906

Merged

8 tasks

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch from 682179b to 0d739ca Compare March 31, 2021 23:11

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch 3 times, most recently from bfc7d90 to 4e2796b Compare April 14, 2021 17:13

kamronbatman mentioned this pull request Apr 18, 2021

Source generated Serialization/Deserialization modernuo/ModernUO#550

Merged

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch from 4e2796b to 315bdd1 Compare April 21, 2021 12:56

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch from 44b2a66 to 3c7c7d5 Compare April 28, 2021 15:47

Youssef1313 reviewed Jun 19, 2021

View reviewed changes

Sergio0694 and others added 10 commits July 21, 2021 02:32

Enabled property args caching for non dependent names

386276c

Enabled property args caching for ObservableValidator properties

1ba2b28

Minor code refactoring

4deae23

Improved code generation for ValidateAllProperties generator

9184ba5

Improved type safety, more AOT-friendly, removed Unsafe.As<T> hack

Improved code generation for IRecipient generator

5ba929f

Switched generated command field to concrete type

7f5bd6c

Doing this enables the JIT to devirtualize function calls through the property

Performance improvements to validation/messenger generated code

c5b9050

Renamed [AlsoNotifyFor] attribute to [AlsoNotifyChangeFor]

134058d

Switch branch name to main in analyzer files

fe54a9c

Co-authored-by: Michael Hawker MSFT (XAML Llama) <24302614+michael-hawker@users.noreply.github.com>

Fixed an incorrect example in XML docs

eb0b475

Sergio0694 force-pushed the feature/mvvm-toolkit-sg branch from 16ba7cc to eb0b475 Compare July 21, 2021 00:32

Sergio0694 added 2 commits July 21, 2021 02:38

Improved XML docs for [ObservableProperty]

37caf88

Added more info and remarks about field name conversion

Fixed aka.ms link for generator errors info

806b6da

michael-hawker approved these changes Jul 22, 2021

View reviewed changes

XAML-Knight suggested changes Jul 22, 2021

View reviewed changes

ghost added the needs author feedback 📝 label Jul 22, 2021

Switched to fully qualified names to resolve all symbols

9283f2c

ghost added needs attention 👋 and removed needs author feedback 📝 labels Jul 23, 2021

Simplified generated filenames

fa939b4

Adding the type name of the generator is not necessary since Visual Studio already groups generated sources for each individual generator, which also automatically avoids conflicts

XAML-Knight approved these changes Jul 23, 2021

View reviewed changes

ghost removed the needs attention 👋 label Jul 23, 2021

michael-hawker merged commit 9444484 into CommunityToolkit:main Jul 23, 2021

Sergio0694 deleted the feature/mvvm-toolkit-sg branch July 23, 2021 22:07

Sergio0694 mentioned this pull request Jul 27, 2021

Improved nullability annotations in MVVM Toolkit #4135

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVVM Toolkit vNext: source generators #3873

MVVM Toolkit vNext: source generators #3873

Sergio0694 commented Mar 22, 2021 •

edited

Loading

ghost commented Mar 22, 2021

Youssef1313 Jun 19, 2021

Sergio0694 Jun 23, 2021

Youssef1313 Jun 19, 2021

Sergio0694 Jul 13, 2021

michael-hawker commented Jul 22, 2021

XAML-Knight left a comment

XAML-Knight Jul 22, 2021

XAML-Knight Jul 23, 2021

XAML-Knight Jul 22, 2021

XAML-Knight Jul 22, 2021

XAML-Knight Jul 22, 2021

XAML-Knight Jul 22, 2021

ghost commented Jul 23, 2021

Sergio0694 commented Jul 23, 2021

XAML-Knight Jul 23, 2021

XAML-Knight commented Jul 23, 2021

Sergio0694 commented Jul 23, 2021

MVVM Toolkit vNext: source generators #3873

MVVM Toolkit vNext: source generators #3873

Conversation

Sergio0694 commented Mar 22, 2021 • edited Loading

Contributes to #3872

PR Type

Changelog

PR Checklist

ghost commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-hawker commented Jul 22, 2021

XAML-Knight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 23, 2021

Sergio0694 commented Jul 23, 2021

Choose a reason for hiding this comment

XAML-Knight commented Jul 23, 2021

Sergio0694 commented Jul 23, 2021

Sergio0694 commented Mar 22, 2021 •

edited

Loading