-
Notifications
You must be signed in to change notification settings - Fork 297
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
[Feature] MVVM Toolkit vNext: source generators! 🚀 #8
Comments
We could do simple properties with this too, eh? [ObservableProperty("MyProperty", typeof(int), "Some Comment about what the property is for doc string.")] (comment could be optional I guess?) |
Leaving some more info here for @azchohfi regarding the build infrastructure/packaging needed to support this 😄 Including a source generator with the MVVM Toolkit requires us to ship the <PackFolder>analyzers\dotnet\cs</PackFolder> And then I have a packing project ( Note for F#/VB.NET devs, there shouldn't be any issues for them other than the analyzer simply not working. The main library should just work as usual without any issues though. I haave an F# test project in my lib and also got some feedbacks from an F# user that could confirm there were no build errors or anything, the analyzer will simply not be triggered outside of C# projects. Let me know how you want to go about setting this up so we can start shipping usable preview packages for people 😄 |
I believe there is an easier way. Let me quickly investigate this. |
I think this will do: MyAnalyzerProject: <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net5.0</TargetFramework>
<IsPackable>false</IsPackable>
</PropertyGroup>
</Project> MyPackageProject.csproj: <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFramework>net5.0</TargetFramework>
<TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificContentInPackage);CopyAnalyzerProjectReferencesToPackage</TargetsForTfmSpecificContentInPackage>
</PropertyGroup>
<ItemGroup>
<ProjectReference Include="..\MyAnalyzerProject\MyAnalyzerProject.csproj" PrivateAssets="all" />
</ItemGroup>
<Target Name="CopyAnalyzerProjectReferencesToPackage" DependsOnTargets="BuildOnlySettings;ResolveReferences">
<ItemGroup>
<TfmSpecificPackageFile Include="@(ReferenceCopyLocalPaths->WithMetadataValue('ReferenceSourceTarget', 'ProjectReference')->WithMetadataValue('PrivateAssets', 'All'))">
<PackagePath>analyzers\dotnet\cs</PackagePath>
</TfmSpecificPackageFile>
</ItemGroup>
</Target>
</Project> |
Oh that's awesome! I was sure you'd have some fancier MSBuild magic snippet to make that simpler ahah 😄 |
Looks like the CI failed to create the package with this error:
The fact we're getting two duplicate files here for each separate file, when the MVVM Toolkit itself has 3 targets, makes me think the issue is that the analyzer is being packed again when each individual target is being built and added to the package, instead of just once for all 3 targets. I've pushed a new commit with an extra condition for that EDIT: that worked! 🎉🎉🎉 Everything looks absolutely great from dotPeek! 🚀 EDIT 2: preview package is now out! First one is |
Could an initial default value for the property be added there as well? public class User : ObservableObject {
private string name = "foo";
public string Name {
get => name;
set => SetProperty(ref name, value);
}
} |
Assuming the initial value is a compile-time constant (as that's a limitation with attribute arguments), then yeah that would be doable. Still not 100% sure how I feel about declaring properties through attributes, but I can definitely give it a try 😄 |
@michael-hawker @ismaelestalayo I've reworked the proposal a bit due to the fact that using attributes on types means you have to be more verbose for the type itself, can't use it if you also want to access a type argument in the type, and also have no control on the field name. Instead I've added an User code: public partial class SampleModel : ObservableObject
{
[ObservableProperty]
private int data;
} Generated code: // Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
#pragma warning disable
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
namespace UnitTests.Mvvm
{
public partial class SampleModel
{
[DebuggerNonUserCode]
[ExcludeFromCodeCoverage]
public int Data
{
get => data;
set
{
if (!EqualityComparer<int>.Default.Equals(data, value))
{
OnPropertyChanging();
data = value;
OnPropertyChanged();
}
}
}
}
} Note how the name is automatic here, in this case it's the field name with the first character in uppercase. I'm also trimming field names like I also want to make it so that if you're only implementing |
One gotcha I've discovered is that the compiler is unhappy putting virtual members into a sealed class:
gives error
because the codegen hardcodes a As an aside, I wonder if each generated method should also have Regarding
to generate
That is much closer to how the compiler-generated fields work already, and it also gives a natural and familiar place to put |
Oh, good catch! Yeah I'll have to add special handling for that and strip
I could definitely do that, just wondering how useful that would be? Is that attribute commonly used? 🤔
That's unfortunately not possible with source generators, as they're explicitly additive and can't modify/rewrite existing code. |
It seems like it would suppress certain types of suggestions. E.g., I don't really need the compiler telling me that "object initialization could be simplified" in generated code. |
That should already not be a problem, since all the generated files here will have |
Ah, fair enough, the #pragma might have the same effect. |
Yeah, that just suppresses all warnings in the file, I'm adding that to avoid cases where eg. a consumer might have some custom StyleCop settings in the project which might not match exactly the style in the generated code. This way we're sure that no warnings will ever be generated from code coming from the source generator shipped in the package 🙂 |
Small update (cc. @jtippet) as of CommunityToolkit/WindowsCommunityToolkit@764d49b ✅ Added handling for Plus a bunch of other improvements and tweaks 😄 |
Soo... when I write some good code, it annoys me when people say "yes but..." and proceed to demand new features. Well I'm about to inflict the same sort of demand on you. Sorry. In a few cases, I can't use
which would expand out to something like:
That would make it easy to drop in some validation logic, logging, or a rebuild-cache type callback. If you're not already tired of adding new attributes, one other specific thing I would do in these callbacks would be to [un]subscribe events. For example:
That's formulaic enough that it could all be stuffed into a single attribute:
Ideally, these 3 attributes have But your proposed feature is already very useful on its own -- you don't need to add these features, and it's perfectly okay to say "interesting idea, maybe in v9". |
Following @jtippet 's point, I do think a way to override the setter of the property would be nice. In my case, I dont do much validation, but sometimes I doupdate one property from another property's setter: internal class Contact {
private field _age = 0;
public int Age { get => _x; set => SetProperty(ref _x, value); }
private DateTime _birthDay;
public DateTime Birthday {
get => _y;
set {
SetProperty(ref _y, value);
// Calculate the age of the contact
// and update its age
}
}
} |
Thanks @jtippet and @ismaelestalayo for the additional feedbacks 🙂 There's one thing I should point out here, there's a balance we need to strike between how many new features we want to add, and how overcomplicated the implementation becomes, and also how "weird" the code becomes when generators are involved specifically. At least for a first release with them, I'd like to just stick to common functionality that already exists, but making it more modular, as well as some new small functionality that's minimal enough. This is where the That said, I'm thinking of at least adding some way to specify dependent properties across different properties, so that you'd be able to indicate that a property should also raise the notification events for other properties when its value is changed. This would be something that would also help in the scenario that @ismaelestalayo is describing, something like: internal partial class Contact : ObservableObject
{
[ObservableProperty]
[AlsoNotifyFor(nameof(Age))]
private DateTime birthday;
public int Age => CalculateAgeFromBirthday(Birthday);
} Which would generate something like this: internal partial class Contact : ObservableObject
{
[ObservableProperty]
[AlsoNotifyFor(nameof(Age))]
public DateTime Birthday
{
get => birthday;
set
{
if (!EqualityComparer<DateTime.Default.Equals(birthday, value))
{
OnPropertyChanging();
birthday = value;
OnPropertyChanged();
OnPropertyChanged("Age");
}
}
}
} I'll experiment with this in the next few days and see how it goes. More feedbacks are always welcome of course 🙂 EDIT: the new |
Hi @Sergio0694 do you see any chance (or is it even a good idea) to have an async validation running? I would like to test if a file exists and inform the user if not. My first thought was running a I updated my demo to provide a test case: https://github.com/timunie/MvvmToolkitValidationSample // This is the property to validate
private int _ValidateAsyncExample;
[CustomValidation(typeof(ViewModel), nameof(LongRunningValidation))]
public int ValidateAsyncExample
{
get { return _ValidateAsyncExample; }
set { SetProperty(ref _ValidateAsyncExample, value, true); }
}
// This is my validator
public static ValidationResult LongRunningValidation(int value, ValidationContext _)
{
if (value % 2 == 0)
{
return ValidationResult.Success;
}
else
{
Task.Delay(1000).Wait();
return new ValidationResult("The Value must be even.");
}
} My idea would be to have something like Happy coding |
Hi @timunie, indeed running any kind of IO operation synchronously is always a recipe for disaster, as your example shows 😄 The main issue here is that the validation support in the MVVM Toolkit is closely following the official specification from the Hope this makes sense 🙂 |
Yeah absolutely. I thought already that this will be the case. If anyone else comes across such a situation: I solved it via another helper property of type Happy coding and happy easter |
It looks like the new-kinds-in-the-block (MAUI) decided to call it BindableProperty, breaking the nomenclatures. Too bad and perhaps the code generator is smart enough to be able to generate different codes for a different framework to make it framework agnostic. One thing I was curious about is, how will the debugging work with so many codes are generated behind? Will breakpoint work for instance? If I want to break on the Setter, I'm not even sure where to put the breakpoint on. ^^ |
@chrisk414 underneath |
Yes, MAUI uses its own classes to create Dependency properties and the generator doesn't support that at the moment. I'll add partial MAUI support today, but I'll need help testing it out because I don't have MAUI projects right now. Also at the moment I don't know how to ensure the correct configuration for MAUI and check diagnostics using Roslyn, so the work will be done blindly. On the plus side, MAUI has one method signature for all kinds of properties, which makes the task easier. P.S. I have released a version with MAUI support. |
Sometimes I would cache the value of a property and source generators could be useful for this. Let’s say you write this code: [ObservableProperty]
[NotifyPropertyChangedFor(nameof(Message))]
string _Name;
[CachedObservableProperty]
string GetMessage()
{
return $"Hello {Name}";
} And then this code is generated: public string Name
{
get => _Name;
set
{
if (!global::System.Collections.Generic.EqualityComparer<string>.Default.Equals(_Name, value))
{
OnNameChanging(value);
OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Name);
_Name = value;
// Clear cache
MessageIsCached = false;
OnNameChanged(value);
OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Name);
OnPropertyChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Message);
}
}
}
protected bool MessageIsCached;
protected string MessageCachedValue;
public string Message
{
get
{
if (!MessageIsCached)
{
MessageCachedValue = GetMessage();
MessageIsCached = true;
}
return MessageCachedValue;
}
} Cannot say I would use it very often :-) But sharing it here for feedback. |
Hello, public partial class Feedback : ObservableObject
{
[ObservableProperty]
[NotifyPropertyChangedFor(nameof(CanDoSomething))]
[NotifyCanExecuteChangedFor(nameof(DoSomethingCommand))] // redundant, in the definition of RelayCommand we know that CanDoSomething should trigger CanExecuteChanged on the command
private string fileName1 = "";
[ObservableProperty]
[NotifyPropertyChangedFor(nameof(CanDoSomething))]
[NotifyCanExecuteChangedFor(nameof(DoSomethingCommand))] // redundant, in the definition of RelayCommand we know that CanDoSomething should trigger CanExecuteChanged on the command
private string fileName2 = "";
public bool CanDoSomething => File.Exists(fileName1) && File.Exists(fileName2);
[RelayCommand(CanExecute = nameof(CanDoSomething))]
public void DoSomething() {}
} My suggestion is to automate the I know it's only two lines saved in this scenario, but on more complex viewmodels it would allow the developers to relax and know that whenever If you are interested in the idea I'd be happy to propose a PR. |
Hi, It looks like source generators produce errors on newer SKDs. When building with .NET6 SDKs newer than 6.0.101. I get a bunch (500+) of compilation errors. This does not occur if I build with 6.0.101. All errors look like these:
This is not just about ITaskNotifier but I get duplicates about OnChanged/Changing methods etc. too. |
This is an sdk regression tracked here (dotnet/wpf#6792), identified here (dotnet/wpf#6792 (comment)) and there are workarounds here (dotnet/wpf#6792 (comment)) and here (dotnet/wpf#6792 (comment)) until the fix (dotnet/wpf#6793) is available. |
@MisinformedDNA we shipped a preview of the source generators in 7.1.x. However, they're built on top of the previous version of the Source Generator API from .NET, so they fall over with larger loads. In retrospect we probably should have separated them out as a separate package, but we didn't quite realize the impact at the time. These new generators with 8.0 have been refined and completely rebuilt with the new Incremental generator API, so they'll work under any sized project. I know Sergio is planning a release in the near future, and I don't think there's any planned breaking changes since the last preview release. Did I miss anything @Sergio0694? |
Nope, that's exactly right. I would strongly recommend not to use the attributes in 7.1. They're not supported for production. If you want to use the generators, do use |
The preview version doesn't exist under the |
Yup that's correct, the new package/namespace will be NOTE: the package is still published and maintained by Microsoft, it's just a name change. |
Are all the |
As far as I know, yes. For sure the .NET, Windows and MAUI Community Toolkit-s are.
It just makes things simpler for us for several reasons we don't have to go into 🙂 |
Just to add from my side: I think for people like me who are using Avalonia or any other not MS UI framework may wonder if they can use something that has Microsoft.* in the name. So I like the new name tbh. Happy coding |
Could it be possible for |
That would be useful for read-only properties, yes. I vote for it :-). Let me add: At least in Avalonia one can bind to internal properties. So maybe properties can also be made internal on request. I don't use this feature of Avalonia until today, but probably it's worth to mention here. |
Both the getter and setter visibility could be properties of the attribute, maybe something like: [ObservableProperty(Getter = Visibility.Internal, Setter = Visibility.Private)] If there's interest and we can work out the details of usage and naming, I can make a PR - or just leave it to the maintainers, whichever way would be easier for them. EDIT: See my comment below. |
Quoting the reply from #291:
Personally I disagree that november 2023 is 'short term', and if the solution are partial properties instead of fields then that sounds like it's going to be a breaking change for users of Community Toolkit anyway. I might try to implement this anyway. If the maintainers don't want to accept a PR and officially support this feature, which would be understandable, I can just put it on a self-hosted NuGet repository and use it in my own projects. EDIT: I added the feature in my fork, if anyone wants it feel free to build a custom version from this commit: chylex@0d941a6 If anyone needs help with building and/or creating a custom NuGet repository, feel free to open a discussion in my fork, if there is actually interest I will write a guide. Please don't post any comments in this topic or anywhere in the official repository, I don't want to bother the maintainers. |
I didn't say support for fields would be removed. We can very well consider just leaving it there side by side by properties, or at least keeping it there for one release cycle to give everyone time to migrate to properties. Plus, this is not a show stopper. If you absolutely need a private setter for the time being, you can use manual properties in those cases, and still use generated properties everywhere else 🙂
Don't do this. As the PR template says, PRs without an approved linked proposal will just be closed. Closing this given the source generators have now officially shipped 😄 |
You can find more information about this from one of our blog posts, to quote:
|
Fair enough. And it is now too late to to get this wish in the release, what would have been the best procedure to get the change in an official release. The change that @chylex did is very desirable and I've considered it myself. In other words, how would one have created an approved linked proposal, so a PR can get through. |
A proposal already existed and was denied, so I don't think there was any way for a PR to get through. If the maintainers decided to allow the proposal, I would still be interested in making a PR for the benefit of others, but for now I would recommend what I did - build a custom version from my fork, and create a private NuGet repository. If it helps, I can upload my NuGet repository somewhere, since it is made entirely of static files so you'd just need to upload it to a web server. Or use my repository, but I can't guarantee its uptime or which versions of the toolkit would be available. Details would be best discussed in my fork's discussions. |
The
Microsoft.Toolkit.Mvvm
package (aka MVVM Toolkit) is finally out, so it's time to start planning for the next update 😄This issue is meant to be for tracking planned features, discuss them and possibly propose new ideas well (just like in CommunityToolkit/WindowsCommunityToolkit#3428).
One aspect I want to investigate for this next release, which I think makes sense for the package, is support for Roslyn source generators. Support for these would be added through a secondary project, ie.
Microsoft.Toolkit.Mvvm.SourceGenerators
, that would be shipped together with the MVVM Toolkit, but then added to consuming projects as a development dependency, ie. as an analyzer. They would be included in the same NuGet package, so it wouldn't be possible to just reference the source generator directly (which wouldn't make sense anyway). This is the same setup I'm using in my own library ComputeSharp, in fact I'm applying a lot of the things I learnt while working on that project to the MVVM Toolkit as well 🥳There are
twothree main aspects that can be investigated through support for source generators:Here's some more details about what I mean by these two categories exactly.
Better extensibility
There's one "problem" with C# that has been brought up by devs before while using the MVVM Toolkit and in general (eg. here): lack for multiple inheritance. While that makes perfect sense and there's plenty of valid reasons why that's the case, the fact remains that it makes things sometimes inconvenient, when eg. you want/need to inherit from a specific type but then still want to use the features exposed by other classes in the MVVM Toolkit. The solution? Source generators! 🚀
For now I'm thinking about adding the following attributes to the package:
[INotifyPropertyChanged]
[ObservableObject]
[ObservableRecipient]
The way they work is that they let you annotate a type and then rely on the generator injecting all the APIs from those types automatically, so you don't need to worry about it and it's like you were effectively having multiple inheritance. Here's an example:
This class now inherits from
SomeOtherClass
, but it still has all the same APIs fromObservableObject
! This includes thePropertyChanged
andPropertyChanging
events, the methods to raise them and all the additional helper methods![ObservableRecipient]
does the same but copying members fromObservableRecipient
(eg. this could be used to effectively have a type that is both a validator but also a recipient), and[INotifyPropertyChanged]
instead offers minimal support just forINotifyPropertyChanged
, with optionally also the ability to include additional helpers or not. This approach and the different attributes offer maximum flexibility for users to choose the best way to construct their architecture without having to compromise between what APIs to use from the MVVM Toolkit and how they want/have to organize their type hierarchy. 🎉Less verbosity
This was first suggested by @michael-hawker in this comment, the idea is to also provide helpers to reduce the code verbosity in simple cases, such as when defining classic observable properties. For now I've added these attributes:
[ObservableProperty]
[AlsoNotifyFor]
[ICommand]
The first two can be used to easily declare observable properties, by annotating a field.
[ObservableProperty]
will create the code necessary to implement the property itself, whereas[AlsoNotifyFor]
will customize the generated code by adding extra notification events (ie. calls toOnPropertyChanged
) for properties whose value depends on the property being updated.Here's an example of how these two attributes can be used together:
Viewmodel definition:
Generated code:
There is also a brand new
[ICommand]
attribute type, which can be used to easily create command properties over methods, by leveraging the relay command types in the MVVM Toolkit. The way this works is simple: just write a method with a valid signature for eitherRelayCommand
,RelayCommand<T>
,AsyncRelayCommand
orAsyncRelayCommand<T>
and add the[ICommand]
attribute over it - the generator will create a lazily initialized property with the right command type that will automatically wrap that method. Cancellation tokens for asynchronous commands are supported too! 🚀Here's an example of how this attribute can be used with four different command types:
Viewmodel definition:
Generated code:
Better performance
Another area that I want to investigate with source generators is possibly getting some performance improvemeents by removing reflection where possible. Now, the MVVM Toolkit is already quite light on reflection (as it was designed with that in mind, especially the messenger types), but I think there might be a few places where things could still be improved with source generators. For instance, this method uses quite a bit of reflection.
We could keep this for compatibility and also as a "fallback" implementation, but then we could have the source generator emit a type-specific version of this method with all the necessary handlers already specified, with no reflection. We'd just need to generate the appropriate method in the consuming assembly, and then the C# compiler would automatically pick that one up due to how overload resolution works (since the
object recipient
in the original method is less specific than aMyViewModel recipient
parameter that the generated method would have). Still haven't done a working proof of concept for this point specifically, but it's next on my list and will update as soon as that's done too, just wanted to open this issue in the meantime to start gathering feedbacks and discuss ideas 🙂EDIT: I've now added a generator that will create a method for this for all types implementing
IRecipient<T>
:This is then now picked up automatically when
RegisterAll
is called, so that the LINQ expression can be skipped entirely.There are two generated methods so that the non-generic one can be used in the more common scenario where a registration token is not used, and that completely avoids runtime-code generation of all sorts and also more reflection (no more
MakeDynamicMethod
), making it particularly AOT-friendly 😄EDIT 2: I've applied the same concept to the other place where I was using compiled LINQ expressions too, that is the
ObservableValidator.ValidateAllProperties
method. We now have a new generator that will process all types inheriting fromObservableValidator
, and create helper methods like this that will then be loaded at runtime by the MVVM Toolkit as above:When the source generators are used, the MVVM Toolkit is now 100% without dynamic code generation! 🎉
Tracking changes so far
[INotifyPropertyChangedAttribute]
[ObservableObjectAttribute]
[ObservableValidatorAttribute]
RegisterAll
method when possibleValidateAllProperties
method when possible[ObservableProperty]
[AlsoNotifyChangeFor]
[ICommand]
CanExecute
property for[ICommand]
[AlsoNotifyCanExecuteFor]
Feedbacks and feature ideas are welcome! 🙌
The text was updated successfully, but these errors were encountered: