add keyword suggestions

add keyword suggestions

BUG=
R=brianwilkerson@google.com, paulberry@google.com

Committed: https://code.google.com/p/dart/source/detail?r=39284

[Brian Wilkerson, 2014-08-12 15:33:38]

In addition to the individual comments, a high level question: Some environments
make a distinction between suggestions and templates. Do we really want to
suggest keywords, or do we want to add support for templates? Personally, I'd
rather have a template that will take

  im^

and produce

  import '^';

(and then automatically suggest completions for available dart: and package:
URI's).

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/completion/keyword_computer.dart (right):

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:37: class
_KeywordVisitor extends GeneralizingAstVisitor {
It seems unlikely that we'd ever want to add suggestions for all nodes, so
perhaps extend SimpleAstVisitor.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:42:
visitCompilationUnit(CompilationUnit node) {
Missing @override annotations.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:53:
Keyword.VAR]);
Missing 'typedef'.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:57:
_addSuggestions([Keyword.EXTENDS, Keyword.IMPLEMENTS, Keyword.WITH]);
This should check that we're not within the body of the class.

It should not suggest keywords that are already in use (for example "class C ^
implements A {}" shouldn't suggest "implements").

It should not suggest keywords that can't be used (for example "class C ^
extends B {}" shouldn't suggest any keywords because both "with" and
"implements" would need to follow the extends clause).

[Paul Berry, 2014-08-12 16:32:11]

I agree with Brian that it would be nice to complete some things (such as
imports) with templates, and to avoid suggesting "extends" and "with" in
contexts where they are already present.

But IMO those could be done as follow-up CL's.  So with code generation re-run,
this CL lgtm.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:1777:
<value><code>KEYWORD</code></value>
Since this file is used to generate code as well as documentation, you need to
rerun the generation script using:

  pkg/analysis_server/tool/spec/generate_files

I'm planning on adding code to the build to make sure that we don't forget to do
code generation, but I haven't had a chance to do that yet.

[Brian Wilkerson, 2014-08-13 15:54:15]

LGTM, once comments are addressed

[Brian Wilkerson, 2014-08-13 16:20:15]

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/completion/keyword_computer.dart (right):

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:43:
_addSuggestions(
This should check to see whether we are in the "directives" partition or the
"declarations" partition of the file and only suggest valid completions. For
example, given

  library lib;
  import 'dart:async';
  ab^
  import 'dart:unittest';

I would argue that we shouldn't suggest "abstract" because it isn't valid at
that point. Similarly, given

  class C {}
  im^

we shouldn't suggest "import" because it's also not valid.

[danrubel, 2014-08-15 03:23:26]

On 2014/08/12 15:33:38, Brian Wilkerson wrote:
> Personally, I'd rather have a template that will take
> 
>   im^
> 
> and produce
> 
>   import '^';
> 
> (and then automatically suggest completions for available dart: and package:
> URI's).

Agreed. This is what I'm aiming for but the API needs to be tweaked to support
this. Per discussion, this is an incomplete first cut to get things rolling.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_server/tool/spe...
File pkg/analysis_server/tool/spec/spec_input.html (right):

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_server/tool/spe...
pkg/analysis_server/tool/spec/spec_input.html:1777:
<value><code>KEYWORD</code></value>
On 2014/08/12 16:32:11, Paul Berry wrote:
> Since this file is used to generate code as well as documentation, you need to
> rerun the generation script using:
> 
>   pkg/analysis_server/tool/spec/generate_files
> 
> I'm planning on adding code to the build to make sure that we don't forget to
do
> code generation, but I haven't had a chance to do that yet.

Done.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
File pkg/analysis_services/lib/src/completion/keyword_computer.dart (right):

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:37: class
_KeywordVisitor extends GeneralizingAstVisitor {
On 2014/08/12 15:33:37, Brian Wilkerson wrote:
> It seems unlikely that we'd ever want to add suggestions for all nodes, so
> perhaps extend SimpleAstVisitor.

I'm thinking that there may be some more general situations in which we want to
suggest such things as true, false, null, which is why I chose this base class.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:42:
visitCompilationUnit(CompilationUnit node) {
On 2014/08/12 15:33:37, Brian Wilkerson wrote:
> Missing @override annotations.

I wish that the tool would insert these automatically. I'll have to go through
all my visitors to add these missing annotations in a separate CL.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:43:
_addSuggestions(
On 2014/08/13 16:20:15, Brian Wilkerson wrote:
> This should check to see whether we are in the "directives" partition or the
> "declarations" partition of the file and only suggest valid completions. For
> example, given
> 
>   library lib;
>   import 'dart:async';
>   ab^
>   import 'dart:unittest';
> 
> I would argue that we shouldn't suggest "abstract" because it isn't valid at
> that point. Similarly, given
> 
>   class C {}
>   im^
> 
> we shouldn't suggest "import" because it's also not valid.

Good points. I'll add tests for these. This current implementation is very much
an incomplete first cut just to get things moving.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:53:
Keyword.VAR]);
On 2014/08/12 15:33:37, Brian Wilkerson wrote:
> Missing 'typedef'.

Done.

https://codereview.chromium.org/461063002/diff/1/pkg/analysis_services/lib/sr...
pkg/analysis_services/lib/src/completion/keyword_computer.dart:57:
_addSuggestions([Keyword.EXTENDS, Keyword.IMPLEMENTS, Keyword.WITH]);
On 2014/08/12 15:33:38, Brian Wilkerson wrote:
> This should check that we're not within the body of the class.
> 
> It should not suggest keywords that are already in use (for example "class C ^
> implements A {}" shouldn't suggest "implements").
> 
> It should not suggest keywords that can't be used (for example "class C ^
> extends B {}" shouldn't suggest any keywords because both "with" and
> "implements" would need to follow the extends clause).

Will definitely add these as a follow on CL. As stated this is an incomplete
first cut to get things rolling.

[danrubel, 2014-08-15 03:37:12]

Committed patchset #3 manually as 39284 (presubmit successful).