[mareklangiewicz]

Summary

Fixes #1321. opt.x with ---@type P? (also P|nil, P|Q, ---@param opts? P) hovered as the bare type, with no (field) line and no description. resolve_member leaves union prefixes to flow narrowing, so the access has no member declaration.

 build_hover
   semantic_id = model.find_decl(token)
+  if none: look the key up through the prefix's union type (union_member_at_token)

resolve_member stays the same: resolving union prefixes there breaks 8 flow/cast narrowing tests.

Correction to the issue: I wrote "no hover at all" on main. That is wrong: main shows the bare type (boolean). The missing parts are the (field) line and the description.

Evidence

• Before: test_hover_field_through_union and test_hover_field_through_optional_param (hover_test.rs) fail: no description for opt.x, pnil.x, pq.x, pq.y.
  After: both pass; the whole workspace test suite passes.

Merge Danger

Door: two-way

Blast Radius: hover on field accesses whose prefix is a union; only used when the access has no declaration of its own

Written with help from Claude Code.

🤖 Generated with Claude Code

https://claude.ai/code/session_01T88qMNChLeJksJGRszr7na

[github-actions]

Code Review

Summary

The change adds a fallback in build_semantic_info_hover so that field access through a union prefix (P?, P|nil, P|Q) resolves to a member declaration for hover purposes. Overall the approach is reasonable and well-tested, but there are a few issues worth addressing.

Issues

1. Doc comment is misplaced / merged (bug in comments)

In the diff, the new union_member_at_token function is inserted between the existing doc comment for member_prefix_is_generic_instance and that function's signature:

/// Whether the member access (`x.foo` / `x:foo`) containing the current token is applied to an
/// instantiated generic prefix (`local c ---@type MyTable<int>` -> `c.aaa`), in which case the member
/// type must be projected through the prefix's generic arguments.
/// Member declaration for `x.key` whose prefix is a union (`P?`, `P|Q`).
///
/// `resolve_member` leaves union prefixes to flow narrowing, ...
fn union_member_at_token(...) -> Option<SemanticId> { ... }

fn member_prefix_is_generic_instance(...) -> bool { ... }

The generic-instance doc comment now attaches to union_member_at_token, and member_prefix_is_generic_instance is left undocumented. Move the new function (and its doc) above the existing comment, or place it after member_prefix_is_generic_instance.

2. union_member_at_token only handles LuaIndexExpr parents

let index_expr = LuaIndexExpr::cast(token.parent()?)?;

The existing member_prefix_is_generic_instance uses token.parent_ancestors().find_map(LuaIndexExpr::cast), which tolerates the token being nested (e.g. wrapped in a NameExpr/NameRef node). Using token.parent() directly may miss cases where the name token is not the immediate child of LuaIndexExpr. Consider matching the existing pattern for consistency and robustness:

let index_expr = token.parent_ancestors().find_map(LuaIndexExpr::cast)?;

(Then the get_index_name_token() == Some(token) check still guards correctness.)

3. LuaMemberKey::Name(token.text().into()) — verify key semantics

token.text() returns the raw source text. For a normal identifier this is fine, but if the field is accessed via bracket syntax (x["foo"]) the token text includes quotes, and this path would build the wrong key. Since the guard get_index_name_token() only matches name tokens this is likely safe, but worth a comment or an explicit check that the token is a Name kind.

4. Silent fallback may mask ambiguity

When the union has multiple members defining the same key (e.g. P|Q both with x), model.member_info(&prefix_ty, &key) presumably returns the first match. The hover will show one arbitrary declaration. This is acceptable for hover, but consider:

• Documenting the "first match wins" behavior in the doc comment.
• Optionally returning None (or a combined signature) when members disagree, to avoid showing a misleading type.

5. info.decl mutation only when None

if info.decl.is_none() {
    info.decl = union_member_at_token(model, &token);
}

This is fine, but note that info may carry other fields (e.g. typ) derived from the unresolved union access. If info.typ is still the union/nil type while info.decl now points to a concrete member, the hover output could be inconsistent (decl says boolean, type says boolean?). Worth verifying the downstream rendering uses decl consistently, or also refreshing info.typ from the resolved member.

6. Test coverage gaps

The tests are good, but consider adding:

• A case where the union member is missing on one branch (P|Q accessing x when only P has x) — should still hover P.x or fall back gracefully.
• A case with conflicting member types across union arms to lock in the chosen behavior.
• A negative case: P|Q accessing a key present on neither — should not panic and should return no hover.

Minor / Style

• The new function is placed mid-file between two unrelated items; grouping it near member_prefix_is_generic_instance (both are "member access helpers") would improve readability.
• Consider naming union_member_at_token → union_member_decl_at_token to clarify it returns a declaration, not a type.

Verdict

Functionally sound and well-tested for the happy path. Must fix: the misplaced doc comment (#1). Should fix: parent-ancestors lookup (#2) and type/decl consistency (#5).