[tlively]

Rename Type::isNullable and Type::isNonNullable to Type::isNullableRef and Type::isNonNullableRef to make it clear that they already check that the type is a reference, and remove redundant isRef() checks at call sites.

[gemini-code-assist]

Code Review

This pull request renames isNullable() to isNullableRef() and isNonNullable() to isNonNullableRef() across the codebase to clarify that these methods check reference types. Two critical issues were identified in the review: first, a logical contradiction in DeadArgumentElimination.cpp where a return type is checked for being both null and non-nullable, which always evaluates to false; second, an unsafe call to getHeapType() in wasm-validator.cpp that could cause a crash on invalid modules if the type is not first verified to be a reference type.

[gemini-code-assist]

There is a logical contradiction in this check. func->getResults().isNull() checks if the type is the null type (which is a nullable reference type), while func->getResults().isNonNullableRef() checks if it is non-nullable. A type cannot be both null and non-nullable, so wasReturnUninhabitable will always evaluate to false. Based on the comment, you likely want to check if the return type is a non-nullable reference.

Suggested change

	bool wasReturnUninhabitable =
	func->getResults().isNull() && func->getResults().isNonNullable();
	func->getResults().isNull() && func->getResults().isNonNullableRef();
	bool wasReturnUninhabitable =
	func->getResults().isNonNullableRef();

[tlively]

This is not correct, but also points to a good renaming we could do. I'll also rename "isNull" to "isBottomRef".

[gemini-code-assist]

Calling curr->type.getHeapType() directly is unsafe here because curr->type is not guaranteed to be a reference type if the module being validated is invalid (e.g., if it is annotated with a basic type like i32). This will cause an assertion failure or crash in getHeapType(). You should restore the curr->type.isRef() check to ensure short-circuiting protects the getHeapType() call.

Suggested change

	if (!shouldBeTrue(curr->type.getHeapType().isContinuation(),
	if (!shouldBeTrue(curr->type.isRef() &&
	curr->type.getHeapType().isContinuation(),

[stevenfontanella]

Looks like a false positive since we'll early return if it's not a ref

[kripken]

Isn't isNonNullable already clear enough about implying it is a reference? Only a reference can be nullable or non-nullable. That is, the longer names seem a little unnecessary to me.

[tlively]

Since it's a negative, it's arguably ambiguous whether isNonNullable() it means isRef() && !isNullable() or just !isNullable(). This makes it unambiguous, so I think it's a worthwhile change.

[stevenfontanella]

This looks equivalent to !isRef || !isNullable while the original code is !isRef || isNonNullable. Is !isNullable the same as isNonNullable? I guess this is only in the case when isRef so yes?

[tlively]

Yeah, I think this should be good.

Non-refs: take the branch before and after.
nullable refs: do not take the branch before and after.
non-nullable refs: take the branch before and after.

[kripken]

@tlively I suppose that is arguable... but have you ever read "this is a non-nullable type" and thought "well that might be an i32"..? 🤔

[stevenfontanella]

@tlively I suppose that is arguable... but have you ever read "this is a non-nullable type" and thought "well that might be an i32"..? 🤔

Some code paths make this mistake and add a redundant check for isRef e.g. here

[tlively]

I've been uncertain about this in the past. And Gemini Code Assist was certainly confused about it here: #9282 (comment).

[kripken]

@stevenfontanella that code you linked to could be shortened, but it would need to be reordered (so it is safe to call getHeapType) - maybe that is the reason it is more verbose? Anyhow, we can fix such code paths.

@tlively If this is an AI issue, can we fix it with docs for the AI? If it is really an issue for you, I'd like to understand this better.

I think using longer, excessively-verbose names makes the code harder to read and less fun, as I wrote in the other PR. But I'll change my mind if we have a nice and short option to switch to, or if there is a real (non-AI) problem of understanding here.

[tlively]

isNNRef()?

[kripken]

I can live with isNNRef.

[tlively]

There were lots of redundant code paths, and I think that points to a real problem with the existing names. Both authors and reviewers are currently missing the redundancy.

Here are all 15 locations across the two commits where a redundant isRef() check was removed:

1. From Type::is{,Non}NullableRef (3203196)

1. LocalStructuralDominance::Scanner (LocalStructuralDominance.cpp:81)
2. TNHOracle::scan (possible-contents.cpp:1767)
3. Inhabitator::markBottomRefsNullable (heap-types.cpp:1030)
4. Inhabitator::markExternRefsNullable (heap-types.cpp:1051-1052)
5. Inhabitator::breakNonNullableCycles (heap-types.cpp:1182)
6. isUninhabitable (heap-types.cpp:1403)
7. FunctionValidator::visitContNew (wasm-validator.cpp:4457-4468)
8. FunctionValidator::visitContBind (wasm-validator.cpp:4505-4509)

2. From Type::isBottomRef (f250db9)

9. Literal::operator== (literal.cpp:505-508)
10. FunctionValidator::visitArrayGet (wasm-validator.cpp:3949-3951)
11. FunctionValidator::visitArraySet (wasm-validator.cpp:3973-3975)
12. FunctionValidator::visitArrayLoad (wasm-validator.cpp:4006-4008)
13. FunctionValidator::visitArrayStore (wasm-validator.cpp:4034-4036)
14. FunctionValidator::visitArrayFill (wasm-validator.cpp:4128-4130)
15. FunctionValidator::visitArrayInit (wasm-validator.cpp:4161-4163)

[tlively]

Just to add my 2 cents to the bikeshed, I would prefer isNonNullableRef over isNNRef for symmetry with isNullableRef. I don't think the verbosity is a problem because (IMO) it carries its weight in improved clarity.

[stevenfontanella]

Just to add my 2 cents to the bikeshed, I would prefer isNonNullableRef over isNNRef for symmetry with isNullableRef. I don't think the verbosity is a problem because (IMO) it carries its weight in improved clarity.

I agree FWIW

[kripken]

Ok, fair enough, I don't disagree that strongly. sgtm