DotNetAnalyzers / PropertyChangedAnalyzers

Roslyn analyzers for INotifyPropertyChanged
MIT License
44 stars 4 forks source link

Suppress CS0067 here #175

Open JohanLarsson opened 4 years ago

JohanLarsson commented 4 years ago
public sealed class C : INotifyPropertyChanged
{
                                             ↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓
    public event PropertyChangedEventHandler PropertyChanged;
}

To avoid:

#pragma warning disable CS0067 // The event is never used
        public event PropertyChangedEventHandler PropertyChanged;
#pragma warning restore CS0067

There are memory leaks when binding to properties on members that do not implement INotifyPropertyChanged maybe it was fixed but no huge harm in implementing the interface and legacy.

jnm2 commented 4 years ago

@JohanLarsson IIRC that warning is legit and it goes away if you either have code that invokes the property or you write the event so that it has no backing field.

JohanLarsson commented 4 years ago

if you either have code that invokes the property or you write the event so that it has no backing field.

Not sure how you mean

jnm2 commented 4 years ago

I think the warning disappears if you have PropertyChanged?.Invoke() somewhere in the code. Or anything that reads the PropertyChanged field. Or alternatively, if nothing ever reads the field and this is by design, the warning disappears if you declare the event as:

public event PropertyChangedEventHandler PropertyChanged { add { } remove { } }

"The event is never used" is just the event version of "the field is never read." (Assuming I'm not forgetting how it works.)

JohanLarsson commented 4 years ago

Ok, gotcha. I get why the warning makes sense.

IIRC there used to be memory leak issues when binding to properties on types that don't implement INPC and this is why I'm thinking about suppressing the warning for sealed classes.

Fo nonsealed classes we don't want to suppress as in that case we should add an invocator:

protected void OnPropertyChanged([CallerMemberName] string? propertyName = null)
{
    this.PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
}
JohanLarsson commented 4 years ago

I don't think suppressing will be that bad as we have plenty of nags if forgetting to notify.

jnm2 commented 4 years ago

If the goal is to implement a no-op INotifyPropertyChanged, I'd personally prefer this over public member + an unused backing field + suppression pragmas:

event PropertyChangedEventHandler INotifyPropertyChanged.PropertyChanged { add { } remove { } }
JohanLarsson commented 3 years ago
event PropertyChangedEventHandler INotifyPropertyChanged.PropertyChanged { add { } remove { } }

Is problematic if we later add a mutable property. Then the fix has to rewrite the event and I would hesitate to do it.