Skip to content

fix(check): resolve cross-file enum runtime members - #1292

Open
hgschmie wants to merge 2 commits into
EmmyLuaLs:mainfrom
hgschmie:fix/cross-file-enum-members
Open

hgschmie wants to merge 2 commits into
EmmyLuaLs:mainfrom
hgschmie:fix/cross-file-enum-members

Conversation

@hgschmie

@hgschmie hgschmie commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

An enum runtime table declared in another file reports valid members as
undefined, despite hover finding their values. The checker looks up the resolved
declaration in the current file's facts and, once allowed through, checks the
enum type's field surface instead of the table constructor's keys.

Resolve the runtime declaration using its owning file, then check the
constructor's table member surface. Enum-typed values still do not have enum
table fields. Missing static members remain errors, including inside conditions.
The existing fallback for runtime declaration members now also reads value syntax
from its owning file.

Reproduction:

-- constants.lua
---@enum transport.DistanceResult
DISTANCE_RESULT = { AVAILABLE = 0, UNREACHABLE = -2 }
-- caller.lua
return DISTANCE_RESULT.UNREACHABLE

Four regression tests cover cross-file globals, dot/string-bracket access,
same-file local enums (including a differently named runtime table), dynamic
keys, conditional accesses, misspelled members, enum-typed globals/parameters,
and shadowing locals. The original cross-file positive case fails on unmodified
upstream. The existing conditional missing-member tests remain enabled.

The runtime declaration is borrowed from database-owned file facts, avoiding a
clone or repeated resolution. Constructor keys are checked first; the existing
declaration-owned member/value fallback remains for compatibility. Expression
keys retain their existing permissive handling before static lookup, and an
enum-typed value is still rejected before either table lookup path.

Validation: all 2,406 workspace tests with all features, workspace/all-target/
all-feature Clippy with warnings denied, and rustfmt pass.

@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: Enum Runtime Table Field Validation

Summary

The change refactors enum field validation to correctly handle cross-file global enum declarations. The core logic is sound, but there are a few concerns worth addressing.

Issues & Recommendations

1. Potential clone() cost / borrow pattern in enum_runtime_decl

matches!(LuaExpr::cast(node), Some(LuaExpr::TableExpr(_))).then(|| decl.clone())

decl is borrowed from facts (which is borrowed from semantic_model). Cloning the whole Decl just to return it may be unnecessary if Decl is large. Consider returning a reference tied to the model's lifetime, or return the SemanticId/decl_key instead and let callers re-resolve. If Decl is cheap to clone this is fine, but it's worth confirming.

2. enum_table_access now has two overlapping member-lookup paths

The new block queries member_info on a TableConst derived from decl.value_expr_syntax, while the existing loop below iterates members_of_owner(&decl.id). These two mechanisms can disagree:

  • The new path uses static_member_key (which may return None for some keys), then falls through to the loop.
  • The loop uses index_key.get_path_part() and string comparison.

This dual-path logic is fragile. If the TableConst path is the authoritative runtime surface, consider whether the loop is still needed, or document why both exist. At minimum, add a comment explaining the precedence.

3. static_member_key returning None silently skips the new check

if let Some(value) = decl.value_expr_syntax
    && let Some(key) = static_member_key(semantic_model, index_key)
    && ...

If static_member_key returns None (e.g., dynamic key), the new validation is skipped and control falls through to the old loop. This is likely intentional (dynamic keys are permissive), but the interaction with the early matches!(index_key, LuaIndexKey::Expr(_)) return above should be verified — there may be redundant handling.

4. Behavior change: enum_table_access is now called for all enum prefixes

Previously, prefix_directly_names_decl returned false early for non-direct references (e.g., enum-typed variables), causing valid_named_member to return false immediately. Now:

return enum_runtime_decl(semantic_model, prefix)
    .is_some_and(|decl| enum_table_access(semantic_model, &decl, &def, index_key));

If enum_runtime_decl returns None (enum-typed variable like p.a), this returns false — same as before. ✅ Good. But note that enum_table_access is no longer reachable for the "not a runtime table" case, which is correct. The test enum_runtime_cross_file_values_are_not_runtime_tables covers this.

5. Test coverage gap: local enum in same file

The new tests focus on cross-file globals. The refactor also changed the local path (file_facts_of(decl_key.file_id) vs. file_facts()). Consider adding a test for a local enum table in the same file to guard against regressions in the local branch, since resolve_name for locals returns a Decl whose file_id should match the current file.

6. Minor: comment accuracy

// Enum values have no fields; their runtime table has the constructor's
// keys. Validate that surface before permissive conditional-key handling.

"Enum values have no fields" is slightly misleading — enum values (the ---@type variables) are the ones being excluded, while the enum type has members. Consider rephrasing to: "An enum-typed value has no runtime table; only the enum's constructor table exposes keys."

7. is_some_and + return readability

return enum_runtime_decl(semantic_model, prefix)
    .is_some_and(|decl| enum_table_access(semantic_model, &decl, &def, index_key));

This is concise and correct. No change needed, but a brief comment noting that None (non-runtime-table prefix) intentionally yields false would help future readers.

Verdict

The logic is correct and the cross-file fix is a genuine improvement. Main actionable items:

  • Verify whether decl.clone() is necessary (item 1).
  • Document the two member-lookup paths in enum_table_access (item 2).
  • Add a same-file local enum test (item 5).

No blocking correctness issues found.

@hgschmie
hgschmie marked this pull request as ready for review October 13, 2026 18:56
@hgschmie
hgschmie force-pushed the fix/cross-file-enum-members branch from b104552 to 64c2435 Compare October 14, 2026 05:42
@hgschmie

Copy link
Copy Markdown
Contributor Author

rebased to current main

@hgschmie

Copy link
Copy Markdown
Contributor Author

rebased to current main

@hgschmie
hgschmie force-pushed the fix/cross-file-enum-members branch from 64c2435 to ccae29b Compare October 14, 2026 14:33
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