Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use completion.existingImports to filter completion suggestions #1776

Closed
DanTup opened this issue Jun 5, 2019 · 9 comments · Fixed by #1789
Closed

Use completion.existingImports to filter completion suggestions #1776

DanTup opened this issue Jun 5, 2019 · 9 comments · Fixed by #1789
Labels
in editor Relates to code editing or language features is enhancement
Milestone

Comments

@DanTup
Copy link
Member

DanTup commented Jun 5, 2019

No description provided.

@DanTup DanTup added is enhancement in editor Relates to code editing or language features labels Jun 5, 2019
@DanTup DanTup added this to the v3.2.0 milestone Jun 5, 2019
@DanTup
Copy link
Member Author

DanTup commented Jun 10, 2019

@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 :-)

Screenshot 2019-06-10 at 6 39 23 pm

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

@DanTup
Copy link
Member Author

DanTup commented Jun 10, 2019

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).

@scheglov
Copy link

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.

@DanTup
Copy link
Member Author

DanTup commented Jun 10, 2019

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.

@scheglov
Copy link

scheglov commented Jun 10, 2019

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.

@DanTup
Copy link
Member Author

DanTup commented Jun 11, 2019

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:

Screenshot 2019-06-11 at 11 14 21 am

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

Screenshot 2019-06-11 at 11 15 05 am

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!

DanTup added a commit that referenced this issue Jun 11, 2019
…equest

This doesn't change anything today because the server sends the event right before every completion, however if it's optimised there, we'll get the benefit here.

Fixes #1776.
DanTup added a commit that referenced this issue Jun 11, 2019
…equest

This doesn't change anything today because the server sends the event right before every completion, however if it's optimised there, we'll get the benefit here.

Fixes #1776.
DanTup added a commit that referenced this issue Jun 11, 2019
…equest

This doesn't change anything today because the server sends the event right before every completion, however if it's optimised there, we'll get the benefit here.

Fixes #1776.
DanTup added a commit that referenced this issue Jun 11, 2019
…equest

This doesn't change anything today because the server sends the event right before every completion, however if it's optimised there, we'll get the benefit here.

Fixes #1776.
@DanTup
Copy link
Member Author

DanTup commented Jun 11, 2019

I refactored the code a little so that it now builds into a map so the lookups are faster, which may make it easier to understand (#1789). However, there's no performance benefit yet as existingImports is sent before ever completion request (I think this was discussed on the other PR as a possible optimisation).

@scheglov
Copy link

The code in the PR looks good to me.

@DanTup
Copy link
Member Author

DanTup commented Jun 11, 2019

Great, thanks for looking!

DanTup added a commit that referenced this issue Jun 11, 2019
…equest

This doesn't change anything today because the server sends the event right before every completion, however if it's optimised there, we'll get the benefit here.

Fixes #1776.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features is enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants