"Move declaration near reference" can change behavior of a program for captured variables

[davkean]

1. Use "Move declaration near reference" where indicated:

using System;
using System.Collections.Generic;

class Program
{
    static void Main(string[] args)
    {
        var list = new List<int>();
        list.Add(1);
        list.Add(2);
        list.Add(3);

        var count = 0; <-- "Move declaration near reference"

        list.ForEach(value =>
        {
            count += value;

            Console.WriteLine(count);
        });
    }
}

Results in:

using System;
using System.Collections.Generic;

class Program
{
    static void Main(string[] args)
    {
        var list = new List<int>();
        list.Add(1);
        list.Add(2);
        list.Add(3);
        list.ForEach(value =>
        {

            var count = 0;
            count += value;

            Console.WriteLine(count);
        });
    }
}

Original output:

1
3
6

New output:

1
2
3

Refactorings shouldn't change behavior. While above is fake code - I hit this in a real example that was more convoluted.

[davkean]

Hmm, it does the same thing for loops:

        var list = new List<int>();
        list.Add(1);
        list.Add(2);
        list.Add(3);

        var count = 0;

        for (int i = 0; i < list.Count; i++)
        {
            count += i;
        }

I'm surprised that it would suggest these if they changed behavior.

[CyrusNajmabadi]

Refactorings shouldn't change behavior.

Many refactorings (probably "most") can change behavior. Especially around order of operations. The point of refactorings (as opposed to analyzers/fixers) is to allow you to speed up the operations you want to perform. i.e. you've decided you want to move the code around, so we make it simple to do so. Nothing is in the text buffer itself like a squiggle, or other message, saying "you should do this".

Note: what we sometimes do in these cases is add a warning-annotation to tell you that something interesting is happening that you should be aware of. Moving across function boundaries, or moving into loops sounds like something useful to mention here.

[gafter]

From Wikipedia:

Code refactoring is the process of restructuring existing computer code—changing the factoring—without changing its external behavior.

[CyrusNajmabadi]

We disagree. :) And we're fine not calling them "Refactorings" if necessary. These are just code tools. There's just too many limitations on trying to provide true 'refactorings' that do not change anything.

[Neme12]

We disagree. :) And we're fine not calling them "Refactorings" if necessary. These are just code tools

Maybe there could be a separate category for true refactorings and "code tools" - refactorings could have the screwdriver icon but "code tools" that might break your code would be a sledgehammer 😃

[Neme12]

Nothing is in the text buffer itself like a squiggle, or other message, saying "you should do this".

by your own words there is:

It's not a screwdriver. it's a Popsicle. It's supposed to make you think that there will be something tasty for you if you open the dropdown.

butt seriously: if these are just "code tools" to help you rewrite your code while making no guarantees whatsoever about whether or how they'll change its semantics then maybe it would make sense to not advertise them as much with an icon?

I could definitely imagine having 2 different categories - refactorings would be advertised but "code tools" would not - they would truly only show up on demand.

[Neme12]

Note: what we sometimes do in these cases is add a warning-annotation to tell you that something interesting is happening that you should be aware of. Moving across function boundaries, or moving into loops sounds like something useful to mention here.

I see that's already the case:

[CyrusNajmabadi]

Maybe there could be a separate category for true refactorings and "code tools" - refactorings could have the screwdriver icon but "code tools" that might break your code would be a sledgehammer

in all seriousness, i don't think the split would make sense or be clear to users. And most of our changes would likely not be in the 'true refactoring' category. I think we already have a very usable model here. users have access to these tools, and the tools should be considerate and informative if they may change code (like in the above picture).

This also aligns with the "principles" discussion that happened in this area in hte last meeting:

** Principle 2 **: Warn developers if a refactoring may potentially cause semantic errors.
Notify users if a refactoring on unbroken code may cause semantic errors via the Preview pane and red conflict boxes.

[davkean]

That warning is too subtle - I'd be happier if was more prominent. Up and until this point, I've assumed all refactorings were safe without thought was to whether they change behavior, I was literally surprised by this - hence the bug.

[jmarolf]

all refactorings were safe without thought was to whether they change behavior

This is the thinking of every developer using our tools that I have ever encountered. While refactoring tools shouldn't be limited by absolute correctness (Simply name and other refactorings would never be offered if all semantics needed to be preserved), we still need to balance this with not changing user code unexpectedly. In the case for this bug, I would suggest not offering the fix when lambda captures are involved as it is hard for the developer to reason about what is going to happen.

[CyrusNajmabadi]

That warning is too subtle - I'd be happier if was more prominent.

That's something i would be for. For example, i think it would be good if the entry in the light-bulb menu had a more prominent warning/conflict indicator.

Up and until this point, I've assumed all refactorings were safe without thought was to whether they change behavior, I was literally surprised by this - hence the bug.

Understood. Unfortunately, we discovered a long time ago that if you tried to limit these tools to the situations you could prove could not change behavior, then many of them could not be offered most of the time. For example, it is practically impossible to ever move code around (except in the most trivial of cases) without the possibility of semantic changes.

