Provide an analyzer for On....Changed methods
Closed this issue · 16 comments
PropertyChanged supports automatically calling functions of the form On...Changed when the value of the related property changes.
The problem arises when the property name changes.
Since the name of the property is embedded in the method name, rename refactoring does not change it.
It is also marked as unused by various tools because nobody calls it directly.
I propose adding an analyzer that
- Checks if for every On...Changed method checks that a corresponding property exists
- To avoid conflicts with methods with that name but that do not relate to a property add an attribute such as
[UnrelatedToProperty](or the other way around add one if it should match a property name). Whatever seems better. I believe adding it is better.
- To avoid conflicts with methods with that name but that do not relate to a property add an attribute such as
I like that idea. Maybe instead of writing a Roslyn analyzer (not sure if that's what you suggest here), we could just emit a warning from the weaver.
A warning in the weaver is already very nice but I'm not sure if that appears in Visual Studio Error List.
A Roslyn analyzer is better because it can provide quick fixes in the editor and is visible while editing the code. Long before compiling (which might take long).
A warning in the weaver is already very nice but I'm not sure if that appears in Visual Studio Error List.
That will appear in the error list after compilation.
A Roslyn analyzer is better because it can provide quick fixes in the editor and is visible while editing the code. Long before compiling (which might take long).
Yes, a Roslyn analyzer would be able to provide a warning before compilation, but it would be quite a bit of work to implement, while adding a warning to the Fody weaver would be very straightforward. I don't think it's worth maintaining a Roslyn analyzer just for this feature, as we basically would need to duplicate a significant part of the weaver logic in the analyzer assembly.
Also, I don't really see what quick fix the analyzer could suggest, since that would require it to guess the correct property name, so basically the only upside would be to get a warning before you compile the project.
You can try a first version: https://github.com/RedX2501/PropertyChanged.Analyzer
I agree with the part about not being able to provide a fix to it.
When compilation times are long enough it's good to spot early that one typo you made.
The part about the duplicated logic is of course troubling but can't it be factored out and used by both?
The logic is currently very simple so I don't really see it a burden. I can consider keeping it up to date myself and separate from this project.
The warnings should maybe also extend to properties without setters. Those will also lead to not invoked changed handlers.
You can try a first version: https://github.com/RedX2501/PropertyChanged.Analyzer
Nice! That looks pretty simple indeed. 👍
The part about the duplicated logic is of course troubling but can't it be factored out and used by both?
Not really, because there are different APIs on both sides (Roslyn vs Cecil), and I think writing abstractions for this would be overkill.
The logic is currently very simple so I don't really see it a burden.
Yes it's very simple but unfortunately it seems you don't take some stuff into account. For instance you also perform the check on classes which are not processed by the weaver, but maybe that's intentional?
I can consider keeping it up to date myself and separate from this project.
That's not for me to decide, but I think that would be a good solution, as you could opt-in by installing a separate package.
We could also implement warnings in the weaver, and I still like that idea. I could write a PR for this.
The warnings should maybe also extend to properties without setters. Those will also lead to not invoked changed handlers.
There are more problematic cases, like properties with the [DoNotNotify] attribute for instance.
@SimonCropp what do you think about this?
There are many issues and missing details in that code.
I agree a few more checks are still needed but i wanted to have something quick to show. A proof of concept.
Sure, no problem here 👍
I also do not check if the class inherits from INotifyPropertyChanged. Yet I'm not sure if its necessary.
Note that you can also add [AddINotifyPropertyChangedInterface] (or even the now obsolete [ImplementPropertyChanged]) to a class which does not implement INotifyPropertyChanged to make the weaver implement it.
Having them both together means the analyser matches the features provided by the version of propertyChanged. As a separate packet i think i cannot detect the installed version of the nugget package.
I don't think this will be an issue, as the feature set is pretty stable. Even if the analyzer doesn't match the feature set 100%, it will still provide value.
I do think the warnings could be added to the weaver but how do you suppress them when the naming is only coincidental? Another attribute?
Yes, I have some code which adds a [IgnorePropertyChangedWarnings] attribute (I named it like that so it can serve for other stuff in the future). I tried to use [SuppressMessage("PropertyChanged.Fody", "...")] but it's a conditional attribute and therefore does not get compiled into the assembly, so the weaver can't see it.
I'll submit a PR soon.
I submitted #449 so we have a starting point for discussion on emitting warnings from the weaver.
I have updated the Analyzer to cover as many possible things I could think of. You can try it out on the following file:
using System.ComponentModel;
namespace TestProjectForPropertyChangedAnalyzer
{
// a warning is presented here. But I guess forcing suppression in this case is fine
[PropertyChanged.AddINotifyPropertyChangedInterface]
class BaseFromAttribute
{
}
class DerivedFromAttribute : BaseFromAttribute
{
public bool SomeProp { get; set; }
public void OnSomePropChanged()
{
}
}
class Base : INotifyPropertyChanged
{
public event PropertyChangedEventHandler PropertyChanged;
}
class Derived : Base
{
public bool SomeProp { get; set; }
public void OnSomePropChanged()
{
}
}
class DoesNotInherit
{
public bool SomeProp { get; set; }
public void OnSomePropChanged()
{
}
}
partial class PartialClass : INotifyPropertyChanged
{
public bool SomeProp { get; set; }
}
partial class PartialClass : INotifyPropertyChanged
{
public event PropertyChangedEventHandler PropertyChanged;
public void OnSomePropChanged()
{
}
}
[PropertyChanged.AddINotifyPropertyChangedInterface]
class UsesAttribute
{
public bool SomeProp { get; set; }
public void OnSomePropChanged()
{
}
}
[PropertyChanged.AddINotifyPropertyChangedInterface]
class UnsupportedSignatureIsStatic
{
public bool SomeProp { get; set; }
public static void OnSomePropChanged()
{
}
}
[PropertyChanged.AddINotifyPropertyChangedInterface]
class SuppressedNotification
{
[PropertyChanged.DoNotNotify]
public bool SomeProp { get; }
public void OnSomePropChanged()
{
}
}
class UnnecessaryDoNotNotifyAttribute : INotifyPropertyChanged
{
[PropertyChanged.DoNotNotify]
public bool SomeProp { get; }
public event PropertyChangedEventHandler PropertyChanged;
}
[PropertyChanged.AddINotifyPropertyChangedInterface]
class UnnecessaryAddINotifyPropertyChangedInterfaceAttribute
{
public bool SomeProp { get; }
}
class Program : INotifyPropertyChanged
{
public event PropertyChangedEventHandler PropertyChanged;
static void Main(string[] args)
{
}
public bool SomeProp { get; set; }
public bool NoSetterProp { get; }
public void x(){
}
public static void OnUnsupportedSignature()
{
}
public static void OnUnsupportedSignatureChanged()
{
}
public void OnSomePropChanged()
{
}
public void OnNoSetterPropChanged()
{
}
public void OnSomePrepChanged()
{
}
}
}TBH I am not a fan of shipping a Roslyn Analyzer inside the PropertyChanged. I prefer Analyzers to be an optional additional package that people can opt into. @RedX2501 so if you want to ship your Analyzer as a stand alone nuget, we could add doco to this project that points to it
I have created one that is still unlisted to find any problems bugs: https://www.nuget.org/packages/PropertyChanged.Fody.Analyzer/
Any news?
@GF-Huang have you build one?