Named arg completion fixes (flutter-intellij#551)

Named arg completion fixes.

Related to: https://github.com/flutter/flutter-intellij/issues/551

* fixes named arg selection offset to precede comma
* fixes inconsistent trailing commas between completions w/ and w/o prefixes

BUG=
R=brianwilkerson@google.com

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

[pquitslund, 2017-04-26 20:26:56]

Description was changed from

==========
Named arg completion fixes.

Related to: https://github.com/flutter/flutter-intellij/issues/551

* fixes named arg selection offset to precede comma
* fixes inconsistent trailing commas between completions w/ and w/o prefixes

BUG=
==========

to

==========
Named arg completion fixes.

Related to: https://github.com/flutter/flutter-intellij/issues/551

* fixes named arg selection offset to precede comma
* fixes inconsistent trailing commas between completions w/ and w/o prefixes

BUG=
==========

[pquitslund, 2017-04-26 20:27:12]

pquitslund@google.com changed reviewers:
+ brianwilkerson@google.com, danrubel@google.com

[Brian Wilkerson, 2017-04-26 20:53:40]

lgtm

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

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:312:
entity is AstNode && entity.endToken.next.type == TokenType.COMMA;
Should this / does this need to handle the case where entity is a Token?

[pquitslund, 2017-04-27 13:28:20]

On 2017/04/26 20:53:40, Brian Wilkerson wrote:
> lgtm
> 
>
https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/lib/src...
> File
> pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart
> (right):
> 
>
https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/lib/src...
>
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:312:
> entity is AstNode && entity.endToken.next.type == TokenType.COMMA;
> Should this / does this need to handle the case where entity is a Token?

I have no idea! :)

What use case do you have in mind?  (That is, what should I test?)

[danrubel, 2017-04-27 14:04:14]

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart
(right):

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart:170:
main() { new A(o^ two: 'foo');}''');
I recommend two more tests... each with a trailing comma based upon these two
new tests.

new A(^ , two:

and

new A(o^ , two:

to make sure that we don't append a comma when its not needed.

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart:595:
completion: 'one: ', selectionOffset: 5);
And several more tests similar to this one.

new A(o^)
new A(o^,)
new A(foo: 2, ^)
new A(foo: 2, o^)
new A(foo: 2, o^,)

Welcome to code completion and the explosion of necessary tests :-)

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/dart/completion_contributor_util.dart
(right):

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/completion_contributor_util.dart:138:
if (selectionOffset != null) {
This change will prevent cs.selectionOffset from being checked in 99% of the
cases where it was previously checked. I recommend replacing these three lines
with

expect(cs.selectionOffset, equals(selectionOffset ?? completion.length));

[Brian Wilkerson, 2017-04-27 16:13:50]

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

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:312:
entity is AstNode && entity.endToken.next.type == TokenType.COMMA;
> What use case do you have in mind? (That is, what should I test?)

I don't know whether there *are* any use cases, but if the entity *could* ever
be a token then we could test `entity.next.type == TokenType.COMMA`. Dan might
know when / whether entity could ever be a token at this point.

[danrubel, 2017-04-27 16:27:26]

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

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:312:
entity is AstNode && entity.endToken.next.type == TokenType.COMMA;
On 2017/04/27 16:13:50, Brian Wilkerson wrote:
> > What use case do you have in mind? (That is, what should I test?)
> 
> I don't know whether there *are* any use cases, but if the entity *could* ever
> be a token then we could test `entity.next.type == TokenType.COMMA`. Dan might
> know when / whether entity could ever be a token at this point.

I don't know and if it is not today then it might be tomorrow because the
scanner/parser is being replaced. I recommend coding this defensively to assume
either possibility.

[pquitslund, 2017-04-27 17:09:54]

Thanks!

I'll follow up with fixes for selectionOffset.

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

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/lib/src...
pkg/analysis_server/lib/src/services/completion/dart/arglist_contributor.dart:312:
entity is AstNode && entity.endToken.next.type == TokenType.COMMA;
On 2017/04/27 16:27:26, danrubel wrote:
> On 2017/04/27 16:13:50, Brian Wilkerson wrote:
> > > What use case do you have in mind? (That is, what should I test?)
> > 
> > I don't know whether there *are* any use cases, but if the entity *could*
ever
> > be a token then we could test `entity.next.type == TokenType.COMMA`. Dan
might
> > know when / whether entity could ever be a token at this point.
> 
> I don't know and if it is not today then it might be tomorrow because the
> scanner/parser is being replaced. I recommend coding this defensively to
assume
> either possibility.

Done.

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart
(right):

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart:170:
main() { new A(o^ two: 'foo');}''');
On 2017/04/27 14:04:13, danrubel wrote:
> I recommend two more tests... each with a trailing comma based upon these two
> new tests.
> 
> new A(^ , two:
> 
> and
> 
> new A(o^ , two:
> 
> to make sure that we don't append a comma when its not needed.

Thank you, yes!  Done.

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/arglist_contributor_test.dart:595:
completion: 'one: ', selectionOffset: 5);
On 2017/04/27 14:04:13, danrubel wrote:
> And several more tests similar to this one.
> 
> new A(o^)
> new A(o^,)
> new A(foo: 2, ^)
> new A(foo: 2, o^)
> new A(foo: 2, o^,)
> 
> Welcome to code completion and the explosion of necessary tests :-)

Done.

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
File
pkg/analysis_server/test/services/completion/dart/completion_contributor_util.dart
(right):

https://codereview.chromium.org/2839273002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/dart/completion_contributor_util.dart:138:
if (selectionOffset != null) {
On 2017/04/27 14:04:14, danrubel wrote:
> This change will prevent cs.selectionOffset from being checked in 99% of the
> cases where it was previously checked. I recommend replacing these three lines
> with
> 
> expect(cs.selectionOffset, equals(selectionOffset ?? completion.length));

I need to pick this up in a follow-up CL.  Net-net: we don't want to default to
completion.length for cases where a comma is included and we'll need to update a
bunch of tests.

[pquitslund, 2017-04-27 17:12:55]

Description was changed from

==========
Named arg completion fixes.

Related to: https://github.com/flutter/flutter-intellij/issues/551

* fixes named arg selection offset to precede comma
* fixes inconsistent trailing commas between completions w/ and w/o prefixes

BUG=
==========

to

==========
Named arg completion fixes.

Related to: https://github.com/flutter/flutter-intellij/issues/551

* fixes named arg selection offset to precede comma
* fixes inconsistent trailing commas between completions w/ and w/o prefixes

BUG=
R=brianwilkerson@google.com

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

[pquitslund, 2017-04-27 17:12:56]

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