fix #26141, add support for type arguments to constants

fix #26141, add support for type arguments to constants

R=paulberry@google.com

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

[Brian Wilkerson, 2016-07-21 18:01:49]

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

[Brian Wilkerson, 2016-07-21 18:01:50]

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

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6337: // parameters, then
constructor initializers.
Do you mean that nodes must be cloned in lexical order? If that's true, then
this won't work because you can't assume the order in which these declarations
appear in the source code (fields don't have to appear before constructors).

[Jennifer Messerly, 2016-07-21 18:03:12]

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

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6337: // parameters, then
constructor initializers.
On 2016/07/21 18:01:49, Brian Wilkerson wrote:
> Do you mean that nodes must be cloned in lexical order?

yes

>  If that's true, then
> this won't work because you can't assume the order in which these declarations
> appear in the source code (fields don't have to appear before constructors).

eeeeeep. Yes, good point. Arg. I'll just switch it to make new ConstantAstCloner
objects (that's what got me in trouble)

[Paul Berry, 2016-07-21 20:42:13]

paulberry@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2016-07-21 20:42:15]

Seems like a reasonable approach to me.

You shouldn't need to modify summaries or the element model at all to achieve
this.  The necessary ASTs should already be there.  Probably the reason you
didn't find them is that they're inside the variable elements and the parameter
elements (not in the constructor element).

I've added notes below of the changes I think are unnecessary.

Feel free to hit me up with further questions (VC, chat, or email) if this
doesn't make sense, or isn't working for you.

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1993: List<VariableDeclaration>
_fieldInitializers;
This shouldn't be necessary.  We already store the initializer ASTs for fields
in ConstFieldElementImpl (in the ConstVariableElement mixin).  Note that
ConstFieldElementImpl is a bit of a misnomer--it is actually used both for const
fields and for nonstatic final fields, precisely so that the constant evaluation
engine can evaluate field initializers of classes that have const constructors.

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1999:
List<DefaultFormalParameter> _parameterInitializers;
This shouldn't be necessary either.  We already store the default parameter ASTs
for constant constructors in DefaultParameterElementImpl (again, in the
ConstVariableElement mixin).

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:2059: List<VariableDeclaration>
get fieldInitializers {
This hunk should be unnecessary too.

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

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6333: var clone = new
ConstantAstCloner().cloneNodeList;
This hunk shouldn't be necessary.

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:1722: * If a constant
[UnlinkedExecutableKind.constructor], the field
It should not be necessary to change the summary format.  The field initializers
should already be in UnlinkedVariable.initializer.bodyExpr and the parameter
initializer should already be in UnlinkedParam.initializer.bodyExpr.

[Jennifer Messerly, 2016-07-28 22:28:05]

jmesserly@google.com changed reviewers:
+ leafp@google.com

[Jennifer Messerly, 2016-07-28 22:28:06]

Thanks soooo much Paul! I was able to simplify & clean this up a bunch. PTAL! :)

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
File pkg/analyzer/lib/src/dart/element/element.dart (right):

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1993: List<VariableDeclaration>
_fieldInitializers;
On 2016/07/21 20:42:14, Paul Berry wrote:
> This shouldn't be necessary.  We already store the initializer ASTs for fields
> in ConstFieldElementImpl (in the ConstVariableElement mixin).  Note that
> ConstFieldElementImpl is a bit of a misnomer--it is actually used both for
const
> fields and for nonstatic final fields, precisely so that the constant
evaluation
> engine can evaluate field initializers of classes that have const
constructors.

Nice!!!! Reverted this file :)

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:1999:
List<DefaultFormalParameter> _parameterInitializers;
On 2016/07/21 20:42:14, Paul Berry wrote:
> This shouldn't be necessary either.  We already store the default parameter
ASTs
> for constant constructors in DefaultParameterElementImpl (again, in the
> ConstVariableElement mixin).

Reverted :)

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/element.dart:2059: List<VariableDeclaration>
get fieldInitializers {
On 2016/07/21 20:42:14, Paul Berry wrote:
> This hunk should be unnecessary too.

yup!

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

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/genera...
pkg/analyzer/lib/src/generated/resolver.dart:6333: var clone = new
ConstantAstCloner().cloneNodeList;
On 2016/07/21 20:42:14, Paul Berry wrote:
> This hunk shouldn't be necessary.

Yay! Reverted!

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/summar...
File pkg/analyzer/lib/src/summary/idl.dart (right):

https://codereview.chromium.org/2167263002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/idl.dart:1722: * If a constant
[UnlinkedExecutableKind.constructor], the field
On 2016/07/21 20:42:14, Paul Berry wrote:
> It should not be necessary to change the summary format.  The field
initializers
> should already be in UnlinkedVariable.initializer.bodyExpr and the parameter
> initializer should already be in UnlinkedParam.initializer.bodyExpr.

:D ... moar reverts

[Paul Berry, 2016-07-28 22:50:05]

lgtm

[Jennifer Messerly, 2016-07-28 22:57:17]

Description was changed from

==========
fix #26141, add support for type arguments to constants
==========

to

==========
fix #26141, add support for type arguments to constants

R=paulberry@google.com

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

[Jennifer Messerly, 2016-07-28 22:57:19]

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