Skip to content
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

Add support for [ObservableProperty] on partial properties #555

Closed
7 tasks done
Sergio0694 opened this issue Jan 4, 2023 · 44 comments · Fixed by #966
Closed
7 tasks done

Add support for [ObservableProperty] on partial properties #555

Sergio0694 opened this issue Jan 4, 2023 · 44 comments · Fixed by #966
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit

Comments

@Sergio0694
Copy link
Member

Sergio0694 commented Jan 4, 2023

Overview

This issue tracks adding support for partial properties to the [ObservableProperty] generator. This is blocked on dotnet/csharplang#6420, so support for this will have to wait for that first (hopefully in the C# 12 timeframe).

API breakdown

The changes are pretty straightforward: properties will also be allowed on [ObservableProperty]:

-[AttributeUsage(AttributeTargets.Field, AllowMultiple = false, Inherited = false)]
+[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false, Inherited = false)]
 public sealed class ObservablePropertyAttribute : Attribute
 {
 }

Analyzers will also need to be added to ensure that the annotated properties are partial and valid.

Usage example

Code like this:

[ObservableProperty] 
public partial string Name { get; private set; }

Will generate the following:

/// <inheritdoc cref="_itemsSource"/>
[global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", "8.1.0.0")]
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
public partial string Name
{
    get => field;
    set
    {
        if (!global::System.Collections.Generic.EqualityComparer<string>.Default.Equals(field, value))
        {
            OnNameChanging(value);
            OnPropertyChanging(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangingArgs.Name);
            
            field = value;

            OnItemsSourceChanged(value);
            OnNameChanged(global::CommunityToolkit.Mvvm.ComponentModel.__Internals.__KnownINotifyPropertyChangedArgs.Name);
        }
    }
}

/// <summary>Executes the logic for when <see cref="Name"/> is changing.</summary>
[global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", "8.1.0.0")]
partial void OnNameChanging(string value);

/// <summary>Executes the logic for when <see cref="Name"/> just changed.</summary>
[global::System.CodeDom.Compiler.GeneratedCode("CommunityToolkit.Mvvm.SourceGenerators.ObservablePropertyGenerator", "8.1.0.0")]
partial void OnNameChanged(string value);

This will also enable support for declaring custom accessibility modifiers for property accessors.

Progress tracking

  • Initial code generation support
  • Add analyzer for C# preview
  • Add analyzer to use partial properties instead of fields
  • Add code fixer to convert fields to partial properties
  • Add analyzer for partial property declarations
  • Add analyzer for unsupported Roslyn version for properties
  • Update all titles/descriptions of existing shared diagnostics
@cmiles
Copy link

cmiles commented May 18, 2023

@Sergio0694 I haven't tried to track a language proposal before and couldn't immediately tell from the Partial Properties proposal/labels/information whether it is still expected to be available in the November 2023 time frame that you mentioned in #291 - since you have been tracking and adding to the the Partial Properties proposal/discussion any chance you can add any current information about time frame estimates here?

Fwiw From what I've read here and in other issues Partial Properties seem like a great solution - just wondering if it is truly 'coming soon' or if this pushes into 2024 and beyond.

Thanks for your time and any update!

@Sergio0694
Copy link
Member Author

Hey @cmiles! We don't have any updates/ETAs to share at this time. Keep in mind that no C# features at all are confirmed until the moment they actually ship, so the same applies here. We're still actively looking into this though, just no news just yet 🙂

@modmynitro
Copy link

Its not in C# 12
dotnet/csharplang#6420 (comment)

@cmiles
Copy link

cmiles commented Oct 21, 2023

@Sergio0694 since this did not make it into C#12 any chance that of any of the associated issues will be reconsidered for action in a more immediate time frame?

I understand partial properties would facilitate the ideal implementation of things like 'Required' keyword support - but there is some functionality blocked by this that would be immediately useful and appreciated even if there were breaking changes in the future if/when partial properties make it into C#.

@Sergio0694
Copy link
Member Author

"but there is some functionality blocked by this"

What functionality is blocked?

@cmiles
Copy link

cmiles commented Oct 22, 2023

What functionality is blocked?

@Sergio0694 - It was my understanding that all of the issues/functionality listed below are blocked by wanting to wait and create an implementation based around the partial property language feature.

For me the functionality the required keyword offers is particularly attractive as we work on enabling nullable support in our code - but I'm not aware of any way to combine [ObservableProperty] and required?

Based on your question of 'What functionality is blocked?' maybe I am mistaken and there is a feature or approach that I'm not aware of to use [ObservableProperty] and required together - if so apologies for the wasted time/space in this issue but would very much appreciate a link to get me started!

Add support for "required" and "virtual" keywords to property generator · Issue #679 · CommunityToolkit/dotnet
Make ObservableProperty Generated Code Properties Virtual? · Issue #543 · CommunityToolkit/dotnet
Add support for setter protection level for ObservableProperty · Issue #758 · CommunityToolkit/dotnet
Add abilities to control the access level of generated [ObservableProperty] attribute · Issue #585 · CommunityToolkit/dotnet

@Sergio0694
Copy link
Member Author

"all of the issues/functionality listed below are blocked"

"For me the functionality the required keyword offers is particularly attractive"

No functionality is actually blocked. You can easily use all that — just manually define the properties 🙂

I understand the frustration for wanting a simpler way to do that, and I do agree that of course using the generator is better. But I don't think it's a good idea to dedicate resources to implement all that functionality when we might very well have partial properties out in preview soon, especially because then we'd still have to keep supporting that "workaround" to avoid breaking consumers. Rather, it makes more sense to just wait for partial properties to be available in preview, add support for those, and then release a preview of the MVVM Toolkit too so that people wanting to try those out can do so as well.

@cmiles
Copy link

cmiles commented Oct 23, 2023

@Sergio0694 No frustration here - seems like we just have a different evaluation of the issue.

Fwiw I would still '+1' working on a solution now - but respect and understand your reasoning and decision and no doubt you have 100% better insight on the odds of partial properties arriving in a near future preview.

I hope this issue will continue to be re-evaluated if for some reason partial properties, or something equivalently useful for generators, don't arrive for the next version of C#.

Thanks for the prompt replies - appreciated!

@jhm-ciberman
Copy link

This was not added to C# 12. Is there any reason to not implement the other more traditional alternatives using attributes?

@Sergio0694
Copy link
Member Author

This exact question and the answer for that are in the messages right above yours — the reason has not changed and it's the same as before 🙂

@AignerGames
Copy link

Partial properties are just in proposel state, it's not confirmed that they will be release any time soon (or at all). I don't think that it is a good idea to wait for some unknown future time and close all releated issues.

I understand your argument, that the current solution is a workaround, but it's a workaround that already exists, so adding more features to the already existing / working "workaround" would be better for every current user of the toolkit. You can declare it obsolete as soon as partial properties are implemented (if ever) and then you don't have to worry about maintaning it in the future.

Would it really take that many resources to add a additional parameter to control the accessor kind of the generated property to justify waiting for partial properties?

@DennisWelu
Copy link

@AignerGames is right on. I’ve had similar thoughts in the past about this issue. It could be years or never, yet the value is high and the effort seems at least relatively low. Worth the trouble of a depreciation period if needed.

@jhm-ciberman
Copy link

Personally, I don't use generators in MVVM Toolkit at all only for this sole issue.

@jhorv
Copy link

jhorv commented Apr 27, 2024

@Sergio0694 frustration here.

The fact that [ObservableProperty]s are public-public causes me to barely use it. If only I could specify a private set, then almost every one of my view models could have its lines of code cut in half. What's the point of deriving from ObservableObject if I'm always the one calling SetProperty()? I could have that with an extension method, for crying out loud.

It's becoming increasingly difficult to convince people (and believe myself) that I did a smart thing in choosing this library. To me it doesn't matter how, whether via attribute or some future C# language feature. Just add the ability and make these features useful for more than just demos.

@AignerGames
Copy link

Still...probably I don't understand the "weeks" of implementation needed but I imagine adding something in the meantime as simple as SetterVisibility=Visibility.Private as an option to ObservableProperty, and all that does is change the visibility keyword used in the generated code. FWIW.

I don't have experience with this code base, so my information could be wrong, but after a quick check I think that only a single place of the generator would have to be changed here to add the modifier type.

Well and the code to define the enum somewhere, modify the attribute to accept the enum and maybe unit tests.

@jhorv
Copy link

jhorv commented Apr 30, 2024

I will also say, as a side node and personal opinion, I don't really understand how not having support for private setters is a deal breaker. Yes, it's not as clean, but being pragmatic, it genuinely doesn't matter. Just ignore that, use the generators on fields, don't use the setters from the outside for now, and just go back and update the code once we get partial properties. This is exactly the same we're doing in the Microsoft Store app. The benefit of using the generators and saving hundreds and hundreds of lines of code vastly outweighs the fact that "oh this setter in this viewmodel is public but it should be private, even though literally nobody is setting from the outside anyway so it doesn't make any difference in practice". You're building an app, not a library, so you own the entire stack. Just don't use the setters for now, it will be fine.

For my organization and project, it matters a lot. The project I'm working on isn't just an app, but also a collection of libraries that other internal development teams will use. There is a client API library, a view model library, and a UI component library, and other devs may use all of it or just parts. As the provider of these libraries, it is a very big deal that these libraries be hard to use incorrectly. If Intellisense shows that something can be done/set/called then it must be a valid operation. That is the point of object-oriented encapsulation/data hiding, and all these public:public properties is the opposite of that: a nightmare for supportability, dev productivity, and our project's success.

@pos777
Copy link

pos777 commented Jul 5, 2024

What about a separate attribute? Something like:

[AttributeUsage(AttributeTargets.Field, AllowMultiple = false, Inherited = false)]
public sealed class SetterSettingsAttribute : Attribute
{
	public SetterSettingsAttribute(Accessibility accessibility) => Accessibility = accessibility;
	public Accessibility Accessibility { get; }
	//...
}
public enum Accessibility
{
	//...
	Private
	//...
}

public partial class Sample : ObservableObject
{
	[ObservableProperty, SetterSettings(Accessibility.Private)]
	private string name;
	public Sample(string name) => Name = name;
}

@Sergio0694
Copy link
Member Author

Using an attribute is strictly worse than using partial properties.

@AignerGames
Copy link

I think we all agree that partial properties would be the best solution, but they don't exist yet and will not be released anytime soon.

@Sergio0694
Copy link
Member Author

They are quite literally already merged in the Roslyn repo and will be available in VS 17.11 Preview 3, out shortly 😄
With them being part of C# 13 pretty much for sure at this point, I'm not going to be implementing a workaround for this.

@AignerGames
Copy link

AignerGames commented Jul 6, 2024

Yes the preview version, but most people will not use a preview version. The next full release is currently planned for November 24 (if no delays happen) so almost 2 years after this issue was opened. I agree that we can wait now, but adding a "workaround" 2 years ago would still have been better, since all the closed requests over the years prove that there was a use case for it.

@Sergio0694
Copy link
Member Author

Hindsight is 20/20. Partial properties were pushed back multiple times, we didn't know it'd take this long at the time 🙂

@AignerGames
Copy link

AignerGames commented Jul 6, 2024

I guess you are more optimistic than me, I don't trust any of their release dates until the feature is implemented, because I waited for many C# features in the past that were pushed back multiple times or only proposed and then never worked on until this day 😂

@pos777
Copy link

pos777 commented Jul 6, 2024

Using an attribute is strictly worse than using partial properties.

Agreed. If you can use partial properties, then you should do so =) But they are not always available. And I do not mean that they are not implemented yet. For example, the codebase may be pinned to C# 11 for some reason.

As stated in the https://github.com/dotnet/csharplang/blob/main/proposals/partial-properties.md article, the programmer must specify the access modifier when implementing partial properties:

partial class C1
{
    public partial string Prop { get; private set; }
    public partial string Prop { get => field; private set => field = value; }
}

Thus, the logic for generating the access modifier is required in this case too. In the case of partial properties, the information about required access modifier is available in the defining declaration. In turn, in the case of using fields, similar information could be taken from a special attribute. So we get

  • if it is possible to use partial properties:
[ObservableProperty]
public partial string Name { get; private set; }
  • if the use is limited to fields:
[ObservableProperty, WithPrivateSetter]
private string name;

If I understood correctly from the discussion, you do not want to modify the ObservableProperty attribute so as not to confuse users after the partial properties appear. Storing information in a separate attribute solves this problem, in my opinion. I will prepare a request to demonstrate the approach.

However, all this makes sense if you do not plan to abandon the approach with fields.

@LE-MarkusW
Copy link

Hi! I just came across this issue as I was looking for a solution to a problem that might be solved by this. I would definitely appreciate if a feature like this were available. I also just found out that the C# updates in .NET 9 Preview 6 actually includes the partial properties!

A first step might be for the [ObservableProperty] to generate a partial property, which to my knowledge should not break anything, while preserving the field approach. I am not sure if this will solve all of my use cases, but I would at least give it a try.

@Sergio0694
Copy link
Member Author

Generating a partial property wouldn't really do anything, other than making your code fail to compile 🙂

As for the real support in the MVVM Toolkit, I'm waiting to see whether field will make it as well.

@LE-MarkusW
Copy link

Ok, then my knowledge is not reaching far enough. I hope something like this can be implemented though.
I would need to annotate the generated Properties with Attributes for EF Core. But since the Navigation Properties have to be virtual anyway, I will probably have to o it by hand for the foreseeable Future anyway.

@LE-MarkusW
Copy link

LE-MarkusW commented Jul 18, 2024

Generating a partial property wouldn't really do anything, other than making your code fail to compile 🙂

Ok I played around with the Preview a little bit. I guess the Introduction of [PartialProperty] and [VirtualProperty] would probably be necessary for my use case. 😞

@cmiles
Copy link

cmiles commented Jul 18, 2024

@LE-MarkusW I don't really know your use case but if you haven't already it might be worth your time to look at Metalama for code generation. Metalama is a paid tool with a free tier - Pricing, License Information.

I used it to write my own INotifyPropertyChanged a little over a year ago in order to take advantage of the required keyword and and now use it for a few other generation tasks including Commands. For my simple requirements getting to working code was pretty quick.

Metalama isn't my focus - you can see much more beautiful code in the Metalama documentation - but here is my current code PointlessWaymarks.LlamaAspects.

@hez2010
Copy link

hez2010 commented Oct 11, 2024

It would be better to warn users on the old field usage as well and provide a code fix to help developers migrate from attributes on fields to attributes on partial properties after we ship the support.
The current way doesn't work well with COM/WinRT generators as all those properties are generated so that they cannot be seen by other source generators.

@Sergio0694
Copy link
Member Author

I plan on adding a code fixer for that as well, yes 🙂

@heartacker
Copy link

hello, when can we get this with dotnet 9 released?

@AndrewKeepCoding
Copy link

hello, when can we get this with dotnet 9 released?

  • Install CommunityToolkit.Mvvm v8.4.0-preview1.
    (Make sure you enabled the Include prerelease checkbox.)
  • In your *.csproj file, set the language version to preview.
    (field didn't make it to C#13 but is expected to be on C#14.)
    <PropertyGroup>
      <LangVersion>preview</LangVersion>
    </PropertyGroup>

@Mrxx99
Copy link

Mrxx99 commented Nov 15, 2024

When is a stable version released with this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request 📬 A request for new changes to improve functionality mvvm-toolkit 🧰 Issues/PRs for the MVVM Toolkit
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.