Flutter-intellij: Auto assist method override suggestions, such as initState

[branflake2267]

When I know there is a method I could override, such as initState, I'd like to start typing in ini and hit control space for suggestions and ask for method override options, and hit enter to write the ast.

Steps to Reproduce

1. Start typing in ini.
2. Hit control-space for auto suggestions
3. Nothing comes up yet.

Version info

[✓] Flutter (on Mac OS X 10.12.5 16F73, locale en-US, channel master)
    • Flutter at /Users/branflake2267/git/flutter
    • Framework revision 28fd54c116 (35 hours ago), 2017-06-16 23:54:27 -0700
    • Engine revision 784e975672
    • Tools Dart version 1.24.0-dev.6.7

[✓] Android toolchain - develop for Android devices (Android SDK 26.0.0)
    • Android SDK at /Users/branflake2267/Library/Android/sdk
    • Platform android-26, build-tools 26.0.0
    • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java
    • Java version OpenJDK Runtime Environment (build 1.8.0_112-release-b06)

[✓] iOS toolchain - develop for iOS devices (Xcode 8.3.3)
    • Xcode at /Applications/Xcode.app/Contents/Developer
    • Xcode 8.3.3, Build version 8E3004b
    • ios-deploy 1.9.1
    • CocoaPods version 1.2.1

[✓] Android Studio (version 2.3)
    • Android Studio at /Applications/Android Studio.app/Contents
    • Gradle version 3.2
    • Java version OpenJDK Runtime Environment (build 1.8.0_112-release-b06)

[✓] IntelliJ IDEA Community Edition (version 2017.1.4)
    • Flutter plugin version 14.0
    • Dart plugin version 171.4694.29

[✓] IntelliJ IDEA Ultimate Edition (version 2017.1.4)
    • Flutter plugin version 14.0
    • Dart plugin version 171.4694.29

[✓] Connected devices
    • iPhone 7 • 458BC4FE-8205-4369-8718-193B9FC88C8B • ios • iOS 10.3 (simulator)

[pq]

I believe that the standard way to do that in IntelliJ vernacular is invoking the "Generate... (CMD+N)" action or more directly with ^O:

Having said that, I frequently find myself doing the same thing. I'm guessing this was the eclipse way...

cc @alexander-doroshko

[branflake2267]

Yeah, works like that in Eclipse, but it's the same in Android Studio and IDEA.

Here I'm trying to override my superclass method in Android Studio. Same with IDEA and a Java project.

[bwilkerson]

We have the code to perform this, and server can send it as a completion suggestion (it might be disabled at the moment), but it doesn't show up in IntelliJ. I don't remember what the problem was, but I think we're about 80% of the way to having this wired up.

[pq]

@branflake2267 wrote:

Yeah, works like that in Eclipse, but it's the same in Android Studio and IDEA.

Ah! Well if it doesn't pull against the IDEA-way, I think we should absolutely implement it.

@alexander-doroshko : is this something we should implement on our side as a code completion or on the client (IDEA) side? I was assuming it would be just like any other server-provided completion but @bwilkerson's comment makes me want to double check.

[bwilkerson]

IMO, it should definitely be implemented in server. I think the question is: should this be done via code completion, or some other API?

[pq]

@bwilkerson 👍 . That's a better way of putting it. I'm just wondering if it needs to be handled specially and whether we need to make any client-side changes.

[pq]

As of https://dart-review.googlesource.com/c/sdk/+/38143, this suggestion should be coming from server. So far I'm not having any luck picking it up on the IDEA side however. @alexander-doroshko : I wonder if you have any ideas? I don't see anything suspicious in DartServerCompletionContributor but maybe I'm looking in the wrong places?

[pq]

@bwilkerson : Doing a little snooping through the server logs, I'm not sure I'm seeing these completions getting sent. Is it possible that there's one more piece on the server side?

[bwilkerson]

Have you uncommented line 95 in pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart? I didn't think there was any point in taking the time to compute the completions until we knew we were going to get them plumbed through.

[pq]

Aha! I had not. Thanks for the tip Brian!

[pq]

Hmmm. Still no luck. Maybe we can scour the logs together tomorrow.

[alexander-doroshko]

If you see that server sends completion for this case but it doesn't show up in the IDE please ping me once again with STR.

[bwilkerson]

Thanks!

