Roslyn: IDE0052 incorrectly suggested for entire property when only getter is never used

[jnm2]

Version Used: VS2019 RC.1 SVC1

The code evolved to a place where the getter was no longer used, but because I don't like set-only properties, I left it. Now Roslyn suggests removing the entire property, but this is misleading because the side effects of the setter are semantically quite important:

public class Foo
{
    private IBindingList source;

    public void Bar(object thing)
    {
        Source = thing as IBindingList;
    }

    // IDE0052 Private member 'Foo.Source' can be removed as the value assigned to it is never read.
    private IBindingList Source
    {
        get => source;
        set
        {
            if (source == value) return;

            if (source != null)
                source.ListChanged -= Source_ListChanged;

            source = value;

            if (source != null)
                source.ListChanged += Source_ListChanged;
        }
    }

    private void Source_ListChanged(object sender, ListChangedEventArgs e)
    {
        // Important stuff
    }
}

It should not report IDE0052 on the whole property. If anything, it should be reported on just the getter with a fix to remove the getter or to replace the property with a SetSource(IBindingList) method.

[sharwell]

@mavasani is this a duplicate?

[mavasani]

Diagnostic is indeed valid here as the value assigned to property Source is never read (as you mentioned the getter is unused, which is the exact thing being flagged).

@jnm2 Can't this be written without using a property? It would make the semantics much more explicit and avoid requiring a property setter with side effects and an unused property getter - both of which seem to make the code difficult to understand.

public class Foo
{
    private IBindingList source;

    public void Bar(object thing)
    {
        SetSource(thing as IBindingList);
    }

    private void SetSource(IBindingList value)
    {
            if (source == value) return;

            if (source != null)
                source.ListChanged -= Source_ListChanged;

            source = value;

            if (source != null)
                source.ListChanged += Source_ListChanged;
    }

    private void Source_ListChanged(object sender, ListChangedEventArgs e)
    {
        // Important stuff
    }
}

[mavasani]

Tagged this for design review and tentatively marked as by design.

[jnm2]

@mavasani "Private member 'Foo.Source' can be removed" is simply not a true statement. Removing the property would not be a valid refactoring.

SetSource: yes, like I mentioned:

It should not report IDE0052 on the whole property. If anything, it should be reported on just the getter with a fix to remove the getter or to replace the property with a SetSource(IBindingList) method.

[mavasani]

"Private member 'Foo.Source' can be removed" is simply not a true statement.

This diagnostic is _not_ about flagging which exact symbol (accessor) is removable/unused. It is about flagging _member symbols_, which are assigned a value, such that the assigned value is never read. We can potentially change the diagnostic message for such a case to indicate the code can be refactored to avoid using a property, but I feel it is such a rare case that it is not worth adding complexity to the diagnostic message, especially given:

1. Majority of cases when IDE0052 fires for properties, the property is indeed removable.
2. Analyzer bails out on write-only properties with no getter to account for cases similar to yours where property is just wrapping a mutation operation in a setter for style preference.

[jnm2]

What makes me uncomfortable is the unequivocal wording of the message. Far from wanting to add complexity to the message, I'm thinking the current message is overspecified. I'm used to more precision in the suggestions that are given. However, I'm not going to act blindly either way when I follow up with the message and remove the property, so feel free to close if it's too niche.

[mavasani]

@jnm2 I believe the message is precise and succinct for majority of cases where it fires. I am fine with special casing this scenario (property with unused getter but used setter) to give a different message, if the design discussion leads us to conclude that we need to make the message more crisp for this case. Tentative resolution is just my stance, which can easily be overturned in the design meeting :)

[menees]

I just ran into this case in VS 2019 RC4: "Private member 'FormSaver.Form' can be removed as the value assigned to it is never used." It's telling me to remove the entire private property when only the getter is unused. The code very much depends on the logic in the setter though, so removing the entire property would break things. I'd like to see this case of "unused getter but used setter" handled with a better message. Thanks!

[mavasani]

Given that quite a few people find the current messaging confusing, I am going to special case this scenario and update the diagnostic message.