Complain about duplicated names.

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

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library fasta.body_builder;
 
 import '../fasta_codes.dart'
     show FastaMessage, codeExpectedButGot, codeExpectedFunctionBody;
 
 import '../parser/parser.dart' show FormalParameterType, MemberKind, optional;
@@ skipping 288 matching lines @@
           addCompileTimeError(-1, "Label not found: '$name'.");
         } else {
           outerSwitchScope.forwardDeclareLabel(name, builder);
         }
       });
     }
     switchScope = outerSwitchScope;
   }
 
   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(
             variable, member ?? classBuilder ?? library, uri),
         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);
     }
   }
 
   @override
   JumpTarget createJumpTarget(JumpTargetKind kind, int charOffset) {
     return new JumpTarget(kind, functionNestingLevel, member, charOffset);
   }
 
@@ skipping 1341 matching lines @@
       count--;
     }
     FormalParameters formals = new FormalParameters(
         popList(count) ?? <VariableDeclaration>[],
         optional,
         beginToken.charOffset);
     push(formals);
     if ((inCatchClause || functionNestingLevel != 0) &&
         kind != MemberKind.GeneralizedFunctionType) {
       enterLocalScope(formals.computeFormalParameterScope(
-          scope, member ?? classBuilder ?? library));
+          scope, member ?? classBuilder ?? library, this));
     }
   }
 
   @override
   void beginCatchClause(Token token) {
     debugEvent("beginCatchClause");
     inCatchClause = true;
   }
 
   @override
@@ skipping 432 matching lines @@
         typeParameters: typeParameters, asyncMarker: asyncModifier)
       ..fileOffset = formals.charOffset
       ..fileEndOffset = endToken.charOffset));
   }
 
   @override
   void endFunctionDeclaration(Token token) {
     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);
   }
 
   @override
   void endUnnamedFunction(Token beginToken, Token token) {
     debugEvent("UnnamedFunction");
     Statement body = popStatement();
     AsyncMarker asyncModifier = pop();
     exitLocalScope();
     FormalParameters formals = pop();
@@ skipping 484 matching lines @@
     debugEvent("Operator");
     push(new Operator(token.stringValue)..fileOffset = token.charOffset);
   }
 
   @override
   void handleSymbolVoid(Token token) {
     debugEvent("SymbolVoid");
     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
     // avoid a long list of errors.
     return library.addCompileTimeError(charOffset, message, fileUri: uri);
   }
 
   @override
   void handleInvalidFunctionBody(Token token) {
     if (member.isNative) {
@@ skipping 463 matching lines @@
           namedParameters.add(new NamedType(parameter.name, parameter.type));
         }
         namedParameters.sort();
       }
     }
     return new FunctionType(positionalParameters, returnType,
         namedParameters: namedParameters,
         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);
       }
     }
     return new Scope(local, null, parent, isModifiable: false);
   }
 }
 
 /// Returns a block like this:
 ///
@@ skipping 46 matching lines @@
     if (starToken == null) {
       return AsyncMarker.Async;
     } else {
       assert(identical(starToken.stringValue, "*"));
       return AsyncMarker.AsyncStar;
     }
   } else {
     return internalError("Unknown async modifier: $asyncToken");
   }
 }