Not as easy as it looks

Not as easy as it looks

Rate This
  • Comments 41

My colleague Kevin works on (among many other things) the refactoring engine in the C# IDE. He and I were at the end of last year discussing the possible cases for a hypothetical "eliminate variable" refactoring. I thought that it might be of interest to you guys to get a glimpse of the sorts of issues that the IDE team faces on a daily basis.

First off, what is the "eliminate variable" refactoring? It is seemingly straightforward, but as we'll see, appearances can be deceiving. Suppose you have a local variable that is initialized in its declaration, never written to otherwise, and you have only one place where that variable is used. (A generalization of this refactoring is to replace every usage; for simplicity, let's consider only the single usage case.) For example:

void M()
{
    int x = 2 + R();
    Q();
    N(x);
}

You can eliminate the variable by refactoring the code to:

void M()
{
    Q();
    N(2 + R());
}

Now, obviously this changes the meaning of the code. If Q() throws an exception then now R() is never called, whereas before it was always called. If Q() observed a side effect of R() in the first version, it does not after the refactoring. And so on; there are numerous ways in which this code is different. We assume that the user is all right with these changes; that's why they're using the refactoring.

How, at a high level, would you implement this? It seems straightforward:

  • Find the text of the initializer
  • Find the single usage (which the IDE already knows how to do somehow)
  • Textually replace the single usage with the initializer text
  • Erase the declaration and initialization of the variable

That last step might be slightly tricky if you have something like

    int y, x = 2 + R();

because we have to make sure to erase the comma and the declaration and initialization of x, but not erase the "int y;". But that is not hard to deal with.

You also have to think a bit about comments. What should happen here?

    // x is blah blah blah
    int x = 2 /* blahblah */ + R(); // Blah!
    Q();
    N(x);

Should that become

    // x is blah blah blah
    /* blahblah */  // Blah!
    Q();
    N(2 + R());

Or should any of the three comments before, inside or after the declaration be inserted where x used to be used? Notice that if the one that comes after is inserted, it's going to have to be changed to an inline comment, or a newline is going to have to be inserted in the inside of the call. Also, a comment refers to a local variable that has been eliminated, which seems bogus.

Assume that we can solve these problems. Are we done? Let's apply our refactoring to a few test cases. In every case, we eliminate "x".

    const short s = 123;
    int x = s;
    N(x);

becomes

    const short s = 123;
    N(s);

Is that right? Not necessarily. What if N has two overloads, one for int and one for short? Before we called N(int); now we are calling N(short)! This seems qualitatively different than merely reordering a possible side effect; now our refactoring is changing overload resolution, which seems wrong. Really we should be generating

    const short s = 123;
    N((int)s);

OK, is that the right refactoring? Let's keep on trying test cases.

    int x = 123;
    N(ref x);

becomes... what? We can't say N(ref 123) or even N(ref (int)123).

In this case, the refactoring engine has to fail. There is no way to eliminate the variable when what you're passing is a reference to the variable itself.

How about this?

    int x = R() + S();
    N(Q() * x);

becomes

    N(Q() * R() + S());

Argh, we have now introduced a semantic change because of operator precedence. That should be

    N(Q() * (R() + S()));

How about this?

    Func<int, int> x = z=>z+1;
    object n = x;

becomes

    object n = z=>z+1;

Nope; you can't implicitly convert a lambda to anything other than a delegate or expression type. That's got to be

    object n = (Func<int, int>)(z=>z+1);

Whew. What have we learned so far? To eliminate T x = expr, there are scenarios where you can use:

  • nothing at all; give an error
  • expr
  • (expr)
  • (T)expr
  • (T)(expr)

An exercise for the reader: find scenarios in which none of the above are good enough either. Can you find a scenario in which you have to say ((T)expr)?  How about ((T)(expr))? The latter was the worst I could find easily; are there any more that I missed?

Understandably, we would not want to implement this refactoring in a hypothetical future version of Visual Studio if every time you did an elimination of a variable, the compiler conservatively inserted four parens plus a cast; what we really need are heuristics which can tell us when each of the above forms is necessary. That is a surprisingly tricky problem in language analysis!

And these are just the issues that Kevin and I came up with in a few minutes of whiteboard doodling; it is entirely possible that on deeper analysis, we'll find more interesting problems.

Next time you use the refactoring tools in Visual Studio, think about everything that is happening behind the scenes to make those tools work seamlessly. Like playing the piano, the unseen effort required to make it look easy is enormous.

  • Has Kevin seen ReSharper's equivalent of "eliminate variable"?

  • All of which makes me even more impressed by tools like ReSharper that have dozens of refactorings like this. I'm always a little bit surprised when I run across another one of these edge cases and they get it right!

  • How about:

    class C
    {
       int y = 2;
       void M()
       {
           int x = y;
           int y = 4; // shadows field y
           Foo(x);
       }
    }

    Then, you need to change Foo(x) to Foo(C.x)

    Interesting idea, but the analysis is not quite right. Remember, y is in scope throughout the block, not just after the declaration, so this is a "variable used before declaration" error; the first 'y' refers to the local, not the field. You're thinking of C++. (Also, don't forget that C# makes it illegal for the same simple name to have two meanings in one block.) - Eric

  • How about

    static void Main()
    {
      Funny a = null;
      Func<int> b = () => 1;
      Console.WriteLine(a + b);
      Console.WriteLine(a + ((Func<int>)(() => 1)));
    }
    class Funny {
      public static int operator +(Funny a, Func<int> b) { return 1 + b(); }
    }

    Yep, that's the same case that I came up with. Great minds think alike! - Eric

  • What about a generalized refactoring function to insert* or remove unnecessary parentheses and casts in a selected expression? This is something that could be useful on its own, and it could simply hand off to that after it's made its own version with ((T)(expr)) - though this could result in the undesirable effect of altering the text of expr.

    Are there any situations where this could result in needing to cast to an anonymous type?

    * yes, insert.

  • Seems to me that you'd start with ((T)(expr)) and then look to see which parts of it were redundant, and eliminate them.

    Having "eliminate redundant parentheses" and "eliminate redundant casts" as primitive operations within the refactoring engine seems like something that would be needed, or at least useful, for all kinds of refactoring operations (and possibly it'd be useful to expose them directly to end users, too).

  • Sometimes you need to rename variables, e.g. inline "f" here:

           var f = (Func<int, int>) (x => x);

           using (null)

           {

             var x = "sdfsdf";

             var y = f(1);

           }

  • @Khoth you code has error, because you cannot use "y" in M() before it is declared.

  • Another example, inline "numbers":

         int[] numbers = {1,2,3};

         Action<int[]> a = ints =>{};

         a(numbers);

  • @Knoth giving it another thought, we can change your sample as follows, inline "x" here:

    class C
    {
     private int y = 2;
     private void M()
     {
       var x = (Func<int>) (() => y); 
       {
         int y = 4; // shadows field y
         int p = x();
       }
     }
    }

    It should change y in lamda to "this.y"

    Nice. Because the two simple names 'y' are not first used in overlapping declaration spaces, they are permitted to have different meanings. The refactoring causes the declaration spaces to overlap.

  • var x = 123;

    var y = new {x};

  • The problem description seems strange - are refactorings even thought of in text replacement terms?

    It seems natural to me that since refactorings can only operate over a connected graph of recovered structure from the text (it has to be able to parse largely everything from the start to the end braces of a method body, for the posited refactoring, for example), refactorings proper should not be rewriting source *text* at all. It (again, to me) seems more natural that a refactoring engine would be generating AST transforms which it passes to a source code rewriter. In this case, the initial parsing would decide how comments attach to source and validation passes would catch missing parens and invalid comment positioning in the AST - all before the rewriter applies the changes.

    The AST is largely already there for intellesense purporses; a lot of the logic you have to do the way you are describing is pretty much the same, just in different places; and it generally seems to be it's aligned better with a lot of the directions Visual Studio seems to be taking (extensibility of the editor and the compiler teams compiler-as-a-service work, for example).

    Is this a "that's what we are doing", a "yeah that's nice, but bad cost/benefit" or a "that doesn't work"?

    The relationships amongst the "top-level-type tree", the fully-analyzed "expression" tree, the syntax tree, the token stream and the hunk of characters managed by the editor are complex, to say the least. In practice, many refactorings are done by manipulating the syntax tree as you describe, re-running the analysis pass to make sure the expressions still are correct, and then turning the syntax tree changes into a set of text diffs that can be applied to the editor. This can be an expensive and difficult process with many different levels interacting with each other. I'm glossing over most of those details here. -- Eric

  • I tend to have the opposite problem of the first one stated. I find myself with

    void M()

    {

    Q();

    N(2 + R());

    }

    and I'll want to pull out 2+R(). Since I'm assigning it to a variable, I'd like it if the Visual Studio IDE atuomatically put the variable I just created as the parameter in the call to N(). Sure, that would create more work if I was trying to change the meaning of the code and wanted to call a parameterless N(), but I can't never recall a time where I wanted that, and I cut and pasted the old parameter. If I cut and paste a parameter into an assignment of a variable, that should give the IDE enough of a hint to put the variable in the spot of the parameter for me.

  • I don't know whether you'd count this, but we may need to include "checked" or "unchecked". For example, take:

       unchecked
       {
           int z = X() + Y();
           checked
           {
               M(z + 1);
           }
           // Other code (just for simplicity so we can't completely remove the unchecked block)
       }

    If we want to extract z, I think we'd need:

       unchecked
       {
           checked
           {
               M(unchecked(X() + Y()) + 1);
           }
           // Other code
       }

    That could probably be combined with the example from SSLaks.

    Nice one! - Eric

  • You can come up with some quite nasty ones that won't work with checked contexts. though you could fairly easily refuse to move any arithmetic operation from outside a checked, into a checked.

    int a = 0;

    int one = 1;

    int bs = int.MaxValue + one;

    Console.WriteLine(checked(a + bs)); // works

    Console.WriteLine(checked(a + (int.MaxValue + one))); // throws

Page 1 of 3 (41 items) 123