Use completion.existingImports to filter completion suggestions #665

lambdabaa · 2019-06-01T00:15:20Z

This CL processes completion.existingImports notifications and uses them to build and maintain a map from file onto import URI onto names exported from the import. Then, at completion time, CompletionSuggestion.libraryFile is used to lookup a map of all imported libraries and their declared names. Then only libraries which are already imported are allowed to suggest it if some library has already imported the name. This prevents the scenario where I've already imported 'package:foo/foo.dart' which provides Foo and not-yet-imported 'package:bar/bar' suggests Foo causing you to import a new package unnecessarily.

Related to flutter/flutter-intellij#3415.

/cc @alexander-doroshko @scheglov @jwren @devoncarew

scheglov · 2019-06-01T16:54:42Z

...le/dart/server/internal/remote/processor/NotificationCompletionExistingImportsProcessor.java

+        }
+
+        String name = strings.get(names.get(index));
+        uriToNames.get(uri).add(name);


Would it be more efficient to use the usual pattern "get, if null then put" here?

Will do, thanks!

scheglov · 2019-06-01T16:55:45Z

...le/dart/server/internal/remote/processor/NotificationCompletionExistingImportsProcessor.java

+    List<Integer> uris = new ArrayList<>();
+    elements.getAsJsonArray("uris").forEach(item -> uris.add(item.getAsInt()));
+    List<Integer> names = new ArrayList<>();
+    elements.getAsJsonArray("names").forEach(item -> names.add(item.getAsInt()));


We could just as well decode directly into String(s) and avoid Integer wrappers.

Then we would need to use hashmaps instead of lists for constant lookup though, right?

No, but yes, in some sense :-)
List<String> uris maps int index to String value.

But then if we get a String from the uris or names lookup we have to use it as an index into strings?

Okay, discussed offline and fixed :)

scheglov · 2019-06-01T17:05:12Z

...le/dart/server/internal/remote/processor/NotificationCompletionExistingImportsProcessor.java