We're seeing the suggestion, but working to improve the UI. For example, we generate an @override annotation before the member, and it shows up in the label in the drop down. An annotation is generated for Java as well, but it doesn't show up in the label.We think it's better for it to not be there, and we know it's possible to hide it, but haven't figured out for sure how that's happening (though we have a guess).

[alexander-doroshko]

An item in the completion list is rendered in this method. We have pretty much full control over what and how is rendered, thanks to the class LookupElementBuilder. So I'm ready to tune IDE side if needed.

[pq]

Corresponding Jetbrains issue: https://youtrack.jetbrains.com/issue/WEB-31130.

[pq]

@DanTup : I've got a proposed server API change up for discussion here: https://dart-review.googlesource.com/c/sdk/+/40101. It'd be great to know if this makes sense from the VSCode perspective. Input welcome!

[DanTup]

@pq Sounds good to me. Currently we mangle the text (both for display and insertion) to support things like argument placeholders and stripping the trailing commas from display. With this we may want to tweak that (it may also be worth seeing if any of that makes sense to be done in the server now it has its own display label? Our logic is here).

I think it's a good idea for us to push updates to clients before the server starts sending these to ensure we don't end up with weird labels in the completion list (and we don't choke on the new type). If you let me know once this field/type are agreed, I can push it out in a patch.

[bwilkerson]

We definitely want to make sure clients are ready for the new data. On the other hand, it's a new field that existing clients will presumably ignore, so it ought to be harmless (sounds like famous last words :-).

[DanTup]

If I understand correctly, although the displayText field is new the existing completion text field is going to go from one-line to two-lines with an @override prefix? If so, that might look really wonky in the existing completion list; so I think clients need to move over to displayText before the change to the completion text is made?

[bwilkerson]

Or handle the case where the completion contains an end-of-line marker. What does DartCode do in this case today? (We also need to consider the case where a user has not updated to the latest version of DartCode but does update their SDK.)

[DanTup]

Currently I'm not sure if we do anything - are there any completions that are already multi-line? (It's possible if so we're just passing them to Code and maybe it's handling them, or chopping them).

We also need to consider the case where a user has not updated to the latest version of DartCode but does update their SDK

