Compute names referenced in a unit.

Compute names referenced in a unit.

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

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

[Brian Wilkerson, 2016-11-18 17:55:51]

Other than the fact that this appears to be very similar to other existing code
(duplication bothers me), lgtm.

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/referenced_names.dart (right):

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:17: class
_LocalNameScope {
Consider renaming (and documenting) the class. It appears that it represents a
general name scope rather than a name scope that only contains local names.

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/test/src/dart/...
File pkg/analyzer/test/src/dart/analysis/referenced_names_test.dart (right):

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/test/src/dart/...
pkg/analyzer/test/src/dart/analysis/referenced_names_test.dart:19: class
ReferencedNamesBuilderTest {
I might be missing something, but it seems to be missing tests to ensure that we
don't return names that are defined locally. Also, that we do return names that
match locally defined names where (a) the declaration is not in scope, or (b)
the declaration is qualified.

[Paul Berry, 2016-11-18 17:59:59]

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/referenced_names.dart (right):

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:182: if (localLevel ==
0) {
Why do we skip this logic for local functions?  It seems like this would cause
us to do the wrong thing for the following code:

void f() {
  void g(x) {
    print(x);  // "x" references a local parameter, not a global.
  }
}

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:197: if (localLevel ==
0) {
Why is this necessary?  I thought function type aliases could only occur at top
level.

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:245: return;
What happens if the code looks like this?

void f(x, C g()) {
  print(g().x);
}

I think we should record "x" as a referenced name, because it refers to the "x"
member of class C.  But it looks like we won't record "x" as a referenced name,
because there is a local in scope called "x".

[scheglov, 2016-11-18 18:33:44]

PTAL

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/referenced_names.dart (right):

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:17: class
_LocalNameScope {
On 2016/11/18 17:55:51, Brian Wilkerson wrote:
> Consider renaming (and documenting) the class. It appears that it represents a
> general name scope rather than a name scope that only contains local names.

I've added a comment.

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:182: if (localLevel ==
0) {
On 2016/11/18 17:59:59, Paul Berry wrote:
> Why do we skip this logic for local functions?  It seems like this would cause
> us to do the wrong thing for the following code:
> 
> void f() {
>   void g(x) {
>     print(x);  // "x" references a local parameter, not a global.
>   }
> }

Acknowledged.
You're right.
I've added the corresponding test.
It turns out we don't need the "localLevel" at all.

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:197: if (localLevel ==
0) {
On 2016/11/18 17:59:59, Paul Berry wrote:
> Why is this necessary?  I thought function type aliases could only occur at
top
> level.

Acknowledged.

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:245: return;
On 2016/11/18 17:59:59, Paul Berry wrote:
> What happens if the code looks like this?
> 
> void f(x, C g()) {
>   print(g().x);
> }
> 
> I think we should record "x" as a referenced name, because it refers to the
"x"
> member of class C.  But it looks like we won't record "x" as a referenced
name,
> because there is a local in scope called "x".

Actually this works.
Local declarations hide only when used unqualified, see "if (!node.isQualified)"
above.
In this example "x" in "g().x" is qualified, so will be added.
I've added the corresponding test.

[Brian Wilkerson, 2016-11-18 18:39:41]

lgtm

[Paul Berry, 2016-11-18 19:15:44]

lgtm

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
File pkg/analyzer/lib/src/dart/analysis/referenced_names.dart (right):

https://codereview.chromium.org/2519443003/diff/1/pkg/analyzer/lib/src/dart/a...
pkg/analyzer/lib/src/dart/analysis/referenced_names.dart:245: return;
On 2016/11/18 18:33:44, scheglov wrote:
> On 2016/11/18 17:59:59, Paul Berry wrote:
> > What happens if the code looks like this?
> > 
> > void f(x, C g()) {
> >   print(g().x);
> > }
> > 
> > I think we should record "x" as a referenced name, because it refers to the
> "x"
> > member of class C.  But it looks like we won't record "x" as a referenced
> name,
> > because there is a local in scope called "x".
> 
> Actually this works.
> Local declarations hide only when used unqualified, see "if
(!node.isQualified)"
> above.
> In this example "x" in "g().x" is qualified, so will be added.
> I've added the corresponding test.

Ah, ok.  I mistakenly thought that "isQualified" just returned true for prefixed
identifiers.  Thanks.

[scheglov, 2016-11-18 19:19:09]

Description was changed from

==========
Compute names referenced in a unit.

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

to

==========
Compute names referenced in a unit.

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

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

[scheglov, 2016-11-18 19:19:10]

Committed patchset #2 (id:20001) manually as
f52b5973c57555c46f88fb617a5666e7266ac254 (presubmit successful).