Roslyn: Rule to check for variable shadowing

[felixhirt]

I have wondered if there is a rule to check for variable shadowing already (e.g. generate a warning for a local variable in a function which has the same name as a class member)
If there is no such rule, what is the reasoning? Is it just that no one made one yet, or is there a reason against it (e.g. because member access should be identified by "this.")?

[Evangelink]

@mavasani I think the rule is a good idea, let me know if you agree too.

[mavasani]

Seems like a good fit for an IDE style/naming rule. I also thought there was a similar rule proposal in Roslyn, though I cannot find it right now. Porting this to Roslyn.

[sharwell]

There are two ways that people tend to address this in C#:

1. Name fields with an underscore prefix and locals with camelCase
2. Name fields with camelCase, and use this.fieldName _always_ when accessing fields

Both of these approaches are supported by analyzers that are already available. I'm not sure a new analyzer would help for the following reasons:

1. It adds a third style option where two great options are already supported and in widespread usage
2. The new style doesn't seem to add significant value over the existing options

My preference would be suggesting that this be implemented as a standalone analyzer rather than implementing it as a built-in analyzer. If we do implement it as a built-in analyzer, we need to make sure it doesn't conflict with existing behavior for code style (2) above, and doesn't interfere with intentional aliasing of names as part of lambdas.

[felixhirt]

I would like to mention that i did not think of this as a naming rule or something. I do not usually have shadowing problems (my properties, fields and arguments/local variables have a different naming scheme (in fact, exactly the number 1. mentioned by @sharwell).

I stumbled across this when we made some changes in some ancient class which was written long before such naming rules existed, which had camelCase fields, which was something the colleague who made the modifications did not realize.

I just double checked, and unfortunately we both don't remember anymore what exactly happened (i should have made a better initial issue description, sorry). If we figure out again what we did, i will post an update.

The meaning of this issue was really just to show a warning if you are shadowing a class member somehow. To me, that's one of those language features i do not actually want to use because it is way to brittle. Also, a warning would have helped to find the bug faster :-).

I'm aware that adding a warning about using a legitimate language feature can be considered a breaking change, which is why i expected that this would become an optional FxCopAnalyzer at most (and i thought it might potentially already be an optional analyzer that i missed)

I hope this helps to clear things up

[jmarolf]

Design Meeting Conclusion

• We feel that the existing naming style enforcement allows users to resolve this issue
• @felixhirt if you have a specific repro that you can give that would help us understand how existing mechanisms are insufficient.

[felixhirt]

I agree that the naming rules (if they are applied) should suffice to prevent issues like this. If i ever encounter a case where this is not accurate i will post it here (don't think i will though)

[felixhirt]

We just had an idea what the original issue might have been: The winforms designer uses private camelCase fields to identify components. We are pretty sure this was an old winforms control with lots of code behind etc. where we ended up with a name conflict. Still not that big of an issue (for us) since we don't use winforms for new things