Disallow exact references in public types #7554
Conversation
Use the standard utility rather than reimplementing type intersection. The new code is simpler, shorter, and properly supports exactness, avoiding an assertion failure in the added test case. The other functional change is that when one of the intersected heap types is bottom and the type GLB is a non-nullable reference to bottom, the result of the intersection is `None` where it was previously a `Cone`.
Previously doing e.g. `type.with(HeapType::none)` would cause an assertion failure if `type` was exact because `.with()` would only replace the heap type and exact references to basic heap types are disallowed. Rather than checking for and avoiding this error in all the callers, simply drop exactness when `.with()` is called with a basic heap type. This is reasonable behavior because the only alternative is never correct. Add a test that hits an assertion failure without this fix. AbstractTypeRefining replaces a defined type with `none` and the type updating utility does not check whether the new heap type is basic before doing the replacement.
When custom descriptors are disabled, validate that public types do not contain exact references. If they did, we would drop the exactness and change the identity of the public type during binary writing, which would be incorrect. This still allows internal usage of exact types without custom descriptors enabled, and it is up to the individual passes to ensure that the eventual erasing of exactness does not cause any problems.
| if (module.features.hasCustomDescriptors()) { | ||
| return; | ||
| } | ||
| for (auto type : ModuleUtils::getPublicHeapTypes(module)) { |
There was a problem hiding this comment.
It looks like this adds a scan of the entire module in the common case (no CD, and validation is enabled)?
Perhaps we can avoid that overhead by instead erroring in the binary writer? That code already scans the module for heap types, and we could reuse that scan.
There was a problem hiding this comment.
I think the validator is a more principled location for this check. I'll investigate reimplementing getPublicHeapTypes to scan only the imports and exports and the types they can reach, though.
There was a problem hiding this comment.
I assume with that optimization this is practically a no-op now?
There was a problem hiding this comment.
Well it still has to scan the imports and exports and do an arbitrarily large traversal of the type graph, but at least it's not scanning the whole module now, so I wouldn't expect this to be a significant performance problem.
| if (module.features.hasCustomDescriptors()) { | ||
| return; | ||
| } | ||
| for (auto type : ModuleUtils::getPublicHeapTypes(module)) { |
There was a problem hiding this comment.
I assume with that optimization this is practically a no-op now?
When custom descriptors are disabled, validate that public types do not
contain exact references. If they did, we would drop the exactness and
change the identity of the public type during binary writing, which
would be incorrect. This still allows internal usage of exact types
without custom descriptors enabled, and it is up to the individual
passes to ensure that the eventual erasing of exactness does not cause
any problems.