Use completion.existingImports to filter completion suggestions

No description provided.

@scheglov

@scheglov would you mind scanning over this code to see if my understanding of how this works is correct? I'll optimise the performance, but I want to ensure the logic is correct.

I tested it by having the same class exported in multiple files, but also multiple classes with the same name in each. With this code, it does what I'd expect (the duplicated class shows up 3 times in completion, and two of them will generate prefixes, and the re-exported class appears only once), so I think it's good - it just seems much simpler than I was expecting :-)

It caught some dupes in the SDK too!

Item DannyClassOnlyOne from package:danny/real_def.dart is already imported by package:danny/real_def.dart so filtering out the suggestion for package:danny/proxy2.dart
Item DannyClassOnlyOne from package:danny/real_def.dart is already imported by package:danny/real_def.dart so filtering out the suggestion for package:danny/proxy1.dart
Item Future from dart:async is already imported by dart:core so filtering out the suggestion for dart:async
Item Stream from dart:async is already imported by dart:core so filtering out the suggestion for dart:async

I didn't really add much context to the code... the .filter at the top is filtering the array of suggestions from an included suggestion set. The existingImports variable is the existing imports for the libraryFile in the response.

That said, the comments may explain it better than code - I just want to be sure I haven't misunderstood anything (like I say, it seems to work in my tests - but it's harder to test against Flutter code right now because of the SDK version).

Skip completions for symbols that are already imported via another li…

Not quite.

You check that the library that declares the element of the suggestion is already imported.

But in Flutter material.dart re-exports Widget from src/widgets/framework.dart.
And what we want is to avoid suggesting Widget from package:flutter/widgets.dart when there is already package:flutter/material.dart import. Because Widget is already available from an imported library. The uses has already made the decision what to import.

Thanks! Though I'm not sure I understand. Do you mean line 268 is wrong? I believe this is comparing the declared library URI on both sides (so it's resolving the item in the suggestion to its declaration, and also the item that's available in the import to its declaration). This way we only filter out things that are exact dupes (eg. the same declaration).

So in your example, I believe I would get flutter/widgets.dart on both sides (because the re-export for material.dart would return its declaring library as flutter/widgets.dart)? If the user hadn't imported anything, we would still show all of them.

If the user has not imported anything yet, then we show for an element all possible suggestion sets (URIs to import).

If there is an existing import such as package:flutter/material.dart, it provides element package:flutter/src/widgets/framework.dart::Widget, so we don't suggest this element from package:flutter/widgets.dart.

Direct declaration does not take preference when there is already an import that provides the same element.

Look at this as this: each library exports / provides a set of elements. We have already imported libraries, which provide sets of elements. We iterate over available suggestion sets and their suggestions. Each suggestion is based on an element (declaringLibraryUri and label). If this element is already provided by an imported library, we include this suggestion only if the suggestion set is an already imported library.

Sorry, I still don't understand which bit is incorrect :(

I tested it with Flutter master, and here's what I see when there are no imports.. EdgeInsets shows up 5 times from the different libraries that export it:

If I import it from widgets then it only shows up once (with no auto-import tag):

This is because when we found it inside the suggestion sets for other libraries, we noticed that it resolves to the same declaration as the the one in an existing import, so we skipped it. The log looked like this:

Item EdgeInsets from package:flutter/src/painting/edge_insets.dart is already imported by package:flutter/widgets.dart so filtering out the suggestion for package:flutter/material.dart

Item EdgeInsets from package:flutter/src/painting/edge_insets.dart is already imported by package:flutter/widgets.dart so filtering out the suggestion for package:flutter/painting.dart

Item EdgeInsets from package:flutter/src/painting/edge_insets.dart is already imported by package:flutter/widgets.dart so filtering out the suggestion for package:flutter/cupertino.dart

Item EdgeInsets from package:flutter/src/painting/edge_insets.dart is already imported by package:flutter/widgets.dart so filtering out the suggestion for package:flutter/rendering.dart

Note the first URI in my log is the declared URI, the one from the suggestion set is at the end of the line (I realise in my first comment this was scrolled off the edge, so this may have been misleading!).

If this seems wrong to you, can you give an example of a symbol and where I should import it from that you think I'll get the wrong result, and I can do some testing of it. Thanks!