dotnet / csharplang

The official repo for the design of the C# programming language
11.49k stars 1.02k forks source link

Proposal: Make the NotifyChanged event part of each object #1419

Closed Starwer closed 6 years ago

Starwer commented 6 years ago

In my view, C#/WPF is the magic combo promoted by Microsoft. These are designed to work hand in hand and should support each-other optimally in a MVVM world. And I must say I'm pretty convinced by this association. Now, whereas the learning in C# is a snap for an experienced programmer, I feel that learning WPF is quite a long way interrupted with road-blocks and paved with frustration. The main reason: The Binding can be done only on class properties that implement INotifyPropertyChanged interface. I think the experienced WPF developers are smiling now thinking: "Of course ! This is an easy one !" But I think every newcomer in WPF has faced this problem, where he was trying to bind a non-observable property, or not even a property, or binding an ObservableCollection wishfully thinking that the changes on items on the collection would be detected... But also admit it, how much time did you spend to solve, implement or work-around this limitation or draw fancy dependency graphs to bind your properties correctly ?

This could have been avoided by having the change event implemented differently. Instead of having it patched over to the .NET library with the quick Implementation of the INotifyPropertyChanged interface, it could be handled by the compiler.

Think of an NotifyChanged event on the object base class, that would be handled directly by the C# Compiler... All the restriction to bind objects only if these are properties from an instance of object implementing an INotifyPropertyChanged would disappear, your Model could become your ViewModel, the binding would become simpler... and the performance would be way better: Less code size, faster response !

If this is too complicated for the compiler to figure out where the NotifyChanged should be added to the IL, we may consider to add a compiler attribute like [Observable]

Think of this as a new ViewModel:

[Observable]
public class Person
{
    [Observable]
    public string Name;

    [Observable]
    public Person[] Friends; 
}

That you can simply bind with:

<Border DataContext={Binding MainPersonInstance} >
    <TextBox Text={Binding} />
    <ItemsControl ItemsSource={Binding Friends}>
        <ItemsControl.ItemTemplate>
            <DataTemplate>
                <TextBox Text={Binding}/>
            </DataTemplate>
        </ItemsControl.ItemTemplate>
    /<ItemsControl>
</Border>

Wouldn't that make your day ?

bondsbw commented 6 years ago

Doing this automatically for all objects would incur a major performance penalty when it isn't needed.

