Report DUPLICATE_CONSTRUCTOR_NAME and other scope fixes

Report DUPLICATE_CONSTRUCTOR_NAME and other scope fixes

R=brianwilkerson@google.com, jwren@google.com
BUG=

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

[jwren, 2013-05-20 20:01:00]

LGTM.  Wait for committing for Brian to look at the scoping changes.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:115:
DUPLICATE_CONSTRUCTOR_NAME("There is other constructor with the same name"),
The enum name should match the other kinds of this error code, maybe something
like CONFLICTING_CONSTRUCTOR_NAME_AND_CONSTRUCTOR_NAME.  Also, what about the
messaging more consistent too: The name XXX is used to name this two
constructors in this class.

[Brian Wilkerson, 2013-05-20 20:21:29]

Let's discuss this before you commit it.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:115:
DUPLICATE_CONSTRUCTOR_NAME("There is other constructor with the same name"),
I'm actually fine with the name, but it should be alphabetized.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:484:
visitWithNameScope(thenStatement);
I don't understand the reason for these changes. Where in the specification does
it say that the 'then' and 'else' clauses of an if statement, the body of a
'while' statement, or any of the other places that use visitWithNameScope()
introduce a new scope?

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ScopedVisitor.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ScopedVisitor.java:277:
if (!(function.getEnclosingElement() instanceof CompilationUnitElement)) {
We shouldn't add the function to the scope until after the declaration.
According to the specification:

A function declaration statement of one of the forms id signature {statements}
or T id signature {statements} causes a new function named id to be added to the
innermost enclosing scope at the point immediately following the function
declaration statement.

I believe that the original source was correct.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/FunctionScope.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/FunctionScope.java:42:
Scope parameterScope = getEnclosingScope();
Again, I think the original code was correct. According to the specification:

The scope of a library function is the scope of the enclosing library. The scope
of a local function is  described below. In both cases, the name of the function
is in scope in the formal parameters scope of the function.

What isn't clear is whether it is defined in the formal parameter scope's outer
scope or in the formal parameter scope itself. Perhaps an e-mail to Gilad to
clarify the question is in order.

[scheglov, 2013-05-21 03:37:20]

PTAL

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/error/CompileTimeErrorCode.java:115:
DUPLICATE_CONSTRUCTOR_NAME("There is other constructor with the same name"),
On 2013/05/20 20:21:29, Brian Wilkerson wrote:
> I'm actually fine with the name, but it should be alphabetized.

I've made some changed, please look again.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ResolverVisitor.java:484:
visitWithNameScope(thenStatement);
On 2013/05/20 20:21:29, Brian Wilkerson wrote:
> I don't understand the reason for these changes. Where in the specification
does
> it say that the 'then' and 'else' clauses of an if statement, the body of a
> 'while' statement, or any of the other places that use visitWithNameScope()
> introduce a new scope?

I've got clarification from Gilad, rolled back.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ScopedVisitor.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/resolver/ScopedVisitor.java:277:
if (!(function.getEnclosingElement() instanceof CompilationUnitElement)) {
On 2013/05/20 20:21:29, Brian Wilkerson wrote:
> We shouldn't add the function to the scope until after the declaration.
> According to the specification:
> 
> A function declaration statement of one of the forms id signature {statements}
> or T id signature {statements} causes a new function named id to be added to
the
> innermost enclosing scope at the point immediately following the function
> declaration statement.
> 
> I believe that the original source was correct.

Done.

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/FunctionScope.java
(right):

https://codereview.chromium.org/15271006/diff/1/editor/tools/plugins/com.goog...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/scope/FunctionScope.java:42:
Scope parameterScope = getEnclosingScope();
On 2013/05/20 20:21:29, Brian Wilkerson wrote:
> Again, I think the original code was correct. According to the specification:
> 
> The scope of a library function is the scope of the enclosing library. The
scope
> of a local function is  described below. In both cases, the name of the
function
> is in scope in the formal parameters scope of the function.
> 
> What isn't clear is whether it is defined in the formal parameter scope's
outer
> scope or in the formal parameter scope itself. Perhaps an e-mail to Gilad to
> clarify the question is in order.

I've got clarification from Gilad "In the formal parameter scope itself."

[Brian Wilkerson, 2013-05-21 13:55:44]

LGTM

[scheglov, 2013-05-21 15:35:50]

Committed patchset #2 manually as r22945 (presubmit successful).