Skip to content

fix: separate unreachable conditions from never types#1098

Draft
lewis6991 wants to merge 1 commit into
EmmyLuaLs:mainfrom
lewis6991:fix/separate-unreachable-condition-flow
Draft

fix: separate unreachable conditions from never types#1098
lewis6991 wants to merge 1 commit into
EmmyLuaLs:mainfrom
lewis6991:fix/separate-unreachable-condition-flow

Conversation

@lewis6991
Copy link
Copy Markdown
Collaborator

Problem

Truthiness checks for call conditions treated an impossible branch as a never result for the queried symbol. That made unrelated symbols inside impossible if/else bodies look like never, which could surface as bogus diagnostics such as a callable local becoming non-callable inside the branch.

Solution

Add an explicit unreachable condition action and handle it by flow query mode. Normal point queries continue past unreachable condition edges so unrelated symbols keep their declared types. Merge-branch queries convert an unreachable condition edge into never, preserving branch-merge behavior where impossible predecessors should not contribute to the final union.

This also renames the flow modes around their actual use, keeps single-predecessor branch labels out of merge mode, and adds regression coverage for plain call conditions plus always-false and always-true call-condition branches.

Tests

  • cargo fmt --all --check
  • git diff --check
  • cargo test -p emmylua_code_analysis
  • TMPDIR=/private/tmp cargo test

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::Normal is used inside an unreachable branch, the function returns SchedulerStep::ContinueWalk(walk) without modifying the type. This could cause the type to propagate incorrectly through unreachable code paths.
  • Recommendation: Consider whether Normal mode inside unreachable branches should also return LuaType::Never to prevent type pollution.

2. Missing Test for Edge Case

  • Issue: The new tests cover true/false return 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 elseif branches
    • Conditions with and/or operators
  • Recommendation: Add tests for these edge cases to ensure the new Unreachable action 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 MergeBranch is named differently from the related IgnoreConditions variant. MergeBranch describes what it does, while IgnoreConditions describes how it behaves.
  • Recommendation: Consider renaming to MergeContribution or BranchContribution for consistency with the "what it does" pattern.

Positive Aspects

  1. Clear separation of concerns: The new Unreachable action cleanly separates unreachable conditions from type results.
  2. Good test coverage: The three new tests cover basic scenarios for function calls in conditions.
  3. Backward compatible: The changes maintain existing behavior for non-merge queries.

Minor Suggestions

  1. Documentation: Add comments explaining why MergeBranch mode returns Never while Normal mode continues the walk in finish_unreachable_condition.
  2. Type safety: Consider making FlowMode implement Copy explicitly (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.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/emmylua_code_analysis/src/semantic/infer/narrow/get_type_at_flow.rs Outdated
Comment thread crates/emmylua_code_analysis/src/semantic/infer/narrow/get_type_at_flow.rs Outdated
@lewis6991 lewis6991 force-pushed the fix/separate-unreachable-condition-flow branch 2 times, most recently from 3e26dbd to 84f482f Compare June 3, 2026 16:22
@lewis6991 lewis6991 marked this pull request as draft June 3, 2026 16:32
Truthiness checks for call conditions treated an impossible branch as
`Result(Never)`. That made the queried symbol look like `never` while walking
the branch, so unrelated callable values inside impossible `if`/`else` bodies
could be narrowed to non-callable.

Add an explicit unreachable condition action and handle it by query mode.
Normal point queries continue past an unreachable condition edge so unrelated
symbols keep their declared types. Merge-branch queries finish with `never`, so
unreachable branch predecessors still drop out of the merged type.

Carry branch reachability as an internal flow-query result instead of inferring
it from `LuaType::Never`. Assignment and cast continuations can then distinguish
an unreachable predecessor from an ordinary type result that happens to be
`never`.

Preserve merge-branch mode through assignment, tag-cast, and correlated
condition subqueries. That prevents assignments, annotated assignments, and
casts inside impossible branches from contributing their statement-local type
effects to the final merge.

Rename flow modes around their actual use and keep single-predecessor
branch labels out of merge mode. Add coverage for plain call conditions,
always-false and always-true call-condition branches, and unreachable
assignment, annotated-assignment, and cast merge contributions.

Assisted-by: Codex
@lewis6991 lewis6991 force-pushed the fix/separate-unreachable-condition-flow branch from 84f482f to 66b0434 Compare June 3, 2026 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant