feat(infer): resolve nested table literals as Instance types#987
feat(infer): resolve nested table literals as Instance types#987NeOzay wants to merge 10 commits intoEmmyLuaLs:mainfrom
Conversation
When a typed class field is provided with a table literal that is itself a sub-table (TableConst), wrap the result in an Instance type instead of falling back to the bare class Ref. This preserves the literal context for recursive member access, enabling correct nil-stripping on optional fields at any depth. - infer_index: wrap TableConst fallback in Instance(base, range) - humanize_type: render Instance with per-field nil-strip display - semantic_info: create Instance for LocalName hover - get_type_at_flow: narrow class types via table literal at flow - tests: add recursive depth-3 member resolution test - tests: add recursive hover test (test.a.b.c → integer = 1) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the type inference system by introducing Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to type inference for nested table literals, resolving them as Instance types. This change correctly preserves literal context for recursive member access and enables accurate nil-stripping of optional fields at any nesting depth. The humanize_type function has been updated to render these Instance types with per-field nil-strip display, which is a great enhancement for hover functionality. New test cases have been added to cover these scenarios, demonstrating the effectiveness of the changes. The overall implementation is robust and addresses a key aspect of type narrowing.
There was a problem hiding this comment.
Pull request overview
This PR enhances EmmyLua’s type inference so nested table literals used as class instances are preserved as Instance(base, range), enabling recursive member access to keep literal context (including nil-stripping of optional fields) and improving hover rendering to show which optional fields are present vs absent.
Changes:
- Wrap nested
TableConstmember results inLuaType::Instanceto preserve literal context for deep member inference. - Add
Instance-aware rendering inhumanize_typeto display per-field optional narrowing based on literal-provided keys. - Extend semantic/flow narrowing and add tests for deep recursive resolution and hover output.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/emmylua_ls/src/handlers/test/hover_test.rs | Adds hover tests validating optional-field narrowing and deep nested hover resolution. |
| crates/emmylua_code_analysis/src/semantic/semantic_info/mod.rs | Narrows hovered LocalName class types to Instance when initialized with a non-empty table literal. |
| crates/emmylua_code_analysis/src/semantic/infer/narrow/get_type_at_flow.rs | Narrows class types at flow decl position based on non-empty table literal initializers. |
| crates/emmylua_code_analysis/src/semantic/infer/infer_index/mod.rs | Adjusts Instance member inference to strip nil for literal-provided fields and wrap nested table literals as Instance. |
| crates/emmylua_code_analysis/src/db_index/type/humanize_type.rs | Renders Instance as a narrowed struct view with optional-field presence/absence display. |
| crates/emmylua_code_analysis/src/compilation/test/member_infer_test.rs | Updates existing assertions and adds tests for optional narrowing + recursive instance member resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/emmylua_code_analysis/src/semantic/infer/infer_index/mod.rs
Outdated
Show resolved
Hide resolved
crates/emmylua_code_analysis/src/compilation/test/member_infer_test.rs
Outdated
Show resolved
Hide resolved
Deduplicate is_class_type into LuaType method, conditionally strip nil
only when literal value is non-nullable (fixes `{ a = nil }` case),
restrict Instance narrowing to LocalName only, strengthen test assertion,
and fix hover display for nil-valued literal fields.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
please fix the check |
Replace `.unwrap()` with `?` in `infer_token_semantic_info` to satisfy clippy's restriction on unwrap in Option-returning functions. Apply `cargo fmt` to fix code style check failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves type inference for Lua class instances initialized with nested table literals by preserving literal context via Instance types, enabling recursive nil-stripping for optional fields and enhancing hover rendering to show which optional fields are present vs absent.
Changes:
- Wrap nested
TableConstmember-resolution fallbacks inLuaType::Instanceto preserve literal context for recursive member access. - Narrow class types at declaration/hover time based on non-empty table literal initializers (including flow analysis updates).
- Extend
humanize_typeto renderInstancetypes with per-field optionality based on literal-provided keys, and add tests for deep recursive resolution + hover output.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| crates/emmylua_ls/src/handlers/test/hover_test.rs | Adds hover tests covering optional-field narrowing (partial/all/empty) and deep nested access hover (test.a.b.c). |
| crates/emmylua_code_analysis/src/semantic/semantic_info/mod.rs | Narrows LocalName hover types by wrapping class types in Instance when initialized with a non-empty table literal. |
| crates/emmylua_code_analysis/src/semantic/infer/narrow/get_type_at_flow.rs | Narrows class-typed locals at decl-position flow nodes using the table literal initializer (when non-empty). |
| crates/emmylua_code_analysis/src/semantic/infer/infer_index/mod.rs | Adjusts Instance member inference to strip nil when a concrete literal value is present and to wrap nested table literals as Instance on fallback. |
| crates/emmylua_code_analysis/src/db_index/type/types.rs | Adds LuaType::is_class_type(db) helper for class-type detection. |
| crates/emmylua_code_analysis/src/db_index/type/humanize_type.rs | Adds Instance-aware rendering that marks absent optional fields as name?: type and provided fields as non-optional in the expanded view. |
| crates/emmylua_code_analysis/src/compilation/test/member_infer_test.rs | Adds tests for optional field narrowing, nil-literal behavior, and deep recursive instance member resolution; relaxes some integer const vs integer assertions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // or via the class declaration (Integer); both are valid integer types | ||
| assert!( | ||
| matches!(a_ty, LuaType::Integer | LuaType::IntegerConst(_)), | ||
| "expected integer type for a, got {:?}", |
There was a problem hiding this comment.
why match LuaType::Integer | LuaType::IntegerConst?
|
|
||
| // Only narrow LocalName declarations — ForStat/ForRangeStat cannot have | ||
| // table literal initializers. | ||
| if matches!(parent.kind().into(), LuaSyntaxKind::LocalName) && typ.is_class_type(db) { |
There was a problem hiding this comment.
The if nesting is too deep; I think we should invert the if.
- Remove Intersect in infer_instance_member to prevent narrowing declared types (e.g. integer) down to literal constants (e.g. IntegerConst(1)); the literal value is now used only to decide whether to strip nil - Restore exact assert_eq! assertions in test_issue_397 (no longer need to accept both Integer and IntegerConst) - Extract try_narrow_local_to_instance helper in semantic_info/mod.rs to reduce 6 levels of if-let nesting to 1 - Flatten DeclPosition narrowing in get_type_at_flow.rs with a labeled block to reduce 5 levels of nesting to 2 - Rename stripped/narrowed to without_nil and fix misleading comment in write_instance_type (both branches already stripped nil from the type) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The unlabeled `break` inside `'narrow:` was ambiguous — Rust requires an explicit label when breaking out of an enclosing loop from within a labeled block. Label the outer loop as `'flow:` and use `break 'flow`. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve conflicts: - get_type_at_flow.rs: adopt upstream's refactored structure (get_type_at_flow_internal, labeled conditions, pending narrows); re-integrate Instance narrowing via try_narrow_decl_to_instance helper - member_infer_test.rs: keep both our Instance tests and upstream's new never-preservation tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Restore the Intersect in infer_instance_member (needed for assignment
narrowing: `a = { a = "hello" }` → `a.a` should be `StringConst`).
Add `literal_provides_optional_class_field` guard in both
`try_narrow_local_to_instance` and `try_narrow_decl_to_instance`:
only create Instance when the literal provides at least one field that
is declared optional in the class. Without this guard, initial
declarations like `local b: B = { field = 1 }` (B.field: integer,
non-optional) would have their fields narrowed to `IntegerConst(1)`
instead of `Integer`.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move the function to narrow/mod.rs (pub(in crate::semantic)), re-export it via infer/mod.rs, and remove the identical copy from both get_type_at_flow.rs and semantic_info/mod.rs. Also fix two issues in write_instance_type (humanize_type.rs): - compute without_nil once instead of three times in the same loop - guard function_vec loop with count < max_display_count instead of count < all_count Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/emmylua_code_analysis/src/db_index/type/humanize_type.rs
Outdated
Show resolved
Hide resolved
crates/emmylua_code_analysis/src/db_index/type/humanize_type.rs
Outdated
Show resolved
Hide resolved
| literal_members.iter().any(|lit_member| { | ||
| let lit_key = lit_member.get_key(); | ||
| class_members.iter().any(|cls_member| { | ||
| cls_member.get_key() == lit_key | ||
| && db | ||
| .get_type_index() | ||
| .get_type_cache(&cls_member.get_id().into()) | ||
| .is_some_and(|tc| tc.as_type().is_nullable()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
literal_provides_optional_class_field currently does an O(n*m) nested scan over literal members and class members. Consider precomputing a HashSet/map of optional class member keys (or building a lookup table keyed by LuaMemberKey) to reduce this to near O(n+m), since this helper can be called repeatedly during hover/flow inference.
| literal_members.iter().any(|lit_member| { | |
| let lit_key = lit_member.get_key(); | |
| class_members.iter().any(|cls_member| { | |
| cls_member.get_key() == lit_key | |
| && db | |
| .get_type_index() | |
| .get_type_cache(&cls_member.get_id().into()) | |
| .is_some_and(|tc| tc.as_type().is_nullable()) | |
| }) | |
| }) | |
| let optional_class_member_keys: std::collections::HashSet<_> = class_members | |
| .iter() | |
| .filter(|cls_member| { | |
| db.get_type_index() | |
| .get_type_cache(&cls_member.get_id().into()) | |
| .is_some_and(|tc| tc.as_type().is_nullable()) | |
| }) | |
| .map(|cls_member| cls_member.get_key()) | |
| .collect(); | |
| if optional_class_member_keys.is_empty() { | |
| return false; | |
| } | |
| literal_members | |
| .iter() | |
| .any(|lit_member| optional_class_member_keys.contains(&lit_member.get_key())) |
Replace get_sorted_members with get_members when building the literal_keys HashMap in write_instance_type, since ordering is irrelevant once collected into a map. Also clarify the doc comment to reflect that nil is only stripped for non-nullable literal values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
crates/emmylua_code_analysis/src/semantic/infer/narrow/get_type_at_flow.rs
Outdated
Show resolved
Hide resolved
|
|
||
| let c_ty = ws.expr_ty("c"); | ||
| assert!( | ||
| matches!(c_ty, LuaType::Integer | LuaType::IntegerConst(_)), |
There was a problem hiding this comment.
Hi, the two variants are there because the current inference intersects the declared type with the literal value (e.g. { a = 1 } → IntegerConst(1) instead of Integer).
Should accessing a provided field return the declared type or the narrowed literal constant?
There was a problem hiding this comment.
The returned type must be one of these; the | here indicates you're not sure which one it is, and that shouldn't happen in tests.
| let Some(literal_members) = db.get_member_index().get_members(literal_owner) else { | ||
| return false; | ||
| }; | ||
| literal_members.iter().any(|lit_member| { |
There was a problem hiding this comment.
hi, I don't know, so I asked Claude Code.
here is his response:
In practice, both collections are small (class fields), so the O(n×m) double iteration is unlikely to matter. If you'd prefer a HashSet-based
O(n+m) approach for correctness at scale, I can apply it — otherwise I'll leave it as-is.
There was a problem hiding this comment.
Lua is often used as a configuration table, so this may be a very large table; an O(n²) algorithm here could cause a drastic performance drop.
…ield Follow-up to 0bf0265: the function was extracted to the parent module but the call site still used `super::` instead of the now-explicit import.
|
|
||
| let c_ty = ws.expr_ty("c"); | ||
| assert!( | ||
| matches!(c_ty, LuaType::Integer | LuaType::IntegerConst(_)), |
There was a problem hiding this comment.
The returned type must be one of these; the | here indicates you're not sure which one it is, and that shouldn't happen in tests.
| matches!(self, LuaType::Unknown) | ||
| } | ||
|
|
||
| pub fn is_class_type(&self, db: &DbIndex) -> bool { |
There was a problem hiding this comment.
This function probably shouldn't be here; it belongs in the specific context where you use it and should be implemented as a private function.
| let Some(literal_members) = db.get_member_index().get_members(literal_owner) else { | ||
| return false; | ||
| }; | ||
| literal_members.iter().any(|lit_member| { |
There was a problem hiding this comment.
Lua is often used as a configuration table, so this may be a very large table; an O(n²) algorithm here could cause a drastic performance drop.
Summary
TableConst), wrap the result inInstance(base, range)instead of returning the bareRef. This preserves the literal context for recursive member access.test.a.b.cthrough 3 levels of@classwith@field?).Instance-aware rendering inhumanize_typeso hover displays which optional fields are provided vs absent.Example
Changes
infer_index/mod.rsTableConstfallback inInstance(stripped, nested_range)humanize_type.rsInstancetypes with per-field nil-strip displaysemantic_info/mod.rsInstanceforLocalNamehover when initializer is a non-empty table literalget_type_at_flow.rsmember_infer_test.rshover_test.rstest.a.b.c → integer = 1)Test plan
cargo test -p emmylua_code_analysis— 617 passedcargo test -p emmylua_ls— 178 passedtest_recursive_instance_member_resolution(depth-3test.a.b.c)test_recursive_nested_hover(hover showsinteger = 1)🤖 Generated with Claude Code