Skip to content

Fix: undefined-field false positive for table<K,V> when value type is generic and key is not string/integer#1109

Open
ShenzhenGopher wants to merge 1 commit into
EmmyLuaLs:mainfrom
ShenzhenGopher:fix/table-generic-value-undefined-field
Open

Fix: undefined-field false positive for table<K,V> when value type is generic and key is not string/integer#1109
ShenzhenGopher wants to merge 1 commit into
EmmyLuaLs:mainfrom
ShenzhenGopher:fix/table-generic-value-undefined-field

Conversation

@ShenzhenGopher

Copy link
Copy Markdown

Related Issue

Fixes #1108

Problem

The bug has two layers in check_field.rs:

Layer 1: get_key_types() drops non-string/integer built-in types

// Before (check_field.rs, get_key_types function, around line 355)
match &current_type {
    LuaType::String => { type_set.insert(current_type); }
    LuaType::Integer => { type_set.insert(current_type); }
    LuaType::Union(union_typ) => { /* expand union */ }
    LuaType::StrTplRef(_) | LuaType::Ref(_) => { type_set.insert(current_type); }
    LuaType::DocStringConst(_) | LuaType::DocIntegerConst(_) => { type_set.insert(current_type); }
    LuaType::Call(alias_call) => { /* expand keyof */ }
    _ => {}  // ← thread, boolean, number, userdata silently dropped!
}

When the key type is thread, get_key_types() returns an empty set → key_types.is_empty() → early return → reports undefined-field.

Layer 2: ExprType matching only handles string and integer

// Before (check_field.rs, is_valid_member function, around line 266)
LuaMemberKey::ExprType(typ) => {
    if typ.is_string() { /* pass */ }
    else if typ.is_integer() { /* pass */ }
    // ← thread, boolean, etc. fall through without any check
}

Fix

Fix 1: Add built-in types to get_key_types()

// After
LuaType::Boolean | LuaType::Thread | LuaType::Number | LuaType::Userdata => {
    type_set.insert(current_type);
}

Fix 2: Add fallback equality check in ExprType matching

// After
} else if key_types.iter().any(|kt| kt == typ) {
    return Some(());
}

Files Changed

  • crates/emmylua_code_analysis/src/diagnostic/checker/check_field.rs — fix both layers
  • crates/emmylua_code_analysis/src/diagnostic/test/undefined_field_test.rs — add 2 test cases

Test

Add test cases in undefined_field_test.rs:

#[test]
fn test_table_generic_value_with_thread_key() {
    let mut ws = VirtualWorkspace::new();
    assert!(ws.has_no_diagnostic(
        DiagnosticCode::UndefinedField,
        r#"
            ---@class Box<T>
            ---@field value T

            ---@class Container
            ---@field items table<thread, Box<unknown>?>
            local Container = {}

            ---@param co thread
            function Container:get(co)
                local item = self.items[co]
                return item
            end
        "#
    ));
}

#[test]
fn test_table_generic_value_with_boolean_key() {
    let mut ws = VirtualWorkspace::new();
    assert!(ws.has_no_diagnostic(
        DiagnosticCode::UndefinedField,
        r#"
            ---@class Box<T>
            ---@field value T

            ---@class Container
            ---@field items table<boolean, Box<unknown>?>
            local Container = {}

            ---@param key boolean
            function Container:get(key)
                local item = self.items[key]
                return item
            end
        "#
    ));
}

Verification

Before fix:

---@class Box<T>
---@field value T

---@class Container
---@field items table<thread, Box<unknown>?>
local Container = {}
---@param co thread
function Container:get(co)
    local item = self.items[co]  -- ⚠️ Undefined field `[co]`
    return item
end

After fix: No warning.

…eric and key is not string/integer

When a table<K,V> field has a generic value type (e.g. Box<unknown>?)
and the key type K is not string or integer (e.g. thread, boolean),
indexing the table with a correctly-typed key falsely reports
undefined-field.

Root cause: get_key_types() only recognized string, integer and a few
other types, silently dropping thread/boolean/number/userdata. The
subsequent ExprType member matching also only handled string and integer.

Fix:
- Add thread/boolean/number/userdata to get_key_types()
- Add fallback equality check in ExprType matching branch
- Add test cases for thread and boolean key types

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

Code Review: check_field.rs Changes

Issues Found:

  1. Potential Logic Error in Key Type Comparison (Line 277)

    • The comparison kt == typ uses PartialEq for LuaType which may not handle all type variations correctly (e.g., generic types, type aliases)
    • Consider using a more robust comparison method like type_match or structural equality check
  2. Inconsistent Type Handling (Lines 380-383)

    • Adding Boolean, Thread, Number, Userdata to get_key_types is correct for basic types
    • However, Number might overlap with DocIntegerConst - ensure no duplicate handling issues
  3. Missing Edge Cases (Lines 380-383)

    • String type is not included in the new block, but DocStringConst is already handled above
    • Consider if Nil type should also be included as a potential key type

Recommendations:

  1. Line 277: Replace direct equality with a type compatibility check:
} else if key_types.iter().any(|kt| typ.is_subtype_of(kt) || kt.is_subtype_of(typ)) {
    return Some(());
}
  1. Lines 380-383: Add String type for consistency:
LuaType::Boolean | LuaType::Thread | LuaType::Number | LuaType::Userdata | LuaType::String => {
    type_set.insert(current_type);
}
  1. Test Coverage: Consider adding tests for:
    • Number key type (not just boolean and thread)
    • Userdata key type
    • Mixed key types in table generics

Test File Changes:

  • ✅ Tests are well-structured and test the specific scenarios
  • ✅ Good use of VirtualWorkspace and has_no_diagnostic
  • Consider adding negative test cases (where undefined field should be reported)

@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 expands the field checking diagnostic logic to support additional Lua types (Boolean, Thread, Number, and Userdata) as valid table keys, and includes corresponding unit tests. A review comment suggests optimizing the key type check by using HashSet::contains instead of a linear search, which improves performance.

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 on lines +277 to +278
} else if key_types.iter().any(|kt| kt == typ) {
return Some(());

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.

medium

Since key_types is a HashSet<LuaType>, you can use contains for an $O(1)$ lookup instead of performing a linear $O(N)$ scan with iter().any(...). This improves performance during diagnostic analysis.

Suggested change
} else if key_types.iter().any(|kt| kt == typ) {
return Some(());
} else if key_types.contains(typ) {
return Some(());

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.

Bug: table<K,V> with generic value type reports false undefined-field when key is not string/integer

1 participant