fix(workspace): avoid duplicate classes across main roots

[hgschmie]

This regression came up when working on the suggested change that replaces #1283. I use a "main project" in intellij and then add factorio mods as intellij modules. Each of those runs its own LSP server instance in addition to the "main project". Currently the main server sees all the submodules and gets confused because they all advertise as "MAIN". This PR changes that.

GPT had this to say:

Problem

This corrects a regression introduced by our merged #1266 (ef3cf3c8). That change retains multiple main workspace roots so module resolution can use the caller's source root. However, compute_workspace_ids still collects one ID per physical root, even when several roots share WorkspaceId::MAIN.

The global type aggregate consequently appends the same workspace's declarations repeatedly. With two workspace folders and just one declaration:

---@class Metrics
local Metrics = {}

The analyzer reports duplicate-type and returns two definition locations for that single class. Adding more roots repeats it further. This was observed with a multi-folder IntelliJ workspace, but the problem is in the analyzer's workspace index rather than the editor.

A focused regression passes on #1266's immediate parent (1c8589bb), fails on ef3cf3c8, and passes with this patch.

Fix

• Deduplicate logical workspace IDs while preserving registration order. Physical roots remain available for caller-relative module resolution; each shared index is aggregated only once.
• Preserve genuine same-name declarations when removing a file's indexed contributions. The new negative case exposed a separate existing problem: named semantic IDs are shared, so removing by ID alone also removed the other file's declaration. Removal now matches both the semantic ID and owning file.

The regression covers multiple MAIN roots, a library root, root changes, incremental edits, real duplicate declarations in different files, and removing one of those declarations without losing the other.

[github-actions]

Code Review

1. types.rs — Filter logic change

.filter(|def| def.id != type_id || def.file_id != file_id)

Issue: potential logic inversion / unclear intent.

The comment says "Remove only this file's contribution, preserving real duplicates." To remove a def, the predicate must return false. The predicate returns false only when def.id == type_id && def.file_id == file_id. That does match the stated intent (remove only the matching id and file), so the logic is correct.

However, a subtle concern: if type_id is a named type ID shared across files, then a def with the same type_id but a different file_id is preserved. This is intended per the comment, but it means the function now silently keeps entries that previously would have been removed. Confirm that callers relying on "remove all defs with this id" semantics (e.g., full invalidation paths) are not affected. Consider renaming the parameter or adding a doc comment on the function to make the "per-file removal" contract explicit.

Minor style: the inline comment is placed between the iterator chain and .filter, which is fine, but the wording "real duplicates" is ambiguous — clarify that these are same-name declarations from other files.

---

2. mod.rs — Deduplication of workspace IDs

let mut seen = HashSet::new();
ids.retain(|id| seen.insert(*id));
ids

Correctness: HashSet::insert returns true on first insertion, so retain keeps the first occurrence and drops later duplicates — preserving registration order as the comment claims. Good.

Issues:

• Missing import: HashSet must be in scope. Verify use std::collections::HashSet; (or equivalent) exists in this module. The diff does not show it being added — this will fail to compile if absent.
• Performance: HashSet is allocated even when ids is small (typically ≤ 3–4 entries). For such tiny inputs, a linear scan (ids.iter().position(...)) or Vec::dedup after sorting would avoid the allocation. Not a blocker, but worth considering given this runs during workspace setup.
• Alternative: itertools::unique() would express intent more directly if the crate already depends on itertools.

---

3. Test p10_multiple_main_roots_do_not_duplicate_type_definitions

Good coverage of the regression: duplicate MAIN roots, index rebuild after root change, and the negative case (genuinely distinct declarations still counted).

Concerns:

• Hard-coded count 3: assert_eq!(db.all_workspace_ids().len(), 3) assumes MAIN + library + REMOTE. If add_main_workspace or add_library_workspace semantics change (e.g., REMOTE auto-added differently), this breaks for unrelated reasons. Consider asserting the set of IDs rather than the count.
• Magic file IDs (1, 2): consistent with surrounding tests, so acceptable, but a named constant would improve readability.
• Trailing-newline test: format!("{source}\n") is used to force a re-index. This is a fragile way to trigger invalidation — if the parser normalizes trailing whitespace, the test may not exercise the intended path. A comment explaining why the newline matters would help future maintainers.
• Missing assertion on workspace_lookup_order after the second add_main_workspace: the test checks dedup only once, before adding the third root. Consider re-asserting after db.add_main_workspace(p10_base().join("third")) to confirm dedup holds across repeated additions.

---

Summary

Severity	Item
High	Verify HashSet import exists in mod.rs (compile error otherwise)
Medium	Confirm types.rs filter change doesn't break callers expecting full-id removal
Low	Avoid HashSet allocation for tiny ids; consider itertools::unique
Low	Test: replace hard-coded len() == 3 with set comparison; re-assert dedup after third root

The core logic in both production changes appears correct and the test meaningfully guards the regression. The main actionable risk is the missing HashSet import.

[hgschmie]

I had GPT look at the feedback:

• Documented WorkspaceTypeIndex::remove_file's per-file contract and clarified that same-name declarations from other files must survive. Its caller updates one file's contribution; full rebuilds reconstruct the type indexes, and database clearing replaces the entire index cache. Neither relies on removing every declaration sharing an ID.
• The regression now checks the exact MAIN/library/REMOTE IDs and lookup order, including after adding the third root.
• Replaced the trailing-newline edit with a class rename and rename back, checking that the old name disappears and the new name has exactly one declaration.
• Switched to itertools::Itertools::unique(), directly while collecting workspace IDs. It preserves first-occurrence order and removes the explicit seen/retain step.