Support runtime check of function types.

Support runtime check of function types.

New scheme for checking function subtyping:

1) Generate an $isX predicate on objects that are statically known subtypes of a given function type
2) Generate a signature encoding the whole function type on objects when a runtime check is needed.

A check succeeds if either 1) or 2) is true.

If too many predicates are needed a signature is generated instead, and if a signature is needed, no predicates are generated. Maximum number of predicates is currently determined by the magic constant MAX_FUNCTION_TYPE_PREDICATES in emitter.dart.

R=karlklose@google.com

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

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

[Johnni Winther, 2013-02-26 15:48:36]

Hi Karl,

Take a look at this implementation. Is uses the same overall strategy of having
an is-operator check and a subsequent signature-check if needed.

Bounded closure still have no runtime type information and
variability points are not used yet.

[Johnni Winther, 2013-03-13 08:53:12]

PTAL

[ngeoffray, 2013-03-13 09:30:46]

Thank you for all the tests!

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1513: var
targetSignature = getField(target, r'$signature');
Using $signature directly is brittle, you should probably add a new JS.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1515: // The target
is statically known to be a subtype so further check is
further or no further? Could you add an example?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1602: if
(getField(t, 'func') == true) {
What is this? Probably needs an abstraction.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1666: // using the
JavaScript variable [:$name:] to hold the property names.
To not make it a hack, please create a JavaScript closure. We should make sure
all JS are expressions.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1966:
ClassElement thisClass = enclosingElement.getEnclosingClass();
Please add a comment.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2253: ClassElement
thisClass = work.element.getEnclosingClass();
thisClass can be null right? Don't you need to check it?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2265:
createForeign('this.$substitutionName', HType.UNKNOWN, []);
Change "this" to use #.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2687: if (type is
FunctionType) {
Use the kind instead?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2688:
compiler.enqueuer.codegen.registerIsCheck(type, work.resolutionTree);
Add a TODO that this should be done in codegen.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2697: ClassElement
contextClass = Types.getClassContext(type);
What is this?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2700: context = new
HThis(contextClass, HType.UNKNOWN);
What's this HThis? Why isn't it the one of the builder?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2259: if (type is
FunctionType) {
Use the kind?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:52: final
Set<Element> callMethods;
Please add comments on callMethods and closurizedMembers.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:137:
potentiallyAddForRti(variable.enclosingElement);
Why wasn't the previous code for type variable good enough?

[karlklose, 2013-03-13 10:33:34]

I'll wait with the full review until you upload the next version we talked
about.

https://codereview.chromium.org/12334070/diff/35001/tests/language/function_s...
File tests/language/function_subtype_call1_test.dart (right):

https://codereview.chromium.org/12334070/diff/35001/tests/language/function_s...
tests/language/function_subtype_call1_test.dart:25: //print(new
C1<bool>().runtimeType);
Debug code.

[Johnni Winther, 2013-03-22 07:30:24]

New scheme for checking subtypes. PTAL

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1513: var
targetSignature = getField(target, r'$signature');
On 2013/03/13 09:30:46, ngeoffray wrote:
> Using $signature directly is brittle, you should probably add a new JS.

Done.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1515: // The target
is statically known to be a subtype so further check is
On 2013/03/13 09:30:46, ngeoffray wrote:
> further or no further? Could you add an example?

Not needed in the new encoding.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1602: if
(getField(t, 'func') == true) {
On 2013/03/13 09:30:46, ngeoffray wrote:
> What is this? Probably needs an abstraction.

A tag used to identify function type representations. 'func' changed to used
foreign helper.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:1666: // using the
JavaScript variable [:$name:] to hold the property names.
On 2013/03/13 09:30:46, ngeoffray wrote:
> To not make it a hack, please create a JavaScript closure. We should make sure
> all JS are expressions.

How?

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:1966:
ClassElement thisClass = enclosingElement.getEnclosingClass();
On 2013/03/13 09:30:46, ngeoffray wrote:
> Please add a comment.

Changed to use the type of [function] itself, thus registering the function if
is uses type variables.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2253: ClassElement
thisClass = work.element.getEnclosingClass();
On 2013/03/13 09:30:46, ngeoffray wrote:
> thisClass can be null right? Don't you need to check it?

Stale

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2265:
createForeign('this.$substitutionName', HType.UNKNOWN, []);
On 2013/03/13 09:30:46, ngeoffray wrote:
> Change "this" to use #.

Stale

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2687: if (type is
FunctionType) {
On 2013/03/13 09:30:46, ngeoffray wrote:
> Use the kind instead?

Done.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2697: ClassElement
contextClass = Types.getClassContext(type);
On 2013/03/13 09:30:46, ngeoffray wrote:
> What is this?

The class which declared the used type variables, if any.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2700: context = new
HThis(contextClass, HType.UNKNOWN);
On 2013/03/13 09:30:46, ngeoffray wrote:
> What's this HThis? Why isn't it the one of the builder?

Changed.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2259: if (type is
FunctionType) {
On 2013/03/13 09:30:46, ngeoffray wrote:
> Use the kind?

Refactored.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:52: final
Set<Element> callMethods;
On 2013/03/13 09:30:46, ngeoffray wrote:
> Please add comments on callMethods and closurizedMembers.

Done.

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/world.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/world.dart:137:
potentiallyAddForRti(variable.enclosingElement);
On 2013/03/13 09:30:46, ngeoffray wrote:
> Why wasn't the previous code for type variable good enough?

This code handles both types [:T:] and [:void f(T t):].

https://codereview.chromium.org/12334070/diff/35001/tests/language/function_s...
File tests/language/function_subtype_call1_test.dart (right):

https://codereview.chromium.org/12334070/diff/35001/tests/language/function_s...
tests/language/function_subtype_call1_test.dart:25: //print(new
C1<bool>().runtimeType);
On 2013/03/13 10:33:34, karlklose wrote:
> Debug code.

Removed.

[karlklose, 2013-03-22 13:17:46]

LGTM.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:437: if (operatorString
== 'is' || operatorString == 'as') {
I would really like to encapsulate this pattern. I think we have it in multiple
places now.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:471: if (type == null)
return;
How can this happen?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:133: bool
needsRti(ClassElement cls);
Should we rename this to classNeedsRti?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:134: bool
methodNeedsRti(Element cls);
'Element' -> 'FunctionElement'.
'cls' -> 'function'.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:163: Enqueuer enqueuer,
TreeElements elements) {}
Break line after each parameter to keep the code in the same style as above.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:221: bool
methodNeedsRti(Element cls) => false;
See comments on abstract super.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:154: if
(!cls.typeVariables.isEmpty) {
use '&&'.  Consider storing '!cls.typeVariables.isEmpty' in a local.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:282:
instanceMembersByName[memberName] = const Link<Element>();
Could you add a comment why you do this?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:109: * program
contains `x is F<int>` and `x is F<bool>` then the TypedefElement
Please update comment and change to use [::] instead of ``.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:115: = new
Map<ClassElement, Set<FunctionType>>();
Break after '='.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:117:
Set<FunctionType> checkedNonGenericFunctionTypes
Ditto.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1327:
buffer.write('$holder.${namer.substitutionName(check)}$_=${_}$body$N');
Long line.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1339: for
(FunctionType type in functionTypes) {
I would indent the body of the function by two spaces.

Did you need to break before the first argument; would that work?

checkedGenericFunctionTypes.forEach((ClassElement cls,             
                                     Set<FunctionType> functionTypes) {

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1690: *
statically known to be a subtype of it.
Explain what being false means.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1719: void
emitFunctionTypeSignature(Element method, FunctionType type),
Long line.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1860: bool
knownSubtype) {
Align with '(FunctionType...'.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1862:
registerDynamicFunctionTypeCheck(functionType);
Why are we registering it in the emitter. Shouldn't the codegen register it?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1868: bool
hasSignature = false;
'signatureEmitted'?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1875: bool
knownSubtype) {
Align.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2018: String
encoding = rti.getSignatureEncoding(methodType, () => 'null');
Please explain what null means here.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:787: return
'${CURRENT_ISOLATE}';
No {} needed.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:876: for
(Link<DartType> link = type.optionalParameterTypes;
Don't we need a separator between the sets of paramters?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:102: //
Compute the set of all classes that need runtime type information.
Update the comment.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:123:
compiler.resolverWorld.closurizedGenericMembers.forEach(analyzeMethod);
Long line.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:18:
forwardRuntimeTypeInfo(target, substitution, source) {
Is this still used?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:116: } else if
(isJsFunction(substitution)) {
Is this path used?

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:165: *
[signatureName]. The
Please finish the

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:185: var
targetSignature =  invokeOn(targetSignatureFunction, target, null);
remove space.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:318: if (t == null)
return true;
What does null mean here?

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_bound_closure5_test.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_bound_closure5_test.dart:34: class D<S,T>
extends C<T> {}
Space, please.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_call2_test.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_call2_test.dart:28: Expect.isFalse(new
D1<String, bool>() is Baz, 'new D1<String, bool>() is Baz');
Long line here and 2x below.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_local4_test.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_local4_test.dart:6: // Check function subtyping
for generic bound closures.
Please add that this is testing type argument substitution.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_local4_test.dart:30: class D<S,T> extends C<T>
{}
Space.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_null.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_null.dart:12: Expect.isFalse(null is Foo, 'foo
is Foo');
Is that true?

[Johnni Winther, 2013-06-12 14:39:10]

PTAL

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:437: if (operatorString
== 'is' || operatorString == 'as') {
On 2013/03/22 13:17:46, karlklose wrote:
> I would really like to encapsulate this pattern. I think we have it in
multiple
> places now.

I looked. They are all slightly different.

[karlklose, 2013-06-19 14:37:05]

Comments so far.

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:132: bool
needsRti(ClassElement cls);
Should we rename it to classNeedsRti?

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1412:
compiler.findHelper(const SourceString('defineProperty')));
Why is that necessary? Shouldn't it be enough to register the class as
instantiated?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_types.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_types.dart:1153: new
SubtypeVisitor(compiler, dynamicType, voidType);
Can these two allocations be const? (see comment on the PotentialSubtype
constrcutor)

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_types.dart:1233: class
PotentialSubtypeVisitor extends SubtypeVisitor {
Please add a comment on what this class does (in comparison to the
SubtypeVisitor).

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_types.dart:1234:
PotentialSubtypeVisitor(Compiler compiler,
Can this constructor be const?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:180: * Documentation
wanted -- johnniwinther
I think we can remove this 'documentation'.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:235: if
(!cls.typeVariables.isEmpty) {
Join the two conditions.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:404: map[memberName] =
remaining.toLink(map[memberName]);
Isn't the constant empty link already the default value for the parameter?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:560: void
registerClosure(Element element, TreeElements elements) {
'registerClosurizedMember'?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:567: void
registerGenericClosure(Element element, TreeElements elements) {
'registerClosurizedGenericMember'?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:795: if
(type.isMalformed) {
Merge into an else if.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:804:
world.addToWorkList(
Merge error?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1097: ? const
PropertyCheckedModeHelper(const
SourceString("numberOrStringSuperNativeTypeCast"))
A lot of long lines from here on...

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:113: * Raw
Typedef symbols occuring in is-checks and type assertions.  If the
Please update the comment.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1312: //
TODO(karlklose): support typedefs with variables.
This TODO is obsolete now, isn't it?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1353: String
encoding = rti.getTypeEncoding(type);
Create a function addEncoding for l.1353-1354 and call
checkedNonGenericFunctionTypes.forEach(addEncoding)

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1358:
(ClassElement cls, Set<FunctionType> functionTypes) {
If you replace 'ClassElement cls' with '_' I think you can fit it into the
previous line. Or call
checkedGenericFunctionTypes.values().forEach(addEncoding).

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1499: String
helperName = namer.isolateAccess(helperElement);
You can inline helperElement here (it would remove the check for FunctionElement
though).

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1766: void
emitIsFunctionTypeTest(FunctionType type),
You could add Typedefs for these three function types.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1875: void
generateFunctionTypeTests(
Could you get rid of this method? I don't think it adds readability and you can
simply add inline the call to getFunctionTypeChecksOn at the call site.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1904: bool
hasSignature = false;
Perhaps alwaysUseSignature would be clearer.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1911: bool
knownSubtype) {
This should fit on one line.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1912: if
(knownSubtype || !hasDynamicFunctionTypeCheck) {
!knownSubtype implies hasDynamicFunctionTypeCheck, so that last check is not
necessary.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2226:
RuntimeTypes rti = backend.rti;
You can inline rti.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2234: void
emitIsFunctionTypeTest(FunctionType functionType) {
I think we should be more careful with names in the emitter; this is not a test
we are generating but a flag/properties/...

We should clean that up in the whole emitter.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2272:
jsAst.Expression newClosure = js(closureClass).newWith(arguments);
Why this change?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2314: DartType
memberType = member.computeType(compiler);
This is not used.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (left):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:844: 
Is this intentional? See also l. 1018.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:797: String
operatorIsType(DartType type) {
Would it make sense to replace operatorIs with a version taking a DartType and
use that instead?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:168:
ClassElement contextClass = Types.getClassContext(type);
I find it quite hard to follow this code because of getClassContext; it is not
clear what happens with type variables. Perhaps add a comment?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:181:
compiler.resolverWorld.closurizedGenericMembers.forEach(analyzeMethod);
Long line.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/fo...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/fo...
sdk/lib/_internal/lib/foreign_helper.dart:217: JS_SETUP_OBJECT() {}
Do we call it 'setup object' somewhere else? How about 'global object'?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:29: UNINTERCEPTED;
Remove?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:106: return JS('var', r'#.apply(null, #)',
function, arguments);
How about
  Object invoke(function, arguments) => invokeOn(function, null, arguments).

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:123: arguments = invoke(substitution,
arguments);
This happens for thunks (if I understand correctly), so instead of arguments you
should use [].

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:169: throw new
CastErrorImplementation(actualType, typeName);
Perhaps we should move the type lookup to CastErrorImplementation, similar to
TypeerrorImplementation.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:222: * [signatureName]. The
This comment is not

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:232: var signatureLocation =
JS_SETUP_OBJECT();
var -> String.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:245: var targetSignature = 
invokeOn(targetSignatureFunction, interceptor, null);
Remove one space.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:263: applySignature(var signature, var
context, var contextName) {
Perhaps computeSignature would be a better name.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:426: * If [allowShorter] is [:true:], [t] is
allowed to be shorter that [s].
'that' -> 'than'.

Perhaps explain when this is the case and a add a general comment as first
paragraph.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:524: if
(!areAssignable(sOptionalParameterTypes, tOptionalParameterTypes, true)) {
Long line.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:550: for (; tPos < tOptionalParametersLen ;
sPos++, tPos++) {
Remove space before ';'.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:562: if (!areAssignableMaps(sNamedParameters,
tNamedParameters)) return false;
'return areAssignableMaps(sNamedParameters, tNamedParameters);'

[karlklose, 2013-06-20 07:32:55]

Final comments on the implementation. Looking at the tests now.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/closure.dart:528: if (operatorString
== 'is' || operatorString == 'as') {
Use predicates on the node?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1808: return new
HTypeConversion.withContext(type, kind, subtype,
I'd rather not create a new conversion withContext for the case of no context.
Couldn't you just create a normal conversion and get rid of the else-branch in
1805?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2775: type =
type.unalias(compiler);
Move to buildTypeConversion?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2794: type =
type.unalias(compiler);
Move to buildIsNode? isMalformed on an unaliased type could simply unalias
itself and call isMalformed on the result.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2832: context =
graph.addConstantNull(compiler);
This code (and the else-branch) could be shared with buildTypeConversion.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2853:
pushInvokeStatic(node, checkFunctionSubtype, inputs, HType.BOOLEAN);
Could you move this to the codegen by creating a new version of HIs?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3338: String
substitutionNameString = backend.namer.getName(cls);
How does this work? substitutionName contains the asPrefix.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1118: // TODO(5022):
Generic typedefs should not be handled here.
This TODO is obsolete now.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2300: bool get
hasContext => inputs.length > 1;
So we distinguish an HIs withTypeRepresentation from on withContext by
type.kind? Perhaps the predicates hasTypeRepresentation and hasContext should
include that check.  Otherwise we could just have one getter called something
like hasAdditionalData.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:599: if (type !=
null && type.kind == TypeKind.FUNCTION) {
Merge the conditions? Or have one if (type != null) around both?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:112: DartType
registerIsCheck(DartType type, Compiler compiler) {
Which method is this replacing?

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/util/link.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/util/link.dart:114: Link<T>
toLink([Link<T> tail = const Link()]);
Is there more than one place we use it? And is the only reason for that use to
preserve the type parameter?

[karlklose, 2013-06-20 13:11:04]

Comments on the tests.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_bound_closure0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_bound_closure0_test.dart:10: typedef int
Foo(bool a, [String b]);
Did you mean 'typedef int Foo(bool a, String b)'? (also in the next tests)

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_bound_closure5_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_bound_closure5_test.dart:36: class D<S,T>
extends C<T> {}
Space between type variables.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_call2_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_call2_test.dart:19: class D1<S,T> extends C1<T>
{}
Space between type variables (also l.25)

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_call2_test.dart:30: Expect.isFalse(new
D1<String, bool>() is Baz, 'new D1<String, bool>() is Baz');
Three long lines (l.30, 43, 44).

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_closure0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:6: // Check function
subtyping of static closures.
'closures' -> 'functions'?

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:8: import
'package:expect/expect.dart';
Currently this is not used.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:17: C(f2<T> f, C<J> c);
Why this second argument?

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:24: new C<int>(X.f1, null);
Please add a negative test, too. You'll have to wrap them in an isCheckedMode
test, so that the negative test is not run in unchecked mode.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_closure1_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure1_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
Same comments as with previous test.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_factory0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_factory0_test.dart:26: Expect.isNotNull(new
C<int>(method1));
I think you could get rid of method1. Alternatively, check different optional
argument and return type patterns here (if you did not already in another test.)

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_local0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_local0_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
Is this test needed? The next test duplicates some of it.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_local0_test.dart:6: // Check function subtyping
for bound closures.
'bound closures' -> 'closures' or 'local functions'.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_null.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_null.dart:14: Expect.isFalse(null is Foo, 'foo
is Foo');
What is foo?

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_setter0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_setter0_test.dart:26: String a = 42;
Use
 var x = 42;
 String a = x;
to avoid warnings from the analyzer.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_static0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_static0_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
Is this a duplicate test?

https://codereview.chromium.org/12334070/diff/127063/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/12334070/diff/127063/tests/lib/lib.status#new...
tests/lib/lib.status:35: [ $compiler == dart2js && $checked ]
Remove the section.

[Johnni Winther, 2013-06-21 12:19:14]

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12334070/diff/35001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2688:
compiler.enqueuer.codegen.registerIsCheck(type, work.resolutionTree);
On 2013/03/13 09:30:46, ngeoffray wrote:
> Add a TODO that this should be done in codegen.

Stale.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:471: if (type == null)
return;
On 2013/03/22 13:17:46, karlklose wrote:
> How can this happen?

Bad handling of is!
Now fixed.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:133: bool
needsRti(ClassElement cls);
On 2013/03/22 13:17:46, karlklose wrote:
> Should we rename this to classNeedsRti?

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:134: bool
methodNeedsRti(Element cls);
On 2013/03/22 13:17:46, karlklose wrote:
> 'Element' -> 'FunctionElement'.
> 'cls' -> 'function'.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:163: Enqueuer enqueuer,
TreeElements elements) {}
On 2013/03/22 13:17:46, karlklose wrote:
> Break line after each parameter to keep the code in the same style as above.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:221: bool
methodNeedsRti(Element cls) => false;
On 2013/03/22 13:17:46, karlklose wrote:
> See comments on abstract super.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:154: if
(!cls.typeVariables.isEmpty) {
On 2013/03/22 13:17:46, karlklose wrote:
> use '&&'.  Consider storing '!cls.typeVariables.isEmpty' in a local. 

Done. '!cls.typeVariables.isEmpty' is only used once now.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/enqueue.dart:282:
instanceMembersByName[memberName] = const Link<Element>();
On 2013/03/22 13:17:46, karlklose wrote:
> Could you add a comment why you do this?

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:109: * program
contains `x is F<int>` and `x is F<bool>` then the TypedefElement
On 2013/03/22 13:17:46, karlklose wrote:
> Please update comment and change to use [::] instead of ``.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:115: = new
Map<ClassElement, Set<FunctionType>>();
On 2013/03/22 13:17:46, karlklose wrote:
> Break after '='.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:117:
Set<FunctionType> checkedNonGenericFunctionTypes
On 2013/03/22 13:17:46, karlklose wrote:
> Ditto.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1327:
buffer.write('$holder.${namer.substitutionName(check)}$_=${_}$body$N');
On 2013/03/22 13:17:46, karlklose wrote:
> Long line.

Stale.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1339: for
(FunctionType type in functionTypes) {
On 2013/03/22 13:17:46, karlklose wrote:
> I would indent the body of the function by two spaces.
> 
> Did you need to break before the first argument; would that work?
> 
> checkedGenericFunctionTypes.forEach((ClassElement cls,             
>                                      Set<FunctionType> functionTypes) {

Stale.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1690: *
statically known to be a subtype of it.
On 2013/03/22 13:17:46, karlklose wrote:
> Explain what being false means.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1719: void
emitFunctionTypeSignature(Element method, FunctionType type),
On 2013/03/22 13:17:46, karlklose wrote:
> Long line.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1860: bool
knownSubtype) {
On 2013/03/22 13:17:46, karlklose wrote:
> Align with '(FunctionType...'.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1862:
registerDynamicFunctionTypeCheck(functionType);
On 2013/03/22 13:17:46, karlklose wrote:
> Why are we registering it in the emitter. Shouldn't the codegen register it?

Which function type check that are dynamic depends on both the target and the
type so it cannot be determined before we know all potentially closurized
methods.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1868: bool
hasSignature = false;
On 2013/03/22 13:17:46, karlklose wrote:
> 'signatureEmitted'?

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1875: bool
knownSubtype) {
On 2013/03/22 13:17:46, karlklose wrote:
> Align.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2018: String
encoding = rti.getSignatureEncoding(methodType, () => 'null');
On 2013/03/22 13:17:46, karlklose wrote:
> Please explain what null means here.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:787: return
'${CURRENT_ISOLATE}';
On 2013/03/22 13:17:46, karlklose wrote:
> No {} needed.

I like them, anyway!

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:876: for
(Link<DartType> link = type.optionalParameterTypes;
On 2013/03/22 13:17:46, karlklose wrote:
> Don't we need a separator between the sets of paramters?

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:102: //
Compute the set of all classes that need runtime type information.
On 2013/03/22 13:17:46, karlklose wrote:
> Update the comment.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:123:
compiler.resolverWorld.closurizedGenericMembers.forEach(analyzeMethod);
On 2013/03/22 13:17:46, karlklose wrote:
> Long line.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:18:
forwardRuntimeTypeInfo(target, substitution, source) {
On 2013/03/22 13:17:46, karlklose wrote:
> Is this still used?

Removed.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:116: } else if
(isJsFunction(substitution)) {
On 2013/03/22 13:17:46, karlklose wrote:
> Is this path used?

Not sure. Added a TODO.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:165: *
[signatureName]. The
On 2013/03/22 13:17:46, karlklose wrote:
> Please finish the

Now it

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:185: var
targetSignature =  invokeOn(targetSignatureFunction, target, null);
On 2013/03/22 13:17:46, karlklose wrote:
> remove space.

Done.

https://codereview.chromium.org/12334070/diff/59001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:318: if (t == null)
return true;
On 2013/03/22 13:17:46, karlklose wrote:
> What does null mean here?

Empty lists, i.e. not arguments of that kind.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_bound_closure5_test.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_bound_closure5_test.dart:34: class D<S,T>
extends C<T> {}
On 2013/03/22 13:17:46, karlklose wrote:
> Space, please.

Done.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_call2_test.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_call2_test.dart:28: Expect.isFalse(new
D1<String, bool>() is Baz, 'new D1<String, bool>() is Baz');
On 2013/03/22 13:17:46, karlklose wrote:
> Long line here and 2x below.

Done.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_local4_test.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_local4_test.dart:6: // Check function subtyping
for generic bound closures.
On 2013/03/22 13:17:46, karlklose wrote:
> Please add that this is testing type argument substitution.

Done.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_local4_test.dart:30: class D<S,T> extends C<T>
{}
On 2013/03/22 13:17:46, karlklose wrote:
> Space.

Done.

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
File tests/language/function_subtype_null.dart (right):

https://codereview.chromium.org/12334070/diff/59001/tests/language/function_s...
tests/language/function_subtype_null.dart:12: Expect.isFalse(null is Foo, 'foo
is Foo');
On 2013/03/22 13:17:46, karlklose wrote:
> Is that true?

Surprisingly!

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:132: bool
needsRti(ClassElement cls);
On 2013/06/19 14:37:05, karlklose wrote:
> Should we rename it to classNeedsRti?

Done.

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12334070/diff/86001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1412:
compiler.findHelper(const SourceString('defineProperty')));
On 2013/06/19 14:37:05, karlklose wrote:
> Why is that necessary? Shouldn't it be enough to register the class as
> instantiated?

I have no idea. The code was just moved from below.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/closure.dart:528: if (operatorString
== 'is' || operatorString == 'as') {
On 2013/06/20 07:32:55, karlklose wrote:
> Use predicates on the node?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_types.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_types.dart:1153: new
SubtypeVisitor(compiler, dynamicType, voidType);
On 2013/06/19 14:37:05, karlklose wrote:
> Can these two allocations be const? (see comment on the PotentialSubtype
> constrcutor)

No. See below.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_types.dart:1233: class
PotentialSubtypeVisitor extends SubtypeVisitor {
On 2013/06/19 14:37:05, karlklose wrote:
> Please add a comment on what this class does (in comparison to the
> SubtypeVisitor).

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_types.dart:1234:
PotentialSubtypeVisitor(Compiler compiler,
On 2013/06/19 14:37:05, karlklose wrote:
> Can this constructor be const?

It could, but we can provide no constant values for [compiler], [dynamicType]
and [voidType].

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:180: * Documentation
wanted -- johnniwinther
On 2013/06/19 14:37:05, karlklose wrote:
> I think we can remove this 'documentation'.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:235: if
(!cls.typeVariables.isEmpty) {
On 2013/06/19 14:37:05, karlklose wrote:
> Join the two conditions.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:404: map[memberName] =
remaining.toLink(map[memberName]);
On 2013/06/19 14:37:05, karlklose wrote:
> Isn't the constant empty link already the default value for the parameter?

Comment added.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:560: void
registerClosure(Element element, TreeElements elements) {
On 2013/06/19 14:37:05, karlklose wrote:
> 'registerClosurizedMember'?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/enqueue.dart:567: void
registerGenericClosure(Element element, TreeElements elements) {
On 2013/06/19 14:37:05, karlklose wrote:
> 'registerClosurizedGenericMember'?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:795: if
(type.isMalformed) {
On 2013/06/19 14:37:05, karlklose wrote:
> Merge into an else if.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:804:
world.addToWorkList(
On 2013/06/19 14:37:05, karlklose wrote:
> Merge error?

Bad move.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1097: ? const
PropertyCheckedModeHelper(const
SourceString("numberOrStringSuperNativeTypeCast"))
On 2013/06/19 14:37:05, karlklose wrote:
> A lot of long lines from here on...

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:113: * Raw
Typedef symbols occuring in is-checks and type assertions.  If the
On 2013/06/19 14:37:05, karlklose wrote:
> Please update the comment.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1312: //
TODO(karlklose): support typedefs with variables.
On 2013/06/19 14:37:05, karlklose wrote:
> This TODO is obsolete now, isn't it?

Yes. Removed.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1353: String
encoding = rti.getTypeEncoding(type);
On 2013/06/19 14:37:05, karlklose wrote:
> Create a function addEncoding for l.1353-1354 and call
> checkedNonGenericFunctionTypes.forEach(addEncoding)

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1358:
(ClassElement cls, Set<FunctionType> functionTypes) {
On 2013/06/19 14:37:05, karlklose wrote:
> If you replace 'ClassElement cls' with '_' I think you can fit it into the
> previous line. Or call
> checkedGenericFunctionTypes.values().forEach(addEncoding).

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1499: String
helperName = namer.isolateAccess(helperElement);
On 2013/06/19 14:37:05, karlklose wrote:
> You can inline helperElement here (it would remove the check for
FunctionElement
> though).

No. [helperElement] is used below.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1766: void
emitIsFunctionTypeTest(FunctionType type),
On 2013/06/19 14:37:05, karlklose wrote:
> You could add Typedefs for these three function types.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1875: void
generateFunctionTypeTests(
On 2013/06/19 14:37:05, karlklose wrote:
> Could you get rid of this method? I don't think it adds readability and you
can
> simply add inline the call to getFunctionTypeChecksOn at the call site.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1904: bool
hasSignature = false;
On 2013/06/19 14:37:05, karlklose wrote:
> Perhaps alwaysUseSignature would be clearer.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1911: bool
knownSubtype) {
On 2013/06/19 14:37:05, karlklose wrote:
> This should fit on one line.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:1912: if
(knownSubtype || !hasDynamicFunctionTypeCheck) {
On 2013/06/19 14:37:05, karlklose wrote:
> !knownSubtype implies hasDynamicFunctionTypeCheck, so that last check is not
> necessary.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2226:
RuntimeTypes rti = backend.rti;
On 2013/06/19 14:37:05, karlklose wrote:
> You can inline rti.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2234: void
emitIsFunctionTypeTest(FunctionType functionType) {
On 2013/06/19 14:37:05, karlklose wrote:
> I think we should be more careful with names in the emitter; this is not a
test
> we are generating but a flag/properties/...
> 
> We should clean that up in the whole emitter.

Yes. Added a TODO to the newly added typedef for this method.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2272:
jsAst.Expression newClosure = js(closureClass).newWith(arguments);
On 2013/06/19 14:37:05, karlklose wrote:
> Why this change?

It was needed in a previous iteration. Removed.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/emitter.dart:2314: DartType
memberType = member.computeType(compiler);
On 2013/06/19 14:37:05, karlklose wrote:
> This is not used.

Removed.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (left):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:844: 
On 2013/06/19 14:37:05, karlklose wrote:
> Is this intentional? See also l. 1018.

No. I often accidentally create too many blank lines, so I probably removed
these as part of a clean up of the ones I created.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/namer.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/namer.dart:797: String
operatorIsType(DartType type) {
On 2013/06/19 14:37:05, karlklose wrote:
> Would it make sense to replace operatorIs with a version taking a DartType and
> use that instead?

I tried, but it made several of the original call sites awkward (they had to
call element.computeType(compiler) !!).

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart
(right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:168:
ClassElement contextClass = Types.getClassContext(type);
On 2013/06/19 14:37:05, karlklose wrote:
> I find it quite hard to follow this code because of getClassContext; it is not
> clear what happens with type variables. Perhaps add a comment?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/js_backend/runtime_types.dart:181:
compiler.resolverWorld.closurizedGenericMembers.forEach(analyzeMethod);
On 2013/06/19 14:37:05, karlklose wrote:
> Long line.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1808: return new
HTypeConversion.withContext(type, kind, subtype,
On 2013/06/20 07:32:55, karlklose wrote:
> I'd rather not create a new conversion withContext for the case of no context.
> Couldn't you just create a normal conversion and get rid of the else-branch in
> 1805?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2775: type =
type.unalias(compiler);
On 2013/06/20 07:32:55, karlklose wrote:
> Move to buildTypeConversion?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2794: type =
type.unalias(compiler);
On 2013/06/20 07:32:55, karlklose wrote:
> Move to buildIsNode? isMalformed on an unaliased type could simply unalias
> itself and call isMalformed on the result.

Currently we can't. And this will be removed when the new malformedness is
implemented.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2832: context =
graph.addConstantNull(compiler);
On 2013/06/20 07:32:55, karlklose wrote:
> This code (and the else-branch) could be shared with buildTypeConversion.

Added a buildTypeVariableList method used by both buildTypeConversion and
buildIsNode.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2853:
pushInvokeStatic(node, checkFunctionSubtype, inputs, HType.BOOLEAN);
On 2013/06/20 07:32:55, karlklose wrote:
> Could you move this to the codegen by creating a new version of HIs?

The generation is already handle in codegen. We just prepare the needed call
here.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3338: String
substitutionNameString = backend.namer.getName(cls);
On 2013/06/20 07:32:55, karlklose wrote:
> How does this work? substitutionName contains the asPrefix.

getRuntimeTypeArgument has been changed to take the name of the class instead.
The helper then use this to create the is/as-predicate names as needed.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1118: // TODO(5022):
Generic typedefs should not be handled here.
On 2013/06/20 07:32:55, karlklose wrote:
> This TODO is obsolete now.

Removed.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:2300: bool get
hasContext => inputs.length > 1;
On 2013/06/20 07:32:55, karlklose wrote:
> So we distinguish an HIs withTypeRepresentation from on withContext by
> type.kind? Perhaps the predicates hasTypeRepresentation and hasContext should
> include that check.  Otherwise we could just have one getter called something
> like hasAdditionalData.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:599: if (type !=
null && type.kind == TypeKind.FUNCTION) {
On 2013/06/20 07:32:55, karlklose wrote:
> Merge the conditions? Or have one if (type != null) around both?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/universe/universe.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/universe/universe.dart:112: DartType
registerIsCheck(DartType type, Compiler compiler) {
On 2013/06/20 07:32:55, karlklose wrote:
> Which method is this replacing?

It new. Used from the enqueuer to register checks of the unalias types.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/util/link.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/util/link.dart:114: Link<T>
toLink([Link<T> tail = const Link()]);
On 2013/06/20 07:32:55, karlklose wrote:
> Is there more than one place we use it? And is the only reason for that use to
> preserve the type parameter?

There is only one use case. There it is used to add all the elements in the
builder to an existing list.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/fo...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/fo...
sdk/lib/_internal/lib/foreign_helper.dart:217: JS_SETUP_OBJECT() {}
On 2013/06/19 14:37:05, karlklose wrote:
> Do we call it 'setup object' somewhere else? How about 'global object'?

Renamed.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_helper.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_helper.dart:29: UNINTERCEPTED;
On 2013/06/19 14:37:05, karlklose wrote:
> Remove?

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:106: return JS('var', r'#.apply(null, #)',
function, arguments);
On 2013/06/19 14:37:05, karlklose wrote:
> How about
>   Object invoke(function, arguments) => invokeOn(function, null, arguments).

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:123: arguments = invoke(substitution,
arguments);
On 2013/06/19 14:37:05, karlklose wrote:
> This happens for thunks (if I understand correctly), so instead of arguments
you
> should use [].

I think it that case it is the first [invoke] call that could/should have [] as
argument.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:169: throw new
CastErrorImplementation(actualType, typeName);
On 2013/06/19 14:37:05, karlklose wrote:
> Perhaps we should move the type lookup to CastErrorImplementation, similar to
> TypeerrorImplementation.

Added a TODO.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:222: * [signatureName]. The
On 2013/06/19 14:37:05, karlklose wrote:
> This comment is not

Now it

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:232: var signatureLocation =
JS_SETUP_OBJECT();
On 2013/06/19 14:37:05, karlklose wrote:
> var -> String.

No. JS_SETUP_OBJECT returns a JS object.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:245: var targetSignature = 
invokeOn(targetSignatureFunction, interceptor, null);
On 2013/06/19 14:37:05, karlklose wrote:
> Remove one space.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:263: applySignature(var signature, var
context, var contextName) {
On 2013/06/19 14:37:05, karlklose wrote:
> Perhaps computeSignature would be a better name.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:426: * If [allowShorter] is [:true:], [t] is
allowed to be shorter that [s].
On 2013/06/19 14:37:05, karlklose wrote:
> 'that' -> 'than'.
> 
> Perhaps explain when this is the case and a add a general comment as first
> paragraph.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:524: if
(!areAssignable(sOptionalParameterTypes, tOptionalParameterTypes, true)) {
On 2013/06/19 14:37:05, karlklose wrote:
> Long line.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:550: for (; tPos < tOptionalParametersLen ;
sPos++, tPos++) {
On 2013/06/19 14:37:05, karlklose wrote:
> Remove space before ';'.

Done.

https://codereview.chromium.org/12334070/diff/127063/sdk/lib/_internal/lib/js...
sdk/lib/_internal/lib/js_rti.dart:562: if (!areAssignableMaps(sNamedParameters,
tNamedParameters)) return false;
On 2013/06/19 14:37:05, karlklose wrote:
> 'return areAssignableMaps(sNamedParameters, tNamedParameters);'

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_bound_closure0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_bound_closure0_test.dart:10: typedef int
Foo(bool a, [String b]);
On 2013/06/20 13:11:04, karlklose wrote:
> Did you mean 'typedef int Foo(bool a, String b)'? (also in the next tests)

No. The intension was to ensure that two typedefs with equivalent definitions
are handled the same way.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_bound_closure5_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_bound_closure5_test.dart:36: class D<S,T>
extends C<T> {}
On 2013/06/20 13:11:04, karlklose wrote:
> Space between type variables. 

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_call2_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_call2_test.dart:19: class D1<S,T> extends C1<T>
{}
On 2013/06/20 13:11:04, karlklose wrote:
> Space between type variables (also l.25)

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_call2_test.dart:30: Expect.isFalse(new
D1<String, bool>() is Baz, 'new D1<String, bool>() is Baz');
On 2013/06/20 13:11:04, karlklose wrote:
> Three long lines (l.30, 43, 44).

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_closure0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:6: // Check function
subtyping of static closures.
On 2013/06/20 13:11:04, karlklose wrote:
> 'closures' -> 'functions'?

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:8: import
'package:expect/expect.dart';
On 2013/06/20 13:11:04, karlklose wrote:
> Currently this is not used.

It is now.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:17: C(f2<T> f, C<J> c);
On 2013/06/20 13:11:04, karlklose wrote:
> Why this second argument?

Not needed. Removed.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure0_test.dart:24: new C<int>(X.f1, null);
On 2013/06/20 13:11:04, karlklose wrote:
> Please add a negative test, too. You'll have to wrap them in an isCheckedMode
> test, so that the negative test is not run in unchecked mode.

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_closure1_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_closure1_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
On 2013/06/20 13:11:04, karlklose wrote:
> Same comments as with previous test.

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_factory0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_factory0_test.dart:26: Expect.isNotNull(new
C<int>(method1));
On 2013/06/20 13:11:04, karlklose wrote:
> I think you could get rid of method1. Alternatively, check different optional
> argument and return type patterns here (if you did not already in another
test.)

Removed.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_local0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_local0_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
On 2013/06/20 13:11:04, karlklose wrote:
> Is this test needed? The next test duplicates some of it.

The different is whether the typedefs are generic.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_local0_test.dart:6: // Check function subtyping
for bound closures.
On 2013/06/20 13:11:04, karlklose wrote:
> 'bound closures' -> 'closures' or 'local functions'.

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_null.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_null.dart:14: Expect.isFalse(null is Foo, 'foo
is Foo');
On 2013/06/20 13:11:04, karlklose wrote:
> What is foo?

foo is null

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_setter0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_setter0_test.dart:26: String a = 42;
On 2013/06/20 13:11:04, karlklose wrote:
> Use
>  var x = 42;
>  String a = x;
> to avoid warnings from the analyzer.

Done.

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
File tests/language/function_subtype_static0_test.dart (right):

https://codereview.chromium.org/12334070/diff/127063/tests/language/function_...
tests/language/function_subtype_static0_test.dart:1: // Copyright (c) 2013, the
Dart project authors.  Please see the AUTHORS file
On 2013/06/20 13:11:04, karlklose wrote:
> Is this a duplicate test?

Yes. Deleted.

https://codereview.chromium.org/12334070/diff/127063/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/12334070/diff/127063/tests/lib/lib.status#new...
tests/lib/lib.status:35: [ $compiler == dart2js && $checked ]
On 2013/06/20 13:11:04, karlklose wrote:
> Remove the section.

Done.

[karlklose, 2013-06-21 12:48:41]

LGTM.

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
File tests/language/function_subtype_bound_closure4_test.dart (right):

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
tests/language/function_subtype_bound_closure4_test.dart:32: class D<S,T>
extends C<T> {}
Add a space between type variables.

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
File tests/language/function_subtype_named2_test.dart (right):

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
tests/language/function_subtype_named2_test.dart:2: // for details. All rights
reserved. Use of this source code>() is C<governed by a
Remove '>() is C<'.

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
File tests/language/function_subtype_null.dart (right):

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
tests/language/function_subtype_null.dart:15: Expect.isFalse(foo is Bar<bool>,
'foo is Bar<bool>');
What is foo?

[Johnni Winther, 2013-06-21 13:32:21]

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
File tests/language/function_subtype_bound_closure4_test.dart (right):

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
tests/language/function_subtype_bound_closure4_test.dart:32: class D<S,T>
extends C<T> {}
On 2013/06/21 12:48:41, karlklose wrote:
> Add a space between type variables.

Done.

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
File tests/language/function_subtype_named2_test.dart (right):

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
tests/language/function_subtype_named2_test.dart:2: // for details. All rights
reserved. Use of this source code>() is C<governed by a
On 2013/06/21 12:48:41, karlklose wrote:
> Remove '>() is C<'.

Done.

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
File tests/language/function_subtype_null.dart (right):

https://codereview.chromium.org/12334070/diff/111003/tests/language/function_...
tests/language/function_subtype_null.dart:15: Expect.isFalse(foo is Bar<bool>,
'foo is Bar<bool>');
On 2013/06/21 12:48:41, karlklose wrote:
> What is foo? 

Should be null.

[Johnni Winther, 2013-06-21 13:37:33]

Committed patchset #13 manually as r24286 (presubmit successful).

[Johnni Winther, 2013-06-24 07:00:32]

Committed patchset #15 manually as r24309 (presubmit successful).

[ahe, 2013-08-07 21:50:12]

https://codereview.chromium.org/12334070/diff/170001/sdk/lib/_internal/lib/fo...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/12334070/diff/170001/sdk/lib/_internal/lib/fo...
sdk/lib/_internal/lib/foreign_helper.dart:217: JS_GLOBAL_OBJECT() {}
How is this different from JS_CURRENT_ISOLATE?

[Johnni Winther, 2013-08-08 05:44:36]

https://codereview.chromium.org/12334070/diff/170001/sdk/lib/_internal/lib/fo...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/12334070/diff/170001/sdk/lib/_internal/lib/fo...
sdk/lib/_internal/lib/foreign_helper.dart:217: JS_GLOBAL_OBJECT() {}
On 2013/08/07 21:50:12, ahe wrote:
> How is this different from JS_CURRENT_ISOLATE?

Depending on the use of isolates, JS_CURRENT_ISOLATE might not be the $ object
(= JS_GLOBAL_OBJECT).

[ahe, 2013-08-21 12:42:14]

https://codereview.chromium.org/12334070/diff/170001/sdk/lib/_internal/lib/fo...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/12334070/diff/170001/sdk/lib/_internal/lib/fo...
sdk/lib/_internal/lib/foreign_helper.dart:217: JS_GLOBAL_OBJECT() {}
On 2013/08/08 05:44:37, Johnni Winther wrote:
> On 2013/08/07 21:50:12, ahe wrote:
> > How is this different from JS_CURRENT_ISOLATE?
> 
> Depending on the use of isolates, JS_CURRENT_ISOLATE might not be the $ object
> (= JS_GLOBAL_OBJECT).

I'm talking about JS_CURRENT_ISOLATE. Are you talking about
JS_CURRENT_ISOLATE_CONTEXT?