Skip to content

fix(render): preserve explicit generic type arguments - #1289

Merged
CppCXY merged 2 commits into
EmmyLuaLs:mainfrom
hgschmie:fix/render-explicit-generic-arguments
Oct 14, 2026
Merged

CppCXY merged 2 commits into
EmmyLuaLs:mainfrom
hgschmie:fix/render-explicit-generic-arguments

Conversation

@hgschmie

@hgschmie hgschmie commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Explicit type arguments disappear from hover text and other rendered type displays when the resolved definition declares fewer generic parameters. With the standard library loaded:

---@class Train
---@field id integer
---@type table<integer, Train>
local trains = {}

The hover shows local trains: table<> instead of local trains: table<integer,Train>. The annotation's arguments are still present in the inferred type; rendering loses them.

The generic default resolver sized its result only from the definition's declared parameters. The standard library's table definition has none, so rendering dropped both explicit arguments. Size the result to retain all provided arguments while continuing to fill missing declared parameters from their defaults or unknown. This also preserves excess arguments in other generic displays rather than hiding them.

Five regression tests cover standard-library tables (including single arguments, unknown arguments and nested tables), definitions with fewer parameters, unresolved types, explicit overrides, dependent defaults, alias/index-call expansion and unchanged missing-argument diagnostics. Both simple and detailed rendering are checked where applicable. The first two tests fail before the fix.

This helper is private to the renderer: preserving excess arguments does not change type inference, assignability or accepted generic arity. Showing the stored arguments makes an arity mistake visible instead of erasing it. The supplied list comes from LuaGenericType::get_params() and may include inferred or unknown values; the renderer preserves it without assuming that every entry is raw user syntax. Existing missing-argument diagnostics remain responsible for arity validation; adding an excess-argument diagnostic is outside this display fix.

Validation: all 2,405 workspace tests with all features, workspace/all-target Clippy with warnings denied, and rustfmt pass. The review follow-up changes comments/tests only; all five expanded renderer tests 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

Summary

The change modifies resolve_generic_defaults to preserve explicit generic arguments that exceed the number of declared generic parameters (e.g., table<integer, Train> where table declares no generics). Tests are added to cover the new behavior.

Issues & Observations

1. Potential behavioral regression for non-built-in types (Medium)

The fix uses max(provided.len()) unconditionally, which affects all type definitions, not just built-ins. Previously, extra arguments beyond def.generic_params.len() were silently dropped. Now they are preserved and rendered.

The test rendering_preserves_arguments_beyond_declared_parameters explicitly asserts this new behavior for user-defined classes (Plain<string>, Box<string,integer>), so this appears intentional. However, this is a semantic change worth confirming:

  • Is it desirable to render Plain<string> when Plain declares no generics? This may mask user errors (typos, wrong generic arity) that were previously normalized away.
  • Consider whether a diagnostic/warning should be emitted for arity mismatches rather than silently preserving them.

2. provided slice length vs. actual generic count (Low)

provided.len() counts all supplied arguments, but if provided can contain placeholders or unknown fillers from earlier resolution steps, n may be inflated. Verify that provided here always represents the raw user-supplied argument list and not an already-padded vector.

3. Test coverage gaps (Low)

  • No test verifies the interaction with expand_alias_generic / expand_index_call when extra args are present.
  • No negative test confirming that genuinely missing parameters still get unknown (partially covered by Required<string> → Required<string,unknown>, but not for the max path).
  • Consider adding a test for a built-in with a partial argument list, e.g. table<integer> alone, to confirm single-arg built-ins render correctly.

4. Test style (Nit)

  • rendering_preserves_arguments_beyond_declared_parameters uses ws.humanize_type only, while the first test also checks humanize_type_detailed. Consider checking both consistently, since the two renderers may diverge.
  • The for annotation in [...] loops are fine, but on failure the assertion message won't indicate which annotation failed. assert_that! with googletest typically includes the value, but adding a label (e.g., via with_message) would improve debuggability.

5. Comment accuracy (Nit)

The comment says "Built-ins such as table have no declared generic parameters." This is accurate for table, but the code path also affects user classes with fewer declared params than provided (as the second test demonstrates). Consider broadening the comment to reflect the general rule, e.g.:

// Preserve explicit arguments even when the definition declares fewer
// parameters (e.g. built-in `table`, or user classes with arity mismatch).

Recommendation

The change is small and well-tested for the stated goal. Before merging, confirm that preserving excess generic arguments for user-defined types is the intended semantics (vs. only for built-ins), and consider whether an arity-mismatch diagnostic should accompany this behavior.

@hgschmie
hgschmie marked this pull request as ready for review October 13, 2026 18:07
@hgschmie
hgschmie force-pushed the fix/render-explicit-generic-arguments branch from eb8004e to d58af91 Compare October 14, 2026 05:42
@hgschmie

Copy link
Copy Markdown
Contributor Author

rebased to current main

@CppCXY
CppCXY merged commit 14f75f7 into EmmyLuaLs:main Oct 14, 2026
16 checks passed
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.

2 participants