CS0219 or new warning for unused variable when language version supports discard

[binki]

No warning, but I expect one:

public class C {
    public void M(IThing thing) {
        var needUpdateVariable = thing.Something;
    }
    public interface IThing {
        object Something { get; }
    }
}

The expected CS0219 warning:

public class C {
    public void M() {
        var needUpdateVariable = 0;
    }
}

Now that the discards feature exists, there is no need to treat the first case any differently than the second. However, changing code to be warningful that wasn’t before is breaking. So, there should be a new warning which will report assigned-but-never-used-variables like in the first example which can be rewritten without using a variable like:

public class C {
    public void M(IThing thing) {
        _ = thing.Something;
    }
    public interface IThing {
        object Something { get; }
    }
}

The point of the warning would be to give the developer the opportunity to research whether or not evaluating the RHS of the declaration is actually necessary. For example, using a discard like above makes it obvious that the getter has a side-effect (eww!). Or, as the example for discards shows, you might call a method which returns a Task which you intentionally decided not to await for some reason. Or, the variable declaration may have been entirely superfluous and can be removed. In all of these cases, I would appreciate the compiler’s help with a warning to clean my code of declared-and-assigned-but-never-read variables (beyond what CS0219 gets you).

[Scott-Caldwell]

If you're using an IDE that supports .editorconfig files, you can already accomplish this today with the unused value preference settings.

Reference: https://docs.microsoft.com/visualstudio/ide/editorconfig-language-conventions#unused-value-preferences

[binki]

@Scott-Caldwell No, that does something different. Turning on csharp_style_unused_value_expression_statement_preference forces me to insert a bunch of discards or local variables that I previously didn’t need. It also has no “don’t assign the result to anything” option (at least that I see in the documentation).

What I am trying to do is convert to discards the local variables that were created to

• suppress CS4014
• call the getter on properties which have side-effects
• …

The option you gave me causes the compiler to force me to start using discards for all unused expressions. That means adding discards where neither of the two above issues originally required me to assign the value to something. I.e., turning that on forces me to add code.

I get the purpose of the rule. Such a rule would make sense if one wanted their code to observe/handle the return values of all methods. However, there are a lot of methods whose return value I don’t care about. For example, int.TryParse() when I want to use 0 if the input string is unparsable. Right now, at least, I am not convinced that I would want to be forced to add discards to my code all over the place. I want the compiler to help me remove unread locals (giving me an opportunity to either replace the variable with a discard or remove the entire statement/RHS altogether).

[Scott-Caldwell]

There are actually two different .editorconfig settings in my link, and it seems like you just stopped at the first one.

The second setting (csharp_style_unused_value_assignment_preference) tells the IDE how to behave when you try to assign something to an otherwise unused variable, which, now that you've clarified your intent more, sounds like what you want.

So if you had csharp_style_unused_value_assignment_preference = discard_variable:warning:

int GetInt32OrDefault1(string str)
{
    // Warning: Unnecessary assignment of a value to 'unused'
    // Visual Studio offers a code fix to add a discard
    var unused = int.TryParse(str, out var result);
    return result;
}

int GetInt32OrDefault2(string str)
{
    int.TryParse(str, out var result); // No warning
    return result;
}

[binki]

@Scott-Caldwell

So if you had csharp_style_unused_value_assignment_preference = discard_variable:warning:

int GetInt32OrDefault1(string str)
{
    // Warning: Unnecessary assignment of a value to 'unused'
    // Visual Studio offers a code fix to add a discard
    var unused = int.TryParse(str, out var result);
    return result;
}

This is not quite exactly what I want. I want the codefix to be to remove the variable, not to use a discard. I only want the codefix to be to use a discard if that is necessary to:

• suppress CS4014
• call the getter on properties which have side-effects

int GetInt32OrDefault2(string str)
{
    int.TryParse(str, out var result); // No warning
    return result;
}

But there still is an IDE0058 diagnostic and codefix for adding a discard—unless I replace silent with none. But that is insufficient to address the issue. There’s a missing option for csharp_style_unused_value_expression_statement_preference to remove the discard if it isn’t needed. It should be possible to have the following be a codefix to remove the discard:

int GetInt32OrDefault3(string str) {
  _ = int.TryParse(str, out var result);
  return result;
}

For the following to have a codefix to remove the unused variable (without replacing it with a discard):

int GetInt32OrDefault4(string str) {
  var unused = int.TryParse(str, out var result);
  return result;
}

And at the same time have a codefix to replace the unused variable with a discard in the following (because removing it would result in CS4014):

async Task DoSomethingAsync5() {
  var ignored = StartSomeBackgroundProcessWeKindOfDoNotCareAboutAsync();
}

And also be able to have a codefix replace the unused variable with a discard when invoking a getter with side-effects:

void DoSomething6() {
  var unused = blah.QuestionableProperty;
}

In my understanding, to do this, there would need to be something like these imaginary .editorconfig settings. The thing is, these are imaginary—they don’t exist.

[*.cs]
# Would need a new codefix option “leave_unused” so that code generation does not emit a discard or variable.
csharp_style_unused_value_expression_statement_preference = leave_unused:warning
# Would need a new option which applies only to Task expressions to allow discard to be used when suppressing CS4014
csharp_style_unused_value_expression_task_statement_preference = discard_variable:warning
# Would need a new option which only applies to getter invocations which requires an assignment operator
csharp_style_unused_value_expression_getter_statement_preference = discard_variable:warning
# Would need a new codefix option “remove_lhs_and_operator” or something so that both discard and unused variable can be removed via a codefix
csharp_style_unused_value_assignment_preference = remove_lhs_and_operator:warning

I hope that clarifies what I am trying to accomplish and demonstrates how the existing options are insufficient for my use case.

Thanks for the followup!

[Scott-Caldwell]

Adding some additional configuration options for .editorconfig files to support your code style sounds like a reasonable request. Probably more of a dotnet/roslyn feature, though.

[daiplusplus]

(With apologies for the thread necromancy - I found this thread looking for info on some editorconfig options and it's currently 2:30am and y'know...)

@Scott-Caldwell @binki

int GetInt32OrDefault1(string str)
{
    // Warning: Unnecessary assignment of a value to 'unused'
    // Visual Studio offers a code fix to add a discard
    var unused = int.TryParse(str, out var result);
    return result;
}

int GetInt32OrDefault2(string str)
{
    int.TryParse(str, out var result); // No warning
    return result;
}

int GetInt32OrDefault3(string str) {
  _ = int.TryParse(str, out var result);
  return result;
}

int GetInt32OrDefault4(string str) {
  var unused = int.TryParse(str, out var result);
  return result;
}

I feel all 4 of these are not ideal because if TryParse fails the value of result may be undefined, not default. While in the case of Int32.TryParse specifically the result is indeed documented to be set to 0 when the parse fails, in other cases in other libraries (especially those without documentation) we should avoid assuming result == default.

In my code, I do this:

int GetInt32OrDefault5(string str) {
  return int.TryParse(str, out var result) ? result : default;
}

Or better yet:

int GetInt32OrDefault6(string str, int defaultValue = default) {
  return int.TryParse(str, out var result) ? result : defaultValue;
}

Both of these have the benefit of not triggering any warnings because no value is ignored or discarded, and it's self-documenting: it doesn't require anyone looking at the source of GetInt32OrDefault5 to open the documentation for Int32.TryPrse to find out what value result will have if TryParse returns false.

Unrelated-but-related, I also use this expression style, since C# allows a local variable declaration to be referenced in its own initializer:

int something = int.TryParse(str, out something) ? something : 0;

[333fred]

The results of TryParse are absolutely well-defined:

When this method returns, contains the 32-bit signed integer value equivalent of the number contained in s, if the conversion succeeded, or zero if the conversion failed.

https://docs.microsoft.com/en-us/dotnet/api/system.int32.tryparse?view=netcore-3.1#System_Int32_TryParse_System_String_System_Int32__

Generally, all out parameters must be actually assigned by a method. Unless you use unsafe tricks, you can't skip assigning them with some value.

[HaloFour]

@333fred

Unless you use unsafe tricks, you can't skip assigning them with some value.

Or call a method in a VB.NET assembly. 😉

[daiplusplus]

@333fred Correct, they must be well-defined as far as the CLR/CLI is concerned, but I meant "well-defined" in a documentation/API-design sense: viz: "if MySpecialType.TryParse returns false the out parameter's value is undefined" - so it could be set to 0 or 123. I just don't think it's wise to always assume that every TryParse method in in every library from every author will always set the out value to default(T) in event of failure.

A good example is Array.BinarySearch. (Note that it predates the TryParse pattern). You'd maybe expect that BinarySearch would return -1 if it can't find a value, so you might check if( binarySearchResult == -1 ), but it actually returns the bitwise-complement of the last index in the array it searched at - so the check should be if( binarySearchResult < 0 ) instead - I wonder how many people have been stung by that because of an assumption they made about the nature of the output/return value.

(Another favourite is when an IComparable<T> implementation returns the difference between two values instead of only -1, 0, 1. A neat trick when implementing IComparable<int> is to just do Int32 Compare( Int32 left, Int32 right ) => left - right;).

Also this is yet another example of why C# needs code-contracts to come back.

[HaloFour]

@Jehoel

The compiler isn't assuming that the out parameter is assigned to default, it's assuming that the parameter has been assigned, which is a fairly safe bet. The compiler doesn't recognize that there is a relationship between the out parameters and any other parameter, or the return value.

[daiplusplus]

@HaloFour Yes, but I'm not talking about the compiler, I'm talking about the programmer writing the code making assumptions without having looked at the documentation, that's all.

[333fred]

Also this is yet another example of why C# needs code-contracts to come back.

I don't see how code contracts would help at all with your example of binary search. Also, I don't believe that people will expect that binary search definitely returns -1; after all, you could well be finding where you need to insert into a list next (that's certainly how I use it most often). Nor would it help with CompareTo: what kind of contract could you enforce? The result can be any int, there's nothing else the compiler could say. The only thing that could actually help there is encoding some form of a linear type system into C#, and then actually have exhaustiveness-checking on the results in said type system.

Regardless, this is getting off the original topic on this issue. We could certainly consider more analyzer options for when things need to be discarded or not, but that is an issue for dotnet/roslyn, not for this repo. As such, I'm closing this out.