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

Omit unnecessary initial assignments #47

Open
amanda-mitchell opened this issue Feb 15, 2019 · 0 comments
Open

Omit unnecessary initial assignments #47

amanda-mitchell opened this issue Feb 15, 2019 · 0 comments
Labels
new analyzer Suggestion for a new analyzer to add

Comments

@amanda-mitchell
Copy link
Contributor

A pattern that I sometimes see in C# is:

var foo = default(string);
if (condition1)
  foo = bar;
else if (condition2)
  foo = baz;
else
  foo = foobar;

In cases like this, the initial assignment of foo is unnecessary and arguably harmful.

If a variable is left unassigned, C# will analyze possible code paths and issue errors for any instances in which the variable is accessed before being definitely assigned.

Assigning an initial value circumvents this check and obscures developer intent. (Aside: I suspect that this practice is inspired by languages such as C++, in which a declaration without an assignment results in undefined behavior)

The proposed analyzer would flag the above snippet as an error. Similarly, any case in which a variable has two assignments without any possible accesses between them should be flagged as an error. (I'm uncertain of the practicality of searching for the latter case—this one may need to be punted)

Another common pattern is to have an initial assignment followed immediately by a check that sets the variable to a new value:

var foo = default(string);
if (condition)
  foo = overriddenValue;

These would be better written as

var foo = condition ? overriddenValue : default(string);

or

string foo;
if (condition)
  foo = overriddenValue;
else
  foo = default(string);

Although this pattern is sufficiently common that it may be more appropriate to warn instead of error for these.

Code fix nuances

For the first pattern, the code fix provider should offer to remove the initial assignment; this will need to be able to transform a var into an explicitly declared type.

There may be some edge cases related to ref and out parameters, especially when considering the pairs of assignments without intermediate access and early return statements.

Some initial assignments may have side effects (e.g. an assignment to the result of a method call). Code fix providers should not remove the method call when removing the additional assignment. Similarly, when assignments may have side effects, code fix providers should take care not to change the order in which statements are executed.

It would be better to provide no fix than to provide a fix that might change the behavior of the method.

@amanda-mitchell amanda-mitchell added the new analyzer Suggestion for a new analyzer to add label Feb 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new analyzer Suggestion for a new analyzer to add
Development

No branches or pull requests

1 participant