Enable full type-checks in checked mode.

Enable full type-checks in checked mode.

BUG=
R=ngeoffray@google.com

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

[ahe, 2013-04-10 16:18:30]

DBC

Not sure if this is relevant.  But you may miss an issue of code needing access
to this in closure.dart.

For example:

class Foo {
  x() => () => y;
}

class SubFoo extends Foo {
  var y;
}

main() {
  var s = new SubFoo();
  s.y = 42;
  Expect.equals(42, s.x()());
}

[ngeoffray, 2013-04-15 10:59:14]

Initial comments, but as discussed, we should not use HIs for checked mode
checks. I think you can keep the original SSA instructions and still implement
this CL.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:419: // therefor the
closure needs a this-element.
therefore

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:421: // from a parameter;
in checked mode, we need to resolve all types.
I don't understand this TODO. How can it be null?

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:476: registerNeedsThis();
Not sure you need this:
- If element is a field, then "this" is already needed
- If element is a local, then its type annotation already said it needed "this".

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:686: if
(element.computeType(compiler).containsTypeVariables) {
So that's the function type if the element? It matches both parameters and
return type right? Please add a comment.

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

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1286:
(compiler.enableTypeAssertions && !cls.typeVariables.isEmpty);
Is that because we never know if there will be a type annotation that needs
checking? Why is that not covered with regular is checks?

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:134: if (object ==
null) return;
return null;

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:135: var target =
isJsArray(object) ? getInterceptor(object) : object;
You should not need to do this anymore but just:
var target = getInterceptor(object)

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:246: //
TODO(johnniwinther): remove this.
What is this for? Please explain why we should remove it and why we need it.

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

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:365: element:
builder.compiler.currentElement);
Thank you :)

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1653: } else {
You should remove this else. A checked mode check should not be a
HTypeConversion anymore.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2727: {bool
checkedModeTest: false}) {
I'd prefer if the call sites always added the named argument, and the default
value would be null.

[Johnni Winther, 2013-04-17 08:04:20]

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:247: if (JS('bool',
'#.func', s) == true || JS('bool', '#.func', s) == true ) {
t -> s in one of the cases.

[karlklose, 2013-05-02 15:04:54]

Please take another look, Nicolas.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/closure.dart (right):

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:419: // therefor the
closure needs a this-element.
On 2013/04/15 10:59:14, ngeoffray wrote:
> therefore

Done.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:421: // from a parameter;
in checked mode, we need to resolve all types.
Maybe we do not store the result in the TreeElements when computing the
signature. Investigating...

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:476: registerNeedsThis();
It is needed.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/closure.dart:686: if
(element.computeType(compiler).containsTypeVariables) {
On 2013/04/15 10:59:14, ngeoffray wrote:
> So that's the function type if the element? It matches both parameters and
> return type right? Please add a comment.

Done.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/lib/js_rti.dart (right):

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:134: if (object ==
null) return;
On 2013/04/15 10:59:14, ngeoffray wrote:
> return null;

Done.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:135: var target =
isJsArray(object) ? getInterceptor(object) : object;
On 2013/04/15 10:59:14, ngeoffray wrote:
> You should not need to do this anymore but just:
> var target = getInterceptor(object)

Done.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:246: //
TODO(johnniwinther): remove this.
This will be replaced with the dynamic function subtype function.

https://codereview.chromium.org/13019003/diff/7001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:247: if (JS('bool',
'#.func', s) == true || JS('bool', '#.func', s) == true ) {
On 2013/04/17 08:04:20, Johnni Winther wrote:
> t -> s in one of the cases.

Done.

[karlklose, 2013-05-13 08:28:50]

ping.

[ngeoffray, 2013-05-13 09:10:59]

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:436: // from a parameter;
in checked mode, we need to resolve all types.
So for foo(T a), we don't resolve the "T" ?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:488: if (element != null
&& element is VariableElement) {
Should that be Elements.isLocal(element) ?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:490: if
(variable.variables.type.containsTypeVariables) {
Change to element.computeType(compiler).containsTypeVariables ?

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1225: bool
isCheckedMode = compiler.enableTypeAssertions;
isCheckedMode -> inCheckedMode ?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1332:
(compiler.enableTypeAssertions && !cls.typeVariables.isEmpty);
Why would a class with type variable need rti? What if the type variable is
never used?

Also, maybe make the code a bit simpler to comment on, like:

// A class that has type variables always needs RTI.
if (compiler.enableTypeAssertions && !cls.typesVariables.isEmpty) return true;
...

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1716: return
const SourceString('assertObjectIsSubtype');
Why is that called assertObjectIsSubtype? For me it's a confusing name,
shouldn't it be called assertIsSubtypeOfTypeVariable?

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:133: Object
assertSubtype(Object object, String isField, List checks, String asField) {
Line too long.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1126:
FunctionSignature signature = function.computeSignature(compiler);
Also add a similar comment to line 1675 here.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1128: if
(isInstanceMember) index++;
Why not using argumentIndex?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1217: //
TODO(karlklose): remove this and enable inlining of these methods.
Could you also explain why it does not work?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1726: DartType type,
int kind) {
Please do not duplicate what's already in convertType. Are there other users of
"converType"? If yes, can you move all the code below to "convertType"?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1748:
assert(type.kind == TypeKind.TYPE_VARIABLE);
Move that assert one up.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2821: } else {
You don't need this change anymore.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2473: } else if
(node.isArgumentTypeCheck || node.isReceiverTypeCheck) {
I would remove the else.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (left):

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:274: if (user
is HIs) return false;
I think you still need this.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:609: if (type !=
null && (!type.isRaw || type.kind == TypeKind.TYPE_VARIABLE)) {
type.containsTypeVariable?

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:729: if (!type.isRaw
|| type.kind == TypeKind.TYPE_VARIABLE) {
type.containsTypeVariable?

[karlklose, 2013-05-14 13:49:41]

Thanks, Nicolas!

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:436: // from a parameter;
in checked mode, we need to resolve all types.
I updated the comment to be more precise.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:488: if (element != null
&& element is VariableElement) {
On 2013/05/13 09:10:59, ngeoffray wrote:
> Should that be Elements.isLocal(element) ?

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:488: if (element != null
&& element is VariableElement) {
On 2013/05/13 09:10:59, ngeoffray wrote:
> Should that be Elements.isLocal(element) ?

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:490: if
(variable.variables.type.containsTypeVariables) {
On 2013/05/13 09:10:59, ngeoffray wrote:
> Change to element.computeType(compiler).containsTypeVariables ?

Done.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1225: bool
isCheckedMode = compiler.enableTypeAssertions;
On 2013/05/13 09:10:59, ngeoffray wrote:
> isCheckedMode -> inCheckedMode ?

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1332:
(compiler.enableTypeAssertions && !cls.typeVariables.isEmpty);
Removed, it was not necessary.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1716: return
const SourceString('assertObjectIsSubtype');
I renamed the functions.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/lib/js_rti.dart:133: Object
assertSubtype(Object object, String isField, List checks, String asField) {
On 2013/05/13 09:10:59, ngeoffray wrote:
> Line too long.

Done.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1126:
FunctionSignature signature = function.computeSignature(compiler);
On 2013/05/13 09:10:59, ngeoffray wrote:
> Also add a similar comment to line 1675 here.

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1128: if
(isInstanceMember) index++;
On 2013/05/13 09:10:59, ngeoffray wrote:
> Why not using argumentIndex?

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1217: //
TODO(karlklose): remove this and enable inlining of these methods.
On 2013/05/13 09:10:59, ngeoffray wrote:
> Could you also explain why it does not work?

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1726: DartType type,
int kind) {
There are users in the optimizer, but they cannot add checks for type variables
and compound types.

I removed 1727-1736 and called convertType instead.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1748:
assert(type.kind == TypeKind.TYPE_VARIABLE);
This assert is obsolete now.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2821: } else {
I know, but I think it reads better. In particular, the negation logic at the
end is easier to find.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:2473: } else if
(node.isArgumentTypeCheck || node.isReceiverTypeCheck) {
On 2013/05/13 09:10:59, ngeoffray wrote:
> I would remove the else.

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart (left):

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/codegen_helpers.dart:274: if (user
is HIs) return false;
On 2013/05/13 09:10:59, ngeoffray wrote:
> I think you still need this.

Done.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:609: if (type !=
null && (!type.isRaw || type.kind == TypeKind.TYPE_VARIABLE)) {
No, this also checks for literal type arguments.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:729: if (!type.isRaw
|| type.kind == TypeKind.TYPE_VARIABLE) {
Ditto.

[ngeoffray, 2013-05-15 08:45:32]

LGTM! Nice.

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

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/closure.dart:436: // from a parameter;
in checked mode, we need to resolve all types.
On 2013/05/14 13:49:41, karlklose wrote:
> I updated the comment to be more precise.

Thank you. I still find it a bit obscure: what is the result, why is it being
thrown away?

Also an example of which cases we're missing would be helpful.

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/13019003/diff/20001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:609: if (type !=
null && (!type.isRaw || type.kind == TypeKind.TYPE_VARIABLE)) {
On 2013/05/14 13:49:41, karlklose wrote:
> No, this also checks for literal type arguments.

I don't understand. Why isn't (!type.isRaw || type.kind ==
TypeKind.TYPE_VARIABLE) equivalent to type.containsTypeVariable?

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

https://codereview.chromium.org/13019003/diff/32001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1744: if
(identical(type.element, compiler.objectClass)) return original;
Can you remove those two checks above? They should be covered by convertType.

https://codereview.chromium.org/13019003/diff/32001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/13019003/diff/32001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1090: if (kind ==
HTypeConversion.CHECKED_MODE_CHECK && !type.isRaw) {
Use:

} else if {
  throw...
} else {
  ...
}

instead?

https://codereview.chromium.org/13019003/diff/32001/tests/utils/utils.status
File tests/utils/utils.status (right):

https://codereview.chromium.org/13019003/diff/32001/tests/utils/utils.status#...
tests/utils/utils.status:12: recursive_import_test: Fail # wrong code generated.
Could you file a bug and what is the wrong code?

[karlklose, 2013-05-15 12:59:16]

Thanks, Nicolas!

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

https://codereview.chromium.org/13019003/diff/32001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1744: if
(identical(type.element, compiler.objectClass)) return original;
Removed. They will always end up calling convertType since they are raw.

https://codereview.chromium.org/13019003/diff/32001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/13019003/diff/32001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1090: if (kind ==
HTypeConversion.CHECKED_MODE_CHECK && !type.isRaw) {
On 2013/05/15 08:45:33, ngeoffray wrote:
> Use:
> 
> } else if {
>   throw...
> } else {
>   ...
> }
> 
> instead?

Done.

https://codereview.chromium.org/13019003/diff/32001/tests/utils/utils.status
File tests/utils/utils.status (right):

https://codereview.chromium.org/13019003/diff/32001/tests/utils/utils.status#...
tests/utils/utils.status:12: recursive_import_test: Fail # wrong code generated.
This seems to been fixed in the meantime, removed.

[karlklose, 2013-05-15 13:41:10]

Committed patchset #7 manually as r22735 (presubmit successful).

[kasperl, 2013-05-15 14:01:31]

w00t!