Consolidate the code that replaces a TypeParameter with its bound.

Consolidate the code that replaces a TypeParameter with its bound.

In the process, I fixed a bug where one version of this same lump of
code wasn't correct anymore now that you can have arbitrarily long
chains of type parameters.

DartType isn't maybe the ideal place for resolveToBound(). I wanted it
somewhere that:

1. Would be obvious enough that someone coming along later would
   discover it instead of reinventing it again.
2. Had access to the various data it needs.
3. Wouldn't need a "is TypeParameterType" guard at every callsite
   before it could be used.

I could put it on TypeProvider instead, since it needs access to Object,
but that felt arbitrary.

It could be a top level function, but then I don't think it would be
very discoverable.

R=brianwilkerson@google.com, jmesserly@google.com

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

[Bob Nystrom, 2016-02-10 01:09:41]

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

[Brian Wilkerson, 2016-02-10 01:54:13]

LGTM

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

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2175: * If this type is a
[TypeParameterType], returns its bound if it has one, or
Judging from the code, if this type is not a TypeParameterType this method
should return `this`. If that's true, we should document it.

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2181: DartType
resolveToBound(DartType objectType) {
It might be more efficient to move this implementation to TypeParameterTypeImpl
and for this method to be a simple "return this;". Not sure this gets called
enough to make it worth doing, though.

Also, consider adding a few tests.

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2185: if (element.bound == null)
return objectType;
nit: block

[Jennifer Messerly, 2016-02-10 16:52:12]

lgtm

[Bob Nystrom, 2016-02-10 21:05:41]

Thanks for the feedback! I have one question about tests before I land this.

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

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2175: * If this type is a
[TypeParameterType], returns its bound if it has one, or
On 2016/02/10 01:54:13, Brian Wilkerson wrote:
> Judging from the code, if this type is not a TypeParameterType this method
> should return `this`. If that's true, we should document it.

Done.

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2181: DartType
resolveToBound(DartType objectType) {
On 2016/02/10 01:54:13, Brian Wilkerson wrote:
> It might be more efficient to move this implementation to
TypeParameterTypeImpl
> and for this method to be a simple "return this;". Not sure this gets called
> enough to make it worth doing, though.

Good call. Done. "is" checks creep me out.

> Also, consider adding a few tests.

This does get tested implicitly by other tests, I think, but I don't mind adding
unit tests. Do you know where I should put them?

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2185: if (element.bound == null)
return objectType;
On 2016/02/10 01:54:13, Brian Wilkerson wrote:
> nit: block

Oops, fixed. Hard habit to change. :(

[Brian Wilkerson, 2016-02-11 15:45:40]

LGTM

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

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2181: DartType
resolveToBound(DartType objectType) {
> Do you know where I should put them?

The rest of the tests for classes in this file are in
.../analyzer/test/src/dart/element/element_test.dart.

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2185: if (element.bound == null)
return objectType;
I have several of those too.

[Bob Nystrom, 2016-02-11 19:27:04]

Description was changed from

==========
Consolidate the code that replaces a TypeParameter with its bound.

In the process, I fixed a bug where one version of this same lump of
code wasn't correct anymore now that you can have arbitrarily long
chains of type parameters.

DartType isn't maybe the ideal place for resolveToBound(). I wanted it
somewhere that:

1. Would be obvious enough that someone coming along later would
   discover it instead of reinventing it again.
2. Had access to the various data it needs.
3. Wouldn't need a "is TypeParameterType" guard at every callsite
   before it could be used.

I could put it on TypeProvider instead, since it needs access to Object,
but that felt arbitrary.

It could be a top level function, but then I don't think it would be
very discoverable.
==========

to

==========
Consolidate the code that replaces a TypeParameter with its bound.

In the process, I fixed a bug where one version of this same lump of
code wasn't correct anymore now that you can have arbitrarily long
chains of type parameters.

DartType isn't maybe the ideal place for resolveToBound(). I wanted it
somewhere that:

1. Would be obvious enough that someone coming along later would
   discover it instead of reinventing it again.
2. Had access to the various data it needs.
3. Wouldn't need a "is TypeParameterType" guard at every callsite
   before it could be used.

I could put it on TypeProvider instead, since it needs access to Object,
but that felt arbitrary.

It could be a top level function, but then I don't think it would be
very discoverable.

R=brianwilkerson@google.com, jmesserly@google.com

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

[Bob Nystrom, 2016-02-11 19:27:05]

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

[Bob Nystrom, 2016-02-11 19:27:25]

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

https://codereview.chromium.org/1686893002/diff/1/pkg/analyzer/lib/src/dart/e...
pkg/analyzer/lib/src/dart/element/type.dart:2181: DartType
resolveToBound(DartType objectType) {
On 2016/02/11 15:45:40, Brian Wilkerson wrote:
> > Do you know where I should put them?
> 
> The rest of the tests for classes in this file are in
> .../analyzer/test/src/dart/element/element_test.dart.

Done.