use CompletionTarget for replacement offset/length

use CompletionTarget for replacement offset/length
add code completion replacementLength and replacementOffset assertions

BUG=dartbug.com/21768
R=paulberry@google.com

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

[danrubel, 2015-01-28 13:57:10]

danrubel@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2015-01-28 16:32:21]

lgtm assuming you like my suggestion for computeFast().

https://codereview.chromium.org/886463002/diff/1/pkg/analysis_server/lib/src/...
File
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
(left):

https://codereview.chromium.org/886463002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:130:
if (controller.isClosed) {
Nit: can we keep this?  Granted, it's unnecessary because of your change to
sendResults() below, but it shouldn't cause any harm, and it will save us from
doing unnecessary work in the case where the completion results aren't needed.

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

https://codereview.chromium.org/886463002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:113:
request.replacementOffset = request.offset;
Some issues with this code:
- It doesn't handle the case where entity is a keyword token but not an AstNode.
 (This would happen when the cursor is on an "is" or "as" keyword).
- The "if" and "else if" clauses below are redundant, since SimpleIdentifier is
a subclass of AstNode (and the logic is equivalent).
- The test "token.offset <= request.offset" is redundant, since the
CompletionTarget logic will never return a completion target that is after the
requested offset position.

Suggested alternative:

request.replacementOffset = request.offset;
request.replacementLength = 0;
var entity = request.target.entity;
Token targetToken = entity is AstNode ? entity.beginToken : entity;
if (targetToken != null &&
    (targetToken.type == TokenType.KEYWORD ||
     targetToken.type == TokenType.IDENTIFIER)) {
  request.replacementOffset = targetToken.offset;
  request.replacementLength = targetToken.length;
}

[danrubel, 2015-01-28 20:10:43]

https://codereview.chromium.org/886463002/diff/1/pkg/analysis_server/lib/src/...
File
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart
(left):

https://codereview.chromium.org/886463002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:130:
if (controller.isClosed) {
On 2015/01/28 16:32:21, Paul Berry wrote:
> Nit: can we keep this?  Granted, it's unnecessary because of your change to
> sendResults() below, but it shouldn't cause any harm, and it will save us from
> doing unnecessary work in the case where the completion results aren't needed.

Makes sense. I'll put this back in. I've made the test code use this class's
computeFast method rather than rolling it into the test infrastructure. I want
to do the same for this computeFull method which is why I removed the
controller.isClosed() statement, but based on your suggestion I'll find a
different modification that will satisfy testing and keeping this statement for
efficiency.

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

https://codereview.chromium.org/886463002/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/services/completion/dart_completion_manager.dart:113:
request.replacementOffset = request.offset;
On 2015/01/28 16:32:21, Paul Berry wrote:
> Some issues with this code:
> - It doesn't handle the case where entity is a keyword token but not an
AstNode.
>  (This would happen when the cursor is on an "is" or "as" keyword).
> - The "if" and "else if" clauses below are redundant, since SimpleIdentifier
is
> a subclass of AstNode (and the logic is equivalent).
> - The test "token.offset <= request.offset" is redundant, since the
> CompletionTarget logic will never return a completion target that is after the
> requested offset position.
> 
> Suggested alternative:
> 
> request.replacementOffset = request.offset;
> request.replacementLength = 0;
> var entity = request.target.entity;
> Token targetToken = entity is AstNode ? entity.beginToken : entity;
> if (targetToken != null &&
>     (targetToken.type == TokenType.KEYWORD ||
>      targetToken.type == TokenType.IDENTIFIER)) {
>   request.replacementOffset = targetToken.offset;
>   request.replacementLength = targetToken.length;
> }

Good eyes. I like that code better. Done with a slight tweak.

[danrubel, 2015-01-28 20:11:13]

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