Is the UX around fixes confusing?

[bwilkerson]

Normally the analysis server will provide one fix for every diagnostic reported on a given line of code. For example, given

void twoDifferentDiagnostics(A a) {
  '${a.m1()} ${a.m2()}'; // diagnostics here
}

class A {}

There are two diagnostics, one indicating that m1 is not a member of A and one indicating that m2 is not a member of A, and there will be two fixes, one to declare m1 and one to declare m2. (Actually, there are four today because it can either be created as a method in A or as an extension method, but the basic behavior is the same: every diagnostic as a fix.)

However, if there are multiple diagnostics of the same kind (and/or the same message, I don't know how the comparison is done), then there is code in the analysis server that deduplicates fix-all-in-file fixes so that we won't have multiple fix-all-in-file fixes presented that will all do the same thing. That's good, but it also deduplicates the single location fixes in at least some cases. For example, given the code

void twoSimilarDiagnostics(A a) {
  useTwoB(a, a); // diagnostics here
}

void useTwoB(B b1, B b2) {}

class A {}

class B {}

There are two diagnostics of the same kind with the same message. There is one "Add cast" fix and one "Add cast everywhere in file" fix. The fact that there's only one fix-all-in-file fix is good, but the behavior of the first fix depends on where the cursor is on the line. In some places it will add a cast to the first argument and in some cases it will add a cast to the second argument. Clicking inside the highlight range of one diagnostic causes the fix to apply to that diagnostic, but clicking outside either highlight range appears to add the cast on the closest diagnostic.

The problem is worse if there are multiple diagnostics of different kinds where all of them have the same fix. The "apply this fix everywhere" messages don't distinguish the kind of diagnostic being fixed, but will only fix one kind of diagnostic. For example, given the code

void f(A a) {
  a = useB(getA()); // diagnostics here
  a = useB(getA()); // diagnostics here
}

A getA() => A();

B useB(B b) => b;

class A {}

class B {}

There are four diagnostics, two on each line. Two indicating that the types of the arguments don't match the types of the parameters, and two indicating that the type of the right-hand side of the assignment doesn't match the type of the variable on the left. Clicking on either line will produce two fixes, one to add a cast and one to add a cast everywhere. The single location fixes behave as above, changing behavior depending on where on the line the cursor is located. The fix-in-file fixes do the same, they change their behavior depending on where the cursor is on the line. But it seems wrong to me that Add cast everywhere in file" won't add a cast everywhere despite the fact that its a fix offered for all of the diagnostics.

The question is: is this confusing for users (in practical use, not just in theory) and if so what should the UX be?

[bwilkerson]

@DanTup

[DanTup]

But it seems wrong to me that "Add cast everywhere in file" won't add a cast everywhere despite the fact that its a fix offered for all of the diagnostics.

This also seems wrong to me (although I think everything else might be good?.. The dedupe code deblierately did all with the same text, it wasn't intended only to do fix-all-in-file.. having two entries with the same text is confusing).

I think this issue is the same regardless of the de-duping - the code action that says "Add cast everywhere in file" will not fix all issues that have a fix "Add cast", it will only fix the ones of the same type. I think it's correct that it doesn't fix different kinds, but I wonder if the text should include the code of the diagnostic being fixed, for example:

• Add cast everywhere in file (argument_type_not_assignment)
• Add cast for all 'argument_type_not_assignment' diagnostics in file

It does make them much more verbose though.

[bwilkerson]

The dedupe code deblierately did all with the same text, it wasn't intended only to do fix-all-in-file.. having two entries with the same text is confusing).

I agree, having two menu items with the same label is confusing. I'm just wondering whether there's a way to change the labels on the single-location fixes so that both can be seen. I chose a lousy example, but if the code was useTwoB(a1, a2); then maybe the labels could be "Add cast to 'a1'" and "Add cast to 'a2'"?

Another alternative would be to only show fixes for the diagnostics whose highlight range includes the selection (point or range). That would reduce the ambiguity, though there are still cases where the highlight ranges can overlap so it doesn't remove all the ambiguity. And I don't know how consistent that would be with the way other languages are supported.

... I wonder if the text should include the code of the diagnostic being fixed ...

Yes, it's longer, but it also makes it more clear what the fix will, and won't, do. But that's a really long description for a menu item. So the question is whether the current state is better or whether having long menu item labels is better (assuming there isn't a third option better than either of those).

(I like the second better, but I'd drop "in file" from the end as I don't think it's necessary in this case. And maybe "diagnostics" as well.)

[DanTup]

maybe the labels could be "Add cast to 'a1'" and "Add cast to 'a2'"?

I think that would be an improvement, but there are probably a lot of places where there isn't a nice short piece of context that could be added (for example in print("a very long string") if I have the single quotes lint enabled).

Another alternative would be to only show fixes for the diagnostics whose highlight range includes the selection (point or range).

Do you mean use the provided range without expanding to include the whole line? I think some people like the convenience of including the whole line (for ex. Dart-Code/Dart-Code#4601 asked for an assist that is also sometimes a fix to appear for the whole line), although it's always felt slightly odd to me that we do that.

• Add cast for all 'argument_type_not_assignment' diagnostics in file

I like the second better, but I'd drop "in file" from the end as I don't think it's necessary in this case. And maybe "diagnostics" as well

Yeah, I think I agree. TypeScript is similar (although it includes "problems" at the end):

[FMorschel]

• Add cast for all 'argument_type_not_assignment' diagnostics in file

I like the second better, but I'd drop "in file" from the end as I don't think it's necessary in this case. And maybe "diagnostics" as well

Yeah, I think I agree.

I agree too. The "all" in the phrase already means the same thing; "diagnostics in file" could be removed, and the meaning would remain. I don't think users expect fixes to work across different files, even with a phrasing like the above. For that, we already have a separate command.