fix(types): check boolean values against literal unions

[lewis6991]

The checker rejects this valid return:

---@return boolean
local function example()
    return true ---@as true | false
end

It also accepts this invalid assignment:

---@type true
local value = false

Compare boolean literal values. When the source is boolean, require both
true and false to fit the target type. This also accepts boolean where
true | false is required, including aliases. Return checks use existing
expression inference for ---@as casts. Boolean enum arguments match member
values rather than member names or quoted strings.

Fixes #1226. Related to #1228.

[github-actions]

Code Review

Overview

This PR addresses boolean type compatibility issues (issue #1226), specifically around union types containing boolean literals (true | false), boolean aliases, and inline ---@as type assertions on return statements. The changes span type checking logic, parameter checking, and removes the inline @as handling for returns.

---

Issues & Concerns

1. Removal of inline_as_type breaks documented behavior (High severity)

In return_type_mismatch.rs, the entire inline_as_type function and its usage are removed:

-        if let Some(as_type) = inline_as_type(semantic_model, ret, &expr) {
-            raw = as_type;
-        }

However, the new test test_inline_as_return_compatibility still expects ---@as casts to affect return type checking:

("string", "1", "string", true),   // return 1 ---@as string
("number", "'text'", "number", true), // return 'text' ---@as number

Problem: If inline_as_type is removed, how does return 1 ---@as string pass against ---@return string? The type_of_expr_at call alone would yield integer, which is not compatible with string. Either:

• The inline @as handling was moved elsewhere (not shown in this diff), or
• These tests will fail.

Recommendation: Clarify where inline @as on returns is now handled. If it was intentionally moved to type_of_expr_at or a lower layer, note that in the PR description. If not, this is a regression.

2. literal_values signature change is a breaking API change

-    literal_values(semantic_model, param_ty, &mut Vec::new())
+    literal_values(semantic_model, param_ty, &mut Vec::new(), boolean_only)

The new boolean_only: bool parameter is a positional boolean flag, which is a classic readability/maintainability smell. Callers must remember the argument order.

Recommendation: Consider an enum or a small struct for clarity:

enum LiteralFilter { All, BooleanOnly }

Or at minimum, document the parameter's semantics in a doc comment.

3. boolean_only filtering logic is subtle and inconsistent

In the enum branch:

if !boolean_only {
    values.push(member_ref.name.to_string());
}
if let Some(member_facts) = ... {
    let raw = node.text().to_string();
    // Boolean arguments match values, not member names or quoted text.
    if !boolean_only || matches!(raw.as_str(), "true" | "false") {
        values.push(raw.trim_matches(['"', '\'']).to_string());
    }
}

Concerns:

• The matches!(raw.as_str(), "true" | "false") check operates on the raw source text, which may include whitespace or be a quoted string like "true". After trim_matches, "true" becomes true, but the matches! check happens before trimming, so a quoted "true" would not match. This is likely a bug for enum members declared as ["true"] = ....
• The comment says "Boolean arguments match values, not member names or quoted text," but the code still pushes the trimmed value. The intent is unclear.

Recommendation: Trim first, then check:

let trimmed = raw.trim_matches(['"', '\'']);
if !boolean_only || matches!(trimmed, "true" | "false") {
    values.push(trimmed.to_string());
}

4. check_general_type_compact alias resolution loop may not terminate as expected

let mut resolved = source.clone();
let mut alias_guard = check_guard;
while let LuaType::Ref(id) | LuaType::Def(id) = &resolved
    && let Some(target) = context.alias_target_of(id)
    && let Ok(next_guard) = alias_guard.next_level()
{
    resolved = target;
    alias_guard = next_guard;
}
if resolved.is_boolean() {
    return check_general_type_compact(context, &resolved, compact_type, alias_guard);
}

Concerns:

• If alias_target_of returns the same Ref/Def (self-referential alias), the loop relies on next_level() failing to break. This is fragile but probably acceptable given the guard.
• The recursive call passes alias_guard (already advanced), which is correct, but the recursion could re-enter this same block if resolved is still a Ref to a boolean alias. Since resolved.is_boolean() is checked, and is_boolean() presumably returns true only for LuaType::Boolean, this should be fine — but only if alias_target_of fully resolves to LuaType::Boolean. If it resolves to another alias, the recursion will re-enter and re-resolve. This is redundant work but not incorrect.

Recommendation: Add a comment explaining the guard's role, and consider whether the recursive call should pass resolved directly to check_simple_type_compact instead of re-entering check_general_type_compact.

5. LuaType::Boolean handling in simple_type.rs may cause exponential recursion

LuaType::Boolean => {
    let next_guard = check_guard.next_level()?;
    for value in [true, false] {
        super::check_general_type_compact(
            context,
            &LuaType::BooleanConst(value),
            compact_type,
            next_guard,
        )?;
    }
    return Ok(());
}

Concern: For each boolean value, this calls check_general_type_compact, which may re-enter check_simple_type_compact for union components. With deeply nested unions, this could multiply work. The guard limits depth, but the branching factor is 2 per level.

Recommendation: This is likely acceptable for typical type depths, but consider memoizing or short-circuiting when compact_type is a simple LuaType::Boolean.

6. DocBooleanConst handling was removed from the match arm but not fully replaced

The old code had a dedicated LuaType::DocBooleanConst(b) arm. The new code merges it into:

LuaType::BooleanConst(value) | LuaType::DocBooleanConst(value) => match compact_type {
    LuaType::Boolean => return Ok(()),
    ...
}

Behavioral change: Previously, DocBooleanConst → Boolean returned Err(mismatch). Now it returns Ok(()). This is a semantic change (doc boolean const is now assignable to boolean). This may be intentional (aligning with BooleanConst behavior), but it's not called out in the PR description.

Recommendation: Confirm this is intentional and add a test case for ---@type boolean assigned a DocBooleanConst value.

7. Test helper check_cases has a fragile expected-diagnostics heuristic

if code == DiagnosticCode::ReturnTypeMismatch && target_type == "nil" && !accepted {
    expected.push(DiagnosticCode::RedundantReturnValue);
}

This special-cases a single scenario. If the checker's behavior changes, this test will silently pass/fail incorrectly.

Recommendation: Document why RedundantReturnValue is expected here, or restructure the test to assert on the specific diagnostic rather than a hardcoded list.

8. Test boolean_enum_arguments_match_member_values uses {opposite} in a format string

local Flag = {{
    Value = {value},
    Label = "{opposite}",
    ["{opposite}"] = 0,
}}

The {opposite} is interpolated as !value (a boolean), producing Label = "false" or Label = "true". This is intentional but confusing — the label is a string "true"/"false", while the key is also "true"/"false". The test then checks Flag.Value and raw true/false arguments. The Label and ["{opposite}"] members seem to exist only to test that non-boolean enum members don't match boolean arguments.

Recommendation: Add a comment explaining the purpose of Label and the string-keyed member.

---

Minor / Style

• literal_values recursion: The visited vector is passed by mutable reference and pushed to, but never popped. This means once a type is visited, it's never re-visited in the same call tree, which is correct for cycle detection but could cause missed values if the same type appears in sibling branches. Pre-existing issue, not introduced here.
• Comment placement: The comment // Check the whole boolean domain before selecting a single union component. appears twice (once in param_type_check.rs, once in type_check/mod.rs). Consider extracting to a shared doc or making them more specific.
• matches!(source, LuaType::Ref(_) | LuaType::Def(_)) — this pattern is repeated; consider a helper is_ref_or_def().

---

Summary

Severity	Issue
High	Removal of inline_as_type may break ---@as return casts; tests suggest it's still expected to work
Medium	boolean_only filtering checks raw text before trimming, likely missing quoted "true"/"false" enum members
Medium	DocBooleanConst → Boolean behavior changed from error to OK without documentation
Low	Positional boolean_only flag hurts readability
Low	Alias resolution loop is fragile; add comments
Low	Test helper special-cases RedundantReturnValue without explanation

Recommendation: Before merging, verify the inline_as_type removal is compensated elsewhere, and add a test for quoted boolean enum member values.

[lewis6991]

1. Removal of inline_as_type: Casts already pass through type_of_expr_at to apply_expr_casts, which handles AsCast. This removes duplicate parsing, not cast support. The PR description states this.

2. literal_values signature: This function is private, so the change does not break a public API. An enum would be an optional readability change.

3. boolean_only filtering: Checking before removing quotes is intentional. Boolean true must not match string "true" or an enum member named "true". The suggested change would allow those invalid matches.

4. Alias resolution: The existing guard stops resolution at 100 levels. resolved.is_boolean() excludes unresolved references, so the recursive call cannot re-enter alias resolution with another alias.

5. Boolean recursion: This adds two constant checks. Those constants do not split again, so this branch does not introduce a factor of two at each level. Equal types already return early.

6. DocBooleanConst compatibility: This is an intended correction. Annotated true and false are valid values of type boolean.

7. Nil diagnostic expectation: The comment immediately above the condition explains why RedundantReturnValue is expected. The exact comparison preserves that existing behavior.

8. Enum test members: Label and the string key deliberately ensure that quoted values and member names cannot match boolean arguments. A short comment could make that purpose clearer.