Substitution added to DartType

Substitution added to DartType

Committed: https://code.google.com/p/dart/source/detail?r=15290

[ahe, 2012-11-22 15:43:36]

LGTM!

I'd like to be understand the invariants you have in mind for substitutions. 
Basically, we should just go over the subst methods face-to-face.

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:93: * or interface
type. For example, in class Array<E> { ... }`,
Missing begin `.  Also, we have been pretty consistent about using [: ... :]
through out the compiler, and as long as there isn't a specification of
documentation comments, I prefer to stick with that style.

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:93: * or interface
type. For example, in class Array<E> { ... }`,
Start a new paragraph before "For example".

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:93: * or interface
type. For example, in class Array<E> { ... }`,
Should you get rid of "or interface type"?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:96: * Each
class/interface should have its own unique type variables,
Remove "interface"?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:97: * one for each
type parameter. A class/interface with type parameters
Remove "interface"?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:108: * For example,
given a box, `class Box<T> { T value; }`, the
[: ... :]

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:125: // Return fast
on empty substitutions.
Not sure about this.  What if arguments isn't empty?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:130: // Raw
substitution: Dynamic replaces parameters.
I'm surprised that you're doing this here.  Let's chat about that face-to-face.

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:149: }
What happens if the lists doesn't have the same length?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:249: if
(parameters.isEmpty) {
Not sure about this.  Face-to-face?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:255: return new
InterfaceType(element);
We should consider canonicalizing this.

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:326: if
(parameters.isEmpty) {
Guess what? Not sure about this :-)

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:342: }
Could this loop be a general purpose method and potentially share the parameter
types if the didn't change?

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:411: if
(parameters.isEmpty) {
Still not sure about this :-)

https://codereview.chromium.org/11418113/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/typechecker.dart:430: }
Could loop be shared?

https://codereview.chromium.org/11418113/diff/4001/tests/compiler/dart2js/typ...
File tests/compiler/dart2js/type_substitution_test.dart (right):

https://codereview.chromium.org/11418113/diff/4001/tests/compiler/dart2js/typ...
tests/compiler/dart2js/type_substitution_test.dart:20: var
What is this?

https://codereview.chromium.org/11418113/diff/4001/tests/compiler/dart2js/typ...
tests/compiler/dart2js/type_substitution_test.dart:193: test(compiler,
arguments, const Link<DartType>(), "void1", "void1");
Generally, these tests would be easier to read if the string were type
expressions that were parsed an resolved in a context.

[Johnni Winther, 2012-11-23 13:06:16]

PTAL

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:93: * or interface
type. For example, in class Array<E> { ... }`,
On 2012/11/22 15:43:36, ahe wrote:
> Missing begin `.  Also, we have been pretty consistent about using [: ... :]
> through out the compiler, and as long as there isn't a specification of
> documentation comments, I prefer to stick with that style.

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:93: * or interface
type. For example, in class Array<E> { ... }`,
On 2012/11/22 15:43:36, ahe wrote:
> Start a new paragraph before "For example".

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:93: * or interface
type. For example, in class Array<E> { ... }`,
On 2012/11/22 15:43:36, ahe wrote:
> Should you get rid of "or interface type"?

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:96: * Each
class/interface should have its own unique type variables,
On 2012/11/22 15:43:36, ahe wrote:
> Remove "interface"?

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:97: * one for each
type parameter. A class/interface with type parameters
On 2012/11/22 15:43:36, ahe wrote:
> Remove "interface"?

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:108: * For example,
given a box, `class Box<T> { T value; }`, the
On 2012/11/22 15:43:36, ahe wrote:
> [: ... :]

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:125: // Return fast
on empty substitutions.
On 2012/11/22 15:43:36, ahe wrote:
> Not sure about this.  What if arguments isn't empty?

Assertion inserted.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:130: // Raw
substitution: Dynamic replaces parameters.
On 2012/11/22 15:43:36, ahe wrote:
> I'm surprised that you're doing this here.  Let's chat about that
face-to-face.

Removed.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:149: }
On 2012/11/22 15:43:36, ahe wrote:
> What happens if the lists doesn't have the same length?

Assertion inserted.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:249: if
(parameters.isEmpty) {
On 2012/11/22 15:43:36, ahe wrote:
> Not sure about this.  Face-to-face?

Assertion inserted.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:326: if
(parameters.isEmpty) {
On 2012/11/22 15:43:36, ahe wrote:
> Guess what? Not sure about this :-)

Assertion inserted.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:342: }
On 2012/11/22 15:43:36, ahe wrote:
> Could this loop be a general purpose method and potentially share the
parameter
> types if the didn't change?

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:411: if
(parameters.isEmpty) {
On 2012/11/22 15:43:36, ahe wrote:
> Still not sure about this :-)

Assertion inserted.

https://chromiumcodereview.appspot.com/11418113/diff/4001/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:430: }
On 2012/11/22 15:43:36, ahe wrote:
> Could loop be shared?

Done.

https://chromiumcodereview.appspot.com/11418113/diff/4001/tests/compiler/dart...
File tests/compiler/dart2js/type_substitution_test.dart (right):

https://chromiumcodereview.appspot.com/11418113/diff/4001/tests/compiler/dart...
tests/compiler/dart2js/type_substitution_test.dart:20: var
On 2012/11/22 15:43:36, ahe wrote:
> What is this?

The method has been removed.

[ahe, 2012-11-23 15:06:54]

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:134: if (parameter ==
this) {
assert(argumentLink.tail.isEmpty == parameterLink.tail.isEmpty);

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:452:
DynamicType(Element element) : super(element);
ClassElement?

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:544: if
(arguments.isEmpty) {
throw exception?

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:554:
parameters.forEach((_) {
Check that parameters and arguments have same length.

[Johnni Winther, 2012-11-26 07:38:48]

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
File sdk/lib/_internal/compiler/implementation/typechecker.dart (right):

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:134: if (parameter ==
this) {
On 2012/11/23 15:06:54, ahe wrote:
> assert(argumentLink.tail.isEmpty == parameterLink.tail.isEmpty);

Done.

https://chromiumcodereview.appspot.com/11418113/diff/5014/sdk/lib/_internal/c...
sdk/lib/_internal/compiler/implementation/typechecker.dart:452:
DynamicType(Element element) : super(element);
On 2012/11/23 15:06:54, ahe wrote:
> ClassElement?

Done.