[Request] Add an "add all" quick fix to missing_required_argument

[bernaferrari]

If you are instantiating a class with multiple parameters, it becomes too slow and error prone. I just want them, 'in order', there.

https://user-images.githubusercontent.com/351125/129495633-0bd907fa-ab46-47ee-81ac-edc2065386ea.mov

Suggestion: add an "add all parameters" quick fix.

[Levi-Lesches]

@bwilkerson, @scheglov, would this enough to make the change? I don't know much about the analyzer but I'd like to contribute.

pkg/analysis_server/lib/src/services/correction/fix.dart

  static const ADD_MISSING_REQUIRED_ARGUMENT = FixKind(
      'dart.fix.add.missingRequiredArgument',
      70,
      "Add required argument '{0}'");
+  static const ADD_MISSING_REQUIRED_ARGUMENTS_MULTI = FixKind(
+      'dart.fix.add.missingRequiredArgument.multi',
+      70,
+      'Add all required arguments');

pkg/analysis_server/lib/src/services/correction/dart/add_missing_required_argument.dart

  @override
  FixKind get fixKind => DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT;
+
+ @override
+  FixKind get multiFixKind => DartFixKind.ADD_MISSING_REQUIRED_ARGUMENT_MULTI;

  @override
  Future<void> compute(ChangeBuilder builder) async {

I looked at other examples of _multi, like ADD_OVERRIDE, but I couldn't find any special logic about repeating the fix, so I just assumed that adding multiFixKind would do it.

[bwilkerson]

That might be sufficient as far as implementation goes. We'd also need tests, of course.

But it might be that the add_missing_required_argument fix, as currently written, would conflict with itself or have unfortunate behavior (such as adding the arguments in the wrong order).

If you want to write tests for it they would go in the file add_missing_required_argument_test.dart.

[Levi-Lesches]

Well, I tried it, and I got pretty far. I added a test using assertHasFixAllFix with the following code:

Future<void> test_basic() async {
  await resolveTestCode('''
    void f({required int x, required bool y}) {}
    void g() {
      f();
    }
  ''');
  await assertHasFixAllFix(CompileTimeErrorCode.MISSING_REQUIRED_ARGUMENT, '''
    void f({required int x, required bool y}) {}
    void g() {
      f(x: null, y: null);
    }
  ''');
}

(The strings are formatted properly in the test, I just un-indented it to look nice for GitHub)

It got up to where it tries to merge the fixes together, where it concluded that the fixes can't be merged together. As to why:

https://github.com/dart-lang/sdk/blob/19d12d18d66ea8e6208a920dd0e257cf8d4a0f64/pkg/analysis_server/lib/src/services/correction/fix.dart#L121-L124

https://github.com/dart-lang/sdk/blob/19d12d18d66ea8e6208a920dd0e257cf8d4a0f64/pkg/analysis_server/test/src/services/correction/fix/fix_processor.dart#L572-L575

DartFixKindPriority.IN_FILE is 40, but the priority is set to 70. Is that important? From what I can tell, it's trying to say that the changes made are too close together to guarantee that it won't conflict, whereas IN_FILE is saying "it's probably fine". Is the TODO trying to address this?

---

Anyway, I temporarily switched the priority level to IN_FILE, but it still only applied one fix:

Expected: 'void f({required int x, required bool y}) {}\r\n'
              'void g() {\r\n'
              '  f(x: null, y: null);\r\n'
              '}\r\n'
              ''
    Actual: 'void f({required int x, required bool y}) {}\r\n'
              'void g() {\r\n'
              '  f(x: null);\r\n'
              '}\r\n'
              ''
     Which: is different.
            Expected: ...  f(x: null, y: null) ...
              Actual: ...  f(x: null);\r\n}\r\ ...
                                    ^
             Differ at offset 73

This is my first time digging through the analyzer code so I'm just trying anything that looks right. Got any pointers?

[bwilkerson]

DartFixKindPriority.IN_FILE is 40, but the priority is set to 70. Is that important?

