Skip to content

Code completing a named parameter doesn't show me the types that could go there #998

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

Closed
sethladd opened this issue May 6, 2017 · 36 comments

Comments

@sethladd
Copy link

sethladd commented May 6, 2017

I wonder if we could show all the types of Decoration we have, when trying to code complete decoration: in a Container?

Here's what we see now:

capture10

Those options aren't useful, unfortunately. However, since we know that decoration: takes a Decoration, could we show the options for Decoration here? (e.g. BoxDecoration)

I'm still learning the Flutter APIs, and bouncing back and forth between API docs and my IDE is less than efficient. I want to code complete my way to success! :)

Thanks!

@devoncarew
Copy link
Member

/cc @danrubel, re: adding type info in for named parameters (perhaps just passing Elements in for those completion items would do it?)

@sethladd
Copy link
Author

sethladd commented May 8, 2017

Because Flutter loves to use named parameters in constructors, this would be a big win. Thank you! :)

@pq
Copy link
Contributor

pq commented May 10, 2017

We could do some better filtering (false doesn't make sense for example) but really we want the user to type new... and once they do, we want really good completions. Here we could do some filtering and possibly sorting based on good bets.

cc @lukechurch

@pq
Copy link
Contributor

pq commented May 10, 2017

AI @pq: look at what proposals we get from new.

@sethladd
Copy link
Author

why do I need to type new? Sometimes I might want const. Or, sometimes it's a static or enum that goes here.

@pq
Copy link
Contributor

pq commented May 10, 2017

Well, new (or const) are a good cue (and currently required to get the completion engine thinking that you're in a creation mind-set). It's an interesting question whether we should get more opinionated here.

@sethladd : in an ideal world what completions would you like to see? All extenders/implementers of Decoration?

@danrubel : thoughts?

@sethladd
Copy link
Author

All extenders/implementers of Decoration?

Yup! Even if they come from statics or enums available to me.

@pq
Copy link
Contributor

pq commented May 11, 2017

From chatting with Dan, this could be improved with suggestions based on type matching. Infrastructure is in place and the issue can be solved generally. That said, will investigate with the decorations case as a driving force.

@pq
Copy link
Contributor

pq commented May 11, 2017

@danrubel to investigate.

@sethladd
Copy link
Author

Great, thanks!

@mit-mit
Copy link
Member

mit-mit commented Jan 4, 2018

@pq is this an analyzer issue or an IJ issue?

@pq
Copy link
Contributor

pq commented Jan 4, 2018

Analysis server.

cc @danrubel

@scheglov
Copy link
Contributor

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

Without the change:
image

With the change:
image

@scheglov scheglov assigned scheglov and unassigned danrubel Feb 21, 2018
@sethladd
Copy link
Author

Nice improvement!

What happens if I try to complete without first typing "new" or "const" ?

For example:

padding: <code complete>

@scheglov
Copy link
Contributor

No changes here.
Do you have ideas?

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 21, 2018
[email protected], [email protected]

Bug: flutter/flutter-intellij#998
Change-Id: I88eb345bfa7f0a65d135e5fdcb443d5dadd695c5
Reviewed-on: https://dart-review.googlesource.com/42882
Reviewed-by: Dan Rubel <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
@sethladd
Copy link
Author

As long as we don't do what the screenshot in #998 (comment) is doing, then I'm happy :)

(Put another way, if I try to code complete immediately after a named parameter, show me ONLY what I can put there (both variables whose types match, as well as constructor calls that apply), don't show me variables whose types don't even match.)

@scheglov
Copy link
Contributor

It always raises a question of "transitive applicability".
In the code below b is not of type A, but it has something of type A.
If we don't help users to complete b (a long name in real cases), it would be not nice.

class A {}
class B {
  A a;
}

void foo({A a}) {}

main(B b) {
  foo(a: ^);
}

@sethladd
Copy link
Author

That's fair. I assume that we won't show "true" or "false" as options in your example?

@bwilkerson
Copy link

We will. Because we don't look at the type of the thing being proposed to ensure that there is a chain of invocations that will get you the type of thing you need. (Hence, we can't know that there isn't.)

@sethladd
Copy link
Author

Oh, bummer. That was the original request. :)

In a world with optional new/const, we can expect that developers won't first type 'new' or 'const' in order to trigger the useful code completions.

Are there other completion models here that assume new and const won't be used, but also return the more likely/useful options?

true/false/const/etc just aren't useful :(

@danrubel
Copy link

danrubel commented Feb 22, 2018

IMO it's all about the "relevance" of the suggestion. In this situation, types that match should have a higher relevance than suggestions that do not, and the IDE should display suggestions ranked by the relevance returned by the analysis server... more relevant suggestions first.

@bwilkerson
Copy link

Well, const might be useful if it isn't in a constant context so that the user needs to be explicit.

