Repository navigation
fix: Partial<T> fields accepted and keep T's docs (#1318) - #1338
mareklangiewicz wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
Overall the change is coherent and well-tested. Below are issues and suggestions, ordered by severity.
Potential Bugs / Correctness
1. expand_generic_alias uses != on LuaType to detect "no expansion"
let expanded = expand_alias_generic(semantic_model, ty);
(&expanded != ty).then_some(expanded)- If
expand_alias_genericreturns the sameGenerictype unchanged (e.g. it can't resolve), this is fine. But if it returns a structurally equal but distinct type, or ifLuaTypeequality is expensive/recursive, this is fragile. Prefer an explicitOptionreturn fromexpand_alias_generic, or checkmatches!(expanded, LuaType::Generic(_))to detect "did not expand". - Also,
expand_alias_genericis called twice incheck_against_typepath? No — once. But incollect_object_super_fieldsit's called per base type; fine.
2. expand_mapped optionality change may over-apply
let value = if mapped.is_optional && !value.is_nullable() {
LuaType::from_vec(vec![value, LuaType::Nil])
} else {
value
};- This makes every mapped type with
is_optionalproduceT | nilfields. That's correct forPartial<T>, butis_optionalon a mapped type is a general flag. Verify this doesn't change semantics for other mapped types (e.g.Readonly<T>is not optional, so fine; but any user-defined{ [P in keyof T]?: ... }now gets| nil). This is arguably the correct behavior, but it's a behavioral change beyond the issue scope — worth a dedicated test for a non-Partialoptional mapped type. LuaType::from_vecmay not normalizenilinto a union in the canonical way; consider using the existing union-construction helper if one exists (e.g.LuaType::unionor similar) to avoid duplicatenilentries whenvaluealready containsnilin a nested union.
3. mapped_alias_source_member assumes keyof operand is the first operand
let source = model.substitute_generic_params_named(
call.get_operands().first()?,
...
);KeyOfshould have exactly one operand;.first()is fine but silently ignores extra operands. Consider asserting/usingget_operands()length or a dedicated accessor if available.- The function returns
Noneifdef.generic_paramsis empty or names don't match — butPartial<T>has one paramT, andgeneric.get_params()should align. If a user writesPartial<Linter>whereLinteris itself generic, this still works. OK.
4. access.rs early-return for alias generics bypasses existing logic
if self.type_def_of(generic.get_base_type_id_ref())
.is_some_and(|def| def.kind == TypeDefKind::Alias)
{
let name = index_expr.get_index_name_token()?;
let key = LuaMemberKey::Name(name.text().into());
return self.member_info(&prefix_ty, &key).map(|info| info.typ);
}- This returns
None(via?) ifget_index_name_token()isNone(e.g. dynamic indexp[k]), which short-circuits the original generic handling below. For a dynamic index on an alias generic, the old code path (which presumably handled it) is now skipped. Confirm this is intended; if not, fall through instead ofreturn None. member_infoonprefix_ty(theGeneric) will recurse intodirect_member_info, which now handles alias expansion — so this may be redundant with themember.rschange. Consider whether both are needed, or whether this creates double work / potential recursion.
5. collect_members mutates out[start..] after recursion
let start = out.len();
with_expanded_alias(...);
for info in &mut out[start..] { ... }- This assumes
with_expanded_aliasonly appends tooutand never reorders/removes. Ifcollect_membersdeduplicates or sortsoutinternally,startindexing is wrong. Worth a comment or a guard. info.id.is_none()check: if the expanded member already has anid(e.g. from a nested alias), the source-doc enrichment is skipped. That's probably intended, but document it.
6. direct_member_info overwrites info.id/file_id but not typ
info.id = source.id;
info.file_id = source.file_id;- Good: keeps mapped type (
string?), takes source location. ButMemberInfomay have other fields (e.g.decl_id,property_owner_id,description) that should also come fromsourcefor hover docs to work. The hover test passes, sodescriptionmust be derived fromid/file_idlookup — verify that's the case, otherwise docs may be missing in some paths.
Style / Minor
7. Test module placement
}
#[cfg(test)]
mod test_issue_1318 {- Missing blank line between the closing
}of the previous module and the new#[cfg(test)]. Minor formatting nit (rustfmt may not catch this).
8. Duplicated type_def_of + kind == Alias check
The pattern:
let def = semantic_model.type_def_of(&generic.get_base_type_id())?;
if def.kind != TypeDefKind::Alias { return None; }appears in expand_generic_alias, direct_member_info, collect_members, and access.rs. Consider a small helper fn alias_def_of(model, generic) -> Option<&TypeDef> to reduce repetition and keep the invariant in one place.
9. expand_generic_alias doc comment
Nonefor anything else or when it does not expand.
The (&expanded != ty) check is the "does not expand" condition — worth noting in the comment that equality is the signal, since it's non-obvious.
10. mapped_alias_source_member signature uses fully-qualified crate::LuaGenericType
generic: &crate::LuaGenericType,- Inconsistent with the rest of the file which imports types. Add
LuaGenericTypeto theuseblock.
Test Coverage Suggestions
- Add a negative test:
Partial<T>should still reject unknown fields (InjectFieldfor a key not inT). - Add a test for a non-
Partialoptional mapped type to lock in theexpand_mappedchange (issue #2). - Add a test for dynamic index access on a
Partial<T>value (p[k]) to confirm theaccess.rsearly-return doesn't regress. - Add a test where
Titself is generic (Partial<Box<string>>) to exercisesubstitute_generic_params_named.
Summary
The core approach (expand generic aliases in field checking, enrich mapped-alias members with source declaration info) is sound. The main risks are:
- The
expand_mappedoptionality change is broader than the issue and needs a regression test. - The
access.rsearly-return may skip dynamic-index handling for alias generics. collect_members'sout[start..]mutation relies on append-only behavior — worth a comment/guard.
None of these are clearly blocking, but #1 and #2 should be verified before merge.
…s#1318) ```lua ---@type Partial<Linter> local b = { cmd = 'x' } -- was: Field `cmd` cannot be injected ``` Upstream main (1b6df85) accepts these literals without a code change here (the test passes on it; `expand_mapped` keeps the `?` of mapped fields since EmmyLuaLs#1316). This commit only pins both shapes, `Partial<T>` itself and a class inheriting it (`---@Class Sub: Partial<T>`), for inject-field and missing-fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01923KEUKXBrPZHHVvqPVpA1 Claude-Session: https://claude.ai/code/session_018NxUURS1MVzowfNDjH6GGy
…mmyLuaLs#1318) Hover and completion on a `Partial<T>` field showed only the type (`string?`): the mapped alias expands to an anonymous `Object`, so the field had no member declaration. For a mapped alias over `keyof X` (`{ [P in keyof T]?: T[P] }`), member lookup now attaches `X`'s member as the field's declaration while the field keeps its mapped type. Hover shows `(field) cmd: string?` with `T`'s description; completion resolve shows the description too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01923KEUKXBrPZHHVvqPVpA1
… imports `p[k]` on an alias instance has no name token; keep the generic projection path for it instead of returning `None`. Import `LuaGenericType` and `LuaAliasCallKind` like the rest of member.rs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014NAm41TqAcVsDEzkrkkEDG
0d29a36 to
9b307e9
Compare
…al<T>` literals `test_partial_literal_accepts_fields_of_t` (EmmyLuaLs#1318: `---@type Partial<Linter>` and `---@Class Sub: Partial<Linter>` literals) passes on plain upstream main 1b6df85 without this early return, so EmmyLuaLs#1338 (rebased 9b307e9) no longer carries it. Same here, to keep the fork's diff to upstream minimal. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018NxUURS1MVzowfNDjH6GGy
…mmyLuaLs#1340 (tree unchanged) The four PR branches were rebased onto upstream main 1b6df85 (b44c70c, 0ffb918, 9b307e9, 294d821). Their content is already here: the old heads were merged earlier and later fork commits reworked several of them, so a content merge only duplicated moved tests and re-conflicted reworked code. -s ours records the new heads as ancestors. The one substantive rebase change (EmmyLuaLs#1338 dropping expand_generic_alias) is ported separately in 0cb957b. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018NxUURS1MVzowfNDjH6GGy
Summary
Fixes #1318. Two commits:
---@type Partial<Linter>+{ cmd = 'x' }reported "Fieldcmdcannot be injected". After rebasing onto 1b6df85 main already accepts it, forPartial<T>itself and for a---@class Sub: Partial<T>parent (fix(types): preserve object field nullability and generic scope #1316 also keeps the?of mapped fields). I dropped the code change, and the commit now only pins both cases.Partial<T>field showed onlystring?. For a mapped alias overkeyof X, member lookup now attachesX's member as the field's declaration, and the field keeps its mapped type:The regression half is fixed on main now, so this PR is mainly the hover/completion feature.
Evidence
test_hover_partial_field_keeps_source_docs(hover) andtest_partial_field_keeps_source_docs(completion resolve) fail.test_partial_literal_accepts_fields_of_talready passes on main.After: all pass, and so do the
emmylua_code_analysisandemmylua_lstest suites.Merge Danger
Door: two-way
Blast Radius: member declarations for mapped-alias fields (hover/completion on
Partial<T>-like fields)Written with help from Claude Code.
🤖 Generated with Claude Code
https://claude.ai/code/session_01T88qMNChLeJksJGRszr7na