Repository navigation
Conversation
There was a problem hiding this comment.
Code Review: Cycle Detection in super_generic_type
Summary
The change adds path-based cycle detection to super_generic_type to prevent infinite recursion on cyclic inheritance (including cycles where generic arguments grow on each visit). Overall the approach is sound, but there are a few issues worth addressing.
Issues
1. path.remove(arg_id) is not panic-safe (correctness/robustness)
let result = (|| { ... })();
path.remove(arg_id);
resultIf the closure ever panics (or if future edits introduce an early ?/return outside the closure), arg_id will be left in path, corrupting the caller's state. Since path is shared across sibling branches, a leaked entry could cause false cycle detection in unrelated branches.
Recommendation: Use a guard type (RAII) that removes the id on Drop, or at minimum ensure the removal is unconditional. A small struct PathGuard<'a>(&'a mut HashSet<LuaTypeDeclId>, LuaTypeDeclId) with Drop is the cleanest fix.
2. arg_id.clone() on every recursion
path.insert(arg_id.clone()) clones the id for every node visited. LuaTypeDeclId is likely cheap-ish, but this is on a hot inference path. Consider whether HashSet<&LuaTypeDeclId> with a lifetime tied to the call stack is feasible, or accept the clone if profiling shows it's negligible. Not blocking, but worth noting.
3. Cycle detection is per-path, not per-node — verify intent
The comment says "Keep sibling paths independent," and the implementation does exactly that (insert on entry, remove on exit). This is correct for detecting cycles, but note that it does not memoize results. If the same arg_id is reachable via multiple non-cyclic paths, it will be re-explored each time. For deeply nested diamond hierarchies this can be exponential. If that's a concern, consider a separate visited/memo set for completed nodes (careful: memoization must account for arg_params, since substitute_generic_params can produce different results for the same id).
4. Test coverage gaps
The tests cover:
- Simple cyclic inheritance
- Cyclic inheritance with growing generics
- A valid base reachable after a cycle
Missing:
- Diamond inheritance (non-cyclic) — ensure the per-path removal doesn't accidentally suppress a valid result reached via a second path.
- Self-inheritance (
---@class A: A) — should terminate. - Deeply nested cycle where the target is found before the cycle is entered (ordering of
pendingmatters).
Recommendation: Add at least a diamond test and a self-inheritance test.
5. Minor: Intersection handling and cycle interaction
When an Intersection is encountered, its members are pushed onto pending but the current arg_id remains in path for the whole loop. That's fine, but if an intersection member is the same arg_id, the recursive call will correctly detect the cycle. No bug, just confirming the semantics are intentional.
6. Style: closure-based scoping is a bit opaque
let result = (|| { ... })();
path.remove(arg_id);
resultThis works but is easy to misread. The RAII guard from issue #1 would make the intent clearer and remove the need for the immediately-invoked closure.
Non-issues (verified)
path.insertreturningfalse→Nonecorrectly treats revisiting the same class identity as a cycle, even with differing generic params. This matches the stated intent ("Repeated class identities form an inheritance cycle even when generic arguments grow").- Returning
Noneon cycle is the right conservative behavior for a lookup that can't find the target.
Verdict
Functionally correct for the stated goal. I'd recommend addressing #1 (panic-safety / RAII guard) before merge, and #4 (test coverage) as a follow-up. Items #2, #3, and #6 are optional improvements.
|
Addressed the coverage request and a related regression in ff7f3c8. A class-identity-only guard cut valid finite forwarding such as The path is a fresh local value owned by the wrapper; a panic unwinds and drops it, and no catcher resumes sibling traversal with that value. Normal early returns remain inside the result closure and are followed by Validation: seven focused regressions and static checks on the final head; 2,621 full workspace tests on the production-identical parent, including the linear stress case with none filtered. Independent public inference and instantiated-cycle probes also pass. |
ff7f3c8 to
dc66fe9
Compare
c880c88 to
6f4a284
Compare
6f4a284 to
55cbad1
Compare
Generic superclass lookup can recurse indefinitely through cyclic inheritance, including cycles whose generic arguments grow at each step. Use an iterative traversal in the existing depth-first order and memoize exact instantiated states per lookup. This preserves finite generic forwarding and lateral superclass projections while avoiding repeated diamond subgraphs and cycle states that previously exhausted the query budget before reaching a valid result.
Superclass projection participates in the existing shared query-work and semantic-nesting limits, using one nested evaluation scope. Inheritance traversal itself is iterative. Intersection expansion is charged before it can grow the pending work; unsupported roots are rejected before scope creation. Exceeding an existing limit can produce incomplete/provisional inference and an evaluation error supporting the TypeComplexity diagnostic. The limits remain one million type visits and 64 nested semantic evaluations; expensive finite queries can still be incomplete.
Regressions cover direct and growing cycles, distinct generic instances, finite 128-layer forwarding, lateral projections, repeated diamond subgraphs, repeat-before-charge behavior, intersection accounting, unsupported roots, and independent-query recovery. The lookup retains root exclusion and first-match order.
Validation at the approved source: 19 focused regressions and all 2,634 workspace tests passed, together with workspace all-target/all-feature Clippy with warnings denied, formatting, diff and signature checks. The published single commit has the exact approved source tree; these source-validation results are reused for the history-only squash.