But that aside, there are two things I can think of that we could do. The first is what Dan said: to prioritize suggestions that already have the right type over those that don't, so that less useful ones show up lower in the list. I do think we should do that.

The second is to special case true and false and just don't show them unless the target type is bool, Object or dynamic. I dislike special casing things, and I suspect that if we get the relevance right we won't need to.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 22, 2018
So, we don't suggest EdgeInsetsGeometry for 'padding: ', even though
it is the type of the parameters. But we still suggest its subclasses
EdgeInsets and EdgeInsetsDirectional.

[email protected], [email protected]

Bug: flutter/flutter-intellij#998
Change-Id: If2d7ba550ec76e4f66cb81e1346466b5f1a30464
Reviewed-on: https://dart-review.googlesource.com/42888
Reviewed-by: Brian Wilkerson <[email protected]>
@devoncarew
Copy link
Member

In this situation, types that match should have a higher relevance than suggestions that do not

This sounds like a pretty elegant solution - we'll still show all correct suggestions (and allow users to compete through a few expressions to their end goal) but will show the most immediately useful suggestions first.

@mit-mit
Copy link
Member

mit-mit commented Feb 22, 2018

+1 for sorting matching types higher!

@scheglov
Copy link
Contributor

No prioritizing matching types yet, but this CL will include constructor invocations for implicit creation. So, you can complete padding: ^ to EdgeInsets.all().

image

@sethladd
Copy link
Author

Nice! I see this screenshot starts with "EdIn" ... Does the same result happen if you code complete immediately after the : ?

@scheglov
Copy link
Contributor

Yes, I actually activated completion right after : and then typed EdIn to filter the list. Alternatively I could scroll it down to these items.

@sethladd
Copy link
Author

Well then, double nice! :) Thank you!

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 22, 2018
[email protected]

Bug: flutter/flutter-intellij#998
Change-Id: I43270f47fb43402d1a9e0f7d9814494ffd83bb5e
Reviewed-on: https://dart-review.googlesource.com/43080
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
@scheglov
Copy link
Contributor

OK, we can prioritize completion suggestions in Dart Analysis Server.

Unfortunately IntelliJ has its own opinions about completions and their relevance.
I had to make some hacks into IntelliJ code itself to make it closer to the behavior I would like.
Actual change of IntelliJ completion behavior might happen, or not, I don't know. @alexander-doroshko

Here, nothing is typed yet, but the suggestions with compatible types are on the top, and sorted alphabetically.

image

@sethladd
Copy link
Author

Wow! The top results look very relevant and useful.

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 23, 2018
Exact type get the largest relevance boost, subtypes a smaller boost,
and the rest has default relevance for its element kind.

[email protected], [email protected]

Bug: flutter/flutter-intellij#998
Change-Id: Ibb38caedc314e9cf476de2116aeb61bcec8c8a08
Reviewed-on: https://dart-review.googlesource.com/43263
Commit-Queue: Konstantin Shcheglov <[email protected]>
Reviewed-by: Brian Wilkerson <[email protected]>
@mit-mit
Copy link
Member

mit-mit commented Feb 23, 2018

Very nice!

@scheglov
Copy link
Contributor

scheglov commented Feb 23, 2018

And one more fix, for completing imported enums.

So, you can complete Column(crossAxisAlignment: ^) and get all of the CrossAxisAlignment constants right away for selection. Or you can start typing center and by the time when you typed first letters, you get several completion suggestions. There are several enums with center, but because we give compatible types higher relevance, the correct one is always the first.

Don't pay attention to CrossAxisAlignment in the middle, it is still the same hiccup of IntelliJ's attempt to re-prioritize our already prioritized suggestions. Hopefully this can be disabled, Analysis Server really knows better about types.

image

@sethladd
Copy link
Author

@scheglov yes yes yes yes yes you are on 🔥

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 23, 2018
So, you can complete `Column(crossAxisAlignment: ^)` and get all of
the CrossAxisAlignment right away for selection. Or you can start
typing `center` and by the time when you typed first letters, you get
several completion suggestions. There are several enums with `center`,
but because we give compatible types higher relevance, the correct
one is always the first.

[email protected], [email protected]

Bug: flutter/flutter-intellij#998
Change-Id: I6c2082668c6b25bf23e070915b5df03081ff0d5c
Reviewed-on: https://dart-review.googlesource.com/43485
Reviewed-by: Brian Wilkerson <[email protected]>
Commit-Queue: Konstantin Shcheglov <[email protected]>
@scheglov
Copy link
Contributor

I consider this issue fixed now, at least Analysis Server side of it.
We now prioritize useful suggestions.
If there is anything else, please open new issues.

@sethladd
Copy link
Author

sethladd commented Mar 5, 2018

Thank you very much!

@InMatrix
Copy link

InMatrix commented Mar 5, 2018

This is awesome! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants