Statement completion framework with a few examples

Statement completion framework with a few examples

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/eadc6af22638e756178bfba1884c92c19ec4e770

[messick, 2017-04-07 18:28:45]

messick@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[messick, 2017-04-07 18:28:45]

Here is the framework for statement completion.

It handles only a few kinds of statements now: if, while, and otherwise complete
statements that just need a semicolon.

If you want to run the code yourself I can help you patch IntelliJ with the PR
needed to activate statement completion in the editor.

[Brian Wilkerson, 2017-04-07 19:17:14]

lgtm, assuming none of my questions uncovered an actual problem.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/constants.dart (right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/constants.dart:81: const String
DIAGNOSTIC_GET_DIAGNOSTICS = 'diagnostic.getDiagnostics';
Is the sort function sorting constants? I thought it would leave variable
declarations alone.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/edit/edit_domain.dart (right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/edit/edit_domain.dart:258:
server.contextManager.getDriverFor(params.file);
Are the two lines above necessary because of a side-effect? (If so, we should
document that, otherwise someone is likely to try to remove them.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/edit/edit_domain.dart:279: change = new
SourceChange("", edits: []);
Consider using "??=".

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart
(right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:266:
IfStatement ifNode = node.getAncestor((n) => n is IfStatement);
Given that this can travel an indeterminate distance up the AST, isn't there a
danger of getting an unintended if statement?

[scheglov, 2017-04-07 19:36:07]

LGTM

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/edit/edit_domain.dart (right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/edit/edit_domain.dart:258:
server.contextManager.getDriverFor(params.file);
On 2017/04/07 19:17:14, Brian Wilkerson wrote:
> Are the two lines above necessary because of a side-effect? (If so, we should
> document that, otherwise someone is likely to try to remove them.

I don't think these two statements are useful.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart
(right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:32:
static const PLAIN_OLE_ENTER = const StatementCompletionKind(
What means OLE?
And how about more general COM?

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:73:
final String file;
The order of fields seems random.
Might be worth to order in the way this information is generated: file,
lineInfo, selectionOffset, unit, unitElement, errors.

[messick, 2017-04-07 20:10:09]

Still working on some changes, but here are answers to questions asked in
review.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/constants.dart (right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/constants.dart:81: const String
DIAGNOSTIC_GET_DIAGNOSTICS = 'diagnostic.getDiagnostics';
On 2017/04/07 19:17:14, Brian Wilkerson wrote:
> Is the sort function sorting constants? I thought it would leave variable
> declarations alone.

I was surprised to see this, too. I just sorted every file, along with
formatting and optimizing imports.

Since sorting this file makes the comments confusing I think it should be left
unsorted.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File pkg/analysis_server/lib/src/edit/edit_domain.dart (right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/edit/edit_domain.dart:258:
server.contextManager.getDriverFor(params.file);
On 2017/04/07 19:36:07, scheglov wrote:
> On 2017/04/07 19:17:14, Brian Wilkerson wrote:
> > Are the two lines above necessary because of a side-effect? (If so, we
should
> > document that, otherwise someone is likely to try to remove them.
> 
> I don't think these two statements are useful.

Done.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart
(right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:32:
static const PLAIN_OLE_ENTER = const StatementCompletionKind(
On 2017/04/07 19:36:07, scheglov wrote:
> What means OLE?
> And how about more general COM?

I'll change it something more readable.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:73:
final String file;
On 2017/04/07 19:36:07, scheglov wrote:
> The order of fields seems random.
> Might be worth to order in the way this information is generated: file,
> lineInfo, selectionOffset, unit, unitElement, errors.

It wasn't random but I can change it to the order you prefer.

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:266:
IfStatement ifNode = node.getAncestor((n) => n is IfStatement);
On 2017/04/07 19:17:14, Brian Wilkerson wrote:
> Given that this can travel an indeterminate distance up the AST, isn't there a
> danger of getting an unintended if statement?

Yes, that's a good point. Since the conditional expression can be arbitrarily
complex, what's the recommended way of finding the statement that contains it?
Something like:

node.getAncesotor((n) => n is! Expression)

perhaps? And then check the statement type.

[messick, 2017-04-07 20:46:03]

PTAL

I answered some question in a previous message. This patch should address all
the comments, except one. I did not use ??= in one place because that assignment
should never happen.

[scheglov, 2017-04-07 20:49:47]

LGTM

[messick, 2017-04-07 20:56:03]

Description was changed from

==========
Statement completion framework with a few examples
==========

to

==========
Statement completion framework with a few examples

R=brianwilkerson@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/eadc6af22638e756178bfba1884c92c19ec4e770
==========

[messick, 2017-04-07 20:56:05]

Committed patchset #2 (id:20001) manually as
eadc6af22638e756178bfba1884c92c19ec4e770 (presubmit successful).

[Brian Wilkerson, 2017-04-07 21:23:37]

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
File
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart
(right):

https://codereview.chromium.org/2803313002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:266:
IfStatement ifNode = node.getAncestor((n) => n is IfStatement);
If you know it's contained in a statement (as opposed to a field initializer,
for example) then `n is Statement` would work. You just need to guard against
something like

if (cond1) {
  while (cond2) ^
}

finding the 'if' statement when there's a closer statement.

If you do just look for the nearest enclosing statement, you might want to
compute it once and pass it in to the appropriate _complete methods.