Kernel debugging; service tests

pkg/kernel/lib/analyzer/ast_from_analyzer.dart

Index: pkg/kernel/lib/analyzer/ast_from_analyzer.dart
diff --git a/pkg/kernel/lib/analyzer/ast_from_analyzer.dart b/pkg/kernel/lib/analyzer/ast_from_analyzer.dart
index 45ebe7b996e3d444c6733dba546c37c28cde586f..e78c14f30d590020196d73e33c6402103a2d4ac6 100644
--- a/pkg/kernel/lib/analyzer/ast_from_analyzer.dart
+++ b/pkg/kernel/lib/analyzer/ast_from_analyzer.dart
@@ -524,7 +524,8 @@ class ExpressionScope extends TypeScope {
         if (bodyHasVoidReturn(body)) {
           return new ast.ExpressionStatement(buildExpression(body.expression));
         } else {
-          return new ast.ReturnStatement(buildExpression(body.expression));
+          return new ast.ReturnStatement(buildExpression(body.expression))
+            ..fileOffset = body.expression.offset;
         }
       } else {
         return internalError('Missing function body');
@@ -664,8 +665,9 @@ class ExpressionScope extends TypeScope {
   }
 
   ast.VariableDeclaration makeVariableDeclaration(LocalElement element,
-      {ast.DartType type, ast.Expression initializer}) {
+      {ast.DartType type, ast.Expression initializer, int equalsOffset}) {
     var declaration = getVariableReference(element);
+    if (equalsOffset != null) declaration.fileEqualsOffset = equalsOffset;
     declaration.type = type ?? getInferredVariableType(element);
     if (initializer != null) {
       declaration.initializer = initializer..parent = declaration;
@@ -961,7 +963,8 @@ class StatementBuilder extends GeneralizingAstVisitor<ast.Statement> {
         LocalElement local = decl.element as dynamic; // Cross cast.
         output.add(scope.makeVariableDeclaration(local,
             type: type,
-            initializer: scope.buildOptionalExpression(decl.initializer)));
+            initializer: scope.buildOptionalExpression(decl.initializer),
+            equalsOffset: decl.equals?.offset));
       }
     } else {
       output.add(build(node));
@@ -1175,7 +1178,8 @@ class StatementBuilder extends GeneralizingAstVisitor<ast.Statement> {
         LocalElement local = variable.element as dynamic; // Cross cast.
         variables.add(scope.makeVariableDeclaration(local,
             initializer: scope.buildOptionalExpression(variable.initializer),
-            type: type));
+            type: type,
+            equalsOffset: variable.equals?.offset));
       }
     } else if (node.initialization != null) {
       initialExpression = scope.buildExpression(node.initialization);
@@ -1364,11 +1368,12 @@ class ExpressionBuilder
     if (result is Accessor) {
       result = result.buildSimpleRead();
     }
-    return result..fileOffset = _getOffset(node);
+    return result..fileOffset = _getOffset(node, result);
   }
 
-  int _getOffset(AstNode node) {
+  int _getOffset(AstNode node, ast.Expression expression) {

[Kevin Millikin (Google), 2017/02/08 15:37:51]

This is a warty API.  It takes an analyzer AstNode (isn't it always an
Expression?) and a Kernel Expression.  It just ignores the second argument
except in case the first argument is a MethodInvocation and the second argument
has a non-negative file offset.

As far as I can tell, the left-hand side of an assignment cannot be a method
invocation, so we definitely don't need to pass the second argument to the
recursive call.  And for the non-recursive call (am I missing other call sites?)
we could avoid passing the second argument with:

// For some method invocations we have already set a file offset to
// override the default behavior of _getOffset.
if (node is! MethodInvocation || result.fileOffset < 0) {
  result.fileOffset = _getOffset(node);
}
return result;

[jensj, 2017/02/13 14:04:15]

Done.

     if (node is MethodInvocation) {
+      if (expression.fileOffset >= 0) return expression.fileOffset;
       return node.methodName.offset;
     } else if (node is InstanceCreationExpression) {
       return node.constructorName.offset;
@@ -1377,15 +1382,19 @@ class ExpressionBuilder
     } else if (node is PrefixedIdentifier) {
       return node.identifier.offset;
     } else if (node is AssignmentExpression) {
-      return _getOffset(node.leftHandSide);
+      return _getOffset(node.leftHandSide, expression);
     } else if (node is PropertyAccess) {
       return node.propertyName.offset;
     } else if (node is IsExpression) {
       return node.isOperator.offset;
+    } else if (node is AsExpression) {
+      return node.asOperator.offset;
     } else if (node is StringLiteral) {
       // Use a catch-all for StringInterpolation and AdjacentStrings:
       // the debugger stops at the end.
       return node.end;
+    } else if (node is IndexExpression) {
+      return node.leftBracket.offset;
     }
     return node.offset;
   }
@@ -1867,7 +1876,8 @@ class ExpressionBuilder
   ast.Expression visitIsExpression(IsExpression node) {
     if (node.notOperator != null) {
       return new ast.Not(new ast.IsExpression(
-          build(node.expression), scope.buildTypeAnnotation(node.type)));
+          build(node.expression), scope.buildTypeAnnotation(node.type))
+        ..fileOffset = node.isOperator.offset);

[Kevin Millikin (Google), 2017/02/08 15:37:51]

It seems weird that the IsExpression that comes from 'is!' has a file offset,
but the IsExpression that comes from 'is' does not.  I can't figure out why, so
it probably needs a comment.

It also seems better to call _getOffset(node) instead of writing
node.isOperator.offset explicitly.  The only documentation we currently have of
which offsets we record for which kinds of expressions is by reading _getOffset.
 It would be easy to miss the case for is!.

[jensj, 2017/02/13 14:04:15]

Done.

     } else {
       return new ast.IsExpression(
           build(node.expression), scope.buildTypeAnnotation(node.type));
@@ -1920,7 +1930,8 @@ class ExpressionBuilder
           new ast.VariableGet(scope.getVariableReference(element)),
           callName,
           buildArgumentsForInvocation(node),
-          scope.resolveInterfaceFunctionCall(element));
+          scope.resolveInterfaceFunctionCall(element))
+        ..fileOffset = node.methodName.end;

[Kevin Millikin (Google), 2017/02/08 15:37:51]

This definitely needs a comment.  We are explicitly setting the file offset
here, for the case of invoking a method through a local, to override the default
behavior of the call in 'build' that would use the methodName's (start)
position, right?

Add a comment to that effect here.

[jensj, 2017/02/13 14:04:15]

Done.

     } else if (isStaticMethod(element)) {
       var method = scope.resolveConcreteMethod(element);
       var arguments = buildArgumentsForInvocation(node);
@@ -2000,7 +2011,7 @@ class ExpressionBuilder
         var leftHand = buildLeftHandValue(node.operand);
         var binaryOperator = new ast.Name(operator[0]);
         return leftHand.buildPostfixIncrement(binaryOperator,
-            offset: node.offset,
+            offset: node.operator.offset,
             voidContext: isInVoidContext(node),
             interfaceTarget: scope.resolveInterfaceMethod(node.staticElement));