Roslyn: [Refactoring] Inline Method

[kuhlenh]

We have extract method, it would be nice if we sort of had the reverse: inline method.

This would be invoked from a method call and would essentially copy/paste the body of the method where the method call is.

@wli3 can offer more information.

[wli3]

https://refactoring.com/catalog/inlineMethod.html

[ericmutta]

@kuhlenh We have extract method, it would be nice if we sort of had the reverse: inline method.

It would be more than nice! I just used this refactoring a few minutes ago, but have to do so via CodeRush. Having more refactorings available natively really helps because eventually we can reduce the number of extensions we have to load into Visual Studio 👍

[DustinCampbell]

FWIW, this has always been considered a lower priority refactoring because research has shown that it is not used nearly as often as Extract Method. That said, it's nice to have the inverse.

Note that copying-and-pasting is not how this refactoring would actually be implemented. It's actually quite complex:

1. Locals should be introduced for the parameters to the original function.
2. Identifiers in the method body need to be renamed if they conflict with any identifiers in the target method body.
3. Complexification/simplification needs to be applied to avoid conflicts since this refactoring can potentially affect a large code base.

[ericmutta]

@DustinCampbell FWIW, this has always been considered a lower priority refactoring because research has shown that it is not used nearly as often as Extract Method.

I certainly agree...I am curious though, how do you generally do research for each refactoring to determine its priority?

In some cases, an omission becomes a self-fulfilling prophecy: a refactoring is not implemented so no ever tries/knows to use it. A good example of this is "Break apart arguments" from CodeRush for Rosyln which takes this:

Public Sub Foo(Argument1 As Integer, Argument2 As Integer, Argument3 As Integer)
End Sub

...and turns it into this:

Public Sub Foo(Argument1 As Integer, 
               Argument2 As Integer, 
               Argument3 As Integer)
End Sub

For 17 years I have been doing that transformation manually (thankfully, automatic indenting in the editor helps line up the arguments). Then the other day I discovered "Break apart arguments" and now I use it several times a day.

You are also right that "Inline method" is not an easy thing to do correctly (as is the case for many refactorings). Given some bugs I have seen (like #22012 and #22059) it's really easy to forget some strange corner cases!

[DustinCampbell]

I certainly agree...I am curious though, how do you generally do research for each refactoring to determine its priority?

There are number of ways that we determine priority. Below are a few example, though this is certainly not the full list.

1. Feature requests and customer development.
2. Filling in gaps in our offering. (For me, this is the strongest reason to have Inline Method).
3. Looking at refactoring research (yes, this is a thing :smile:). For example, here a couple of papers that include usage data (link1, link2).

FWIW, I worked on CodeRush at DevExpress for five years of my career. I remember the discussions we had 10 years ago about "Inline Method" and whether it was worth implementing. Essentially, we came to the conclusion that I mentioned about -- that it's valuable to have the inverse refactoring even if it might not be used as often as, say, "Introduce Local" vs. "Inline Temp". I'm glad to hear you've found it useful!

[ericmutta]

@DustinCampbell Looking at refactoring research (yes, this is a thing 😄)

Who would have thunk? 😄 Thanks for the links, I will take a look!

@DustinCampbell FWIW, I worked on CodeRush at DevExpress for five years of my career.

I wasn't sure before if it was proper etiquette to gush over DevExpress, but now I think I can say it without being stoned: the work you guys did over there is just utterly brilliant. Every time I see that little animation for the "reorder parameters" refactoring I am left wondering: how on earth do they do that? 😄 Glad to hear you work on Rosyln now, I am sure you will inspire the team to do more refactorings natively (fingers crossed!).

[DustinCampbell]

Glad to hear you work on Rosyln now, I am sure you will inspire the team to do more refactorings natively (fingers crossed!).

Well, I hate to dash your hopes, but I've been on Roslyn since the project kicked off in 2010. At this point, I'm just the old man on the team. :smile:

[fschwiet]

+1 please

[Epstone]

+1 from my side. I find it quite natural that 'extract' is used more often because going from spaghetti to structured methods is the 'normal' way I work and most others I guess. When something went wrong we can also use undo instead of inlining.
Inlining really needs to be teached, trained and repeated before it gets wired into our fingers. But then it feels like magic at your fingertips needed for shaping the dough :)

[genlu]

As @DustinCampbell pointed out above, if unrestricted, "inline method" refactoring could have significant impact on the code base, and potentially introduce errors and breaks.
Just name a few that come to my mind:

• handling parameters passed by ref vs by value
• handling nullable annotation
• when applied on externally accessible methods, it might break public API
• if we opt to add missing usings at consumer sites, it might introduce conflicts

I think it make sense to provide a scaled-down version to start with which would still cover a lot scenarios people run into often. For example: only allowed on private method with simple body (single statement body and expression body?)

[mwelsh1118]

I would say the most common use case for "inline method" is to alias another method. For example, say I have:
```C#
public class Widget
{
public static Widget Create()
{
return new Widget();
}
}

I might end up creating a separate WidgetFactory class:
```C#
public static class WidgetFactory 
{
    public static Widget CreateWidget() 
    {
        return new Widget();
    }
}

And then aliasing the old factory method to the new one:
C# public class Widget { public static Widget Create() { return WidgetFactory.CreateWidget(); } }
Finally, I can inline the original method and now all callers of that method are instead calling my new factory class. This combines very well with a "Replace constructor with a factory method" refactoring.

I think inlining private methods is probably not going to add a lot of value, since that's relatively easy to do manually (you're unlikely to have more than a handful of callers). The real value is where there are many external callers of a method and you want to redirect those calls somewhere else.

Another common use case is to turn an instance method into an extension method, which goes something like this:

1. Define a new extension method Y with the functionality from instance method X
2. Change instance method X to call extension method Y
3. Inline method X and now all callers are instead calling extension method Y.

[martijnhoekstra]

+1 on that there is value in inlining a single call site, regardless of whether that means the extracted method can then be eliminated.

[genlu]

@mwelsh1118 Makes sense. I didn't think of this refactoring as a way to do some other things indirectly (in your examples, change the target method of invocations). Sounds like the restriction on non-private method can be relaxed to just showing a warning says this would break external consumers, while still being disallowed on methods with complex body?

[mwelsh1118]

@genlu That seems reasonable.

[kendrahavens]

Design meeting update:

Enable the simple case proposed in @genlu's comment: https://github.com/dotnet/roslyn/issues/22052#issuecomment-591660345

[fschwiet]

referring to the cases @genlu listed, I would recommend seeing Resharper does and doing that. Sorry not too original sounding, but its implementation never really let me down. Another scenario this can enable is doing change signature operations that don't fit in the current change signature UI (wrap the method you want, inline the wrapper).

handling parameters passed by ref vs by value

I believe resharper creates variables around the inlined code in place of the ref or out parameters.

handling nullable annotation

This I don't know, it maybe makes sense to just drop the annotations.

when applied on externally accessible methods, it might break public API

Resharper would also give a warning then proceed.

if we opt to add missing usings at consumer sites, it might introduce conflicts

I believe Resharper would introduce "using alias=n" for the types it needs that conflict, but you could also just fully qualify those names (with their namespace).

[Cosifne]

@vatsalyaagrawal This is the inline method isssue

[Cosifne]

Thanks for all the posts and discussion here. I have checked in a PR implements what @genlu purposes.