Repository navigation
fix(flow): the false branch of a member test keeps owners whose member can be falsy - #1361
mareklangiewicz wants to merge 1 commit into
Conversation
…r can be falsy `filter_type_by_member_truthiness` asked one question for both branches: can the member be truthy? The false branch then dropped every owner whose member can be truthy, even a `boolean` that can also be false. After `if res and res.ok then return end` with `res: Res?`, `res` was `nil`. The false branch now keeps an owner unless its member is always truthy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018NxUUQmc3YFNEXrUcrVPxV
There was a problem hiding this comment.
Code Review
Summary
The change fixes a real semantic bug: the false branch of a member truthiness test previously used type_is_truthy (which asks "can this be truthy?") and compared it against keep_matching, so a boolean member was incorrectly dropped from the false branch. The new logic correctly distinguishes "can be truthy" vs. "can be falsy". The fix is sound and well-tested.
Issues & Suggestions
1. Asymmetry between the two branches (minor, readability)
let possible = if keep_matching {
type_is_truthy(&member_ty) // "can be truthy"
} else {
!member_ty.is_always_truthy() // "can be falsy"
};The two branches use different helper functions (type_is_truthy vs. !is_always_truthy). This is correct, but the asymmetry makes it harder to verify at a glance that the two are exact logical duals. Consider making the intent explicit, e.g.:
let possible = if keep_matching {
type_is_truthy(&member_ty)
} else {
type_is_falsy(&member_ty) // if such a helper exists / can be added
};or add a short comment noting that !is_always_truthy() is the "can be falsy" predicate. This guards against future divergence if one helper's semantics change.
2. Verify is_always_truthy semantics for nil/missing members
The unwrap_or(LuaType::Nil) path relies on Nil.is_always_truthy() == false, so missing members are kept in the false branch. That is the desired behavior, but it's worth confirming is_always_truthy is conservative (returns false for unions containing nil, boolean, false, etc.) rather than optimistic. If it ever returns true for a type that can actually be falsy, the false branch would silently drop valid components. A unit test covering a union member type (e.g. string | nil) in the false branch would harden this.
3. Test coverage gaps
The added test is good but could be extended:
- No case exercises the true branch (
keep_matching == true) with abooleanmember to confirm it is still kept (regression guard for the other side of the change). - No case with a union member type (e.g.
---@field flag string|nil) to validate theis_always_truthyboundary. ALWAYS_TRUTHY_MEMBERassertingnilis a nice negative case; consider also asserting the positive counterpart (if res and res.name then ... endkeepsRes).
4. Comment accuracy
The comment says "the false branch one whose member can be falsy (a boolean member can be either)." This is accurate, but the phrase "can be either" is slightly ambiguous — it means the member may be truthy or falsy, hence kept in both branches. Minor wording suggestion:
"...the false branch keeps a component whose member can be falsy. A
booleanmember can be either, so it is kept in both branches."
Verdict
The logic fix is correct and the test demonstrates the intended behavior. The main actionable items are (a) making the two-branch predicate symmetry explicit and (b) adding a true-branch / union-member regression test to lock in the behavior. No blocking issues.
Summary
The false branch of a member test drops an owner whose member can be falsy:
filter_type_by_member_truthinessasks "can the member be truthy?" for both branches, so abooleanmember counts as truthy and the false branch removes its owner:Evidence
test_member_false_branch_keeps_owner_whose_member_can_be_falsy, onresafter the guard:mainif res and res.ok then return endnilRes?if res then if res.ok ...nilRes?if not (res and res.ok) thennilRes?boolean?membernilRes?stringmember, always truthynilnilmainde89376): the test fails,Nil≠Res?.ifwithtruefails a test. Fullemmylua_code_analysissuite passes.clippyreports nothing new.Merge Danger
Door: two-way
Blast Radius: narrowing
emmylua_checkfrommainand from this branch on three real Lua codebases:mainThe 2 added in the second codebase look false: a module field read after
if M.flag then return endreportsmay be nilandundefined-field. I haven't found why, and they stay with the other PRs applied. The other added one, anunnecessary-if, is gone once #1360 is applied too.This is one of two fixes #1359 needs: see that PR.
Written with help from Claude Code.
🤖 Generated with Claude Code
https://claude.ai/code/session_018NxUUQmc3YFNEXrUcrVPxV