Roslyn: IDE0059 false positive

[wanton7]

Version Used:
Visual Studio 2019 16.02 and .NET Core 3.0 preview4

Steps to Reproduce:

    class Program
    {
        static void Main(string[] args)
        {
            GlobalImage globalImage = null;
            if (args[0] == "0")
            {
                globalImage = GetGlobalImage(0);
            }
            else
            {
                globalImage = GetGlobalImage(1);
            }

            if (globalImage != null)
            {
                Console.WriteLine(globalImage.ContentLength);
            }
        }

        static GlobalImage GetGlobalImage(int divisionId)
        {
            return null;
        }

        public class GlobalImage
        {
            public int ContentLength { get; set; }
        }
    }

Will produce IDE0059 Value assigned to 'globalImage' is never used

Expected Behavior:
No IDE0059
Actual Behavior:
IDE0059

[canton7]

This is working as expected, I think?

The diagnostic is raised against the line GlobalImage globalImage = null;, which is a redundant assignment because the null is replaced (just writing GlobalImage globalImage; here is fine, because it's definitely assigned below). Indeed, doing the quick fix takes off the = null.

So what IDE0059 wants you to write is:

GlobalImage globalImage;
if (args[0] == "0")
{
    globalImage = GetGlobalImage(0);
}
else
{
    globalImage = GetGlobalImage(1);
}

[wanton7]

@canton7 no it's not working as expected because it's returning IDE0059 "Value assigned to 'globalImage' is never used" that means globalImage variable is never used, but it is used. If it was working as expected it would return something else like "null assigment is unnecessary".

[canton7]

@wanton7 it highlighted the line GlobalImage globalImage = null;, and said "value assigned to 'globalImage' is never used". It didn't say that the globalImage variable is never used, it said that the value assigned on that line (i.e. the null) is never used. And in that, it is correct.

So I do think that the diagnostic is working as the people who wrote it intended. If you were confused by it perhaps there's room for improvement, but that's a different thing.

Are you saying that you would like to change the message from "value assigned to 'globalImage' is never used" to something like "assignment to 'globalImage' is unnecessary"?

[wanton7]

@canton7 You are right! I'm not an English speaker and I read it like it's globalImage variable is never used. Closing this because it's probably working as intended.

[mavasani]

@wanton7 @canton7 Thanks for all the comments in the issue. We have got few more VS feedback issues opened where people got confused with the message and thought it was flagging an unnecessary variable as opposed to unnecessary value assignment. I am going to re-open this issue to track improving the message. Possibly something like "Remove unnecessary assignment to '{0}'" OR "Assignment to '{0}' is unnecessary"

Also tagging our documentation expert @gewarren if she has any suggestions on the message wording here for IDE0059 to avoid this confusion.

[mavasani]

Also note that IDE0059 will also be raised for unnecessary out variable value assignments. For e.g.

void M(int key)
{
   if (TryGetValue(key, out var value)) // Value assigned to 'value' from TryGetValue call is unused.
   {
       value = ...
   }
   else
   {
       value = ...
   }
}

Are the new suggested messages fine for this case as well?

[canton7]

Personally I'd say something like "Value is written to '{0}' which is never read, or is overwritten before being read". Perhaps "or is rewritten" or "is reassigned"

I think I'd try and avoid the word "assignment" in the active sense, since it's a little unclear in the context of out variable assignments ("but I'm not assigning anything to it: I'm just passing it to a method"). Using the passive seems to make it a bit clearer: "is assigned to" or "is written to" (or even the original "value assigned to") leaves open the possibility that it might be the user's code that assigns to the variable, or it might someone else's code (via an out var).

It also seems we need to avoid "never used" - it sounds like the variable is never used, rather than a particular value assigned to it. Talking about the value being read might make this clearer perhaps?

We've also got the two cases that the variable was written and then never touched again; and the variable was written, overwritten, and then used. It might make things clearer to call them out individually.

[gewarren]

Perhaps we should just leave out the reason the value assignment is unnecessary (i.e. that it's unused) and say:

Unnecessary assignment of a value to <variable name>.

[mavasani]

@gewarren - Sounds good, thanks!