Make final types the default #5918
Conversation
Match the spec and parse the shorthand binary and text formats as final and emit final types without supertypes using the shorthands as well. This is a potentially-breaking change, since the text and binary shorthands can no longer be used to define types that have subtypes. Also make TypeBuilder entries final by default to better match the spec and update the internal APIs to use the "open" terminology rather than "final" terminology. Future changes will update the text format to use the standard "sub open" rather than the current "sub final" keywords. The exception is the new wat parser, which supporst "sub open" as of this change, since it didn't support final types at all previously.
|
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Codecov Report
@@ Coverage Diff @@
## main #5918 +/- ##
==========================================
- Coverage 42.61% 42.60% -0.02%
==========================================
Files 484 484
Lines 74844 74837 -7
Branches 11924 11922 -2
==========================================
- Hits 31897 31886 -11
- Misses 39747 39749 +2
- Partials 3200 3202 +2
|
|
Oh neat, the coverage report strongly suggests that the TypeSSA tests have regressed, and indeed if you look at the diff, there should definitely not be so many final types. Will fix that when I'm done moving. |
|
(github errors on trying to review this PR, so trying a toplevel comment) In
Shouldn't that default to true? |
No, we're changing the default to be final, i.e. not open. |
| @@ -1,5 +1,5 @@ | |||
| (module | |||
| (type $struct (struct (field i32))) | |||
| (type $struct (sub (struct (field i32)))) | |||
There was a problem hiding this comment.
This is a non-final type. Now that we interpret the shorthand (type $... (struct ...)) as a final type, we need to add (sub ...) to the printed output to correctly communicate that this is a non-final type.
There was a problem hiding this comment.
I'm still confused then, sorry. Why is this non-final? Final is the default, and I don't see "open" here, so isn't it final?
There was a problem hiding this comment.
Right now Binaryen's text format does not include any of the changes from WebAssembly/gc#413, so (type (struct ...)) is final, (type (sub (struct ...))) is open, and (type (sub final (struct ...))) is final. Yes, this is confusing, so we're going to fix it in that PR.
| @@ -255,6 +255,7 @@ struct TypeSSA : public Pass { | |||
| builder[i] = oldType.getArray(); | |||
| } | |||
| builder[i].subTypeOf(oldType); | |||
| builder[i].setOpen(); | |||
There was a problem hiding this comment.
Why do the new types need to be open? I'd expect the opposite, since they are fresh types created here with no subtypes.
There was a problem hiding this comment.
This was the easiest way to get the test for clashing rec groups to work out. It should be fine, since the optimal thing for us to be doing is to make all types open up front and then have another pass (not written yet) to close all the types we can at the end.
| (type $sig (func_subtype (param i32) func)) | ||
|
|
||
| ;; As above, but now an import also uses this signature, which prevents us | ||
| ;; from changing anything. | ||
| ;; CHECK: (import "out" "func" (func $import (type $sig) (param i32))) | ||
| ;; CHECK: (import "out" "func" (func $import (type $func.0) (param i32))) |
There was a problem hiding this comment.
Not specific to this PR, but this automatic type picking has always felt surprising and dangerous to me...
There was a problem hiding this comment.
Oh yikes, this appears to be a real bug. $func.0 does not exist.
There was a problem hiding this comment.
Fixed separately in #5927.
|
Going to land this before the separate bug fix instead of after because this is much more at risk of other merge conflicts. |
Match the spec and parse the shorthand binary and text formats as final and emit final types without supertypes using the shorthands as well. This is a potentially-breaking change, since the text and binary shorthands can no longer be used to define types that have subtypes. Also make TypeBuilder entries final by default to better match the spec and update the internal APIs to use the "open" terminology rather than "final" terminology. Future changes will update the text format to use the standard "sub open" rather than the current "sub final" keywords. The exception is the new wat parser, which supporst "sub open" as of this change, since it didn't support final types at all previously.
Match the spec and parse the shorthand binary and text formats as final and emit
final types without supertypes using the shorthands as well. This is a
potentially-breaking change, since the text and binary shorthands can no longer
be used to define types that have subtypes.
Also make TypeBuilder entries final by default to better match the spec and
update the internal APIs to use the "open" terminology rather than "final"
terminology. Future changes will update the text format to use the standard "sub
open" rather than the current "sub final" keywords. The exception is the new wat
parser, which supporst "sub open" as of this change, since it didn't support
final types at all previously.