Share via

Rename refactoring on a private field takes too long; analyzes full solution

  • Article

We received an excellent bug from Michael Teper
commenting that rename was taking too long in a scenario that should
have been faster.  Not only does he mention his dissatisfaction
but he also took the time to suggest a way that we could be much faster
in a very specific renaming scenario.  Specifically, if you were
renaming a private field then it shouldn't be necessary to compile the
full solution.  First off, i want to thank Michael for submitting
this feedback to us.  It's extraordinarily helpful to know if our
features are or are not meeting your needs.  When we don't hear
anything from users we will often have to guess or hope that it's
working out well.  (and, sometimes you want something to be
working so well that users never even notice it).  However, when
we do hear back from a user we know that this is something we have to
look at.  Now, i wanted to dive deep into the optimization
suggestion that was made.

Specifically, Michael says "This class lives in a library project that
depends on one other library project. Current implementation aside, I
can't see any reason why you would need/want to compile the entire
solution just to analyze usage for a *private* field."

This is a very reasonable complaint.  It certainly appears that if you had the following code:

 internal sealed class TextBoxDefinition : FieldDefinitionBase {
    private int _maxLength;

    public int MaxLength {
        get { return _maxLength; }
        set { _maxLength = value; }
    }
}

and you renamed "_maxLength" to something else (say "_maxSize"), then
we could put in a quick optimization and and only try to compile the
current type.  The reasoning behind this is that as the field is
private, nothing else could possibly reference it and so it suffices to
look in the current type only when doing the compile.  However, as
it turns out this is not the case.  And it is necessary when
renaming a member (even a private one) to do a full project
analysis.  Why?  Well consider the following trivial example:

 public class Foo {
    private int member;

    void Method() {
        Bar.CreateFoo().member = 4;
    }
}

public class Bar {
    public static Foo CreateFoo() {
        return new Foo();
    }
}

As you can see, "member" is referenced in one place:

 Bar.CreateFoo().member = 4;

This is an example of referencing a private member of the current type
through an expression that binds to an instance of the current
type.  However, in order to know that "Bar.CreateFoo()" binds to
an instance of "Foo" we must compile "Bar".  Since arbitrary
expressions can end up returning an instance of the current type we
basically need to compile everything to ensure that accurately rename
everything.

Now, one area that we can we can speed this up in is when you rename a
local variable.  Because the local variable cannot be referenced
by anything outside the current member, and because you can only refer
to it with it's name and not through an arbitrary expression, it's
possible to rename a local quite quickly.

When we started work on refactorings we read a lot of the literature on
it, and we talked to many customers to see what they wanted out of
it. 
And the message we got back was that if they could not absolutely trust
refactorings then they would not use them.  Refactorings are
supposed
to keep the semantics of your code the same and if that invariant is
not held, then refactorings like renames are no better than just a
find/replace.   And, like with most development, and
especially something like refactorings, it
was our top priority to get the feature working to specification first,
and
then make it work performantly next.   So, for whidbey, we
will continue to rebuild a fair bit when you rename a class
member (private or otherwise).  However, it's still possible for
us to find other ways to make this faster through other means, and we'd
like to make these kinds of user actions much faster for our RTM
release. 

Michael: Once again, thank you very much for your feedback.  We'll
definitely be keeping that bug open so that we can track the issue of
performance with this feature.  I hope that when we finally ship
you'll be much happier with the product!