Only indirectly. The priority is used to sort the actions, and we want the "fix all in file" actions to come after the "fix at this location action", hence the 40 and 70. (The absolute values aren't important, just their relative magnitude.)

Got any pointers?

In order to convert a fix so that it can be applied everywhere in a file a couple of things need to be done.

1. Create a fix kind like the existing one with a suffix of '_MULTI' and a priority of IN_FILE.
2. Override the getters canBeAppliedToFile, multiFixKind (to return the fix kind added in step 1), and possibly multiFixArguments (if the fix kind's label has any parameters).

If you've already done all of that and the fixes still aren't being applied, then it's likely that a ConflictingEditException is being thrown somewhere during the execution of the compute method. You can add a breakpoint at analysis_server/lib/src/services/correction/fix_internal.dart:1297 to see the stack trace to figure out where it's being thrown.

My guess is that the code is treating two insertions at the same offset as a conflict. I'm not sure we can fix this because the two insertions are not necessarily being added in the correct order. For example, let's assume we had two separate fixes, one to add 'static ' and another to add 'final ' immediately before a field declaration. The order in which those two edits are applied matters for the correctness of the code, but can't be controlled. (The same mechanism is used by dart fix to apply fixes of different diagnostics.)

But even if it were safe to just merge the two pieces of inserted text, I suspect the test would still fail. (Sorry I didn't think of this earlier.) The correction producer is looking at the empty argument list and deciding to insert an argument without a trailing comma (x: null in the first case, y: null in the second case). Simply appending them would result in f(x: nully: null).

I don't think we want to add a trailing comma for a single argument, nor the trailing space that would also be required, so I'm guessing that we need a different approach here.

The best approach I can think of at the moment is to not make this a fix-in-file fix, but rather to have the fix notice when there are multiple missing required arguments in a single invocation and to add all of them (rather than just adding one, which it does today). If there's only one missing argument then it should work the same as it does today, but when there are multiple it should return a different fixKind (perhaps ADD_ALL_MISSING_REQUIRED_ARGUMENTS) and fixArguments so that the menu label is accurate.

[Levi-Lesches]

Agreed, I came to the same conclusion last night. Again it's my first time, so it took a lot of testing for me to realize this, but I realized it was a big problem that each missing parameter gets its own error with an associated fix. Which means even if we could figure out conflict resolution, there still wouldn't be a way for the user to say "insert all missing arguments in this function". It would either be "insert one missing argument in this function" or "insert all missing arguments in the entire file" (which I was actually sort of able to get working).

The only way I can see this working is by doing what you said, which is using an assist (I think that's the right term?) instead of a fix, so that it wouldn't need to be triggered by an error in the first place.

Is there any precedent for this I can adapt? Specifically, a case where one error ("you forgot some arguments") is actually represented by many smaller errors (one for each argument), and the analyzer does some work to generate one assist that addresses all those issues? Maybe something like ?. short circuiting, where an additional warning is generated for each redundant ?.? I'll check myself, just wanted to know if you have a good example in mind.

[bwilkerson]

No, I don't have a good example in mind at the moment. I know we've run into it in the past, but I'm drawing a blank at the moment.

I do, however, think we want it to be a fix.

Assists are actions that are available whether or not there's a diagnostic. There are a couple of problems with using an assist for this purpose. First, every assist has to check to see whether it should be suggested every time the user moves the cursor, so we need to be very careful that the cost of determining whether to show the assist is justified by the value of the assist. Second, clients don't always display assists the same way they display fixes, so it might be confusing in the UI to have what looks like an assist be associated with a fix.

What I was suggesting was for the fix's correction producer to determine whether there are multiple missing required arguments and if so to generate a "fix all" fix instead of a "fix one" fix. To prevent having this fix duplicated in the list we'd need to only do this when the current diagnostic is associated with the lexically first of the required parameters and not produce any fix for the rest of the diagnostics. I'm not sure how hard that would be.

[Levi-Lesches]

I see. Basically modify the current implementation (if possible) such that

• if there's more than one missing argument:
  • if this is the first missing argument, build the entire argument list (on a new, indented line?)
  • if this is not the first argument, do nothing as to not conflict with the above
• if there's only one missing argument, keep the current behavior

And this way, it would still be a regular fix, not a _MULTI fix, right?

[bwilkerson]

Correct on all counts.

on a new, indented line?

We generally don't worry too much about formatting the results. At some point it would be nice to be able to have an option to run the formatter over all produced code, but in the meantime we assume that users will format their code manually apply applying fixes.

Also, if we replace the whole argument list, rather than just inserting new arguments in the appropriate places, then we run a bigger risk of colliding with other edits to the arguments, making it harder to compose fixes in the future. For example, consider

String f(String s, bool b) => s
void g() {
  f(f('1'));
}

If we insert , false just before the closing parenthesis in each invocation then everything will be fine. But if we replace everything between the parentheses then either edit will prevent the other.

[Levi-Lesches]

Well currently there's code to check if the user already started typing some arguments, and if so adds , to it, so I'll just keep that in there. I guess that also means keeping everything on one line then, so we don't move f('1') in your example to a new line. It's probably also fine to let the user do a bit of work in terms of spacing because the default value is null, and most times people are going to want to change that anyway.

[FMorschel]

Related to https://github.com/dart-lang/sdk/issues/38876.

@Levi-Lesches have you made a draft CL? Would you like some help?

[Levi-Lesches]

I couldn't find any WIPs in my computer, sorry. Feel free to take over from the discussion above, though

[FMorschel]

Here is the CL https://dart-review.googlesource.com/c/sdk/+/410740.