Using an attribute would be covered by code generation (#107).

Starwer commented 6 years ago

Maybe I should add some example code to explain how you could handle this NotifyChanged event in the Model, in case my previous post was not already clear enough...

public class Example
{
    public void Register(Person pers)
    {
        // no null-check: remember that C# 8.0 makes value-types by default ;)
        pers.Name.NotifyChanged += OnNotifyChanged;
        pers.Friends.NotifyChanged += OnNotifyChanged;
    }

    private void OnNotifyChanged(object sender, NotifyChangedEventArgs e)
    {
        Console.WriteLine("Parent object is: {0}", e.Parent)
        if (sender is string str) {
            Console.WriteLine("The name of the person is now: {0}", str);
        } 
        else if (sender is Person[] friends)
        {
            if (e.Action == CollectionChangeAction.Add) 
            {
                var pers = e.Parent as Person;
                Console.WriteLine("{0} has a new friend !", pers );
            }
        }
    }

}
Starwer commented 6 years ago

@basoundr : From what I understand, CodeGenerator would make it easier to write a standard INotifyPropertyChanged implementation, but that would be just cosmetic. This would still have the limitations of the INotifyPropertyChanged:

  1. Only a property from an instance can be observed (bound). Not possible:

    [Observable]
    public string Name;
  2. To bind a property, the binding must know the source instance and the property path;

  3. To have the items of a collection observable, all object in the collection must be ViewModel class. Not possible:

    [Observable]
    public Person[] Friends; 
  4. If I want to be notified in the Model of change (any) in a list, the solution without the proposed change is quite expensive and complicated, involving creating a list of class containing observable string properties that we must all bind to their parent like in : https://gist.github.com/itajaja/7507120. A compiler could just detect (instrument) any access to an instance of Person.Friends sub items and generate the notification code only when needed.

svick commented 6 years ago

@Starwer How does your proposal remove those limitations?

jnm2 commented 6 years ago

In the meantime, this comment makes the case that there's a way to make life much easier in those objects which must implement or inherit from a class implementing INotifyPropertyChanged: https://github.com/dotnet/csharplang/issues/140#issuecomment-368590469

Starwer commented 6 years ago

@svick: As explained, the proposal is not to add just to add a some cosmetic [Observable] or to make the compiler handle the exact copy of INotifyPropertyChanged. My proposal is to enable any object, regardless whether it is a value-type, an instance, a property, a field, a local variable to be observable. Only the compiler can do that because it has knowledge on the dependency graph.

On the following for example, it is quite obvious, at design time, for the Compiler that the collection is being changed:

[Observable]
string [] friends = new string[] { "John", "Ray", "Steeve" };
friends.NotifyChanged += OnFriendChanged;
friends[1] = "Jane";

Think about the amount of code you need to do that with INotifyPropertyChanged, and how it will (not) be optimized in that case.

Now if you have an call like that:

groupA.NotifyChanged += NeedToBeSaved;
groupA.SubGroup.Person = "Tom";

Pretty obvious for the compiler that groupA content changed... Imagine the gears required in INotifyPropertyChanged and all the LINQ involved.

Think also about applications like for debugging, if you could say: Track any change in groupA (wherever it is happening in the code, whatever sub-child it could be). That's the same idea.

It just strikes me that such a critical feature in VMMV is not natively handled by the compiler. Looking on the amount of post of people puzzled about the INotifyPropertyChanged and its limitations, I don't understand why I get so little support here.

svick commented 6 years ago

@Starwer

On the following for example, it is quite obvious, at design time, for the Compiler that the collection is being changed

Sure, but my question is: how do you handle the non-obvious cases? How exactly does the compiler track the "dependency graph", across multiple methods and even assemblies?

Consider e.g.:

void ChangeAFriend(ref string friend)
{
    friend = "Jane";
}

ChangeAFriend(ref friends[1]);

Or:

departments[0].NotifyChanged += FirstDepartmentChanged;
foreach (var employee in allEmployees)
{
    // how does the compiler know which employees belong to departments[0]?
    employee.Salary *= 1.05m;
}
Logerfo commented 6 years ago

This might help you.

Starwer commented 6 years ago

@svick : Ok, now we start talking... I had the impression, because of your -1 vote that you thought that the feature described was totally pointless. It now seems like you find it difficult to implement, which is already way better...

First of all, let me introduce myself: I spent 2 years full time in developing a C Compiler in Philips (although it was in 2004-6... I'm not getting younger...). So I do know it is a challenging thing. We make it easy for the user, but difficult for the compiler (but that's what a compiler is for ... IMHO). Precisely, I do ask it because it is difficult, and that's precisely the reason why this feature would make a difference on C#. This is where it can truly differentiate from C++ or Java... another way than in syntactical sweeteners. An universal change/event, that only exists in C, Tcl ... debuggers.

For your first example:

void ChangeAFriend(ref string friend)
{
    friend = "Jane";
}

ChangeAFriend(ref friends[1]);

We handle that the same way we handle polymorphism inheriting implicitly from an Observable class. If we know that friends[] is Observable. This class has in a sense an implicit = operator

class ObservableString : String{ <instrumentation of String, overload = > }
var friends = new ObservableString[]();
...
void ChangeAFriend(ref string friend)
{
    friend = "Jane"; // inside the pseudo class, delegate callback is handled
}

ChangeAFriend(ref friends[1]);

But wait ! We can not inherit from string ! Exact ! That's why we need the compiler to handle that, and to relax this restriction behind the scene. All the fancy libraries that generate some INotifyPropertyChanged can't do that.

Same for the second example. As expensive as a INotifyPropertyChanged. At least it is way simpler for the user. And for the above-mentioned obvious case where the compiler can do it better, well this is a gain !

Starwer commented 6 years ago

Note: I have now reached the 4x :-1: and a confused emoji... So what is sure is that my proposal is not popular. What I don't understand is the discrepancies on the comments. Those oscillate between:

I'm willing to take a lesson on this total failure.... but from the contradictory comments I can't really get the take-away message...

theunrepentantgeek commented 6 years ago

As I see it, there are two ways to achieve your proposed idea.

1 Implement the existing INotifyPropertyChanged on every object

This would have a significant negative performance impact on a very large amount of code. While there are many projects that use WPF style databinding where this would be useful, there are even more where INotifyPropertyChanged is never used at all.

Doing this instead by opt-in on specific objects is already easily done, whether by hand coding, aspect-oriented programming (e.g. PostSharp), code generation or other approaches.

So, this is trivial.

2 Enhance the compiler to automagically modify just the right properties when needed

While the data flow analysis for this might (might!) be possible for trivial projects, I suggest its impossible for even moderately complex situations.

It seems you're wanting the compiler to consider every use of a class/property throughout the entire project when deciding how to compile an individual property.

Issues that arise from this ...

FWIW, these examples stem from code I've worked on just this week.

This isn't just complicated, it's impossible.

There are existing proposals that would make correct implementation of INotifyPropertyChanged a trivial matter - I particularly like one that combines code generation, partial classes, and a superceeds keyword. YMMV.

jnm2 commented 6 years ago

@Starwer ❤️ It can be counter-instinctive to see your idea being downvoted without taking it personally, so I very much respect the constructive approach you are taking.

bondsbw commented 6 years ago

Agreed with @jnm2.

FWIW I feel there is merit in language features built around push semantics. Keeping multiple objects in sync when changes are made to any of them is a clunky process, and I welcome proposals that can make this significantly better.

ufcpp commented 6 years ago

There are some generators for this purpose:

https://www.nuget.org/packages/ValueChangedGanerator/ https://www.nuget.org/packages/NotifyPropertyChangedGenerator/

orthoxerox commented 6 years ago

@Starwer something like Kotlin's delegated properties (with an option to delegate all properties of a type) would be a more controllable mechanism for what you're trying to achieve.

Starwer commented 6 years ago

Thanks @theunrepentantgeek . I really value the time and effort (even more than a hypothetical thumb up) you spent to explain your view and show that I going too far into the concept.

I indeed didn't come up with all the solution and the implementation, but wanted to initiate an concept. Maybe that's the problem: I've jumped too quick into the implementation detail. It's surely impossible for the compiler to figure out when an object is accessed. Something more impossible than getting a Garbage Collector working, which I also thought impossible the first time I've heard from that.

So, this is my last shot.

I'd really like to know (please!) if you're all against the very concept of this change, regardless of the implementation itself (the "What/why", before digging on the "how"):

What: (Specifications)

  1. You can bind any object, not only a INotifyPropertyChanged-property;
  2. the [Observable] attribute is inherited to child objects
  3. the child object automatically notify the parent object on change.

Why:

  1. Many people struggle with the INotifyPropertyChanged
  2. implementing the wiring to notify the parents in a complicated object tree is difficult, time-consuming and error-prone... but can be automated.
  3. This simplifies life of quite an important community of C#/VMMV developpers.

How:

  1. I'm ok to get the same overhead as with INotifyPropertyChanged (but remove it where possible)
  2. I'm ok to have a INotifyPropertyChanged-like mechanism behind the scene (but abstracted to the user)
  3. I'm ok if we need to help the compiler sometimes with hints (keywords) to handle this.

I believe there is something between the "trivial" and the "impossible" approach... to be discussed... if you're not already against the "What".

I could think that when somebody came up with the idea of enabling any method to be asynchronous automagically, he/she had the very similar objections:

  1. This is trivial:

    • we already have BackgroundWorker to do that !
    • we already have 3-4 libraries to make it easier
    • we can do a code generator to implement BackgroundWorker behind the scene !
    • This is only interesting to 50% of the users...
  2. This is impossible:

    • How can the compiler know that this sub-sub-method in an extrnal library should be called asynchronously;
    • Too expensive: I don't want to have the state-machine handling asynchronous calls to every method call !

But eventually, they extended the concept with the Task/async/await combo, and for me there's no doubt now that this is a real improvement for the C# language.

I wanted to know if you were open for a similar discussion here.

Starwer commented 6 years ago

BTW, thanks @jnm2 to show some empathy... thanks @bondsbw for your support, but the fact that you didn't thumb up the proposal make me think that you find it conceptually a real bad idea, even though you're not against pushing features around semantics...

bondsbw commented 6 years ago

@Starwer Not the case at all. If I thought it was bad, I would have given it a thumbs down. I just don't feel the proposal is very compelling in its current form.

HaloFour commented 6 years ago

I'm not seeing how much of this can be implemented. The C# compiler doesn't have that level of control over what it compiles. It certainly can't, say, "lift restrictions" around String or replace how it works in order to make it somehow observable. The C# compiler can only work with the surface that the CLR offers. The compiler would also have zero visibility into referencing assemblies, which may change values at will, especially ref values which are just pointers. The compiler has no visibility into what compiled types do.

I do understand that observing object changes is fairly important, particularly in the WinForm/WPF world. I could see enabling the compiler to automatically generate the boilerplate to invoke that event when a class is adorned with something like Observable or NotifyPropertyChanged attributes, either through direct compiler support or through the mythical source generator feature. But that would stop at the adorned class. If you wanted a type of a child property to be similarly observable you'd need to adorn that class. For collection types or arrays you'd be SOL as the compiler can't modify how existing containers work. You'd need to use some existing type that implements INotifyCollectionChanged.

sylveon commented 6 years ago

I'm surprised nobody mentionned Fody.PropertyChanged yet (https://github.com/Fody/PropertyChanged)

It works a lot like @Starwer's original idea:

[AddINotifyPropertyChangedInterface]
public class Person
{
    public string GivenNames { get; set; }
    public string FamilyName { get; set; }

    [DependsOn(nameof(GivenName), nameof(FamilyName))]
    public string FullName => $"{GivenNames} {FamilyName}";
}
Starwer commented 6 years ago

@Logerfo, @ufcpp, @orthoxerox, @sylveon : I pretty much appreciate the effort put into all the libraries and code generators to simplify the use of INotifyPropertyChanged. However, the fact that there are so many different solutions to solve the same problem flourishing on the WEB is for me a sign that there is a hole to fill in the language itself. Also, this proves one thing: if a code-generator can do it, a compiler can also do it.

@HaloFour, @bondsbw, @theunrepentantgeek, @svick: the observations/reluctances you presented challenged me to deepen the concept. A more mature proposal starts to shape in my mind (similar to async/await model in fact). However, I'm afraid I've burnt down all my credibility on that topic. But if you think I should continue and come back we a more thorough plan (but less open for discussion de facto), I'd be glad to spend the required hours and come back with a solid proposal.

HaloFour commented 6 years ago

@Starwer

Also, this proves one thing: if a code-generator can do it, a compiler can also do it.

Part of the mantra behind the source generators proposal is that if it can be accomplished relatively easily through annotations and auto-generated boilerplate source that it should be done that way rather than impacting the language proper. This is to both enable the community to iterate on ideas like this and ship product much faster than waiting on the compiler team to get around to implementing it directly in the compiler. Language changes are incredibly expensive in terms of time and resources and many good ideas have been sitting in a holding pattern since Roslyn first went open-source back in 2014.

That said, source generators themselves are quite expensive and have proven difficult to implement due to the tooling considerations, so it's hard to say whether deferring to them is really that appropriate.

similar to async/await model in fact

I think the comparison to async/await isn't quite appropriate. The motivation there was very different. It wasn't about trying to codify a way to implement background work. As you mentioned there are quite a few ways to accomplish that through various libraries. It was about trying to re-invert the inverted style of code that you must adopt when writing anything asynchronous due to nested callbacks. That could only be solved via a language feature of some kind. There's no amount of boilerplate code that could be possibly written that could fix the problem. You'll also note that async/await is incredibly unopinionated and you can quite literally take any of the existing libraries and get them to work with await by following a pattern that can be achieved through extension methods.

Logerfo commented 6 years ago

@Starwer

if a code-generator can do it, a compiler can also do it.

The discussion here is if the compiler should do it.

svick commented 6 years ago

@Starwer

I think that most people here will agree that the current situation with INotifyPropertyChanged is not ideal. But I think that approaching it from this direction, you have a very steep hill to climb.

For me, for a solution to be considered, it has to satisfy the following points:

  1. It can't significantly affect performance of code that doesn't use this feature. That means that performance of programs that don't use this feature, and even parts of programs that don't use it, has to stay roughly the same, both on the "micro" level (e.g. a loop that reads a property) and on the "macro" level (the overall performance of a program).
  2. It has to work well for all kinds of object graphs. You mentioned "object trees" and "parent objects", but real object graphs are often not trees. What happens if the object graph contains a cycle? What happens if the object graph contains a huge strongly connected component? Notifying all those objects when any one object changes is likely not acceptable.
  3. It has to work on top of the existing CLR and standard library. You mentioned that "the compiler" can create a type that inherits from string. But the C# compiler can't do that. The CLR might be able to do it, but changing the CLR is even harder than changing just the C# compiler, so it's probably not acceptable. (And if you don't understand the relationship between the C# compiler, the CLR and the standard library well, it's probably worth learning about that before you make a proposal.)
Starwer commented 6 years ago

@svick : Thanks for you constructive post.

Even though it seems contradictory to what I proposed, I acknowledge all you wrote. And I ensure you that I perfectly understand the difference between the relationship between the compiler, the CLR, the standard library, a code-generator and so on... The String inheritance was an over-simplification of a potential solution that I have already dropped thanks to our discussions.

@HaloFour : I could only disagree with you on that thing: IMHO the comparison await/async is quite relevant. But on the others things you stated, I agree on every word.

There is indeed a steep hill to climb, and I have the impression I will have to climb it alone... which I just can't. Also, I didn't realize that people seem pretty satisfied about using workarounds code-generators... I think I will have to lower my ambitions on C# future. After all, I don't ask this for me. I've been through the learning curve on INotifyPropertyChanged and can code it without effort now.

I was expecting enthusiasm about this proposal, but got exactly the opposite (the only encouragement I got is when I stated this proposal was a fail). I'll sleep on it, but I think I'll just give up the fight.

Don't get me wrong, despite my disappointment I really appreciated your dedication to drive the discussion in the right direction (to all).

theunrepentantgeek commented 6 years ago

@Starwer Your motivations are in the right place and I'd encourage you to submit further proposals when you have them. Submitting your ideas to the scrutiny of critics is never easy, but it is worthwhile.

I've made a couple of proposals for C# features that I thought were really good ideas - one (#589) was met by a resounding meh and the other by disagreement (#1332).

The threshold for an idea to make it into the language is incredibly high - and that's a good thing because it means that those language features will be genuinely and widely useful.

As to your specific proposal …

I can think of ways (perhaps based on your [Observable] suggestion) for the implementation of INotifyPropertyChanged to be automatically generated by a tool. This might be the compiler, it might be a code generator, maybe by another approach.

However, not all objects need an implementation of INotifyPropertyChanged that behaves in the same ways. Harking back to my own WPF experience, I remember one application where I needed to explicitly not propagate change notifications up a tree of nodes because the performance of the screen suffered tremendously - tabbing out of one particular field would take 10-15 seconds because of the large number of WPF control updates triggered by the property write. I had to (carefully) break the rules to make the application usable.

And here we run into a paradox. If developers don't know how to correctly implement INotifyPropertyChanged in a simple way (because it's done automatically by a tool), they won't be able to correctly implement the complex cases when the automated approach lets them down.

