[hgschmie]

UP-119: Finite inheritance traversal.

Source snapshot: dc66fe9e6fa2de2a0ca10f63afe87c4c32cf007d.

Terminate generic inheritance cycles while preserving finite superclass traversal.

This internal conversation remains open for user discussion. Changes require a separate visible commit and user approval before squashing or closing.

[github-actions]

Review of the code changes

The change adds cycle detection to super_generic_type by threading a path of visited (LuaTypeDeclId, LuaType) pairs through the recursive lookup. The intent is good, and the added tests cover several important cases. However, there are a few correctness and compatibility concerns.

---

1. Cycle detection can prematurely stop valid inheritance paths

The condition:

if let Some((_, previous)) = path.iter().rev().find(|(id, _)| id == arg_id)
    && !previous.any_nested_type(|ty| ty == arg_ty)
{
    return None;
}

treats any repeated class id with a non-nested type argument as a cycle. This is too aggressive.

Example:

---@class A<T>: B<T>
---@class B<T>: A<Unrelated>, T
---@class Unrelated: Parent
---@class Parent

Starting from A<integer>:

• A<integer> → B<integer>
• B<integer> → A<Unrelated> and integer
• A<Unrelated> → B<Unrelated>
• B<Unrelated> → A<Unrelated> and Unrelated
• Unrelated → Parent

Parent is a valid superclass of A<integer>, but the current check stops at A<Unrelated> because the previous A entry is A<integer> and does not contain A<Unrelated> as a nested type. The lookup then misses Parent.

Recommendation:
Track exact visited instantiations, e.g. a HashSet<(LuaTypeDeclId, LuaType)>, and only stop when the exact same instantiation repeats. To prevent infinite growth such as A<T>: A<Wrap<T>>, add a separate growth check or a depth limit. At minimum, add a regression test for the scenario above.

---

2. Equal generic cycles are not explicitly tested

The comment says “Equal or growing instances form a cycle”, but the tests do not cover an equal generic cycle such as:

---@class Child<T>: Parent<T>
---@class Parent<T>: Child<T>

with Child<integer> as the starting type.

This relies on LuaType::any_nested_type not including the receiver itself. If any_nested_type ever includes self, equal generic cycles will recurse infinitely.

Recommendation:
Add a test for an equal generic cycle, and/or explicitly document that any_nested_type excludes the receiver. This is a critical invariant for the current logic.

---

3. Let-chain may break MSRV compatibility

The new code uses a let-chain:

if let Some((_, previous)) = path.iter().rev().find(|(id, _)| id == arg_id)
    && !previous.any_nested_type(|ty| ty == arg_ty)
{
    return None;
}

Let-chains were stabilized in Rust 1.88. If the project’s MSRV is lower, this will fail to compile.

Recommendation:
If the project does not already require Rust 1.88+, rewrite as nested if let:

if let Some((_, previous)) = path.iter().rev().find(|(id, _)| id == arg_id) {
    if !previous.any_nested_type(|ty| ty == arg_ty) {
        return None;
    }
}

---

4. Minor style / test improvements

• assert_eq!(generic.get_params(), &vec![LuaType::Integer]); allocates a temporary Vec. Prefer a slice comparison if the API supports it:

  assert_eq!(generic.get_params(), &[LuaType::Integer]);

• The immediately-invoked closure used to ensure path.pop() is correct but slightly unusual. A small guard struct or explicit let result = ...; path.pop(); result is fine, but consider a comment explaining why the closure is used.
• The comment “A repeated class must consume a strict nested type” is a bit unclear. Consider rephrasing to “Only allow repeated class ids when the new instantiation is strictly nested inside the previous one; otherwise treat it as a cycle.”

---

Summary

The main issue is that the cycle detection is overly conservative and can miss valid superclasses when the same generic class appears with different, non-nested arguments. The implementation should distinguish between true cycles/growth and finite lateral instantiations. The added tests are valuable, but an equal-generic-cycle test and a lateral-instantiation regression test would strengthen confidence. Also verify the project’s MSRV supports let-chains.