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

Suppress CS0067 here #175

Open
JohanLarsson opened this issue Oct 9, 2020 · 7 comments
Open

Suppress CS0067 here #175

JohanLarsson opened this issue Oct 9, 2020 · 7 comments

Comments

@JohanLarsson
Copy link
Collaborator

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
Copy link
Collaborator

jnm2 commented Oct 9, 2020

@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
Copy link
Collaborator Author

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
Copy link
Collaborator

jnm2 commented Oct 9, 2020

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
Copy link
Collaborator Author

JohanLarsson commented Oct 9, 2020

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
Copy link
Collaborator Author

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

@jnm2
Copy link
Collaborator

jnm2 commented Oct 9, 2020

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants