Reboot TypeGeneralizing to use SubtypingDiscoverer by kripken · Pull Request #9263 · WebAssembly/binaryen

kripken · 2026-10-14T22:10:33Z

Now that we have a "late opts" stage, we can do things like generalize
types. This initial PR rewrites the pass to use the SubtypingDiscoverer
logic that was added since, and which makes the pass a lot
simpler (about half the length).

More work is needed to get this fuzzed etc., but just this rewrite
already removes some TODOs and asserts that are no longer relevant.
The pass can now run on a large Kotlin testcase without erroring, and
it shrinks the size there by 0.25% (not much yet, but after adding cast
generalization, perhaps more..?)

All old tests pass like before. (One new test is added, to handle a
crash that happened during this porting process.)

This is basically NFC, except that removing the old asserts mean
that more things can run.

tlively

Some initial comments before I get into the meat of the new implementation.

Test coverage doesn't look great, especially for the new changes in type-generalizing.

  Metric       Covered / Total        Details
  ------------ ---------------------- ----------------------------------
  Lines        243 / 326 (74.5%)      Executable lines in diff
  Regions      195 / 232 (84.1%)      Sub-line / block AST regions in diff
  Branches     131 / 170 (77.1%)      True/False branch outcomes in diff
  MC/DC        6 / 21 (28.6%)         Condition independence pairs in diff
  Functions    28 / 103 (27.2%)       Modified functions called >= 1x

tlively · 2026-10-15T00:52:54Z

  void visitRethrow(Rethrow* curr) {}
-  void visitThrowRef(ThrowRef* curr) {}
+  void visitThrowRef(ThrowRef* curr) {
+    self()->noteNonFlowSubtype(curr->exnref, Type(HeapType::exn, Nullable));


Why "non-flow" here? The ref does flow into the throw and out of a later catch.

Yes, but it doesn't flow out of this expression. Reading the docs on top, I guess this can be ambiguous. I can reword that?

tlively · 2026-10-15T00:55:02Z

+    if (curr->ref->type.isRef() && curr->ref->type.getHeapType().isShared()) {
+      share = Shared;
+    }
+    self()->noteNonFlowSubtype(


Again, why non-flow? Previously the only use of noteNonFlowSubtype was for ref.eq. If all these new uses of noteNonFlowSubtype are correct, then I suspect there are many more places that should be using it.

I think this is correct (for the reasons I mentioned above) but yes, there might be more places it makes sense to use. I didn't try to find them all in this PR - would make more sense in a place where we can cover them more easily with tests, though I don't know that we have a better pass, actually...

I can remove the ones not directly needed for tests in this PR if you prefer, though e.g. the String ones seem obvious?

(Aside from them, the ThrowRef one can also be removed from this PR, it looks like.)

tlively · 2026-10-15T01:11:14Z

+    if (curr->desc) {
+      auto descType = curr->type.getHeapType().getDescriptorType();
+      assert(descType);
+      self()->noteSubtype(curr->desc, Type(*descType, Nullable, Exact));
+    }


If visitation order matters, this should come after visiting the fields. (And even if visitation order doesn't matter now, it would probably be better to be consistent about it.)

BrOn and RefCast are also missing descriptor handling.

The order doesn't matter, but I agree it is nice to follow. Fixed.

BrOn and RefCast are also missing descriptor handling.

How about leaving those for later + tests? This PR is mainly focused on the reboot, but not adding new functionality.

tlively · 2026-10-15T01:20:11Z

Is this change related?

Yes. The fuzzer uses SubtypingDiscoverer internally (to find mutations, of the proper types). So improvements here lead to fuzzer improvements.

tlively · 2026-10-15T01:24:46Z

It's probably worth a TODO about additional things that are not handled optimally by SubtypeDiscoverer because it assumes the result types cannot be generalized: specifically ref.as_non_null, br_on_null, publish, br_if, and maybe a couple others.

Added a TODO

tlively · 2026-10-15T01:33:59Z

ChildTyper would probably be a better utility to use here because it does not assume the parent expression's type cannot be generalized.

(In fact, ChildTyper is a better fit for all current users of SubtypingDiscoverer except for Unsubtyping, which is where SubtypingDiscoverer originally came from.)

tlively · 2026-10-15T03:53:30Z

Actually, ChildTyper is not enough because it does not know about the required upper bounds on the parent expression's type, which means it cannot reflect those bounds in the constraints it reports for child types. We would need to build a full principal types utility (built on top of ChildTyper) to let us propagate type contraints from parents to children. I'll consider that as a follow-on project rather than holding up this PR.

Co-authored-by: Thomas Lively <tlively123@gmail.com>

kripken · 2026-10-15T18:29:46Z

Commit pushed with comprehensive tests for new functionality in this PR.

The one thing I did not do here is verify they are new and not previously covered (as I didn't write the old ones originally, so I'd need to do a lot of work to match things up). But I had Gemini check that. In the worst case, if it got it wrong, we might be adding some small amount of overlap.

tlively · 2026-10-16T20:56:45Z

Thanks, that sounds like the right trade off to me.

kripken added 17 commits October 13, 2026 15:07

work

950784a

oar

eb92c6f

work

479cf92

fix

327c476

fuzz

78630e6

undo

01ec083

undo

7d4362d

undo

30d4880

undo

62d6f45

undo

6df116a

undo

d5649c8

simpl

3b83bd7

simpl

e2745e8

form

cc70c10

comment

3aad80b

simpl

7f496de

simpl

13d2b15

kripken requested a review from tlively October 14, 2026 22:10

kripken requested a review from a team as a code owner October 14, 2026 22:10

kripken added 2 commits October 14, 2026 15:24

Merge remote-tracking branch 'origin/main' into typegen

4dd3c5f

update fuzz test

c3067a1

tlively reviewed Oct 15, 2026

View reviewed changes

kripken and others added 6 commits October 15, 2026 09:22

Merge remote-tracking branch 'origin/main' into typegen

ac8eaa5

reorder to execution order

1bedbe6

Update test/lit/passes/type-generalizing.wast

3c629de

Co-authored-by: Thomas Lively <tlively123@gmail.com>

rephrease test

7dd90b7

todo

44bff47

add tests for gaps

84e4977

Conversation

kripken commented Oct 14, 2026

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Oct 15, 2026

Uh oh!

tlively commented Oct 15, 2026

Uh oh!

kripken commented Oct 15, 2026

Uh oh!

tlively commented Oct 16, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants