Fixes to associating existing elements with an AST

Fixes to associating existing elements with an AST

R=paulberry@google.com, scheglov@google.com

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

[Brian Wilkerson, 2016-03-04 21:37:32]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com, scheglov@google.com

[scheglov, 2016-03-04 21:49:30]

LGTM

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/element.dart:1209: annotationMap = new
HashMap<int, List<ElementAnnotation>>();
annotationMap ??= new HashMap<int, List<ElementAnnotation>>(); would be a bit
more compact.

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/dart.dart:1018: cacheEntry, COMPILATION_UNIT_ELEMENT))
{
LIBRARY_ELEMENT2

[Paul Berry, 2016-03-04 22:02:11]

Comments below assume that if time allows, we would like to try to cherry-pick
this CL to the stable release candidate (which is what I recall us discussing).

If I'm remembering wrong, and you don't intend to try to cherry-pick this CL,
feel free to ignore my comments about reducing the risk of merge conflicts and
minimizing behavioral changes.

lgtm

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/element.dart:1208: if (annotationMap == null)
{
Nit: this seems like a premature optimization (there are not very compilation
units and an empty hash map is cheap to create).  I'd recommend making
annotationMap final and initializing it at the declaration site.

(It's possible that that might even be a speed improvement, since making the
annotationMap final and initializing it at the declaration site guarantees to
the VM that it will never be null, so it may be able to eliminate some null
checks).

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/element.dart:1307: @override
Normally I am ok with rolling this sort of clean-up into an unrelated CL like
this, but since we are likely to want to try cherry-picking this CL over to the
stable release candidate, I think we should keep it as small as possible to
reduce the risk of merge conflicts.

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:2745: if (nodeCount !=
annotations.length) {
Nit: in the previous version of the code, this check is done in an assert(),
meaning that it only happens in checked mode (which in practice means, it only
happens during unit tests).  Now it happens in production as well.

If we weren't planning to try to cherry-pick this CL to the stable release
candidate, I would be ok changing the behavior.  But since we are contemplating
a cherry-pick, I think we should minimize behavioral changes as much as
possible.

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:2747: 'Found $nodeCount annotation
nodes and ${annotations.length} element annotations',
Nit: this line is more than 80 columns.  I recommend splitting it into two
strings so that the formatter can squeeze it into 80 columns, e.g.:

_mismatch(
    'Found $nodeCount annotation nodes and ${annotations.length}'
    ' element annotations',
    parent);

[Brian Wilkerson, 2016-03-07 16:20:28]

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/element.dart:1208: if (annotationMap == null)
{
I have almost 3000 CompilationUnitElements in my environment, and I don't have
nearly as much code loaded as some of our users. But you might be right. I'm
leaving it in for now, but we can back it out later if it proves to be
unnecessary.

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/element.dart:1209: annotationMap = new
HashMap<int, List<ElementAnnotation>>();
Done

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/da...
pkg/analyzer/lib/src/dart/element/element.dart:1307: @override
Not sure it didn't take more time to remove them, but it's done.

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ge...
File pkg/analyzer/lib/src/generated/resolver.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:2745: if (nodeCount !=
annotations.length) {
I made the change intentionally so that we would catch bugs earlier because it
makes it much easier to fix them. Given that this code should never be triggered
anyway, I think it's a positive change for users, but if you feel strongly
enough about it I guess I can temporarily back it out.

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ge...
pkg/analyzer/lib/src/generated/resolver.dart:2747: 'Found $nodeCount annotation
nodes and ${annotations.length} element annotations',
Done

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/dart.dart (right):

https://codereview.chromium.org/1768713002/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/dart.dart:1018: cacheEntry, COMPILATION_UNIT_ELEMENT))
{
Good catch! Done.

[Brian Wilkerson, 2016-03-07 18:36:38]

Description was changed from

==========
Fixes to associating existing elements with an AST
==========

to

==========
Fixes to associating existing elements with an AST

R=paulberry@google.com, scheglov@google.com

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

[Brian Wilkerson, 2016-03-07 18:36:39]

Committed patchset #3 (id:40001) manually as
508492d048848666b4a6166815dcdea281e16558 (presubmit successful).