Detect and reject illegal recursive types (non-contractive types).

Detect and reject illegal recursive types (non-contractive types).
Fixed instantiation and equality of recursive types.
Added many asserts to detect issues with recursive types.
Disabled bad tests. They declare illegal types and will be fixed in a later cl.

R=hausner@google.com, srdjan@google.com

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

[srdjan, 2014-03-26 18:02:11]

Could you add new tests that fail/pass?

https://codereview.chromium.org/211963003/diff/70001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/211963003/diff/70001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:516: // finalized is a non-contractive type. This
can be detected by looking at the
non-contractive types is not a commonly known term, IMO.  Can you briefly
elaborate what it is or give a pointer to an explanation.

https://codereview.chromium.org/211963003/diff/70001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:606: // same time. Canonicalization happens when
pending types are processed.
Maybe document 'trail' in comment.

[regis, 2014-03-26 18:41:29]

Thanks! PTAL.

As discussed offline, the 3 tests currently disabled in language.status will be
modified in a later cl, as to not complicate this cl with status file changes
for dart2js.

The expectation of these tests will change and more tests will be added to test
detection of illegal recursive types.

https://codereview.chromium.org/211963003/diff/70001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/211963003/diff/70001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:516: // finalized is a non-contractive type. This
can be detected by looking at the
On 2014/03/26 18:02:12, srdjan wrote:
> non-contractive types is not a commonly known term, IMO.  Can you briefly
> elaborate what it is or give a pointer to an explanation.

I expanded the comment and added a reference to the spec, which is being
modified to disallow such types.

https://codereview.chromium.org/211963003/diff/70001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:606: // same time. Canonicalization happens when
pending types are processed.
On 2014/03/26 18:02:12, srdjan wrote:
> Maybe document 'trail' in comment.

Added comment about trail.

[srdjan, 2014-03-26 21:35:02]

LGTM

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:541: if ((num_type_params == 0) ||
Does num_type_parames ==0 means that the type parameters are null, or that they
do not exist in type_cls, or is that the same?

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:610: // of the super type.
What does it contain/return?

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:2478: // and resolve super type and mixin types.
This method may benefit from prefetching isolate.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4019: return hash;
Why don't you return uint32_t instead of converting it to int32_t (on ia32) or
int64_t(on x64)? Also below.

[hausner, 2014-03-26 21:37:20]

LGTM2

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4606: #ifdef DEBUG
if you rewrite this as if (DEBUG) {...} then the code gets compiled in both
debug and release builds, making it less likely that a release build fails even
though the debug build succeeds. The compiler will (hopefully) eliminate the
code in the release build because it's unreachable.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:13376: #ifdef DEBUG
ditto

[regis, 2014-03-26 23:24:58]

Thanks!

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:541: if ((num_type_params == 0) ||
On 2014/03/26 21:35:02, srdjan wrote:
> Does num_type_parames ==0 means that the type parameters are null, or that
they
> do not exist in type_cls, or is that the same?

It means that the type_cls is not parameterized (no declared type parameters).
The comment is confusing: "or null" refers to a raw type, not to a
non-parameterized type. I clarified the commment.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:610: // of the super type.
On 2014/03/26 21:35:02, srdjan wrote:
> What does it contain/return?

The trail is not new to this cl. See other functions in object.cc using a trail
to process recursive types.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:2478: // and resolve super type and mixin types.
On 2014/03/26 21:35:02, srdjan wrote:
> This method may benefit from prefetching isolate.

Done.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4019: return hash;
On 2014/03/26 21:35:02, srdjan wrote:
> Why don't you return uint32_t instead of converting it to int32_t (on ia32) or
> int64_t(on x64)? Also below.

Done.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:4606: #ifdef DEBUG
On 2014/03/26 21:37:21, hausner wrote:
> if you rewrite this as if (DEBUG) {...} then the code gets compiled in both
> debug and release builds, making it less likely that a release build fails
even
> though the debug build succeeds. The compiler will (hopefully) eliminate the
> code in the release build because it's unreachable.

I am reluctant to use yet another pattern. We already have if defined(DEBUG) and
ifdef DEBUG. And as Ivan pointed out, we would need to make sure that DEBUG is
defined to be 1.

https://codereview.chromium.org/211963003/diff/90001/runtime/vm/object.cc#new...
runtime/vm/object.cc:13376: #ifdef DEBUG
On 2014/03/26 21:37:21, hausner wrote:
> ditto

Not done.

[regis, 2014-03-26 23:38:22]

Committed patchset #4 manually as r34460 (presubmit successful).