Repository navigation
fix(render): preserve explicit generic type arguments - #1289
Conversation
There was a problem hiding this comment.
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>whenPlaindeclares 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_callwhen extra args are present. - No negative test confirming that genuinely missing parameters still get
unknown(partially covered byRequired<string>→Required<string,unknown>, but not for themaxpath). - 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_parametersusesws.humanize_typeonly, while the first test also checkshumanize_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., viawith_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.
eb8004e to
d58af91
Compare
|
rebased to current main |
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:
The hover shows
local trains: table<>instead oflocal 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
tabledefinition 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 orunknown. 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.