Complain about duplicated names.

Complain about duplicated names.

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/0eae9218f4d5677fcfc909bafada38cf7b7305be

[ahe, 2017-05-24 18:34:35]

Patchset #2 (id:20001) has been deleted

[ahe, 2017-05-24 18:34:44]

Patchset #1 (id:1) has been deleted

[ahe, 2017-05-24 18:36:17]

ahe@google.com changed reviewers:
+ johnniwinther@google.com, paulberry@google.com

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

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:317: return;
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"

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
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.

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2155: // there was a
compile-time error.
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-24 21:06:38]

Thank you, Paul!

I filed this bug: https://github.com/dart-lang/sdk/issues/29717

Would you also like to have TODOs in the code?

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:317: return;
On 2017/05/24 20:46:15, Paul Berry wrote:
> 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"

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.

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/24 20:46:15, Paul Berry wrote:
> 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.

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]

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/24 21:06:38, ahe wrote:
> On 2017/05/24 20:46:15, Paul Berry wrote:
> > 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.
> 
> 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.

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:11:35]

On 2017/05/24 21:06:38, ahe wrote:
> Thank you, Paul!
> 
> I filed this bug: https://github.com/dart-lang/sdk/issues/29717
> 
> Would you also like to have TODOs in the code?

I would still appreciate TODOs in the code; the bug is fairly general and I
worry that we will forget the exact places in the code that need to be updated.

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

lgtm assuming TODO comments are added

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/24 22:10:39, Paul Berry wrote:
> On 2017/05/24 21:06:38, ahe wrote:
> > On 2017/05/24 20:46:15, Paul Berry wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

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

[Paul Berry, 2017-05-24 22:13:00]

lgtm assuming TODO comments are added

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

Thank you, Paul!

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/24 22:12:59, Paul Berry wrote:
> On 2017/05/24 22:10:39, Paul Berry wrote:
> > On 2017/05/24 21:06:38, ahe wrote:
> > > On 2017/05/24 20:46:15, Paul Berry wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> If you want to defer this to a future CL, that's ok with me, provided that you
> add a TODO comment here.

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]

Still lgtm assuming TODO comments are added

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/25 10:38:31, ahe wrote:
> On 2017/05/24 22:12:59, Paul Berry wrote:
> > On 2017/05/24 22:10:39, Paul Berry wrote:
> > > On 2017/05/24 21:06:38, ahe wrote:
> > > > On 2017/05/24 20:46:15, Paul Berry wrote:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> > If you want to defer this to a future CL, that's ok with me, provided that
you
> > add a TODO comment here.
> 
> 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.

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]

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/25 11:47:17, Paul Berry wrote:
> On 2017/05/25 10:38:31, ahe wrote:
> > On 2017/05/24 22:12:59, Paul Berry wrote:
> > > On 2017/05/24 22:10:39, Paul Berry wrote:
> > > > On 2017/05/24 21:06:38, ahe wrote:
> > > > > On 2017/05/24 20:46:15, Paul Berry wrote:
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > If you want to defer this to a future CL, that's ok with me, provided that
> you
> > > add a TODO comment here.
> > 
> > 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.
> 
> 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!

I will add comments.

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

Thank you!

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:317: return;
On 2017/05/24 21:06:38, ahe wrote:
> On 2017/05/24 20:46:15, Paul Berry wrote:
> > 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"
> 
> 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.

Done.

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:326: discardedStatement =
pop();
On 2017/05/25 13:16:15, ahe wrote:
> On 2017/05/25 11:47:17, Paul Berry wrote:
> > On 2017/05/25 10:38:31, ahe wrote:
> > > On 2017/05/24 22:12:59, Paul Berry wrote:
> > > > On 2017/05/24 22:10:39, Paul Berry wrote:
> > > > > On 2017/05/24 21:06:38, ahe wrote:
> > > > > > On 2017/05/24 20:46:15, Paul Berry wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.
> > > > 
> > > > If you want to defer this to a future CL, that's ok with me, provided
that
> > you
> > > > add a TODO comment here.
> > > 
> > > 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.
> > 
> > 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!
> 
> I will add comments.

Done.

https://codereview.chromium.org/2902113005/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2155: // there was a
compile-time error.
On 2017/05/24 20:46:15, Paul Berry wrote:
> 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"

Done.

[ahe, 2017-05-29 07:45:11]

Description was changed from

==========
Complain about duplicated names.
==========

to

==========
Complain about duplicated names.

R=paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0eae9218f4d5677fcfc909bafada38cf7b7305be
==========

[ahe, 2017-05-29 07:45:13]

Committed patchset #5 (id:120001) manually as
0eae9218f4d5677fcfc909bafada38cf7b7305be (presubmit successful).