Auto-completion that adds new import lines (like override) should also update existing imports that use combinators

[FMorschel]

Say you have:

base.dart

class A {}

class B {
  A a => A();
}

main.dart

import 'base.dart' show B;

class C extends B {
  @overr^
}

This completion list would show all things from Object and the completion for overriding B.a.

If A was declared in another file, say a.dart, the new import would be added above, and everything would work as expected. But since it is defined in the same import that uses a combinator, the completion will add the type to the line as the expected return type, but it currently doesn't fix the import line.

CC @bwilkerson @DanTup

[FMorschel]

Just so that the Discord discussion doesn't get lost:

@DanTup had worked on this but was still a WIP: https://dart-review.googlesource.com/c/sdk/+/428821

@bwilkerson's comment on this:

I think this would also be easier if we just recorded which elements we need to import and leave the logic of how to import them until later in the process when we have complete information about what's to be imported.

This does raise the interesting question (which I didn't mention earlier) as to what it means when DartFileEditBuilder.importLibrary is invoked. I think at the moment the semantics is that everything in the library is expected to be imported. I haven't looked at where it's invoked so I don't know whether that should always result in an import without a hide/show clause or whether that just means that we should enumerate all of the elements when building a show clause.

@DanTup's answer to the above:

I think this would also be easier if we just recorded which elements we need to import and leave the logic of how to import them until later in the process when we have complete information about what's to be imported.

I agree - although I think there's a mix of places that want to a) just make this Element visible using whatever import is suitable, b) make this element available specifically using this URI, c) make this element available specifically using show, d) just import this URI (and we don't get any context about what it was for, which makes it difficult to know if we can remove this URI if a later URI provides the element that was wanted and this one is now redundant).

This case for LSP has additional complexity as we use the edit builder while building the completion, but then we try to extract some "metadata" from it (eg. the elements and import URIs) to send back to the client (in as small a payload as we can, because completion can be large), and then we rebuild the edits from that data in the resolve handler.

In some ways it might be better if we round-tripped (to the client) information about the method being overridden instead of all the elements/uris, and then in resolve we use that to build the editor. However it's not very general (for ex. I don't think that would work for @FMorschel's change), so probably round-tripping the element identifiers is still best (which is what my CL started to do, but without changing much about the edit building itself).