Complain about duplicated names.

pkg/front_end/lib/src/fasta/kernel/body_builder.dart

Index: pkg/front_end/lib/src/fasta/kernel/body_builder.dart
diff --git a/pkg/front_end/lib/src/fasta/kernel/body_builder.dart b/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
index e868b64fd5bf7a2150c2afb1d0f3fca63ef8fac5..abe11c953615c736fe3df550b779a6715c4ae2e1 100644
--- a/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
+++ b/pkg/front_end/lib/src/fasta/kernel/body_builder.dart
@@ -306,6 +306,16 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
   }
 
   void declareVariable(VariableDeclaration variable) {
+    // ignore: UNUSED_LOCAL_VARIABLE
+    Statement discardedStatement;
+    String name = variable.name;
+    int offset = variable.fileOffset;
+    if (scope.local[name] != null) {
+      discardedStatement = pop();
+      push(buildCompileTimeErrorStatement(
+          "'$name' already declared in this scope.", offset));
+      return;

[Paul Berry, 2017/05/24 20:46:15]

This may cause problems with the integration into analyzer, because even in the
event of an error, analyzer is going to expect to be given resolution
information for the variable initializer.

Would you mind adding a TODO comment to make sure we come back and address this
in the future?  Maybe something like "TODO(paulberry): figure out a way to avoid
discarding the initializer when performing type inference for analyzer"

[ahe, 2017/05/24 21:06:38]

Good point. We should probably add a compile-time error expression and statement
that can be used to preserve the original stuff. There are many other places
like this, so I probably have to file a bug.

[ahe, 2017/05/29 07:18:00]

Done.

+    }
     InputError error = scope.declare(
         variable.name,
         new KernelVariableBuilder(
@@ -313,10 +323,10 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
         variable.fileOffset,
         uri);
     if (error != null) {
-      addCompileTimeError(
-          variable.fileOffset,
-          "Can't declare '${variable.name}' because it was already used in "
-          "this scope.");
+      discardedStatement = pop();

[Paul Berry, 2017/05/24 20:46:15]

Is this code reachable?  It looks like scope.declare currently always returns
`null`, and the case you're trying to check for (name is already declared) is
handled above.

[ahe, 2017/05/24 21:06:38]

The code isn't reachable at the moment because scope.declare always returns
null.

But this is a different situation. The code above covers this situation:

{
  var x;
  var x;
}

This code deals with this problem:

{
  var x;
  {
    print(x);
    var x; // ERROR
  }
}

The error occurs because print(x) is considered to be a forward reference to the
last declaration.

[Paul Berry, 2017/05/24 22:10:39]

Ah, ok.  Thanks for the clarification.

I have concerns about reporting the error at the location of the second `var x;`
line.  Even though this is where the error is noticed by Fasta, I would argue
that the location of the error is the reference to `x` in `print(x);`, so that's
where we should report it.  (That's the behavior of analyzer, and it would be
unfortunate to lose this when we switch to the new front end).

Maybe what we should do here, at the time the error is detected, is to  walk
through the code that has already been generated, find all the references that
are now forward references, and then report errors at those locations.

[Paul Berry, 2017/05/24 22:12:59]

If you want to defer this to a future CL, that's ok with me, provided that you
add a TODO comment here.

[ahe, 2017/05/25 10:38:31]

This is one of the things that I have implemented in my dev branch. Take a look
at:

https://user.git.corp.google.com/ahe/dart-sdk/+/misc/pkg/front_end/lib/src/fa...

(Sorry, internal to Google, if anyone outside wants access let me know).

It may not be obvious, but I'm actually reporting two errors below, the first is
for the new declaration, the second is for the first use in this scope. That's
why scope.declare returns an InputError, so it can communicate the position.

The only difference between what you suggest and what I have already implemented
in my dev branch is that I only report the first use.

[Paul Berry, 2017/05/25 11:47:17]

Ah, ok.  That wasn't obvious to me.  Thank you.

I would still appreciate a TODO comment briefly explaining the situation so that
others who come across the code won't have the same misconceptions I did. 
Thanks!

[ahe, 2017/05/25 13:16:15]

I will add comments.

[ahe, 2017/05/29 07:18:00]

Done.

+      push(buildCompileTimeErrorStatement(
+          "Can't declare '$name' because it was already used in this scope.",
+          offset));
       library.addCompileTimeError(error.charOffset, error.error,
           fileUri: error.uri);
     }
@@ -1678,7 +1688,7 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
     if ((inCatchClause || functionNestingLevel != 0) &&
         kind != MemberKind.GeneralizedFunctionType) {
       enterLocalScope(formals.computeFormalParameterScope(
-          scope, member ?? classBuilder ?? library));
+          scope, member ?? classBuilder ?? library, this));
     }
   }
 
@@ -2131,13 +2141,20 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
     debugEvent("FunctionDeclaration");
     FunctionNode function = pop();
     exitLocalScope();
-    FunctionDeclaration declaration = pop();
-    function.returnType = pop() ?? const DynamicType();
-    declaration.variable.type = function.functionType;
+    var declaration = pop();
+    var returnType = pop() ?? const DynamicType();
     pop(); // Modifiers.
     exitFunction();
-    declaration.function = function;
-    function.parent = declaration;
+    if (declaration is FunctionDeclaration) {
+      function.returnType = returnType;
+      declaration.variable.type = function.functionType;
+      declaration.function = function;
+      function.parent = declaration;
+    } else {
+      // If [declaration] isn't a [FunctionDeclaration], it must be because
+      // there was a compile-time error.

[Paul Berry, 2017/05/24 20:46:15]

Maybe add a similar todo here, e.g.: "TODO(paulberry): ensure that when
integrating with analyzer, type inference is still performed for the dropped
declaration"

[ahe, 2017/05/29 07:18:00]

Done.

+      assert(library.compileTimeErrors.isNotEmpty);
+    }
     push(declaration);
   }
 
@@ -2642,6 +2659,7 @@ class BodyBuilder extends ScopeListener<JumpTarget> implements BuilderHelper {
     push(new Identifier(token));
   }
 
+  @override
   dynamic addCompileTimeError(int charOffset, String message,
       {bool silent: false}) {
     // TODO(ahe): If constantExpressionRequired is set, set it to false to
@@ -3125,15 +3143,23 @@ class FormalParameters {
         requiredParameterCount: requiredParameterCount);
   }
 
-  Scope computeFormalParameterScope(Scope parent, Builder builder) {
+  Scope computeFormalParameterScope(
+      Scope parent, Builder builder, BuilderHelper helper) {
     if (required.length == 0 && optional == null) return parent;
     Map<String, Builder> local = <String, Builder>{};
+
     for (VariableDeclaration parameter in required) {
+      if (local[parameter.name] != null) {
+        helper.addCompileTimeError(parameter.fileOffset, "Duplicated name.");
+      }
       local[parameter.name] =
           new KernelVariableBuilder(parameter, builder, builder.fileUri);
     }
     if (optional != null) {
       for (VariableDeclaration parameter in optional.formals) {
+        if (local[parameter.name] != null) {
+          helper.addCompileTimeError(parameter.fileOffset, "Duplicated name.");
+        }
         local[parameter.name] =
             new KernelVariableBuilder(parameter, builder, builder.fileUri);
       }