Comments

  • Anonymous
    April 27, 2005
    Could partial classes also cause problems ?
    I'm not sure how you could know what
    files contains a type definition
    without compiling the whole project...
  • Anonymous
    April 27, 2005
    Mike: Yup they definitely could. Althought you could avoid scanning for partial types if you're not currently in a partial type. (you can tell if there's the "partial" keyword right before 'class'
  • Anonymous
    April 28, 2005
    Since renaming the private member ought to let you rename the property at the same time I would think it needs a full solution compilation anyway.
  • Anonymous
    April 28, 2005
    The comment has been removed
  • Anonymous
    April 28, 2005
    DrPizza: "Since renaming the private member ought to let you rename the property at the same time I would think it needs a full solution compilation anyway."

    In the future we'd definitely like to have chained refactorings (including intelligent ones that suggest further refactorings that you could take after the first one). i.e. you choose to rename a field, and we ask you (with the option to never both you again) if you would like to rename the property as well. Since the prop is public we would absolutely have to do a full analysis.
  • Anonymous
    April 28, 2005
    Michael: "So, frankly, it sounds to me like the feature needs to be either optimized QUITE a bit or redesigned to be QUITE a bit more performant. Again, if you are looking for an example of what an acceptable user experience is, install a copy of ReSharper on VS.Net 2003. I realize, btw, that .Net 2.0 introduced new CLR/language features that makes in-place analysis harder, but that still doesnt change the user experience implications with respect to usability of refactoring support. "

    Agreed. I was trying to indicate that that was normal for us given that we get the feature working first and then do the optimizations later :)

    i.e. "And, like with most development, and especially something like refactorings, it was our top priority to get the feature working to specification first, and then make it work performantly next."

    In an ideal world refactorings would take on the order of .1-.2 seconds. i.e., done immediately after you choose the option. We're trying to get there :)
  • Anonymous
    April 28, 2005
    Sounds good, just wanted to make sure I get across what in my opinion constitutes both a usable and a useful implementation. Correctness goes without saying, but performance is no less important. Fractions of a second would be nice, and I think up to a couple of seconds for simple cases and maybe up to 5-10 for complex cases should be your goal. Hopefully if you are able to reach that goal, you could also rework the progress dialog (if its needed at all!) to display something less useless. :)
  • Anonymous
    April 29, 2005
    (It still amazes me that I have to click 'Post...', then scroll to the end. Who designs the usability around here?)

    I have experienced way too many times that after a long wait, the rename refactoring fails and leaves everything as it were. I hope you have fixed that in the real b2, and also let me decide wether I want it to break code or not.

    But why do you have to recompile the whole lot every time? Can't you just track the changes after the first compilation? Can't be that hard?

    And finally: Why the 'partial' keyword? You can't extend a class in this fashion outside of the project, anyway.
  • Anonymous
    April 29, 2005
    Thomas: " (It still amazes me that I have to click 'Post...', then scroll to the end. Who designs the usability around here?)"

    The community server guys i guess.

    "I have experienced way too many times that after a long wait, the rename refactoring fails and leaves everything as it were. I hope you have fixed that in the real b2, and also let me decide wether I want it to break code or not."

    Can you please file a bug on this at http://msdn.microsoft.com/ProductFeedback

    I have no idea if this has been fixed or not since i don't know what the bug.

    "But why do you have to recompile the whole lot every time? Can't you just track the changes after the first compilation? Can't be that hard?"

    Says you :)

    "And finally: Why the 'partial' keyword? You can't extend a class in this fashion outside of the project, anyway."

    Why not the "partial" keyword? What would you use instead?
  • Anonymous
    April 29, 2005
    "In the future we'd definitely like to have chained refactorings"

    Nice! I second what DrPizza said: it would be great to have the field/property combo refactoring option.

    Also, refactoring in VS2005 is still much faster than ReSharper and I trust it more. A little slow doesn't bother me - accuracy must be absolute. Not sure how much I like the modal dialogs though. :-)
  • Anonymous
    April 29, 2005
    That's funny - but this does compile:


    class StructDefinitionBase {
    public struct _maxSize { }
    }
    class ClassDefinitionBase : StructDefinitionBase {
    public class _maxSize { }
    }
    class DelegateDefinitionBase : ClassDefinitionBase {
    public delegate int _maxSize();
    }
    class FieldDefinitionBase : DelegateDefinitionBase {
    public int _maxSize;
    }
    class MethodDefinitionBase : FieldDefinitionBase{
    public int _maxSize() { return 0; }
    }
    class PropertyDefinitionBase : MethodDefinitionBase
    {
    public int _maxSize { get { return 0; } set { } }
    }
    internal sealed class TextBoxDefinition : PropertyDefinitionBase
    {
    private int _maxLength; // Rename me to _maxSize
    public void DoFoo()
    {
    _maxSize = 1;
    }
    }

    BTW, What IntelliSence show for _maxSize = 1 as tooltip for you ?
    For me in March CTP it's delegate ;-)
  • Anonymous
    April 30, 2005
    There's a very simple answer to this problem.

    Even though I (much to my shame) have used code like this, I can't really come up with a convincing argument as to why your example should even compile.

    Really, does "Bar.CreateFoo().member" really make sense? You're circumventing OO to access a private field through an external interface. It's hard to make a case that that's a reasonable thing to do.
  • Anonymous
    May 02, 2005
    The comment has been removed
  • Anonymous
    May 02, 2005
    Tag: i'll take a look at that once i get back into the office.

    I'm pretty sure intellisense gets this right in the current builds.