With Code and extensions both auto-updating I'm not too concerned about this, as long as there's at least a little time between the Dart Code update and the SDK appearing (normally it's stable SDK releases I care about, but because Flutter is taking dev releases quite quickly, the changes often end up in users hands quicker).

I can add handling for a displayText field very easily/quickly, so as soon as we're sure that's what it'll be called, I'm happy to push a get a small patch out (I can also do some testing of multiline completions to see what happens).

[bwilkerson]

Phil should have the CL landed later today or tomorrow, and I'm fairly sure the field name won't change.

By the way, we have a framework for adding generators that can generate code that matches the spec. We currently generate both Dart and Java, and I wouldn't have a problem having one added that generates TS. It would ensure that your code is never out of sync with the latest server.

[DanTup]

I do have code that generates from the spec (one script to generate types, the other to generate the class wrapping all the methods... one is Dart but the other is still C#); it's kinda hacky but works. If you have something and it's not hard to extend, it might be easier to switch to than to finish porting what I have in C# (which is even more of mess than the Dart one! so bad in fact the repo is private =))

[bwilkerson]

The code for the generator is in sdk/pkg/analyzer_plugin/tool/spec. I don't know whether switching to it would be easier, but thought you might like to know about it.

[pq]

@DanTup, @alexander-doroshko :

as of https://github.com/dart-lang/sdk/commit/35221ce1fc95b927b49cd1e3d98818a5c460fc71, you should get sensible displayText back for override completions.

If you want to experiment, be sure and uncomment line 95 in pkg/analysis_server/lib/src/services/completion/dart/completion_manager.dart to get the contributor registered.

[DanTup]

Cool; I don't seem to be able to run the analyzer proplerly locally atm (analysis works, completions etc. return nothing, and all the tests fail).. I'll see if I can get this fixed later today; I suspect it might be related to gclient sync failing on the Visual Studio step (I have a pre-release version of VS 2017 installed) and maybe not running something later that's required (I hope so, it's the only idea I have!).

[DanTup]

Didn't get to this today, but it's first on my list for tomorrow to install VS and see if I can get the analyzer running from source correctly (or debug if it's still not working). If you can give me an example snippet of json that includes an override completion, I could also just rig it in the extension for testing to see how we currently behave.

[alexander-doroshko]

@pq How about enabling it in the SDK so that I could play with nightly build instead of setting up SDK from sources?

[pq]

Absolutely. I'll do that today. 👍

[DanTup]

There's a nightly SDK build?!

[DanTup]

I'm confused - those just look like tags in the repo (eg. source, not builds), and also not nightly?

[alexander-doroshko]

http://gsdview.appspot.com/dart-archive/channels/be/raw/latest/sdk/

[pq]

Sorry for the confusion @DanTup! And thanks @alexander-doroshko for the correction. 👍

[pq]

Enabled in: https://github.com/dart-lang/sdk/commit/be4040214cdf940236cab67f7c279cc32cd6152d.

[DanTup]

Aha, awesome! This'll help me a bunch; I've been waiting for Dev SDKs and faffing with source for so many things in the past!

[alexander-doroshko]

And when they enter cherry-pick mode (close to release) you may get nightly *-dev builds from http://gsdview.appspot.com/dart-archive/channels/dev/raw/latest/sdk/

[DanTup]

Enabled in: dart-lang/sdk@be40402.

Slight tangent, but... https://github.com/flutter/flutter/pull/14610 suggests that Flutter is going to be taking snapshots of the Dart SDK that might not align with dev/stable releases. Does this mean in theory that changes like this might suddenly end up in flutter users hands? (eg. if Flutter decided to update the SDK in the engine now). If so, there's a risk that we're going to have a lot less time between analyzer changes being made and ending up at users in order to make any required changes to editors (and get them released, and everyone updated)?

[DanTup]

@pq I seem to get getting no completions back at all in that nightly:

[09:14:43]: Spawning M:\Apps\Dart\Dart-nightly\bin\dart.exe with args ["M:\\Apps\\Dart\\Dart-nightly\\bin\\snapshots\\analysis_server.dart.snapshot","--client-id=Dart-Code.dart-code","--client-version=2.9.0-dev"]

No matter where I invoke completion, I get:

[09:15:03]: <== {"event":"completion.results","params":{"id":"0","replacementOffset":215,"replacementLength":0,"results":[],"isLast":true}}

[DanTup]

Here's a log:

analyzer.instrum.txt

[alexander-doroshko]

Nightly SDK works fine to me so I was able to use CompletionSuggestionKind.OVERRIDE and CompletionSuggestion.getDisplayText().

[DanTup]

Cool, thanks for confirming - I'll take another look this afternoon then!

[DanTup]

Works on Mac for me which will do for now; though I can't figure out why I get no responses on Windows :/

Unfortunately this is quite broken in Code - the user only sees the first line, so we end up with this:

@pq I'm gonna need to push a release out that can handle displayText before this goes out. Do you have an idea of when you want to ship it (bearing in mind that Flutter is taking cuts fairly frequently)? I wasn't planning on another release until the end of the month but probably that's a pain for you?

[alexander-doroshko]

@pq: an idea for improvement: check whether the overridden method has @mustCallSuper annotation and add the corresponding super call to the generated code if required.

[pq]

Wow. You've both been busy! Thanks for the quick follow-ups! 🎉

@DanTup:

the user only sees the first line

That's what I'd expect if you're using the completion (since it's multi-lined); in this case you'd want the displayText. The corresponding IDEA logic may be clarifying:

https://github.com/JetBrains/intellij-plugins/commit/10b5ca8a63c81de064a41b55c73a703161f9a45b

(Or maybe I'm misunderstanding?)

As for releasing, you make a good point. Let's pick up that conversation offline?

@pq: an idea for improvement: check whether the overridden method has @mustCallSuper annotation and add the corresponding super call to the generated code if required.

@alexander-doroshko: 👍

https://github.com/flutter/flutter-intellij/issues/1551

[DanTup]

@pq Sorry, I didn't describe very well. I understand how to fix, I was just concerned about timing - eg. if you push this out before Code users have a fix, I'll get lots of bug reports :-)

I also need to make another tweak - currently I'm slapping parens on the end of this so it looks like:

toString() { ... }()

This is because normally we'll add them to the ends of methods to make it more obvious whether they take args or not (we show () or (…)). I guess I need to not do that here. Btw, any chance os using a proper ellipsis so it fits in with my suffixes? :D

[DanTup]

We also have a different in arrows (we use →). It would really make sense to get all this logic pushed into the server to avoid inconsistencies like this ;(

[bwilkerson]

an idea for improvement: check whether the overridden method has @mustcallsuper annotation and add the corresponding super call to the generated code if required.

Actually, I think we should just always generate the super invocation. That way the insertion becomes a no-op until the user edits it.

[alexander-doroshko]

we should just always generate the super invocation

Agree.
And maybe set selection when there's no @mustCallSuper. This will be consistent with what IntelliJ does for Java:

[devoncarew]

Hmm, https://github.com/flutter/flutter-intellij/issues/1110#issuecomment-365935525 seems a bit unfortunate. @alexander-doroshko, how does this display in Dart plugin / IntelliJ clients that aren't aware of displayText? Do older Dart plugins display this well? We can ship new versions of dart-code quickly, but not new versions of the dart IntelliJ plugin.

[DanTup]

I have a look at the versions in analytics and it seems like the time from a new Dart Code release to most people (90%+) being on it is around a week (it helps that VS Code auto-updates plugins by default at launch). There are some people on SUPER old versions (like 0.14 - possibly one of the first public releases) but it seems unlikely those guys are gonna be upgrading their SDKs if they haven't updated Code/Dart Code in this long 😁

So for Code, it'd be good to get a release out a week before this starts hitting flutter users. I'd not planned another release before v2.9 (end of the month) but it's not a big deal (probably 30-60min all in); just let me know if you want me to do it.

[alexander-doroshko]

@devoncarew I see nothing criminal with unaware Dart plugin:

[DanTup]

Seems like Code is probably the limiting factor here then. I have just over an hour left of my work week so I'm gonna whip up a small patch for Dart Code 2.8 (the sooner it's out, the less of an issue shipping this is).

[DanTup]

Ok, Code v2.8.2 is out which uses displayText if it exists. It doesn't address other things we might want to tidy up (like making the arrows/ellipsis consistent) but I'm no longer worried about this change ending up in Flutter.

[bwilkerson]

Where do the arrows show up?

[DanTup]

IIRC overrides on fields got a getter and setter pair, and the display text for one of them had a small arrow?

[DanTup]

Here they are - they're different to the arrows we use in Dart Code for some things (now I think of it, possibly you use the other ones in the server too?)

[bwilkerson]

Ah, yes, those are part of the Dart syntax that we're going to be inserting, so I think it's appropriate to leave it that way.

[DanTup]

Sure, but my point was that it's a different arrow to used elsewhere?

Sorry, I see what you mean now! :)

