Repository navigation
Conversation
There was a problem hiding this comment.
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 returnNonefor 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.
b104552 to
64c2435
Compare
|
rebased to current main |
|
rebased to current main |
64c2435 to
ccae29b
Compare
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:
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.