[kripken]

The testcase here passes with --fuzz-exec but fails when adding --roundtrip,
as then the type appears different. Is that expected @tlively ?

[tlively]

If I understand the issue correctly, then yes, it is expected. --roundtrip will freshly parse the types, so any self-referential types will be rebuilt into new copies of themselves. Until we get equirecursive type canonicalization working, the new self-referential types will not have the same identity as the old types.

[kripken]

Oh right, I forgot that part.. yes, then because of recursive types I think we do need this, marking as non-draft.

[tlively]

An alternative would be to not special-case ref types, but to change the condition to a.type != b.type && (a.type < b.type || b.type < a.type), which will detect structurally equivalent types correctly.

[kripken]

But would that work with recursive types, for which we create a new one when roundtripping which is the issue here?

[tlively]

Yes. operator< compares structure, not identity. (The bug it has where it can compare two types as mutually less than each other should not affect this use case.)

[kripken]

Oh right, the problem is just of comparing equality via identity... changed as you suggested.

[kripken]

Ah, as CI shows that does not work actually. The two types are not identical to each other after roundtripping. So the problem is still there.

Is it not as I thought earlier, that roundtripping creating new versions of recursive types is a problem for subtype checks between them later? If that's not it I guess I'll need to debug the comparison code unless you have an idea.

[tlively]

The new types should be structurally identical to the old types and therefore the new and old types should be subtypes of each other (and Type::isSubType should reflect that). If they are not structurally identical after round tripping, that sounds like a bug.

[kripken]

What happens appears to be this:

• The SubTyper compares the Types, which then
• Compares the HeapTypes, which then
• Compares the Signatures.

All that makes sense. Then Signature checking for subTyping does this:

  // TODO: Implement proper signature subtyping, covariant in results and
  // contravariant in params, once V8 implements it.
  // return isSubType(b.params, a.params) && isSubType(a.results, b.results);
  return a == b;

Do you remember why we commented that out? Looks like you added it in #3610

[kripken]

(Even if v8 doesn't have it yet, I think we should maybe uncomment it - hard for me to see the danger the commenting implies.)

[tlively]

Aha, that will do it! Using operator< instead of subtyping here should avoid that problem. Until V8 implements signature subtyping, if we emit binaries that depend on it, they will fail to validate on V8.

[tlively]

Actually, I think the subtyper could be changed to use !(a < b || b < a), since it's not supposed to depend on identity at all until we implement proper canonicalization.

[kripken]

Why would < be correct, though? Isn't it just an arbitrary order we use internally? (It's documented as Order types by some notion of simplicity)

It seems simpler to just not compare reference types at all here as in the first version of this PR? Or, what is the benefit of using <?

[tlively]

Yes, it's an arbitrary order, but it is a total order on structural types, so it can be used to detect structural equality. This will be moot once I update == to use structural equality (which I plan to do by the end of Monday).