"Inline Method" assist may change code semantics when arguments contain invocations

[DanTup]

Given this code which has an invocation as an argument to a function:

void a() {
  b(expensiveFunction());
}

void b(String a) { // Perform "Inline Method" here
  print(a);
  print(a);
}

int count = 1;
String expensiveFunction() => '${count++}';

If you use "Inline Method" on b (which maybe should be "Inline Function"?), you end up with this:

void a() {
  print(expensiveFunction());
  print(expensiveFunction());
}

int count = 1;
String expensiveFunction() => '${count++}';

expensiveFunction() is now called twice. This could be more expensive than the original code, and if the function has side-effects could change the behaviour.

Local variables should probably be introduced for such cases, although this might get noisy (and appear unnecessary) for getters?

[scheglov]

I'd say that this is up to the user to understand the consequences.

[DanTup]

That's probably fair, although it's worth noting that when you invoke this assist from the declaration, you're not necessarily seeing the call sites. There might be 100 places that called the method across your codebase and you'd only see the changes if you look at a diff (which is how I noticed this when I was reviewing code before committing). It could be easy to not notice this (and assume the assist wouldn't change code in this way).

[scheglov]

I agree that when there are many invocations, it is impossible to check manually all of them.

In the initial set of Dart refactorings, implemented for Eclipse as a client, we were producing warnings. These warnings were shown in the refactoring dialog (just the dialog here, no warning shown) with the context where they happen in a mini-editor embedded into the dialog. So, the user can verify unsafe changes. Theoretically, this is something we could add here as well. But this requires the client to be able to show warnings. Unfortunately, even in IntelliJ we treat warnings as errors, and just cancel the refactoring.

[bwilkerson]

At first glance, it seems like we could handle this case automatically. Any arguments other than a simple reference to a local variable could be translated into an assignment to a local variable (whose default name would be the name of the corresponding parameter) and then the local variable could be referenced in the in-lined code. For example, the original example, after the refactor, could look like:

void a() {
  var a1 = expensiveFunction();
  print(a1);
  print(a1);
}

(the 1 being added because of the name conflict with the method name). If the corresponding parameter is only referenced in one place in the function being inlined, then we could optimize the code by not creating the local variable and doing what we're doing today.

Would that be a reasonable approach?

which maybe should be "Inline Function"?

Yes, that would be better.

[DanTup]

Any arguments other than a simple reference to a local variable could be translated into an assignment to a local variable

Would we do this for getters? I feel like the answer is yes (because otherwise the same issue exists), but wonder if users might not understand why and by annoyed that we've added lots of var foo = this.foo; variables above the invocation?

[scheglov]

Yes, we would have to do this for getters.
At least until we get stable getters.