Complete for-statement

Complete for-statement

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

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

[messick, 2017-04-18 16:36:53]

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

[messick, 2017-04-18 16:36:53]

One TODO has incorrect text but since for-each is what I will work on next I
decided not to change it.

[Brian Wilkerson, 2017-04-18 17:28:50]

lgtm

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

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:133:
test_completeForEmptyCond() async {
Just a suggestion: you could put each statement kind in it's own test class just
to group the tests without having to have a common prefix for each test. Might
make the code easier to read and maintain.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:147:
for (int i = 0; ) {
Not knowing exactly how this is suppose to work, I'm a little surprised that
this doesn't automatically add the second semicolon. My expectation, which might
be off, is that this would add as much of the boilerplate as possible.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:168:
for () {
This, on the other hand, looks right, because we don't know which kind of `for`
loop the user is writing.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:257:
}
Add a test for having both an initializer and a condition, but no updater?

main() {
  for (int i = 0; i < 10)
}

[messick, 2017-04-18 18:05:52]

I'm going to refactor tests as Brian suggested before submitting. That will be
later today.

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

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:133:
test_completeForEmptyCond() async {
On 2017/04/18 17:28:50, Brian Wilkerson wrote:
> Just a suggestion: you could put each statement kind in it's own test class
just
> to group the tests without having to have a common prefix for each test. Might
> make the code easier to read and maintain.

Good idea, thanks.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:147:
for (int i = 0; ) {
On 2017/04/18 17:28:50, Brian Wilkerson wrote:
> Not knowing exactly how this is suppose to work, I'm a little surprised that
> this doesn't automatically add the second semicolon. My expectation, which
might
> be off, is that this would add as much of the boilerplate as possible.

I considered that. Two factors dissuaded me. Doing it properly would require
setting up link editing groups. That's not unreasonable but I have not noticed
IJ doing that for statement completion in Java. And IJ only adds at most one
semicolon. However, I should add a TODO to revisit this.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:168:
for () {
On 2017/04/18 17:28:50, Brian Wilkerson wrote:
> This, on the other hand, looks right, because we don't know which kind of
`for`
> loop the user is writing.

Acknowledged.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:257:
}
On 2017/04/18 17:28:50, Brian Wilkerson wrote:
> Add a test for having both an initializer and a condition, but no updater?
> 
> main() {
>   for (int i = 0; i < 10)
> }

Thanks for catching that. I lost track of that branch and forgot to write the
code for it.

[messick, 2017-04-18 21:35:41]

PTAL

Added the check for empty updaters and reorganized the tests.

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

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:133:
test_completeForEmptyCond() async {
On 2017/04/18 17:28:50, Brian Wilkerson wrote:
> Just a suggestion: you could put each statement kind in it's own test class
just
> to group the tests without having to have a common prefix for each test. Might
> make the code easier to read and maintain.

Done.

https://codereview.chromium.org/2824233002/diff/1/pkg/analysis_server/test/se...
pkg/analysis_server/test/services/completion/statement/statement_completion_test.dart:257:
}
On 2017/04/18 17:28:50, Brian Wilkerson wrote:
> Add a test for having both an initializer and a condition, but no updater?
> 
> main() {
>   for (int i = 0; i < 10)
> }

Done.

[Brian Wilkerson, 2017-04-18 21:47:56]

lgtm

[scheglov, 2017-04-18 21:52:09]

lgtm

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

https://codereview.chromium.org/2824233002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:325:
if (forNode.leftParenthesis.lexeme.isEmpty) {
It might be more straightforward to use forNode.leftParenthesis.isSynthetic
instead.

https://codereview.chromium.org/2824233002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:563:
bool _isEmptyExpression(Expression expr) {
This also could be simplified:
return expr is SimpleIdentifier && expr.isSynthetic;

And maybe rename it to _isSyntheticExpression, it's not that it is empty and
valid.

[messick, 2017-04-18 22:16:24]

PTAL

Thanks for the suggestions!

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

https://codereview.chromium.org/2824233002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:325:
if (forNode.leftParenthesis.lexeme.isEmpty) {
On 2017/04/18 21:52:09, scheglov wrote:
> It might be more straightforward to use forNode.leftParenthesis.isSynthetic
> instead.

Done.

https://codereview.chromium.org/2824233002/diff/20001/pkg/analysis_server/lib...
pkg/analysis_server/lib/src/services/completion/statement/statement_completion.dart:563:
bool _isEmptyExpression(Expression expr) {
On 2017/04/18 21:52:09, scheglov wrote:
> This also could be simplified:
> return expr is SimpleIdentifier && expr.isSynthetic;
> 
> And maybe rename it to _isSyntheticExpression, it's not that it is empty and
> valid.

Done.

[scheglov, 2017-04-18 22:18:36]

LGTM

[messick, 2017-04-18 22:20:38]

Description was changed from

==========
Complete for-statement
==========

to

==========
Complete for-statement

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

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

[messick, 2017-04-18 22:20:39]

Committed patchset #3 (id:40001) manually as
6a19d96ee28dd763af40041acff74ac58e5c9f2f (presubmit successful).