[DanTup]

What about the three dots, any chance of using …? I think it looks better (shorter) and Dart Code uses that all over the place for similar things 😁

[bwilkerson]

Yes, the ellipsis should be fixed.

We use the single character arrow for types and signatures, but in this case we're displaying the code that will be inserted, so I think it makes more sense to stick to valid Dart syntax.

[bwilkerson]

I have updated the generator so that we now generate an invocation of the overridden method when the overridden method is not abstract and we should be selecting the right code. In addition, we're using a real ellipsis in the display text. Let me know if you see any additional issues.

[DanTup]

(Edit, rasied dart-lang/sdk#32260)

@pq @bwilkerson I think something may be wonky with the selectionOffset that's coming back:

{
    "kind": "OVERRIDE",
    "relevance": 2000,
    "completion": "@override\n  method() {\n    // TODO: implement method\n    return super.method();\n  }",
    "displayText": "method() { … }",
    "selectionOffset": 59,
    "selectionLength": 22
    // ...
}

@override\n  method() {\n    // TODO: implement method\n    return super.method();\n  }
0123456789 0123456789012 345678901234567890123456789012 3456789

[DanTup]

Found another issue too - I'll raise new issues for these rather than derailing this one!

[DanTup]

I've raise separate issues for these:

dart-lang/sdk#32260 - Override completions have bad selectionOffsets
dart-lang/sdk#32259 - Override completions don't appear if @override is already present

[bwilkerson]

Those two issues have been fixed and closed. Should this issue be closed, or is there more to be done?

[DanTup]

Nothing from me, and I'm happy to raise new issues if anything comes up.

[branflake2267]

Auto completion is working for me as well.