Distinguish SimpleIdentifier uses and declarations at parse time.

Distinguish SimpleIdentifier uses and declarations at parse time.

Instead of a very slow .inDeclarationContext() method which determines
whether a given identifier is in a use position or not, we just create
a different subclass at parse time that encodes that fact directly.

Also, remove an unnecessary _validateElement() method.

On my Mac laptop, this takes benchmark/errors_in_all_libraries.dart

From: 0:00:05.673849
To:   0:00:05.193563 (91.53%)

R=brianwilkerson@google.com

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

[Bob Nystrom, 2016-03-24 23:44:20]

rnystrom@google.com changed reviewers:
+ brianwilkerson@google.com

[Brian Wilkerson, 2016-03-25 15:37:31]

LGTM

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/dart/ast/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast.dart:6708: if (isDeclaration) return new
DeclaredSimpleIdentifier(token);
nit: block

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2575: new
SimpleIdentifier(methodName.token, isDeclaration: false),
Why do we need to create a different node? We passed `true` in when parsing the
identifier. (Here and several places below.)

[Bob Nystrom, 2016-03-25 17:38:25]

Description was changed from

==========
Distinguish SimpleIdentifier uses and declarations at parse time.

Instead of a very slow .inDeclarationContext() method which determines
whether a given identifier is in a use position or not, we just create
a different subclass at parse time that encodes that fact directly.

Also, remove an unnecessary _validateElement() method.

On my Mac laptop, this takes benchmark/errors_in_all_libraries.dart

From: 0:00:05.673849
To:   0:00:05.193563 (91.53%)
==========

to

==========
Distinguish SimpleIdentifier uses and declarations at parse time.

Instead of a very slow .inDeclarationContext() method which determines
whether a given identifier is in a use position or not, we just create
a different subclass at parse time that encodes that fact directly.

Also, remove an unnecessary _validateElement() method.

On my Mac laptop, this takes benchmark/errors_in_all_libraries.dart

From: 0:00:05.673849
To:   0:00:05.193563 (91.53%)

R=brianwilkerson@google.com

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

[Bob Nystrom, 2016-03-25 17:38:27]

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

[Paul Berry, 2016-03-28 15:29:10]

On 2016/03/25 17:38:27, Bob Nystrom wrote:
> Committed patchset #2 (id:20001) manually as
> 31aa8e26df435226bc739e0ac6102381efe0beb1 (presubmit successful).

This CL broke some analysis server tests.  See for example
https://build.chromium.org/p/client.dart/builders/pkg-linux-be/builds/2553/st...

I can repro locally with: python tools/test.py -aia32 --checked -mrelease -t60
pkg/analysis_server/test
(note: the "-aia32" is because my most recent build was a 32-bit build)

Bob, would you mind having a look?

[Bob Nystrom, 2016-03-28 16:09:16]

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/dart/ast/a...
File pkg/analyzer/lib/dart/ast/ast.dart (right):

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/dart/ast/a...
pkg/analyzer/lib/dart/ast/ast.dart:6708: if (isDeclaration) return new
DeclaredSimpleIdentifier(token);
On 2016/03/25 15:37:31, Brian Wilkerson wrote:
> nit: block

Haha, fixed. I swear one day I will remember to not do this.

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/src/genera...
File pkg/analyzer/lib/src/generated/parser.dart (right):

https://codereview.chromium.org/1833573006/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/parser.dart:2575: new
SimpleIdentifier(methodName.token, isDeclaration: false),
On 2016/03/25 15:37:31, Brian Wilkerson wrote:
> Why do we need to create a different node? We passed `true` in when parsing
the
> identifier. (Here and several places below.)

Great question. It's because the methodName parsed on 2564 is used in a
declaration context in some code paths but not in others. On 2587, it's a
declaration context (the constructor name). Here, it's not (it's the return
type). The other similar cases below are the same thing. We parse an identifier
once but it may conditionally end up in a declaration context or not.

There's probably a more elegant way to rearrange the code and only create the
identifier once, but I was trying to minimize the impact of the optimization.
When we've got a little more time, I'm hoping we can do a more comprehensive fix
by splitting SimpleIdentifier and DeclaredSimpleIdentifier completely. If that
happens, it will probably make sense to rearrange the parsing code a little bit
too.