Skip to content

fix: separate unreachable conditions from never types#1098

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

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

@lewis6991 lewis6991 commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Problem

Flow queries used never for two different ideas:

  • a value whose real type is never
  • a condition edge that cannot be reached

Those 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 value itself is still a string:

---@return false
local function always_false()
    return false
end

---@type string
local value = "ok"

if always_false() then
    inner = value -- should be string, not never
end

The same issue could show up as bogus diagnostics for unrelated call targets inside impossible branches:

---@return true
local function always_true()
    return true
end

local function b() end

if always_true() then
else
    b() -- should not become call-non-callable
end

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:

---@return false
local function always_false()
    return false
end

local value = "before"

if always_false() then
    value = 1
end

after = value -- should be string, not string|1

Solution

Separate unreachable condition edges from ordinary never values in flow-query results:

enum FlowQueryResult {
    Type(LuaType),
    Unreachable,
}

Then interpret Unreachable based on the kind of query being answered:

match flow_mode {
    FlowMode::Normal => continue_to_antecedent(),
    FlowMode::IgnoreConditions => continue_to_antecedent(),
    FlowMode::MergeBranch => contribute_never_to_merge(),
}

That gives the two cases different behavior:

  • Point queries continue through impossible condition edges so unrelated symbols keep their real type.
  • Merge-branch queries turn impossible predecessors into never, so unreachable assignments, casts, annotations, and fallback RHS values do not contribute to the joined type.

The change also preserves MergeBranch mode through the paths that can otherwise reintroduce unreachable branch facts:

local value = "before"

if always_false() then
    value = 1                  -- assignment ignored at merge
    ---@cast value integer      -- cast ignored at merge
    ---@type integer
    value = 1                  -- annotated assignment ignored at merge
end

after = value -- remains string

Reachable real never values are still distinct from unreachable branches. This still works because never is not treated as an unreachable edge by itself:

---@class NeverBox
---@field value never

---@return NeverBox
local function make_box() end

local value = make_box().value
local cond ---@type boolean

if cond then
    value = 1
end

after = value -- should be 1

Tests

Added regression coverage for:

  • plain call conditions preserving unrelated call prefixes
  • always-false and always-true call-condition branches
  • unreachable assignments not contributing to merge results
  • unreachable missing-field fallback assignments not contributing to merge results
  • unreachable tag casts not contributing to merge results
  • unreachable annotated assignments not contributing to merge results
  • reachable assignment over a real never value still contributing to merge results

Validated with:

  • cargo fmt --all --check
  • git diff --check HEAD^ HEAD
  • cargo test -p emmylua_code_analysis

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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.

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

Copy link
Copy Markdown
Contributor

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
@lewis6991 lewis6991 force-pushed the fix/separate-unreachable-condition-flow branch from 84f482f to 66b0434 Compare June 3, 2026 16:36
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
@lewis6991 lewis6991 force-pushed the fix/separate-unreachable-condition-flow branch from 66b0434 to 77cc687 Compare June 4, 2026 09:47
@lewis6991 lewis6991 marked this pull request as ready for review June 4, 2026 09:54
@lewis6991

Copy link
Copy Markdown
Collaborator Author

/gemini review

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

Copy link
Copy Markdown
Contributor

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 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.

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