[Feature Request] New refactoring: convert positional parameters to named ones

[escamoteur]

Hi,

when maintaining a project for a longer time its easy to happen that the number of positional parameters grow with new features and suddenly you are faced with 10 positional parameters. Changing them to named ones especially where the function is invoked is a tedious process, so you probably add an 11 positional the next time.

So it would be great if the language server would support two additional options:

• convert to named Parameter
• convert all Parameters to named Parameters.

This would be extremely helpful

[leafpetersen]

cc @bwilkerson @scheglov

[escamoteur]

Oh and I want to thank the team to have added the 'extract local variable' into the LSP. I was badly missing it :-)

[srawlins]

Only 16 days old and this is currently our 4th most upvoted request.

@bwilkerson I'm thinking of tackling this one; never written a refactoring, but looking at convert_getter_to_method, it looks similar to a quick fix.

This is easier to implement for a pre-null safety library than a null safe library; less to worry about. With null safety, we can send clear signals about requirability; pre-null safety, we could use @required if meta is available, and /* required */ if not.

I'm tempted to just implement the second request ("convert ALL positional parameters to named") because the first request ("convert THIS positional parameter to named") would not be possible if there are any remaining optional params (m(int a, [int b], {int c}) is illegal).

Then again, if you only want to convert all of your optional positional parameters to be named, and you also have non-optional positional parameters which you'd like to remain positional, the second request doesn't help you a lot, because you'd have to manually undo the change to those parameters. :(

Let's look at some examples, assuming null safety:

- void m1(int a);
+ void m1({required int a});
- void m2(int a, [int? b]);
+ void m2({required int a, int? b});
- void m3(int a, [int b = 1]);
+ void m3({required int a, int b = 1});
- void m3(int a, {int? b});
+ void m3({required int a, int? b});

Not too tricky:

• previously non-optional positional need the text required prepended.
• previously optional positional keep their nullability and default value, and never get required.
• previously named do not change.
• [ and ] must be removed if present.
• { and } can be removed if present (simple to just unconditionally do this).
• add { to the front of the parameters and } to the back.
• no special considerations for function-typed parameters, or field formals.

[escamoteur]

I'm tempted to just implement the second request ("convert ALL positional parameters to named") because the first request ("convert THIS positional parameter to named") would not be possible if there are any remaining optional params (m(int a, [int b], {int c}) is illegal).

Then again, if you only want to convert all of your optional positional parameters to be named, and you also have non-optional positional parameters which you'd like to remain positional, the second request doesn't help you a lot, because you'd have to manually undo the change to those parameters. :(

I would propose the following rules:

• Change all params to names ones -> no problem
• Change a single parameter to a named one only works if there isn't another optional positional left

[bwilkerson]

@srawlins I would love to see some progress along these lines, but I have a few comments for you to consider.

First, in LSP we can implement some refactorings as code actions, but only those for which no additional input is required from the user. That's how we implemented 'extract local variable'. I suspect that in this case that's sufficient because we can use the selection to indicate the range of parameters to be converted. But in the legacy protocol, used by IntelliJ, we have to do work in both the server and the client in order to implement a refactoring. I have an idea for removing that limitation, and would be happy to discuss it if you're interested, but you need to be aware that we can't support a refactoring in all our clients without a bit more work than it might first appear to require.

Second, I'll note that this is a subset of a larger and more powerful refactoring to change the signature of a method / function. We have most of the implementation for the larger refactoring as part of the data-driven fixes work, so implementing the more general refactoring would primarily require refactoring some of that implementation to make it more reusable. I'd really like to be able to support the fully featured refactoring, but the limitations described above have so far prevented us (we'd need user feedback to specify the changes). It's probably the case that we should just implement the smaller refactorings (subsets of the 'change signature' refactoring) rather than wait for a hoped future ability to implement the full feature set, but I thought it worth mentioning.

Third, and related to the second, I think we need to be careful to not end up with so many smaller refactorings that the user interface becomes unusable. We're certainly nowhere near that point now, of course, but we should think strategically about whether there is overlap that could be avoided. (I don't see a better alternative in these cases, but I haven't spent much time thinking about it.)

Caveats aside, yes, a refactoring is similar to a quick fix. A refactoring can spend the time to search the whole code base for, for example, invocations of the method being refactored in order to clean them up. Quick fixes and quick assists can't. And that's really the only significant difference: the performance characteristics.

I do have one request: it has been a long-standing goal to convert the refactorings over to using ChangeBuilder in order to reduce duplication. One or two have been converted, but it would be nice if any new refactorings were written in the style to which we'd like to move in order to reduce technical debt.

This is easier to implement for a pre-null safety library than a null safe library; less to worry about. With null safety, we can send clear signals about requirability; pre-null safety, we could use @required if meta is available, and /* required */ if not.

I think we should implement this for null-safe libraries only, at least for the first round. The community appears to be moving quickly to null safety, so there might not be as much value to users in implementing support for pre-null-safe libraries.

The one think I'll note is that refactorings can (like quick fixes) add imports when necessary (so we could always use @required and wouldn't need a comment).

[srawlins]

First, in LSP we can implement some refactorings as code actions, but only those for which no additional input is required from the user. ...

I agree that this looks like a great candidate for code actions.

Second, I'll note that this is a subset of a larger ...

I think that implementing these various signature-refactoring ideas one-at-a-time might be a great way to incrementally get to a state where we look at converting to a big "Change signature" refactor which requires UI etc. So whether we get there or not, the individual refactorings help users as they land.

Third, and related to the second, I think we need to be careful to not end up with so many smaller refactorings ...

Ack.

I do have one request: it has been a long-standing goal to convert the refactorings over to using ChangeBuilder ...

Ack, will do.

I think we should implement this for null-safe libraries only, at least for the first round.

Ack.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/306160

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/308841

This CL adds it to VS Code.
I will make it work in IntelliJ as well.

[rrousselGit]

@scheglov That's nice, but what about if we want to refactor only a single parameter?

[scheglov]

This is a beginning, I plan to add a family of commands that manipulate formal parameters.

1. Convert the formal parameter under the caret into required named (and put at the end).
2. Convert the selected positional formal parameters into required named.
3. Move the positional formal parameters one position up / down.
4. Move the named formal parameters one position up / down.
5. Convert the selected named parameters into required positional.
6. (no) Convert to optional positional formal parameters. These are abominations, and should not exist.
7. Suggest anything that you want, but is missing from the list.

Unfortunately, this is what we have to do in text editors (like VS Code) that don't support dialogs.
In IDE, like IntelliJ, we could also have one dialog that does all these modifications in one operation.

[scheglov]

https://dart-review.googlesource.com/c/sdk/+/310764

[rrousselGit]

That looks epic! It'll change the life of many devs for sure

[escamoteur]

Absolutely awesome. Unfortunately two weeks too late for a big refactoring that was almost completely about converting positional to named parameters. Great to know that this will be easier in future now

[GoodAndBeautiful]

Is this still an open issue? It looks like https://dart-review.googlesource.com/c/sdk/+/310764 was merged and that commit was part of the 3.1.0 dart sdk. So it seems like this is done, but we're using dart 3.1.2 and flutter 3.13.5 but I don't see this refactoring option in Android studio or VSCode. Should we be seeing it? If not, what can we can help do to move this forward?

[GoodAndBeautiful]

Turns out it's available, but as an experiment. Searching "refactor" in the latest VSCode settings will show the option to enable experimental refactors. Haven't figured out how to do it in Android Studio yet.