In my opinion, automated approaches are a great idea, as long as they satisfy one of two conditions. Either they solve exactly 100% of the problem, or they provide override/injection/configuration options to allow the edge cases to be correctly handled. The async/await feature does exactly this.

HaloFour commented 6 years ago

This is interesting, apparently the VB.NET team has a bit of interest in this problem:

https://github.com/dotnet/vblang/issues/282

@Starwer

Maybe you're using the wrong language? 😉

orthoxerox commented 6 years ago

@HaloFour that looks very much like Kotlin's delegated properties. Decorated properties?

Starwer commented 6 years ago

The discussion here is if the compiler should do it.

@Logerfo : Thanks for this exemplary truism.

@theunrepentantgeek : Thanks for your encouragements and (again) the effort spent to elaborate on the topic.

@HaloFour: Very interesting link. It indeed seems I'm in the wrong language. That's unfortunate that I dislike VB syntax in the first place. That's purely a matter of personal preference BTW, I don't mean VB is bad or ugly by nature... but I've quit the Basic languages in the 90's (Amiga) and there's no way I'm going back there.

You (all) convinced me that the idea was bad and/or premature. I'll then close this issue as it goes nowhere the way it was started. I'll maybe come back one day with a proper proposal on a built-in Observer Design Pattern, if I'm disappointed about the universal #107 (you triggered a very high expectation there).