+    getListener().computedExistingImports(file, uriToNames);
+  }
+
+  private Map<String, Set<String>> buildUriNamesMap(JsonObject existingImports) {


I think this is wrong.

This method should return something like Map<importedLibraryUri, Map<declaringLibraryUri, declaredName>>. What I see as a single Map<declaringLibraryUri, declaredName>, so we know what is imported into the target library, but not how. This is important for re-exports. For example both "material.dart" and "cupertino.dart" re-export Widget, and we need to know which suggestion set to use for Widget.

Thanks @scheglov! I got confused and missed that one library can re-export multiple names.

alexander-doroshko · 2019-06-03T08:17:02Z

Please add a test to the PR.

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java

scheglov · 2019-06-03T18:19:53Z

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java

+                     Map<String, Set<String>> importedLibrary = entry.getValue();
+                     // Checks whether any of the libraries exported by this import declares the same name as this suggestion.
+                     if (importedLibrary.values().stream().anyMatch(names -> names.contains(suggestion.getLabel()))) {
+                       declaringLibraries.add(importedLibraryUri);


declaringLibraries -> importedLibraries like "libraries which import provide the suggestion".

scheglov · 2019-06-03T18:26:55Z

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java

+                     String importedLibraryUri = entry.getKey();
+                     Map<String, Set<String>> importedLibrary = entry.getValue();
+                     // Checks whether any of the libraries exported by this import declares the same name as this suggestion.
+                     if (importedLibrary.values().stream().anyMatch(names -> names.contains(suggestion.getLabel()))) {


This is not precise.
We need to use the URI and the name of the suggestion, something like importedLibrary[suggestion.declaringLibraryUri][suggestion.name]. However it seems that we don't have URI in AvailableSuggestion. So, we might have to extend AvailableSuggestionSet to provide URIs.

Good catch! We do have uri on AvailableSuggestionSet (not on AvailableSuggestion) so I used that.

lambdabaa · 2019-06-03T21:53:41Z

@alexander-doroshko I wrote a unit test but wasn't able to run it since my intellij-community is broken after updating. If you can check it as part of merging, I would appreciate the help!

alexander-doroshko · 2019-06-04T09:39:52Z

intellij-community is broken after updating

What kind of problems do you have? First of all, make sure that you use git pull/commit/push right from the IDE, not from the command line. IDE integration works transparently with multiple repositories and also, much fewer chances to commit red code or not-related files or to forget to commit related files, etc. There are no reasons not to use IntelliJ's Git integration.

As for the test, I guess you need to use configureByFiles() as you need a multi-file project setup.

alexander-doroshko · 2019-06-04T09:41:33Z

@scheglov thank you for the review! Does the PR LGTY in its current state?

scheglov · 2019-06-04T16:58:20Z

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java

+                     String importedLibraryUri = entry.getKey();
+                     Map<String, Set<String>> importedLibrary = entry.getValue();
+                     // Checks whether any of the libraries exported by this import declares the same name as this suggestion.
+                     if (importedLibrary.entrySet().stream().anyMatch(


I think this is very inefficient.
We don't use the fact that this is a map, and just iterate over entries, checking the key.
We should rewrite it to something like importedElements[suggestion.uri].contains(suggestion.label).

This is especially scary because we do this for every suggestion.

And I need to add uri to AvailableSuggestion.

Whoops, thanks @scheglov! Updated.

scheglov · 2019-06-05T17:35:53Z

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java

-                     // Checks whether any of the libraries exported by this import declares the same name as this suggestion.
-                     if (importedLibrary.entrySet().stream().anyMatch(
-                         importEntry -> doesImportSuggestion(importEntry, suggestionSet.getUri(), suggestion.getLabel()))) {
+                     // TODO: Use individual suggestion uris instead of suggestion set uri.


DAS does provide AvailableSuggestion.declaringLibraryUri with https://dart-review.googlesource.com/c/sdk/+/104808
Is there something that prevents us from using it in this PR?

Nope, I just pushed this before you landed that change :). Updating now!

alexander-doroshko · 2019-06-06T17:49:40Z

@lambdabaa, compilation broken. And please check that the test works.

lambdabaa · 2019-06-06T23:10:26Z

@lambdabaa, compilation broken. And please check that the test works.

Sorry, fixed the compilation issue!

alexander-doroshko · 2019-06-07T15:15:23Z

@lambdabaa I'm getting NPEs with SDK 2.3 or older. We still support SDK 1.12 as a minimal version.

lambdabaa · 2019-06-07T16:54:27Z

@lambdabaa I'm getting NPEs with SDK 2.3 or older. We still support SDK 1.12 as a minimal version.

@alexander-doroshko Thanks for the heads up! I went ahead and updated the logic that reads AvailableSuggestion and CompletionSuggestion to not assume the existence of the new declaringLibraryUri and libraryFile fields.

lambdabaa · 2019-06-07T16:57:55Z

And please check that the test works.

Something strange is happening in the test that I am not fully understanding. This works outside of the context of a test, but the ExistingImports notification is different in test mode (missing the symbols re-exported from ExistingImportLibrary.dart). Any ideas about how to fix it @alexander-doroshko?

alexander-doroshko · 2019-06-07T17:05:55Z

ExistingImports notification is different in test mode

Probably because there's only one file in the actual test project (see doTest() implementation).
See my first comment about the test:

As for the test, I guess you need to use configureByFiles() as you need a multi-file project setup.

lambdabaa · 2019-06-07T18:09:30Z

ExistingImports notification is different in test mode

Probably because there's only one file in the actual test project (see doTest() implementation).
See my first comment about the test:

As for the test, I guess you need to use configureByFiles() as you need a multi-file project setup.

Ah that was it, thanks @alexander-doroshko!

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java

alexander-doroshko · 2019-06-10T15:27:53Z

I tested the feature with today's nightly SDK. Completing in an empty file: I noticed that we've lost some classes from dart:async from the completion list, for example, SynchronousStreamController. I guess it's because dart:core exports 2 classes from dart:async.
Will changing return to continue in DartServerCompletionContributor be a correct fix?

BTW, it sounds like a good scenario for a one more test!

Dart/src/com/jetbrains/lang/dart/analyzer/DartAnalysisServerService.java

alexander-doroshko · 2019-06-10T15:44:29Z

DartServerCompletionTest#testFunctionAfterShow falis for me with this PR applied (against today's nightly SDK). Console contains exception:

java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
	at java.util.ArrayList.rangeCheck(ArrayList.java:657)
	at java.util.ArrayList.get(ArrayList.java:433)
	at com.google.dart.server.internal.remote.processor.NotificationCompletionExistingImportsProcessor.lambda$buildUriNamesMap$4(NotificationCompletionExistingImportsProcessor.java:39)
	at java.lang.Iterable.forEach(Iterable.java:75)
	at com.google.dart.server.internal.remote.processor.NotificationCompletionExistingImportsProcessor.buildUriNamesMap(NotificationCompletionExistingImportsProcessor.java:37)
	at com.google.dart.server.internal.remote.processor.NotificationCompletionExistingImportsProcessor.process(NotificationCompletionExistingImportsProcessor.java:22)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl.processNotification(RemoteAnalysisServerImpl.java:675)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl.processResponse(RemoteAnalysisServerImpl.java:707)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl.access$600(RemoteAnalysisServerImpl.java:48)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl$ServerResponseReaderThread.run(RemoteAnalysisServerImpl.java:1127)

Do you get the same?

Dart/testSrc/com/jetbrains/dart/analysisServer/DartServerCompletionTest.java

lambdabaa · 2019-06-12T22:08:17Z

DartServerCompletionTest#testFunctionAfterShow falis for me with this PR applied (against today's nightly SDK). Console contains exception:

java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
	at java.util.ArrayList.rangeCheck(ArrayList.java:657)
	at java.util.ArrayList.get(ArrayList.java:433)
	at com.google.dart.server.internal.remote.processor.NotificationCompletionExistingImportsProcessor.lambda$buildUriNamesMap$4(NotificationCompletionExistingImportsProcessor.java:39)
	at java.lang.Iterable.forEach(Iterable.java:75)
	at com.google.dart.server.internal.remote.processor.NotificationCompletionExistingImportsProcessor.buildUriNamesMap(NotificationCompletionExistingImportsProcessor.java:37)
	at com.google.dart.server.internal.remote.processor.NotificationCompletionExistingImportsProcessor.process(NotificationCompletionExistingImportsProcessor.java:22)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl.processNotification(RemoteAnalysisServerImpl.java:675)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl.processResponse(RemoteAnalysisServerImpl.java:707)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl.access$600(RemoteAnalysisServerImpl.java:48)
	at com.google.dart.server.internal.remote.RemoteAnalysisServerImpl$ServerResponseReaderThread.run(RemoteAnalysisServerImpl.java:1127)

Do you get the same?

Fixed!

lambdabaa · 2019-06-12T22:08:48Z

Will changing return to continue in DartServerCompletionContributor be a correct fix?

Yes that's correct, done!

lambdabaa · 2019-06-15T13:10:41Z

@alexander-doroshko Gentle ping :)

alexander-doroshko · 2019-06-17T13:53:30Z

Sorry for delay, I had a few days off. The PR is merged now, thanks! The commit will appear here shortly.

maRci002 · 2019-08-20T21:13:58Z

@lambdabaa Is there any option to disable this feature in the IDE?
I get strange completion ordering so I would like to fall back to old completion, in VS Code I am able to control it with "Auto Import Completions"

alexander-doroshko · 2019-08-21T05:17:32Z

@maRci002, there's no option to disable the feature. I think we'd rather fix strange completion ordering than add an option. Can you share the details of the problem?

maRci002 · 2019-08-21T19:38:39Z

@alexander-doroshko thanks for quick answer.

My config:

Android Studio 3.5 Build #AI-191.8026.42.35.5791312, built on August 9, 2019
JRE: 1.8.0_202-release-1483-b03 amd64
JVM: OpenJDK 64-Bit Server VM by JetBrains s.r.o
Windows 10 10.0
Flutter plugin: v38.2.3
Dart plugin: v191.8423
IntelliJ version: according to developer.android IntelliJ IDEA 2019.1

My problem is that when a constructor (let's say new ListView.builder(...)) waits for specific type of arguments then the IDE should consider type matching while using autocomplete feature.

Here's a simplified example of this issue, the padding argument shoud implemet EdgeInsetsGeometry abstract class but autocomplete shows me random methods / keywords...:

Autocomplete should know there are some classes which implement EdgeInsetsGeometry abstract class like: class EdgeInsets extends EdgeInsetsGeometry and should offer them as first results.

So the expected behaviour should be like this (this is old behaviour when completion.existingImports wasn't used):

Moreover the problem exists in simple situations for example argument should be a String

So to sum up, I think this feature significantly slows down development time because I don't want to manually check type hierarchy every single time. 3 months ago I made a VS Code issue and @DanTup figured out the problem occours when completion.existingImports event is used to search completion results, however in VS Code there is an option to ignore this feature via "dart.autoImportCompletions": false in settings.json.

I think I should have created a new Issue on youtrack but there wasn't option to create new account, and I am using Android Studio, but I think the problem occours in every Intellij based IDE which using the newest Dart plugin. (confirm?)

I really appreciate that you try to satisfy the community's request, so keep it up 💪. I also hope you can fix this problem or at least give an option to disable this feature while working on fix.

DanTup · 2019-08-22T08:03:51Z

Possibly we should move Dart-Code/Dart-Code#1742 to the SDK repo. I do think the ranking is worse with this enabled, though I think it's a known trade-off (see Dart-Code/Dart-Code#1742 (comment)).

That said, VS Code ignores our sorting as soon as the user starts typing anyway (microsoft/vscode#79516), so it's possible that even if fixed in the server it won't be perfect in VS Code because the chances of there being other items VS Code thinks are "more relevant" are increased with the unimported items 😞

maRci002 · 2019-08-22T08:45:36Z

I manually downgraded Dart plugin to 191.8026.36, this is the last update which doesn't include this completion.existingImports feature, but the problem still exists. Maybe Android Studio mess up completion order so I should downgrade it to 3.4.2 which is based on (IntelliJ IDEA 2018.3.4) and using Dart plugin 183.*.

maRci002 · 2019-08-22T10:19:59Z

I can confirm

Android Studio 3.4.2 Build #AI-183.6156.11.34.5692245, built on June 27, 2019
Flutter plugin: v38.2.1
Dart plugin: v183.6270
Works fine for me.

So for example if I write StreamController sc = then I can manually import 'dart:async'; or manually trigger Quick Fix -> Import library 'dart:async' and here is the magic: if I trigger autocomplete after assignment then IDE shows me StreamController and StreamController.broadcast contructors 😍

However in newer Android Studio 3.5 the manual import isn't required, but if I trigger autocomplete after assignment then the IDE shows me random stuffs even if I explicilty say I want to search after constructors with new keyword which makes me going crazy

alexander-doroshko · 2019-08-22T13:40:51Z

a new Issue on youtrack but there wasn't option to create new account

There definitely is. You can log in using JetBrains account, as well as GitHub, Google account and more. But at the moment there's no need for YouTrack issue, I think we first need to continue the discussion with the Analysis Server team.

alexander-doroshko · 2019-08-22T13:42:46Z

@scheglov, are there new ideas about improving type-aware completion sorting? Is there anything we are not doing right on the Dart plugin end?

scheglov · 2019-08-26T19:42:59Z

There are several problems here.

DAS does not set relevanceTags for available declarations of constructors. https://dart-review.googlesource.com/c/sdk/+/114563 will fix this.
We don't set includedSuggestionRelevanceTags for variable declarations (only for RHS of assignments). I will fix this.
The relevanceBoost for matching includedSuggestionRelevanceTags is only 10. This is enough to raise matching declaration on top of other available declarations, but not enough to override other completion suggestions. I don't know yet what to do with this, will need to talk to people who have background about how relevance for other suggestions is selected.

So, you get something like this:

[email protected] Bug: JetBrains/intellij-plugins#665 (comment) Change-Id: I347822b84311ed2b94171231597aeceebd900015 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114563 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

maRci002 · 2019-08-26T22:09:13Z

DAS does not set relevanceTags for available declarations of constructors. https://dart-review.googlesource.com/c/sdk/+/114563 will fix this.

I would like to highlight

So for example if I write StreamController sc = and if I trigger autocomplete after assignment then IDE shows me StreamController and StreamController.broadcast contructors 😍

this is not true if I write StreamController<int> sc = then autocomplete fails, however StreamController<dynamic> sc = works fine.

scheglov · 2019-08-29T19:41:54Z

With https://dart-review.googlesource.com/c/sdk/+/114969

[email protected] Bug: JetBrains/intellij-plugins#665 (comment) Change-Id: I254639a1582ca5691e96e15612bd2078c65e0b41 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114969 Reviewed-by: Brian Wilkerson <[email protected]> Commit-Queue: Konstantin Shcheglov <[email protected]>

maRci002 · 2019-09-22T15:10:39Z

Thanks @scheglov your work so far, today I was able to test this feature on flutter dev channel.

Flutter (Channel dev, v1.10.5)
Dart version 2.6.0 (build 2.6.0-dev.1.0 cb80ea7ba9)
Android Studio 3.5
- Flutter plugin: v39.0.3
- Dart plugin: v191.8423

If I create a new empty dart file like you did a test.dart, then it works fine.

However I created a new project flutter create my_test with some library global variable and wrapped the Column widget with Container/AspectRatio/Opacity for illustration (ne need to create new project, just fill test.dart with some classes / global variables / some code). So if I trigger completion then relevance for global variables/classes are heigh.

When I trigger autocomplete in a local method then global variables/classes relevance is still boosted as well as local variables (_counter / widget)/methods (toString()). I like the way autocomplete shows EdgeInsets.all( ) constructor for padding but shows only this one named constructor. Since EdgeInsetsGeometry is an abstract class maybe it shouldn't be boosted until there is no support for anonymous classes/object literals.

Use completion.existingImports to filter completion suggestions

56e019e

scheglov suggested changes Jun 1, 2019

View reviewed changes

alexander-doroshko self-assigned this Jun 3, 2019

scheglov reviewed Jun 3, 2019

View reviewed changes

lambdabaa force-pushed the lambdabaa/completion-existing-imports branch from 53f014b to 95f9049 Compare June 3, 2019 20:25

Respond to Konstantin's feedback

6d8ba07

lambdabaa force-pushed the lambdabaa/completion-existing-imports branch from 95f9049 to 6d8ba07 Compare June 3, 2019 20:43

Add existingImports completion unit test

bdc6150

scheglov suggested changes Jun 4, 2019

View reviewed changes

scheglov mentioned this pull request Jun 4, 2019

Code completion ordering is wrong when autoImportCompletions is enabled Dart-Code/Dart-Code#1742

Closed

Don't iterate through map, just do a lookup

456b5ed

scheglov reviewed Jun 5, 2019

View reviewed changes

Update to use AvailableSuggestion.declaringLibraryUri

a66ed32

scheglov approved these changes Jun 5, 2019

View reviewed changes

Fix compilation issue

b6c1046

Defend against NPEs when talking to older servers

a8ded52

Fix unit test

eaaa49c

alexander-doroshko reviewed Jun 10, 2019

View reviewed changes

Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java Outdated Show resolved Hide resolved

alexander-doroshko reviewed Jun 10, 2019

View reviewed changes

Dart/src/com/jetbrains/lang/dart/analyzer/DartAnalysisServerService.java Outdated Show resolved Hide resolved

alexander-doroshko reviewed Jun 10, 2019

View reviewed changes

Dart/testSrc/com/jetbrains/dart/analysisServer/DartServerCompletionTest.java Show resolved Hide resolved

lambdabaa added 3 commits June 12, 2019 11:11

Make libraryFile @nullable everywhere

ae9d79f

Make sure that there is only one Process in the completion list

5e3f56e

Lookup for uri directly into strings

bdc5bf8

alexander-doroshko added the Merged label Jun 18, 2019

SergeyZh closed this in 152dfe8 Jul 2, 2019

maRci002 mentioned this pull request Oct 13, 2019

Ranking/sorting of code completion is worse with suggestion sets dart-lang/sdk#38739

Closed

Use completion.existingImports to filter completion suggestions #665

Use completion.existingImports to filter completion suggestions #665

Conversation

lambdabaa commented Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-doroshko commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdabaa commented Jun 3, 2019

alexander-doroshko commented Jun 4, 2019

alexander-doroshko commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-doroshko commented Jun 6, 2019

lambdabaa commented Jun 6, 2019

alexander-doroshko commented Jun 7, 2019

lambdabaa commented Jun 7, 2019

lambdabaa commented Jun 7, 2019

alexander-doroshko commented Jun 7, 2019

lambdabaa commented Jun 7, 2019

alexander-doroshko commented Jun 10, 2019

alexander-doroshko commented Jun 10, 2019

lambdabaa commented Jun 12, 2019

lambdabaa commented Jun 12, 2019

lambdabaa commented Jun 15, 2019

alexander-doroshko commented Jun 17, 2019

maRci002 commented Aug 20, 2019

alexander-doroshko commented Aug 21, 2019

maRci002 commented Aug 21, 2019

DanTup commented Aug 22, 2019

maRci002 commented Aug 22, 2019

maRci002 commented Aug 22, 2019

alexander-doroshko commented Aug 22, 2019

alexander-doroshko commented Aug 22, 2019

scheglov commented Aug 26, 2019

maRci002 commented Aug 26, 2019

scheglov commented Aug 29, 2019

maRci002 commented Sep 22, 2019

lambdabaa commented Jun 1, 2019 •

edited

Loading