Resynthesize type references.

Resynthesize type references.

R=paulberry@google.com
BUG=

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

[Paul Berry, 2016-01-31 13:41:02]

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

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:416: if (info.type != null) {
This won't work properly if the class in question takes generic parameters (see
the comment on _ReferenceInfo.type, which says "If this reference refers to a
non-generic type, the type it refers to.  Otherwise `null`."

To fix it, we could either (a) change the code here call `info.buildType` and
pass the appropriate number of instances of `dynamic` to populate the type
parameters, or (b) change the behavior of the _ReferenceInfo constructor so that
it always populates _ReferenceInfo.type.

I think I would prefer (b).  If you'd prefer to leave this as a failing test
case and let me fix it, let me know.

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:455: //    node.staticType =
resynthesizer.summaryResynthesizer.typeProvider.typeType;
I'm confused about this commented out code and the comment above it.  If the
resolver does not do this, how are the tests passing?

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/resynthesize_test.dart (right):

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/resynthesize_test.dart:1011:
test_const_reference_type() {
There should be a test case for the problem mentioned earlier, e.g.:

    class D<T> {}
    const vGenericClass = D;

[scheglov, 2016-01-31 18:55:29]

PTAL

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

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:416: if (info.type != null) {
On 2016/01/31 13:41:02, Paul Berry wrote:
> This won't work properly if the class in question takes generic parameters
(see
> the comment on _ReferenceInfo.type, which says "If this reference refers to a
> non-generic type, the type it refers to.  Otherwise `null`."
> 
> To fix it, we could either (a) change the code here call `info.buildType` and
> pass the appropriate number of instances of `dynamic` to populate the type
> parameters, or (b) change the behavior of the _ReferenceInfo constructor so
that
> it always populates _ReferenceInfo.type.
> 
> I think I would prefer (b).  If you'd prefer to leave this as a failing test
> case and let me fix it, let me know.

Done.

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:455: //    node.staticType =
resynthesizer.summaryResynthesizer.typeProvider.typeType;
On 2016/01/31 13:41:02, Paul Berry wrote:
> I'm confused about this commented out code and the comment above it.  If the
> resolver does not do this, how are the tests passing?

1. We should do this.
2. Resolver does not do this.
3. So, this code is commented out for now in constants AST resynthesizer.
4. If we uncomment the code, tests will start *failing*.

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/test/src/summa...
File pkg/analyzer/test/src/summary/resynthesize_test.dart (right):

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/test/src/summa...
pkg/analyzer/test/src/summary/resynthesize_test.dart:1011:
test_const_reference_type() {
On 2016/01/31 13:41:02, Paul Berry wrote:
> There should be a test case for the problem mentioned earlier, e.g.:
> 
>     class D<T> {}
>     const vGenericClass = D;

Done.

[scheglov, 2016-01-31 20:24:01]

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

https://codereview.chromium.org/1650833002/diff/1/pkg/analyzer/lib/src/summar...
pkg/analyzer/lib/src/summary/resynthesize.dart:455: //    node.staticType =
resynthesizer.summaryResynthesizer.typeProvider.typeType;
On 2016/01/31 18:55:29, scheglov wrote:
> On 2016/01/31 13:41:02, Paul Berry wrote:
> > I'm confused about this commented out code and the comment above it.  If the
> > resolver does not do this, how are the tests passing?
> 
> 1. We should do this.
> 2. Resolver does not do this.
> 3. So, this code is commented out for now in constants AST resynthesizer.
> 4. If we uncomment the code, tests will start *failing*.

Actually, I'm wrong.
Resolver does set the type correctly.
But ConstantAstCloner does not copy it.
I guess we don't use types during computing constant values.
So, I'm removing this commented line and the static type check in tests.

[Paul Berry, 2016-02-01 16:11:40]

lgtm

https://codereview.chromium.org/1650833002/diff/40001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/resynthesize.dart (right):

https://codereview.chromium.org/1650833002/diff/40001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/resynthesize.dart:1670: DartType typeArgument =
getTypeArgument != null
Nit: Rather than make this change (which will burden all calls to _buildType),
how about just changing line 1623 from:

    type = _buildType(null, null);

to:

    type = _buildType((_) => DynamicTypeImpl.instance, null);

https://codereview.chromium.org/1650833002/diff/40001/pkg/analyzer/test/src/s...
File pkg/analyzer/test/src/summary/resynthesize_test.dart (right):

https://codereview.chromium.org/1650833002/diff/40001/pkg/analyzer/test/src/s...
pkg/analyzer/test/src/summary/resynthesize_test.dart:244: expect(r.name,
o.name);
Leave a comment here explaining why we shouldn't compare staticType.

Otherwise I'm likely to add it back in by mistake later this week while I'm
cleaning up the resynthesis code.

[scheglov, 2016-02-01 17:18:05]

Description was changed from

==========
Resynthesize type references.

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

to

==========
Resynthesize type references.

R=paulberry@google.com
BUG=

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

[scheglov, 2016-02-01 17:18:07]

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

[scheglov, 2016-02-01 17:22:26]

https://codereview.chromium.org/1650833002/diff/40001/pkg/analyzer/lib/src/su...
File pkg/analyzer/lib/src/summary/resynthesize.dart (right):

https://codereview.chromium.org/1650833002/diff/40001/pkg/analyzer/lib/src/su...
pkg/analyzer/lib/src/summary/resynthesize.dart:1670: DartType typeArgument =
getTypeArgument != null
On 2016/02/01 16:11:40, Paul Berry wrote:
> Nit: Rather than make this change (which will burden all calls to _buildType),
> how about just changing line 1623 from:
> 
>     type = _buildType(null, null);
> 
> to:
> 
>     type = _buildType((_) => DynamicTypeImpl.instance, null);

Done.