Roslyn: Incorrect "use var" suggestion in a foreach loop

[jasonmalinowski]

Version Used: 2.9.0-beta4-62911-02. Commit Hash: db5486576d92f24a62cf86406753d259ff148d83

Steps to Reproduce:

1. Create a new console app.
2. Add the following to an .editorconfig:

[*.cs]

csharp_style_var_for_built_in_types = true:suggestion
csharp_style_var_when_type_is_apparent = true:suggestion
csharp_style_var_elsewhere = true:suggestion

1. Write this in the console app's Main method:

            foreach (string arg in args)
            {

            }

1. Trigger light bulb.

Expected Behavior: nothing is wrong here.

Actual Behavior:

[jasonmalinowski]

FYI to @jinujoseph -- discovered while dogfooding. This might be a "low impact, high visibility" bug...

[Pilchie]

Just hit this in project-system - we have IDE0001 set to be an error, so it's very visible for us.

[sharwell]

We should definitely figure out how this was introduced.

[CyrusNajmabadi]

I made a bunch of changes to UseImplicit/ExplicitType to unify codepaths and to make it part of the simplifier pass. I could have def unintentionally broken this as specialized 'foreach' code was absolutely impacted by the change. Potentially relevant PRs here:

https://github.com/dotnet/roslyn/pull/25865
https://github.com/dotnet/roslyn/pull/26288

[CyrusNajmabadi]

This is highly likely to be the buggy code in question:

https://github.com/dotnet/roslyn/blob/3fe9e5e3e2990fb5b3a44701ab7c444c72e063e6/src/Workspaces/CSharp/Portable/Utilities/CSharpUseImplicitTypeHelper.cs#L153-L160

Note the lack of checking which child of the 'foreach' the typename is...

Oddly enough, i didn't change this code (afaik). But this likely means some other codepath was likely blocking things from making it here.

[CyrusNajmabadi]

I think i found it. Looks like there was one more check for this in a different location:

https://github.com/dotnet/roslyn/pull/25865/files#diff-a419a7e5e40e4a95b7469c2a47229a56L2332

I didn't realize that that was important for actually blocking the code above from thinking the change should be allowed.

[jasonmalinowski]

@CyrusNajmabadi Are you sending a PR or should one of us take it from here?

[CyrusNajmabadi]

@jasonmalinowski I would not be able to get to this until tomorrow (and then i'm on a flight that day too). So in this case i think it would be better if someone else took this given the severity. My posts were intended to at least help narrow down both how this happened, and where the likely fix would need to go for it.

[CyrusNajmabadi]

so, from now on, should I block all this kind of subsystem changes regardless how beneficial it is if it is not directly fixing bug until it is proven to be not breaking anything at all?

In general, the primary issue for me is hte subtlety of the impact on the user. For example, things that affect muscle memory are pretty difficult issues because not only do you have to try to replicate hte original behavior, but you also not have two behaviors that could be considered 'correct' depending on the user and which version they became most accustomed to.

Such a situatoin does not exist here. This is a not a muscle-memory issue. This is a pure: "this is a bug" issue, with a very clear 'right' behavior and 'wrong' behavior. Fixing the issue would be appropriate, without necessarily restricting ourselves too much toward improvements with clear benefits. For example, in this case, the clear benefit was providing a simpler system so that our features would properly respect user settings. IMO that was worth it. Though it's a pity this issue crept in.

[heejaechang]

but one can argue I have muscle memory to hit ctrl+. and apply right away since I learned to do it for explicit<->var LB without actually looking at preview. in that sense, I broke the muscle memory.

[heejaechang]

I have same thing for how we changed BatchFixer to SyntaxEditor based fix all fixer. since that is quite significant change, and we didn't do extensive check whether that changed any behavior on fix all for that fixer.

[CyrusNajmabadi]

Such a situatoin does not exist here. This is a not a muscle-memory issue. This is a pure: "this is a bug" issue, with a very clear 'right' behavior and 'wrong' behavior.

Muscle memory issues are ones where the behavior has changed from a form that was previously thought to be correct to a form that is also thought to be correct, but which now behaves differently from the user's expectations. These are particularly difficult to deal with because both before and after may be considered 'correct', but people are thrown off because of hte patterns and practices they've become accustomed to. As mentioned above, they're also hard to fix precisely because you may now have a bifurcated user base where people who have been on a previous version of the product for many years becomes used to that style, and people on the current version then become used to the new style. So even trying to reconcile is likely to still impact some set of users.

That's not the situation here. This is a pure regression in functionality unrelated to muscle memory. That someone invokes the feature using their muscles does not a muscle-memory problem make it :)

We should fix this just like we would fix any sort of clear and obvious regressions in any feature area. We should prioritize it based on the severity for users. And we absolutely should discuss and consider how we can do better in the future in the change+review to try to prevent these regressions from happening. I would welcome ideas and brainstorming on how we could try to better defend against these sorts of things.

[CyrusNajmabadi]

As an example, i think it would be a good idea for people (including myself) to be extra vigilent about code that considers nodes in the context of their parents. People writing hte PR as well as reviewers should likely focus some extra energy on asking questions like "are we sure we're operating on the right not? are we sure this is hte only parent this could have? what happens if tree changes come in the future that might violated some implicit assumptions this seems to have? Are we sure we're going to eb finding the right node? What about if the tree has this sort of error in it?" etc. etc.

In general we do try to do this with PRs, but it's clear things can slip through. Extra attention may be valuable considering how tightly tied our features are to the tree, and how 'interesting' the tree can be in practice.

, and we didn't do extensive check whether that changed any behavior on fix all for that fixer.

This is actually one of the reasons i made this change: https://github.com/dotnet/roslyn/pull/26826

During some of the fix-all work i discovered the strangeness of the fix-all tests not really using the fix-all infrastructure properly. it meant tests ended up supplying information which could be bogus, and fix-all tests could pass when they might actually not succeed in practice. This was an effort to actually raise confidence in fix-all tests and to feel more certain that they were actually validating things properly and that their results coudl be better used to have confidence that changes were not breaking things. I'm definitely a fan of putting more assurances in place to prevent regressions, which is why i often ask for (and am happy to add) additional tests to give that confidence.

[CyrusNajmabadi]

If you'd like, i'd be happy to brainstorm more on this on gitter if that would be useful!

[sharwell]

Git bisect found the cause in commit 06393e4e4a9f23150aa4ec984b97f0af4e95bd94, which narrows it down to #25865.

[CyrusNajmabadi]

I felt bad that i introduced. This. Have made a fix. Sorry about this guys.

[akashkc]

@CyrusNajmabadi : What do you mean by muscle-memory issue in this context? I've never heard about this term earlier.

[CyrusNajmabadi]

Muscle-memory isn't really a factor in this particular bug. So it's a bit of a non-sequitor right here.

In general though, it refers to how people tend to remember behaviors in an unconscious manner which they they repeat without thinking. In the context of Roslyn and the IDE, we talk about it a lot in terms of how people get used to typing. For example, without thinking i'll often do something like List<int>ctrl-space,enter,list=new,space,tab knowing i'll end up with the right set of code despite not necessarily thinking about each and every step and what it is doing. People get very used to these patterns and if you accidently make them do things that are subtly different, people will end up with different code than what all that 'muscle memory' led them to expect.

in general we try to avoid unnecessarily doing things that would impact the user in this way.

[akashkc]

Awesome. Now, it makes sense to me about muscle memory and it's beauty of VS encouraging muscle memory day by day. Thanks for great explanation.