Closed Sergio0694 closed 2 years ago
michael-hawker
commented
3 years ago 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?)
Sergio0694
commented
3 years ago 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 Microsoft.Toolkit.Mvvm.SourceGenerators project as an analyzer in the Microsoft.Toolkit.Mvvm package. It would not have to be a separate NuGet package (unless we wanted to for some other reason?), so we'd first have to exclude the .Mvvm.SourceGenerators package from being packaged in the CI script. Then the project would need to be packed into the analyzers\dotnet\cs path of the .Mvvm package. In my own library ComputeSharp (which also ships a source generator that is added as an analyzer to projects referencing the library itself from NuGet) I'm doing this:
<PackFolder>analyzers\dotnet\cs</PackFolder>And then I have a packing project (.msbuildproj) that references both the main lib and the analyzer, and pack that one to actually create the NuGet package. Not sure if there's an easier way, but I can say that's been working pretty well for me so far 🙂
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 😄
azchohfi
commented
3 years ago I believe there is an easier way. Let me quickly investigate this.
azchohfi
commented
3 years ago 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 😄 Added that in https://github.com/Sergio0694/WindowsCommunityToolkit/commit/618d25038bd0413ec0191a6663176373ebb7712e, will try that out with a package from the CI once the pipeline finishes running!
Sergio0694
commented
3 years ago Looks like the CI failed to create the package with this error:
"D:\a\1\s\Windows Community Toolkit.sln" (Pack target) (1) ->
An error occurred when executing task 'Package'.
"D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj" (Pack target) (18) ->
(GenerateNuspec target) ->
C:\Program Files\dotnet\sdk\5.0.201\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: File 'D:\a\1\s\Microsoft.Toolkit.Mvvm.SourceGenerators\bin\Release\netstandard2.0\Microsoft.Toolkit.Mvvm.SourceGenerators.dll' is not added because the package already contains file 'analyzers\dotnet\cs\Microsoft.Toolkit.Mvvm.SourceGenerators.dll' [D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj]
C:\Program Files\dotnet\sdk\5.0.201\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: File 'D:\a\1\s\Microsoft.Toolkit.Mvvm.SourceGenerators\bin\Release\netstandard2.0\Microsoft.Toolkit.Mvvm.SourceGenerators.dll' is not added because the package already contains file 'analyzers\dotnet\cs\Microsoft.Toolkit.Mvvm.SourceGenerators.dll' [D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj]
C:\Program Files\dotnet\sdk\5.0.201\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: File 'D:\a\1\s\Microsoft.Toolkit.Mvvm.SourceGenerators\bin\Release\netstandard2.0\Microsoft.Toolkit.Mvvm.SourceGenerators.pdb' is not added because the package already contains file 'analyzers\dotnet\cs\Microsoft.Toolkit.Mvvm.SourceGenerators.pdb' [D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj]
C:\Program Files\dotnet\sdk\5.0.201\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: File 'D:\a\1\s\Microsoft.Toolkit.Mvvm.SourceGenerators\bin\Release\netstandard2.0\Microsoft.Toolkit.Mvvm.SourceGenerators.pdb' is not added because the package already contains file 'analyzers\dotnet\cs\Microsoft.Toolkit.Mvvm.SourceGenerators.pdb' [D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj]
C:\Program Files\dotnet\sdk\5.0.201\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: File 'D:\a\1\s\Microsoft.Toolkit.Mvvm.SourceGenerators\bin\Release\netstandard2.0\Microsoft.Toolkit.Mvvm.SourceGenerators.xml' is not added because the package already contains file 'analyzers\dotnet\cs\Microsoft.Toolkit.Mvvm.SourceGenerators.xml' [D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj]
C:\Program Files\dotnet\sdk\5.0.201\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(221,5): error NU5118: File 'D:\a\1\s\Microsoft.Toolkit.Mvvm.SourceGenerators\bin\Release\netstandard2.0\Microsoft.Toolkit.Mvvm.SourceGenerators.xml' is not added because the package already contains file 'analyzers\dotnet\cs\Microsoft.Toolkit.Mvvm.SourceGenerators.xml' [D:\a\1\s\Microsoft.Toolkit.Mvvm\Microsoft.Toolkit.Mvvm.csproj]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 <Target/> element so that it's only triggered when the .NET Standard 2.0 target is selected (the analyzer should be shared by all 3 anyway), let's see if it does the trick 😄
EDIT: that worked! 🎉🎉🎉
Everything looks absolutely great from dotPeek! 🚀
EDIT 2: preview package is now out! First one is Microsoft.Toolkit.Mvvm.7.0.0-build.1082.g849a01c6e3 🥳
ismaelestalayo
commented
3 years ago We could do simple properties with this too, eh?
[ObservableProperty("MyProperty", typeof(int), "Some Comment about what the property is for doc string.")]
Could an initial default value for the property be added there as well? 90% of the times I set up a default value for my Observable properties:
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 😄
Sergio0694
commented
3 years ago @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 [ObservableProperty] attribute that can be applied directly on a field, and can also automatically determine the property name based on the field name. This also gives developers the ability to choose the naming convention they prefer for field names. Also, this makes it very easy to add XML docs on a property, as we can just copy and tweak the XML docs applied to fields. This is what I have working so far:
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 _foo (with the same capitalization after trimming, and m_foo for those who love the C++ naming convention 😄
I also want to make it so that if you're only implementing INotifyPropertyChanged in the parent class, OnPropertyChanging is omitted automatically, which gives consumers additional flexibility. Thoughts? 🙂
jtippet
commented
3 years ago One gotcha I've discovered is that the compiler is unhappy putting virtual members into a sealed class:
[ObservableObject]
internal sealed partial class Demo
{
private int _x;
public int X
{
get => _x;
set => SetProperty(ref _x, value);
}
}gives error
Error CS0549 'Demo.OnPropertyChanging(PropertyChangingEventArgs)' is a new virtual member in sealed type 'Demo' ObservableObjectDemobecause the codegen hardcodes a virtual on a few members. I don't suppose the codegen could notice this case and remove the virtual keyword?
As an aside, I wonder if each generated method should also have [GeneratedCodeAttribute] on them?
Regarding [ObservableProperty], it would save me even more lines of code, which I really appreciate. (How is this not built in to the language yet?!) Although it "feels" a bit weird to emphasize the field and not the property. I don't know anything about how code generators work, so maybe this is impossible, but I'd prefer to invert things so you write the property and generate the field. Something like:
internal partial class Demo
{
[ObservableProperty]
public int X { get; set; }
[ObservableProperty]
public int Y { get; private set; } = 42
}to generate
internal partial class Demo
{
private field _x;
public int X { get => _x; set => SetProperty(ref _x, value); }
private int _y = 42;
public int Y { get => _y; private set => SetProperty(ref _y, value); }
}That is much closer to how the compiler-generated fields work already, and it also gives a natural and familiar place to put private or protected. But it might just be flat-out impossible to do with code generation; you might need IL rewrite instead. (Maybe C# 10 will have XAML apps as a theme, and throw a bone to INotifyPropertyChanged...)
Sergio0694
commented
3 years ago "[...] the compiler is unhappy putting virtual members into a sealed class [...]"
Oh, good catch! Yeah I'll have to add special handling for that and strip virtual when the target class is sealed. Thanks!
"I wonder if each generated method should also have
[GeneratedCodeAttribute]on them?"
I could definitely do that, just wondering how useful that would be? Is that attribute commonly used? 🤔
For now I'm adding [DebuggerNonUserCode] and [ExcludeFromCodeCoverage].
"[...] but I'd prefer to invert things so you write the property and generate the field."
That's unfortunately not possible with source generators, as they're explicitly additive and can't modify/rewrite existing code. They could in the original proposal but has since been changed to make them a compiler-only feature without the need to also update the C# language to support it (as there would've been the need of new keywords to indicate rewritable members).
jtippet
commented
3 years ago I could definitely do that, just wondering how useful that would be?
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.
Sergio0694
commented
3 years ago That should already not be a problem, since all the generated files here will have #pragma warning disable 🤔
jtippet
commented
3 years ago Ah, fair enough, the #pragma might have the same effect.
Sergio0694
commented
3 years ago 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 🙂
Sergio0694
commented
3 years ago Small update (cc. @jtippet) as of https://github.com/windows-toolkit/WindowsCommunityToolkit/pull/3873/commits/764d49b120221753e832531f69a69bb25237a0ae
✅ Added handling for sealed class (members will become private, and no virtual anymore)
✅ Added [GeneratedCode] attribute to all members (indicating generator type and version)
Plus a bunch of other improvements and tweaks 😄
jtippet
commented
3 years ago 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 [ObservableProperty], because I need to do something when the value changes. I could subscribe to my own PropertyChanged event, but that's not super efficient. An efficient alternative is to add new attributes that let me drop in a bits of code:
[ObservableProperty]
[OnChanging(nameof(ValidateThing))]
[OnChanged(nameof(LogThingChange))]
private Thing _thing;which would expand out to something like:
private Thing _thing;
public Thing Thing
{
get => _thing;
set
{
if (!EqualityComparer(_thing, value))
{
ValidateThing(nameof(Thing), newValue); // injected by OnChangingAttribute
OnPropertyChanging();
var oldValue = _thing;
_thing = value;
LogThingChange(nameof(Thing), oldValue, value); // injected by OnChangedAttribute
OnPropertyChanged();
}
}
}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:
private Thing _thing;
public Thing Thing
{
get => _thing;
set
{
if (!EqualityComparer(_thing, value))
{
if (_thing is not null)
{
_thing.PropertyChanged -= OnThingPropertyChanged;
}
_thing = value;
OnPropertyChanged();
if (_thing is not null)
{
_thing.PropertyChanged += OnThingPropertyChanged;
}
}
}
}That's formulaic enough that it could all be stuffed into a single attribute:
[ObservableProperty]
[PropertyEventSubscription(nameof(Thing.PropertyChanged), nameof(OnThingPropertyChanged))]
private Thing _thing;Ideally, these 3 attributes have AllowMultiple = true and generate efficient code if you use multiple of them on the same field.
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".
ismaelestalayo
commented
3 years ago 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 [ObservableProperty] comes into play: it's already "unconventional" in the way that it lets you define observable properties by annotating a field, but it's still simple enough to be intuitive to use. If we start to add too many custom features that would only be usable in very specific cases, we quickly make the whole thing much more complex for no real gain. I'd suggest that in these cases you'd be better off just implementing your observable property manually as usual, which would give you full control, rather than trying to get too many custom and very niche attributes into the actual MVVM Toolkit. Hope this makes sense 😄
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 [AlsoNotifyFor] attribute is now implemented and can be tested with the latest preview CI packages 😄
timunie
commented
3 years ago 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 CustomValidation but unfortunately this will make the UI unresponsible if the file is on a network share which is not connected.
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 ValidatePropertyAsync(object value, string PropertyName)
Happy coding Tim
Sergio0694
commented
3 years ago 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 [ValidationAttribute] type in the BCL and all the accompanying classes (this is due to the MVVM Toolkit having the idea of being a "reference implementation" as one of its core principles, so we're following the BCL and the standard types as much as possible). As you can see from the .NET API browser, this attribute is strictly meant to be used for quick, synchronous validation scenarios, and it doesn't really support asynchronous operations. This goes for companion APIs such as Validator too, which we're using in the MVVM Toolkit as well. I think that for asynchronous custom validation methods you should probably do these checks manually through the pattern of your choice (eg. possibly by writing some custom control/extension to support this).
Hope this makes sense 🙂
timunie
commented
3 years ago 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
[ValidationAttribute]type in the BCL and all the accompanying classes (this is due to the MVVM Toolkit having the idea of being a "reference implementation" as one of its core principles, so we're following the BCL and the standard types as much as possible). As you can see from the .NET API browser, this attribute is strictly meant to be used for quick, synchronous validation scenarios, and it doesn't really support asynchronous operations. This goes for companion APIs such asValidatortoo, which we're using in the MVVM Toolkit as well. I think that for asynchronous custom validation methods you should probably do these checks manually through the pattern of your choice (eg. possibly by writing some custom control/extension to support this).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 bool? which indicates the state of the validation. true if the file available, false if the file is not found, null while checking the filename async.
Happy coding and happy easter Tim
sonnemaf
commented
3 years ago I love this. I wonder if there should be a way to change the accessibility of the observable properties. I don't think they should always be public. Maybe you should add some properties in the ObservableProperty which you can use to specify the modifier of the property and one for the setter. Maybe you also need a way to specify that the property is virtual or sealed.
public partial class Employee : ObservableObject
{
[ObservableProperty(Modifier="internal")]
private string name;
[ObservableProperty(ModfierSet = "private")]
private decimal salary;
}This would generate the following code
public sealed partial class Employee : ObservableObject
{
private string name;
internal string Name {
get => name;
set {
// the generated code for a setter
}
}
private decimal salary;
public string Salary {
get => salary;
private set {
// the generated code for a setter
}
}
}
MichaeIDietrich
commented
3 years ago Nice idea to make use of source generators for all this boilerplate code.
I would like to add one addition to the ICommandAttribute to support CanExecute for the generated commands.
e.g. that way:
[ICommand(CanExecute = nameof(CanExecute))]
private void Execute()
{
...
}
private bool CanExecute()
{
...
}This should simply generate something like:
public ICommand ExecuteCommand => new RelayCommand(Execute, CanExecute);Hi together,
@MichaeIDietrich I think this is a good point, lets see if this is possible. But this lets me think of another addition to the ICommand. Let's think of a command that only can run if another property is within a certain value. Let's assume we have a propertybool CanUserSave {get; set;}and if this property changes we want to update theRelayCommand SaveFileCommand {get;}which is only avaialble iftrue`
Option 1:
[ObservableProperty]
private bool canUserSave;
[ICommand]
[UpdateCommandOnPropertyChanged (string PropertyName)]
void SaveFile() { ... }or
Option 2
[ObservableProperty]
[UpdateCommand (string CommandName)]
private bool canUserSave;
[ICommand]
void SaveFile() { ... }Happy coding Tim
virzak
commented
3 years ago Also, there are situations where CanExecute has less parameters than Execute:
[ICommand(CanExecute = nameof(CanExecute))]
private void Execute(SomeMouseArgs e)
{
...
}
private bool CanExecute()
{
...
}Perhaps then the generated code could be:
public ICommand ExecuteCommand => new RelayCommand<SomeMouseArgs>(Execute, _ => CanExecute());
CBenghi
commented
3 years ago Hello,
Is this expected to work for WPF applications as well?
I've tried a minimal implementation on a net5.0-windows Wpf target and I'm finding unexpected compilation errors.
I am not sure if this is down to the build system or the generated code, Visual Studio's behaviour is very erratic as I try using this feature.
At some point I had it working on another application, but as I've tried to create a minimum repro the code that fails is quite small.
There's a minimal repro at: https://github.com/CBenghi/mvvmSourceGenIssue
I hope I have not missed something obvious.
Many thanks. Claudio
michael-hawker
commented
3 years ago @CBenghi the MVVM Toolkit should work regardless of the framework project type, that's our goal and one of our guiding principles.
@Sergio0694 want to take a look?
timunie
commented
3 years ago Hi @CBenghi @michael-hawker @Sergio0694
As I often use WPF for my Apps I had a look in the sample. It does work on my side if I don't use the generated code directly in the given class. The Bindings and Commands work.
Here is a PR: https://github.com/CBenghi/mvvmSourceGenIssue/pull/1
Happy coding Tim
CBenghi
commented
3 years ago @timunie, super thanks, this is a good workaround that makes it possible to use the feature.
I think that this limitation only applies to WPF because a sample that @Sergio0694 posted on twitter makes direct use of the public property (Counter) from the modelview code (https://twitter.com/SergioPedri/status/1377959434119438336).
I'm inclined to think that this is more likely a problem with the source generators infrastructure rather than mvvm toolkit's implementation.
Sergio0694
commented
3 years ago I've tested @CBenghi's repro on my end (which works fine with @timunie's fix), and also tried to add a setup similar to the one I used in that tweet you linked, but that worked just fine for me as well. Have you made sure to be testing this on the latest Preview version of VS? I'm on VS 16.10 Preview 2 and I have no problems at all with this code:
public partial class MainViewModel : ObservableObject
{
[ObservableProperty]
private int counter;
[ICommand]
private void IncrementCounter()
{
Counter++;
}
}Which works as expected:
So to me the issue you had before seemed more like a tooling issue rather than something in the MVVM Toolkit itself, yeah. If you stumble upon this issue again, please take note of what exact error you're getting as well, that'd help! 🙂
timunie
commented
3 years ago Hi @Sergio0694
your example does not compile on my side. It shows this in the output but no error:
error CS0103: The name 'Counter' does not exist in the current contextVS-Version: 16.9.4
EDIT:
And I am getting this after Build:
StreamJsonRpc.RemoteInvocationException: Der Objektverweis wurde nicht auf eine Objektinstanz festgelegt.
at StreamJsonRpc.JsonRpc.<InvokeCoreAsync>d__139`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at System.Threading.Tasks.ValueTask`1.get_Result()
at System.Runtime.CompilerServices.ConfiguredValueTaskAwaitable`1.ConfiguredValueTaskAwaiter.GetResult()
at Microsoft.CodeAnalysis.Remote.BrokeredServiceConnection`1.<TryInvokeAsync>d__17`1.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)
RPC server exception:
System.NullReferenceException: Der Objektverweis wurde nicht auf eine Objektinstanz festgelegt.
bei Microsoft.CodeAnalysis.CodeLens.CodeLensReferencesService.<TryGetMethodDescriptorAsync>d__8.MoveNext()
--- Ende der Stapelüberwachung vom vorhergehenden Ort, an dem die Ausnahme ausgelöst wurde ---
bei System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
bei System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
bei Microsoft.CodeAnalysis.CodeLens.CodeLensReferencesService.<>c__DisplayClass9_0.<<FindReferenceMethodsAsync>b__0>d.MoveNext()
--- Ende der Stapelüberwachung vom vorhergehenden Ort, an dem die Ausnahme ausgelöst wurde ---
bei System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
bei System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
bei Microsoft.CodeAnalysis.CodeLens.CodeLensReferencesService.<FindAsync>d__1`1.MoveNext()
--- Ende der Stapelüberwachung vom vorhergehenden Ort, an dem die Ausnahme ausgelöst wurde ---
bei System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
bei System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
bei Microsoft.CodeAnalysis.Remote.RemoteCodeLensReferencesService.<>c__DisplayClass5_0.<<FindReferenceMethodsAsync>b__0>d.MoveNext()
--- Ende der Stapelüberwachung vom vorhergehenden Ort, an dem die Ausnahme ausgelöst wurde ---
bei System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
bei System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
bei System.Threading.Tasks.ValueTask`1.get_Result()
bei Microsoft.CodeAnalysis.Remote.BrokeredServiceBase.<RunServiceImplAsync>d__12`1.MoveNext()
--- Ende der Stapelüberwachung vom vorhergehenden Ort, an dem die Ausnahme ausgelöst wurde ---
bei System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
bei Microsoft.VisualStudio.Telemetry.WindowsErrorReporting.WatsonReport.GetClrWatsonExceptionInfo(Exception exceptionObject)Unfortunatally it is German, and I have no idea how to change it :-(
Happy coding Tim
Sergio0694
commented
3 years ago That's to be expected, as I said it's important to make sure to be on the latest preview version of Visual Studio 🙂 Source generators are basically still in alpha (not even in preview), and they're still very much work in progress, both with respect to the actual APIs being available to source generators themselves, to tooling in Visual Studio. Thanks for confirming that though, I'd say we can definitely write this off as just some transient bug somewhere in the build infrastructure that has already been solved in the new VS Preview version (which also includes new Roslyn builds as well), so that's good to know 👍
CBenghi
commented
3 years ago I'd say we can definitely write this off as just some transient bug somewhere in the build infrastructure that has already been solved in the new VS Preview version (which also includes new Roslyn builds as well), so that's good to know 👍
Just for completeness let me say that I had already checked on the latest version of VS preview as soon as it was published (Version 16.10.0 Preview 2.0) and it does not work for me, so it must be something more esoteric than that. But we agree it's likely a tooling issue.
michael-hawker
commented
3 years ago @Sergio0694 @CBenghi what version of .NET 5 are you both on as I think there's been updates out-of-band from VS itself, eh?
Sergio0694
commented
3 years ago The latest .NET 5 SDK on my machine is 5.0.300-preview.21180.15 (the latest non-preview I have is 5.0.202).
I should note I also have the 6.0.100-preview.3.21202.5 SDK installed (and I'm using VS 16.10 Preview 2).
CBenghi
commented
3 years ago dotnet --info gives me:
2.1.523 [C:\Program Files\dotnet\sdk] 2.2.107 [C:\Program Files\dotnet\sdk] 3.1.401 [C:\Program Files\dotnet\sdk] 5.0.201 [C:\Program Files\dotnet\sdk] 5.0.202 [C:\Program Files\dotnet\sdk] 5.0.300-preview.21180.15 [C:\Program Files\dotnet\sdk]
So I might just be missing the 6.x sdk.
I'm also on VS16.10 Preview 2
CBenghi
commented
3 years ago Just for completeness my dotnet build .\mvvmIssue.sln on the latest main of https://github.com/CBenghi/mvvmSourceGenIssue
returns
Microsoft (R) Build Engine version 16.10.0-preview-21175-01+afd0b6210 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.
Determining projects to restore...
All projects are up-to-date for restore.
You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
C:\Data\Dev\Tests\mvvmIssue\TryOutClass.cs(25,4): error CS0103: The name 'SomeValue' does not exist in the current context [C:\Data\Dev\Tests\mvvmIssue\mvvmIssue_zsxi3uh1_wpftmp.csproj]
Build FAILED.
C:\Data\Dev\Tests\mvvmIssue\TryOutClass.cs(25,4): error CS0103: The name 'SomeValue' does not exist in the current context [C:\Data\Dev\Tests\mvvmIssue\mvvmIssue_zsxi3uh1_wpftmp.csproj]
0 Warning(s)
1 Error(s)
Time Elapsed 00:00:01.07@Sergio0694 can you confirm you can also compile from command line?
Sergio0694
commented
3 years ago Yup, builds just fine for me with the latest commit from main (e8a549b3f01f5248b932a59e255d929a80d35aa8) 🙂
C:\Users\Sergio\Documents\GitHub\mvvmSourceGenIssue>dotnet build mvvmIssue.sln
Microsoft (R) Build Engine versione 16.10.0-preview-21181-07+073022eb4 per .NET
Copyright (C) Microsoft Corporation. All rights reserved.
Determining projects to restore...
C:\Users\Sergio\Documents\GitHub\mvvmSourceGenIssue\mvvmIssue.csproj ripristinato (in 2,83 sec).
You are using a preview version of .NET. See: https://aka.ms/dotnet-core-preview
mvvmIssue -> C:\Users\Sergio\Documents\GitHub\mvvmSourceGenIssue\bin\Debug\net5.0-windows\mvvmIssue.dll
Build completed.
Warnings: 0
Errors: 0
Time elapsed 00:00:05.17Alright, wanted to post a draft of some ideas I've been thinking about regarding support for "can execute" for the [ICommand] attribute. One thing I'm focusing on is to streamline the workflow for developers like I'm already doing in the other attributes, ie. having the source generators be "smart" enough that it can adapt to your code (to a degree that's indicated in the docs), rather than requiring you to alter your approach depending on the exact scenario. A good example of this is eg. how [ICommand] will automatically create the right command type on its own based on the signature of the annotated methods (instead of eg. having different attributes for each supported command type). Basically we have these requirements:
void-taking), as well as propertiesWith this, I'm basically thinking about something like this, and wanted to gather some early feedbacks 🙂
There's two separate features here, namely the "can execute" callback passing, and actually raising the change there.
For this I was thinking what @MichaeIDietrich proposed as well, ie. a new CanExecute property for [ICommand]:
[AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)]
public sealed class ICommandAttribute : Attribute
{
public string CanExecute { get; init; }
}The caveat though is that I also want this to dynamically adapt to multiple scenarios (similar to what @virzak mentioned).
Here's how I'd imagine the codegen to work in all these individual cases:
Here the solution seems pretty straightforward and it's mostly just about choosing the best name for the attribute. I've thought about just reusing [AlsoNotifyFor], but I don't like the fact that the semantics there would be entirely dependent on the context, since when the target name is a property it'd be converted into OnPropertyChanged, whereas when the target property is a command it'd instead generate a RaiseNotifyChanged call (and not actually notify the command property). It'd also be much more difficult to implement due to source generators not being ordered, and in this case the generated command property would be generated by a separate generator (the one responsible for generating observable properties). So instead I'm thinking of adding a new, single-purpose and self-explanatory attribute such as:
[AttributeUsage(AttributeTargets.Field, AllowMultiple = false, Inherited = false)]
public sealed class AlsoNotifyCanExecuteFor : Attribute
{
public AlsoNotifyCanExecuteFor(string commandName);
public AlsoNotifyCanExecuteFor(string commandName, string[] otherCommandNames);
public string[] CommandNames { get; }
}And at the same time, also renaming the current [AlsoNotifyFor] to [AlsoNotifyChangeFor], so that the two attributes would be completely unambiguous and easier to understand just by their name (one for properties, one for commands). The usage and codegen would be as follows (kinda similar to the 2nd option suggested by @timunie as well):
With this approach it'll also be possible to both have a single generated property raise multiple commands for can execute changed, as well as having the same command being notified for can execute changed by multiple properties (which can be either generated, manual, or also a combination of both). All of this while the source generator implementation also remains pretty modular and easy to work on at the same time, which doesn't hurt 🥳
Here's a composite example of a viewmodel and the complete generated code that would result with these changes:
User code:
[ObservableProperty]
[AlsoNotifyCanExecuteFor(nameof(MyCommand))]
private User? currentUser;
[ICommand(CanExecute = nameof(CanGreetUser))]
private void GreetUser()
{
Console.WriteLine($"Hello {CurrentUser.Name}!");
}
private bool CanGreetUser => CurrentUser is { IsFriendly: true };Generated code (assuming a base ObservableObject class):
public User? CurrentUser
{
get => currentUser;
set
{
if (SetProperty(ref currentUser, value))
{
GreetUser.NotifyCanExecuteChanged();
}
}
}
private RelayCommand? greetUserCommand;
public IRelayCommand GreetUserCommand => greetUserCommand ??= new(GreetUser, () => CanGreetUser);cc. @michael-hawker
michael-hawker
commented
3 years ago @Sergio0694 I think these needed updated docs? We probably just want a one-pager about it to supplement the existing ones. Do you have a PR open on that already?
czdietrich
commented
3 years ago @Sergio0694 So the tasks for "CanExcecute"-support are not checked. Does it mean it is currently not supported by the source generators?
Sergio0694
commented
3 years ago @czdietrich That is correct, the current source generator support in the MVVM Toolkit is still to be considered in preview. We plan to fix all currently known bugs by the time we ship the 7.1 release, but not all planned features will be included by then. The general plan is to make it feature complete with the release after that, which should align with the MVVM Toolkit and the other .NET libraries being split into the new .NET Community Toolkit repo. That said, even with those currently missing features, the source generator support that is available today should be fully functional. The only potential issue is that it might make the IDE slightly slow if you work on large solutions, as it's not using an incremental source generator (that's planned for the final release though). In general though it should work quite well, let us know how it goes! 😄
huoyaoyuan
commented
2 years ago Quick feedback: if I implement IRecipient<> on a user control with generated code from xaml (I know this might be anti-pattern), the generator will attempt to generate twice. Roslyn reports duplicated hint name from the generator.
huoyaoyuan
commented
2 years ago Another quick feedback: the observable property generator doesn't handle generic class. It loses the <T> in class definition in generated code.
Sergio0694
commented
2 years ago Hey @huoyaoyuan, thanks for the feedbacks!
"if I implement
IRecipient<>on a user control with generated code from xaml (I know this might be anti-pattern), the generator will attempt to generate twice. Roslyn reports duplicated hint name from the generator."
To be honest I'm surprised this didn't just fail to build directly. As far as I know there's some issues with generators working on files that also targeted by the XAML compiler, due to how the XAML generator is inserted into the build pipeline. I'm not entirely sure this is something that can be addressed by us, as we're really just shipping a normal source generator with nothing specific for UWP or any other UI framework at all. Maybe there's an open bug in the Roslyn repo or somewhere else (eg. the WinUI repo) that we can link to this to help tracking? If not, please feel free to open one!
"the observable property generator doesn't handle generic class. It loses the
in class definition in generated code."
I think we don't currently support generic types at all, as the whole source generator feature is still in preview. Adding proper support for generic types is definitely on my todo list though, for sure! 😄
kmgallahan
commented
2 years ago @Sergio0694
Great work so far on the source generators! Really liking [ICommand] and looking forward to [CanExecute]. Just wish there was better support for Intellisense from XAML, but AFAIK that is dependent on work being done elsewhere.
Right now I use Fody/PropertyChanged to handle INotifyPropertyChanged via IL weaving. The beauty of it is an opt-out model where the default configuration applies INotifyPropertyChanged logic to all public properties in all types that implement INotifyPropertyChanged automatically. This includes dependent properties, e.g.:
public string FirstName { get; set; }
public string LastName { get; set; }
public string FullName => $"{FirstName} {LastName}";This means I essentially have to write zero code to get property change notifications throughout a project. So questions for you:
[ObservableObject] does this already.[AlsoNotifyFor]?[CanExecute] play nicely with Fody PropertyChanged? Unsure if I'll need to opt out to get those to work properly or not.Thanks for your work here so far.
kmgallahan
commented
2 years ago @Sergio0694 You might have missed my comment above while transferring this issue. If not, apologies, no rush for reply.
Sergio0694
commented
2 years ago Hey @kmgallahan, sorry for the delay 😄 Glad to hear you like this feature so far, that's awesome!
"Really liking
[ICommand]and looking forward to[CanExecute]"
Was planning to do some work on the MVVM Toolkit today, so might go implement just that 😋
Regarding your questions about the differences with Fody, there's some pretty fundamental differences between that and the MVVM Toolkit (which uses source generators instead of IL weaving), which are by design. Going through your questions:
[ObservableProperty] on a field.Hope this helps! 🙂
Sergio0694
commented
2 years ago Hey everyone! 👋 We just published a new preview on NuGet, it's version 8.0.0 Preview 1. It includes all the recent changes to the MVVM Toolkit I've been doing, such as:
CanExecute for commandsPlus a lot of fixes (eg. they should work with generic viewmodels too now, etc.). If you try it out, please let us know how it goes and if you find any issues! 😄
The
Microsoft.Toolkit.Mvvmpackage (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 https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/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 thePropertyChangedandPropertyChangingevents, 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:
```csharp public sealed partial class PersonViewModel : ObservableObject { [ObservableProperty] [AlsoNotifyFor(nameof(FullName))] private string name; [ObservableProperty] [AlsoNotifyFor(nameof(FullName))] private string surname; public string FullName => $"{Name} {Surname}"; } ```
Generated code:
```csharp public sealed partial class PersonViewModel { [global::System.CodeDom.Compiler.GeneratedCode("Microsoft.Toolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", "7.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCode] [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] public string Name { get => name; set { if (!global::System.Collections.Generic.EqualityComparer
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>,AsyncRelayCommandorAsyncRelayCommand<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:
```csharp public sealed partial class MyViewModel { [ICommand] private void Greet() { } [ICommand] private void GreetUser(User user) { } [ICommand] private Task SaveFileAsync() { return Task.CompletedTask; } [ICommand] private Task LogUserAsync(User user) { return Task.CompletedTask; } } ```
Generated code:
```csharp public sealed partial class MyViewModel { [global::System.CodeDom.Compiler.GeneratedCode("Microsoft.Toolkit.Mvvm.SourceGenerators.ICommandGenerator", "7.0.0.0")] private global::Microsoft.Toolkit.Mvvm.Input.RelayCommand? greetCommand; [global::System.CodeDom.Compiler.GeneratedCode("Microsoft.Toolkit.Mvvm.SourceGenerators.ICommandGenerator", "7.0.0.0")] [global::System.Diagnostics.DebuggerNonUserCode] [global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage] public global::Microsoft.Toolkit.Mvvm.Input.IRelayCommand GreetCommand => greetCommand ??= new global::Microsoft.Toolkit.Mvvm.Input.RelayCommand(new global::System.Action(Greet)); [global::System.CodeDom.Compiler.GeneratedCode("Microsoft.Toolkit.Mvvm.SourceGenerators.ICommandGenerator", "7.0.0.0")] private global::Microsoft.Toolkit.Mvvm.Input.RelayCommand
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 recipientin the original method is less specific than aMyViewModel recipientparameter 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
RegisterAllis 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 moreMakeDynamicMethod), 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.ValidateAllPropertiesmethod. 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]RegisterAllmethod when possibleValidateAllPropertiesmethod when possible[ObservableProperty][AlsoNotifyChangeFor][ICommand]CanExecuteproperty for[ICommand][AlsoNotifyCanExecuteFor]Feedbacks and feature ideas are welcome! 🙌