fix: separate unreachable conditions from never types#1098
Conversation
There was a problem hiding this comment.
I've reviewed the code changes. Here are my observations:
Issues Found
1. Potential Logic Error in finish_unreachable_condition
- File:
get_type_at_flow.rs(line ~1545-1552) - Issue: When
FlowMode::Normalis used inside an unreachable branch, the function returnsSchedulerStep::ContinueWalk(walk)without modifying the type. This could cause the type to propagate incorrectly through unreachable code paths. - Recommendation: Consider whether
Normalmode inside unreachable branches should also returnLuaType::Neverto prevent type pollution.
2. Missing Test for Edge Case
- Issue: The new tests cover
true/falsereturn conditions and plain function calls, but there's no test for:- Nested conditions (e.g.,
if a() then if b() then ... end end) - Mixed conditions with
elseifbranches - Conditions with
and/oroperators
- Nested conditions (e.g.,
- Recommendation: Add tests for these edge cases to ensure the new
Unreachableaction works correctly in complex scenarios.
3. Potential Performance Concern
- File:
get_type_at_flow.rs(line ~1320-1330) - Issue: The new code checks
branch_flow_ids.len()and handles 0 and 1 cases separately. For single predecessors, it continues the walk without creating a merge query. This is correct but adds a branch in a hot path. - Recommendation: Consider if this check could be optimized or if the single-predecessor case could be handled earlier in the flow graph construction.
4. Inconsistent Naming
- File:
mod.rs(cache) - Issue: The enum variant
MergeBranchis named differently from the relatedIgnoreConditionsvariant.MergeBranchdescribes what it does, whileIgnoreConditionsdescribes how it behaves. - Recommendation: Consider renaming to
MergeContributionorBranchContributionfor consistency with the "what it does" pattern.
Positive Aspects
- Clear separation of concerns: The new
Unreachableaction cleanly separates unreachable conditions from type results. - Good test coverage: The three new tests cover basic scenarios for function calls in conditions.
- Backward compatible: The changes maintain existing behavior for non-merge queries.
Minor Suggestions
- Documentation: Add comments explaining why
MergeBranchmode returnsNeverwhileNormalmode continues the walk infinish_unreachable_condition. - Type safety: Consider making
FlowModeimplementCopyexplicitly (it already derives it, but explicit documentation would help).
Overall, the changes look well-structured and address the issue of unreachable condition edges in merge scenarios. The logic is sound, but additional edge case testing would improve confidence.
There was a problem hiding this comment.
Code Review
This pull request refactors control flow analysis to better handle unreachable condition edges. It introduces a new FlowMode enum (Normal, MergeBranch, and IgnoreConditions) and a ConditionFlowAction::Unreachable action. This allows unreachable conditions to contribute never to merged types while letting point queries inside impossible branches continue to their antecedents. The feedback highlights three locations in get_type_at_flow.rs where hardcoding FlowMode::Normal discards the MergeBranch mode, which can prevent unreachable condition edges from correctly contributing never to the merged type.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
3e26dbd to
84f482f
Compare
84f482f to
66b0434
Compare
Truthiness checks for call conditions treated impossible branches as `never` values. That let unreachable branch facts leak into point queries and merges: unrelated symbols inside impossible bodies could look non-callable, while assignment and cast effects from impossible branches could still contribute to joined types. Track unreachable condition edges explicitly in flow-query results. Point queries continue through unreachable condition edges, while merge-branch queries contribute `never` for unreachable predecessors. Preserve merge-branch mode through assignments, tag casts, correlated condition subqueries, and recovered assignment fallbacks. This prevents assignments, annotated assignments, casts, and missing-field RHS fallback values inside impossible branches from contributing to final merge types. Add coverage for plain call conditions, always-false and always-true call-condition branches, reachable `never` assignments, and unreachable assignment, annotated-assignment, cast, and fallback merge contributions. Assisted-by: Codex
66b0434 to
77cc687
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the flow analysis engine to better handle unreachable branches and conditions. It introduces a new FlowQueryResult enum and MergeBranch flow mode to distinguish between actual never types and unreachable condition paths, ensuring that impossible branches do not incorrectly contribute fallback types to a merged type. Additionally, comprehensive unit tests have been added to verify these behaviors. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Problem
Flow queries used
neverfor two different ideas:neverThose are not equivalent. A query for a symbol inside an impossible branch still needs to be able to walk past the impossible condition and find the symbol's declared or assigned type.
For example, this branch is unreachable, but
valueitself is still astring:The same issue could show up as bogus diagnostics for unrelated call targets inside impossible branches:
At the same time, merge queries must still treat impossible predecessors as contributing nothing. In this case, the assignment in the impossible branch must not leak into the joined type:
Solution
Separate unreachable condition edges from ordinary
nevervalues in flow-query results:Then interpret
Unreachablebased on the kind of query being answered:That gives the two cases different behavior:
never, so unreachable assignments, casts, annotations, and fallback RHS values do not contribute to the joined type.The change also preserves
MergeBranchmode through the paths that can otherwise reintroduce unreachable branch facts:Reachable real
nevervalues are still distinct from unreachable branches. This still works becauseneveris not treated as an unreachable edge by itself:Tests
Added regression coverage for:
nevervalue still contributing to merge resultsValidated with:
cargo fmt --all --checkgit diff --check HEAD^ HEADcargo test -p emmylua_code_analysis