Allow rec groups of public function types in closed world

[kripken]

Closed-world mode allows function types to escape if they are on exported functions,
because that has been possible since wasm MVP and cannot be avoided. But we need to
also allow all types in those type's rec groups as well. Consider this case:

(module
 (rec
  (type $0 (func))
  (type $1 (func))
 )
 (func "0" (type $0)
  (nop)
 )
 (func "1" (type $1)
  (nop)
 )
)

The two exported functions make the two types public, so this module validates in
closed world mode. Now imagine that metadce removes one export:

(module
 (rec
  (type $0 (func))
  (type $1 (func))
 )
 (func "0" (type $0)
  (nop)
 )
 ;; The export "1" is gone.
)

Before this PR that no longer validates, because it only marks the type $0 as public.
But when a type is public that makes its entire rec group public, so $1 is errored on.

To fix that, this PR allows all types in a rec group of an exported function's type, which
makes that last module validate.

Unfortunately this likely means our validation is not really useful any more, as in
closed world we generally merge all types into a single big rec group anyhow, so pretty
much all types get allowed...? Perhaps we should consider relaxing validation there
even more to simplify things.

(This was found by the fuzzer on wasm-metadce.)

[tlively]

Public types are never combined into the single large rec group, so no need to worry about that. But would it make more sense to prohibit public types from ever being in nontrivial rec groups instead? That seems more consistent with how we treat closed-world as locking everything down.

[kripken]

Public types are never combined into the single large rec group, so no need to worry about that.

Not intentionally public ones, yeah. But that single large rec group ends up containing almost everything, and if that includes even one type of a function that is exported then it's all allowed-to-be-public.

But would it make more sense to prohibit public types from ever being in nontrivial rec groups instead?

What is a "nontrivial rec group"? And by public here do you mean "intentionally public" or "allowed-to-be-public-by-the-validator-because-it-is-on-an-exported-function"?

[tlively]

Public types are never combined into the single large rec group, so no need to worry about that.

Not intentionally public ones, yeah. But that single large rec group ends up containing almost everything, and if that includes even one type of a function that is exported then it's all allowed-to-be-public.

This should never happen, though.

But would it make more sense to prohibit public types from ever being in nontrivial rec groups instead?

What is a "nontrivial rec group"? And by public here do you mean "intentionally public" or "allowed-to-be-public-by-the-validator-because-it-is-on-an-exported-function"?

"nontrivial" meaning a rec group of size greater than 1. I don't see the distinction between those two kinds of public types, so I must be missing something.

[kripken]

Ah, I see, you mean that public types - which existing code correctly identifies using the rec group - are not added to the big new rec group. So that worry of mine is moot.

The distinction I meant is between the normal definition of what is public, and what is considered ok-to-be-public in the hack code that this PR modifies, that is, the exception we make for functions that are exported. The hack considered their types ok to be public even though they are nontrivial types that we normally disallow in closed world. This PR makes us also allow anything in their rec groups, which is necessary as the testcase shows and as you've convinced me, also safe.

[tlively]

I still think the simpler solution of disallowing exported and imported functions from having types in nontrivial rec groups would solve the problem by defining it out of existence. Is there some reason we need to allow public functions to have types in nontrivial rec groups?

[kripken]

I can't think of a reason right now, but what you suggest adds restrictions (as opposed to removing them as this PR does) which might end up causing trouble for users. We'd need to talk to our users, at minimum, to find out that what you suggest can work, I think?

[tlively]

After talking about this offline, this LGTM % test failures.

[kripken]

Test failure was that an existing test broke due to the change. I fixed and extended that test rather than add a new one in this PR.