INPC001 should not trigger for properties decorated with [Bindable(false)]

[Insire]

The analyzer should respect BindableAttribute

[JohanLarsson]

Derp, forgot that one, thanks for reporting.

[Insire]

When fixing this, could you also respect [Bindable(false)] on the class lvl? That way i can remove all the attributes on the properties. It would also be more clear

[jnm2]

When fixing this, could you also respect [Bindable(false)] on the class lvl?

It seems like you're treating [Bindable(false)] as a suppression. A #pragma warning disable for the file is more clear and fits with the way other suppressions work.

[JohanLarsson]

It still makes sense for the analyzer to pick up on this attribute.
But I agree that #pragma is preferable if the only goal is to suppress.

[Insire]

I wanna make it obvious how the class is supposed to be used and in my opinion the BindableAttribute does just that.

I view disabling warnings as a last resort effort to get a desired result.

[jnm2]

Part of my hesitation is that Browsable is an explicitly design-time-only attribute. This seems to back up my intuition that [Browsable(false)] means no more, no less than "hide this property from the designer":

❌ Caution
You can use this attribute at design time only. Nothing prevents you from binding to any property during run time.

https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.bindableattribute#remarks

I would expect non-browsable mutable properties to raise PropertyChanged in some cases just as likely as not.

[Insire]

Part of my hesitation is that Browsable is an explicitly design-time-only attribute.

The analyzer is a design time only functionality aswell. So anything designtime only is as valid as anything else.

A little bit above that warning it says:

If Bindable is No, you can still bind to the property, but it should not be shown in the default set of properties to bind to, because it might or might not raise a property change notification.

this part in particular captures my opinion quite well:

because it might or might not raise a property change notification.

I would expect non-browsable mutable properties to raise PropertyChanged in some cases just as likely as not.

Yea, our expectations diverge here.

[jnm2]

Sounds like my expectation is almost word-for-word the docs quote there 😁 which means, analyzer shouldn't warn because either way is valid.

I don't think the designer or the docs address the awkward fact that it has AttributeTargets.All, but it seems a little out there to apply it to the class as a suppression unless there are other existing tools that follow the same logic. The analyzer should match somewhat to the real world.