To prevent hugely limiting the applicability of the tools, we went with an approach where we offer, but let you know (albeit in a way that might not be clear enough) that there is an issue. I would still like us to offer these things (nearly all of the time they are still useful), but make it clearer there is something ot consider on the part of the developer.

[miloush]

I just got it here in 16.4:

lock (BufferLock)
{
    var temp = BackBuffer;
    BackBuffer = FrontBuffer;
    FrontBuffer = temp;
}

offering to swap the first two lines. I also get inline temporary variable which breaks it as much. I discovered it because there was underlining on var in order to expand it.

I have to say I was surprised. I would also typically treat the IDE suggestions blindly as "you can actually do this smarter way" as is the case with removing unnecessary parenthesis, casting, etc. It's only that the change was obviously wrong in the preview that I didn't do it, so I guess that worked, although arguably this case was trivial to see. I do agree that if similar offerings are to be kept, at least the warnings should be a bit more prominent. The fact that refactoring can break the code behavior yet keep valid syntax is a new thing to keep in mind for me.

I would argue the simplify name case is a bit different because if that causes behavior change then it would be a surprise even if you did it manually. Do you have other examples?

[CyrusNajmabadi]

Can you clarify what you mean by:

offering to swap the first two lines.

We should never specifically be suggesting that you swap these lines. Instead, we have speedup tools that are for the person that says: i'd like to move this quickly, but i don't want ot have to cut the line, then move it, and then fixup formatting and the like.

I would also typically treat the IDE suggestions

This is specifically not a suggestion. We don't put in any squiggles in the code, or even ...s. It's just a editing speedup tool (one i personally use multiple times a day while editing code).

The idea of these helpers is that you can depend on them being there, not that thye're suggestions that you actually make these changes. That's why they're not pushed into your face, but are instead only offered once you say "i want to do something specific at this position".

[CyrusNajmabadi]

I do agree that if similar offerings are to be kept, at least the warnings should be a bit more prominent. The fact that refactoring can break the code behavior yet keep valid syntax is a new thing to keep in mind for me.

This is the case for, i believe, nearly 100% of our refactorings. Great examples:

1. any time you inline code, you'll likely be able to change semantics since you will be chaning what code executes. i.e. what was executed after the variable previously can execute before.
2. many pattern-based fixes assume certain behaviors of code that can't be proven. for example: a.b == null ? null : a.b.c gets you an offer to convert to a.b?.c. This can def break semantics depending on if a.b has side effects.

etc. etc.

I've done the walkthrough of nearly all of our refactorings and I believe i found that most of them (usually all but the most trivial) can break semantics. The trivial things that don't are just the "make explicit something that is implicit" (i.e. adding a modifier), or things that don't even change the token stream (like adding comments).

The moment anything moves or is restructured, then you can basically always find cases where exact order of the instructions is important and any adjustment to things breaks it.

--

Finally, as mentioned already, we tried that approach many years back and basically got feedback across the board from people going:

1. "these tools are useless. they never show up and i basically can't ever expect to be able to use them".
2. "well, in these cases we couldn't prove it was safe, so we didn't offer"
3. "it's both safe in my code, and i would accept it being not safe since i just want these thins to help speed me up".

etc. etc.

[miloush]

I still think the examples, while technically valid, are different in how likely or obvious to the developer the potential breaking is, but I agree that's a hard line to draw.

This is specifically not a suggestion.

I think this is the problem here. Once you open the dropdown, there is no way to distinguish what individual entries are. How about if we put some icons there?

[canpolat]

When I encountered this specific behavior, I looked for a place to report a bug and Visual Studio directed me towards Developer Community instead of Github. So I reported a "duplicate" without being aware of this issue.

Many refactorings (probably "most") can change behavior.

That may be the case, is it that difficult to detect a behavior change for this specific refactoring? Since the scope change is already detected, I would expect the tool to be conservative (and move it only within the same scope) instead of introducing a bug (which was the case for me). I hope you will reconsider how liberal this refactoring should be when changing behavior.

[Neme12]

I still think the examples, while technically valid, are different in how likely or obvious to the developer the potential breaking is

I agree. In the case of something like a.b == null ? null : a.b.c to a.b?.c, it's really unlikely to break someone since the vast majority of property getters don't have side effects. But the change in this issue? This will break everyone.

[CyrusNajmabadi]

@Neme12 Then i would advise the feature attach the warning annotation to convey this information.

But the change in this issue? This will break everyone.

This assumes that the point of using this feature is to not change code meaning. That's not been the goal or aim of many of these features (and we've explicitly gotten requests to do the opposite). Namely: "let me still use the features, because i'm intentionally changing things up and i want the speedup of having the feature do the tranformation for me, instead of making me do it".

In these ways, the features are typing accelerators, not program-meaning-preserving transformers. The model we have in that case is still allow the user to use the feature, but have it notify that there's a potential change in meaning. You can have this in the preview, or in the title text, etc.

[CyrusNajmabadi]

That may be the case, is it that difficult to detect a behavior change for this specific refactoring?

It looks like the refactoring does detect this, and even presents that information.

We would likely be open to further expansion of this. For example, updating the title (to include Move ... (may change semantics)). We already have precedent for that in other features.