[hgschmie]

Member completion stops enumerating when an earlier candidate rejects the typed prefix, hiding later matching fields or methods. Treat rejection as local to that candidate in all three enumeration loops.

For example, completion at train:c should still offer both cancel and connect when activate is encountered first. Regressions cover field and colon completion, empty/unmatched prefixes and repeated requests.

Validation: 229 language-server tests, Clippy (all targets/features, warnings denied) and formatting pass.

[github-actions]

Code Review

Summary

The change fixes a real bug: add_member_completion returns None when a candidate doesn't match the prefix, and the previous ? operator caused the entire loop to abort on the first non-matching member. Replacing ? with let _ = correctly continues iterating. The added comment and tests are helpful.

Issues & Suggestions

1. Silent error swallowing (let _ =)

Location: member_provider.rs lines 35, 162, 169

Using let _ = discards all None results, not just the "prefix mismatch" case. If add_member_completion ever returns None for a genuine error (not just a skip), it will be silently ignored. Consider one of:

• Change the return type to bool or a dedicated enum (Added / Skipped / Error) to make intent explicit.
• At minimum, document the contract clearly (the added comment helps, but it's on the function, not at each call site).

// Prefer explicit intent:
if add_member_completion(builder, member, completion_status).is_none() {
    // expected: candidate filtered out by prefix
}

2. member.clone() in the loop

Location: member_provider.rs line 35

let _ = add_member_completion(builder, member.clone(), CompletionTriggerStatus::Dot);

The clone is unnecessary if add_member_completion only needs a reference or if MemberInfo is cheap to borrow. Check whether the signature can take &MemberInfo; if not, this is a pre-existing inefficiency but worth noting since the line is being touched.

3. Test: redundant loop over identical triggers

Location: completion_test.rs, test_member_completion_skips_nonmatching_candidates

for trigger in [
    CompletionTriggerKind::INVOKED,
    CompletionTriggerKind::INVOKED,
] {

Both iterations use the same trigger kind, so this only tests idempotency of repeated invocations. The comment says "Re-invoking completion must retain all candidates matching the prefix," which is a valid regression check — but consider making the intent clearer, e.g.:

// Invoke twice to ensure repeated completion requests don't drop candidates.
for _ in 0..2 {
    let items = get_completion_items(&mut ws, &code, CompletionTriggerKind::INVOKED)?;
    ...
}

4. Test: expected shadowing

Location: same test

let mut expected = expected.clone();
expected.sort_unstable();

Shadowing the loop variable expected with a mutable clone works but is slightly confusing. Rename to expected_sorted for clarity.

5. Test: empty-prefix expectation may be brittle

Location: test_member_completion_skips_nonmatching_candidates, ("", vec![...])

The empty-prefix case asserts an exact set of four fields. If the completion provider later adds built-in members (e.g., inherited __index, metatable entries, or Lua standard members), this test will break for unrelated reasons. Consider asserting expected ⊆ labels for the empty case, or documenting that only class-declared fields are expected.

6. Test: test_member_completion_skips_nonmatching_methods — colon trigger

Location: completion_test.rs

The test uses train:c<??> (colon trigger) and expects only functions. Good coverage. However, it doesn't verify that non-function members like current_network_id are correctly excluded — that's implicit in the exact-match assertion, which is fine, but a comment noting "colon trigger must exclude non-function fields" would aid future readers.

7. Comment placement

Location: member_provider.rs line 478

// None skips this candidate (for example, a nonmatching prefix), not the remaining members.
fn add_member_completion(...)

Good clarification. Consider also updating the doc comment on add_member_completions (the public-ish entry point) to state that it never fails due to individual candidate filtering, since callers may otherwise assume None means "abort."

Verdict

The core fix is correct and well-tested. Main actionable items: make the None-swallowing intent explicit (item 1), clean up the redundant trigger loop (item 3), and consider the empty-prefix test brittleness (item 5).

[hgschmie]

Addressed in c312fe0: clarified the entrypoint/helper contracts and all three call sites; None means this candidate was filtered, rather than a provider error. The repeated-request test now explicitly runs twice and uses expected_sorted.

The existing clone bridges a borrowed member slice to the owned helper API; changing ownership is outside this fix. I retained exact empty-prefix equality because this fixture declares exactly four fields without inheritance/metatables. The colon test already verifies that the matching non-function field current_network_id is excluded. Weakening either assertion would reduce regression coverage.

Validation: both focused regressions and all 229 language-server tests pass, along with Clippy and formatting. Executable production code and all test fixtures/expected results are unchanged.

[hgschmie]

Follow-up for item 2 in 32207f4: the private helper now borrows MemberInfo, removing the clone in the slice caller. The two owned-loop callers borrow their members, and two redundant inner references are removed: six line substitutions in one private provider file. The public entrypoint, Option<()> filtering contract and all tests are unchanged.

Both focused completion regressions and all 229 language-server tests pass, along with Clippy and formatting.