CommunityToolkit / dotnet

.NET Community Toolkit is a collection of helpers and APIs that work for all .NET developers and are agnostic of any specific UI platform. The toolkit is maintained and published by Microsoft, and part of the .NET Foundation.
https://docs.microsoft.com/dotnet/communitytoolkit/?WT.mc_id=dotnet-0000-bramin
Other
2.99k stars 294 forks source link

ObservableObject annotation with record not generating OnPropertyChanged and OnPropertyChanging invokers #785

Open EvilAngel00 opened 11 months ago

EvilAngel00 commented 11 months ago

Describe the bug

When using the annotations [ObservableObject] and [ObservableProperty] in a class, it correctly generates (at least) two files. One containing the property which when set calls the functions OnPropertyChanging/OnPropertyChanged and another one containing those functions and the handlers for those events.

If we change the class to a record, the first file is correctly generated but not the second, causing the OnPropertyChanging/OnPropertyChanged calls not to compile.

Here's an example project: ObservablePropertyInRecord.zip

Changing the RecordViewModel from a record to a class behaves as expected.

Regression

No response

Steps to reproduce

Example steps to reproduce:

1. Create a `public partial record` with the `[ObservableObject]` annotation
2. Create a variable with the `[ObservableProperty]` annotation
3. Code should not compile and give the errors:

CS0103  The name 'OnPropertyChanging' does not exist in the current context
CS0103  The name 'OnPropertyChanged' does not exist in the current context

Expected behavior

The code generator successfully creates the handlers for a record as it does for a class.

Screenshots

ViewModel as a class:

Screenshot 2023-10-31 093049

Generated code:

Screenshot 2023-10-31 093134 Screenshot 2023-10-31 093146

ViewModel as a record:

Screenshot 2023-10-31 093205

Generated code (the second file is missing):

Screenshot 2023-10-31 093214 Screenshot 2023-10-31 093228

IDE and version

VS 2022

IDE version

17.7.6

Nuget packages

Nuget package version(s)

8.2.2

Additional context

No response

Help us help you

No, just wanted to report this

JochenMader commented 10 months ago

Hi there

I checked this one out.

In TransitiveMembersGenerator.cs:67 is a type check with inline declaration:

node is ClassDeclarationSyntax

A record is not recognized as ClassDeclarationSyntax, so the generation skips here.

Changing the code to the following will do the work:

static (node, _) => (node is ClassDeclarationSyntax classDeclaration && classDeclaration.HasOrPotentiallyHasAttributes())
    || (node is RecordDeclarationSyntax recordDeclaration && recordDeclaration.HasOrPotentiallyHasAttributes()),

But do we really want record view models?

best regards

DrkWzrd commented 10 months ago

Records should not be view models. The mere nature of the record is to be immutable. The viewmodels, au contraire, is a construct with bindings and designed and tought to be mutable.