Architecture improvement plan: reduce duplication, remove legacy traits, leverage modern Rust

[bug-ops]

Summary

Comprehensive architectural review of the deps-lsp codebase (9 crates, 8 ecosystem implementations). This issue documents identified problems grouped by theme, with prioritized improvement items.

P0: Remove dead legacy trait system

The codebase carries two parallel trait hierarchies in deps-core/src/registry.rs:

• New: Registry, Version, Metadata (trait object-based, used by all ecosystems)
• Legacy: PackageRegistry, VersionInfo, PackageMetadata (GAT-based with associated types, used only by handler.rs test mocks)

The legacy traits are explicitly marked deprecated but still exported from deps-core/src/lib.rs. The EcosystemHandler trait in handler.rs is also deprecated (line 8: "being phased out in favor of the new Ecosystem trait") but contains ~1000 lines of generic handler code plus ~900 lines of tests that duplicate what lsp_helpers.rs already provides.

Action items:

• Delete PackageRegistry, VersionInfo, PackageMetadata traits from registry.rs
• Delete EcosystemHandler trait and all generic functions from handler.rs
• Remove InlayHintsConfig, DiagnosticsConfig from handler.rs (duplicated in ecosystem.rs as EcosystemConfig)
• Remove VersionStringGetter, YankedChecker helper traits (only used by deprecated handler)
• Clean up re-exports in deps-core/src/lib.rs

Impact: ~2000 lines removed. Eliminates confusion about which trait system to use.

P0: Eliminate ecosystem boilerplate via default trait methods or macro

Every ecosystem implementation (CargoEcosystem, NpmEcosystem, PypiEcosystem, GoEcosystem, BundlerEcosystem, DartEcosystem, MavenEcosystem, GradleEcosystem) has identical implementations for 4 of the 6 LSP methods on the Ecosystem trait:

// This exact pattern is copy-pasted in ALL 8 ecosystems:
async fn generate_inlay_hints(...) -> Vec<InlayHint> {
    lsp_helpers::generate_inlay_hints(parse_result, cached_versions, resolved_versions, loading_state, config, &self.formatter)
}
async fn generate_hover(...) -> Option<Hover> {
    lsp_helpers::generate_hover(parse_result, position, cached_versions, resolved_versions, self.registry.as_ref(), &self.formatter).await
}
async fn generate_code_actions(...) -> Vec<CodeAction> {
    lsp_helpers::generate_code_actions(parse_result, position, uri, self.registry.as_ref(), &self.formatter).await
}
async fn generate_diagnostics(...) -> Vec<Diagnostic> {
    lsp_helpers::generate_diagnostics_from_cache(parse_result, cached_versions, resolved_versions, &self.formatter)
}

Action items:

• Add fn formatter(&self) -> &dyn EcosystemFormatter to Ecosystem trait
• Move generate_inlay_hints, generate_hover, generate_code_actions, generate_diagnostics to default methods
• Remove identical implementations from all 8 ecosystem crates

Impact: ~400 lines of duplicated code removed across 8 crates.

P0: Deduplicate complete_package_names across ecosystems

The complete_package_names method is copy-pasted identically in 6 ecosystem crates (Cargo, npm, Bundler, Dart, PyPI, Go). The pattern:

async fn complete_package_names(&self, prefix: &str) -> Vec<CompletionItem> {
    if prefix.len() < 2 || prefix.len() > 100 { return vec![]; }
    let results = match self.registry.search(prefix, 20).await { ... };
    results.into_iter().map(|m| build_package_completion(...)).collect()
}

Maven and Gradle already use deps_core::completion::complete_package_names_generic which does the same thing.

Action items:

• Migrate Cargo, npm, Bundler, Dart, PyPI to use complete_package_names_generic — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71

Note: Go ecosystem retains custom implementation due to different min_prefix (3 chars vs 2).

Impact: ~120 lines removed.

P1: Replace async_trait with native async fn in traits

Since Rust 1.75, async fn in traits is stable. The project MSRV is 1.89 which is well above the threshold. All uses of #[async_trait] can be replaced with native async trait methods.

Current affected traits:

• Ecosystem trait (deps-core/src/ecosystem.rs)
• Registry trait (deps-core/src/registry.rs)
• All ecosystem impl Ecosystem for ... blocks (8 crates)
• All registry impl Registry for ... blocks (8 crates)

Action items:

• Remove #[async_trait] annotations — replaced with explicit BoxFuture pattern
• Eliminates async-trait proc-macro overhead (14% build time improvement)

Note: Used BoxFuture instead of native async fn in traits because the Ecosystem and Registry traits are used as trait objects (dyn Ecosystem), which requires Send + Sync bounds that native async fn in traits does not yet support ergonomically with dyn. Follow-up: revisit when async fn in dyn Trait stabilizes.

Impact: 14% faster build times, proc-macro eliminated.

P1: Consolidate duplicate ranges_overlap functions

There are two different ranges_overlap functions:

• deps-core/src/handler.rs:648 — takes (Range, Range), checks if two ranges overlap
• deps-core/src/lsp_helpers.rs:64 — takes (Range, Position), checks if position is in range

Both are public. The handler version will be deleted with P0, but the naming conflict is confusing. The lsp_helpers version is actually position_in_range semantics but named ranges_overlap.

Action items:

• handler.rs deleted — removes the (Range, Range) variant — done in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70
• Rename lsp_helpers::ranges_overlap to position_in_range and audit callers — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71

P1: Test boilerplate — extract shared MockDep/MockParseResult

The lsp_helpers.rs test module has 7 copies of identical MockDep + MockParseResult structs. Each test redefines the same mock types.

Action items:

• Centralize MockDep and MockParseResult in deps-core::lsp_helpers test module — done in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70
• Migrate remaining local mock copies in ecosystem crates — not applicable: ecosystem crates use concrete types (ParsedDependency, GoDependency) that require ecosystem-specific mock impls. Core mocks centralized in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70.

Impact: ~500 lines of test boilerplate removed from lsp_helpers.rs.

P2: Leverage Edition 2024 and modern Rust features

The project already uses Edition 2024 and let chains. Additional opportunities:

• LazyLock vs once_cell — std::sync::LazyLock is stable since 1.80. The project uses both once_cell and LazyLock. Remove once_cell dependency.

Action items:

• Replace once_cell::sync::Lazy with std::sync::LazyLock everywhere — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71
• Remove once_cell from workspace dependencies — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71

P2: Seal the Ecosystem trait

The Ecosystem trait is the central extension point but should only be implemented within the deps-lsp workspace. External implementations would break invariants assumed by the LSP server.

Action items:

• Add mod private { pub trait Sealed {} } to deps-core — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71
• Make Ecosystem: private::Sealed — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71
• Implement Sealed for each ecosystem type — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71

P2: Version formatting inconsistency

EcosystemFormatter::format_version_for_code_action returns different formats:

• Cargo: "1.0.0" (with quotes)
• npm: 1.0.0 (without quotes)
• PyPI: ecosystem-specific
• Go: ecosystem-specific

The inconsistency is intentional (different manifest formats), but the method name doesn't communicate that it includes quoting. Consider renaming to format_version_for_text_edit to make the TextEdit context clear.

Action items:

• Rename format_version_for_code_action to format_version_for_text_edit — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71

P3: Type-safe dependency source (DONE)

DependencySource is an enum (Registry, Git, Path, Workspace) but is stringly-typed in many places. Consider phantom type markers for compile-time differentiation when the source matters for logic branching.

P3: Duplicate is_prerelease logic

Version::is_prerelease() and VersionInfo::is_prerelease() have identical default implementations (substring matching on -alpha, -beta, etc.). After removing legacy traits (P0), only one copy remains.

• Resolved by P0: VersionInfo deleted, single Version::is_prerelease() remains.

Implementation order

1. P0: Remove legacy traits (handler.rs, PackageRegistry, VersionInfo, PackageMetadata) — done in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70
2. P0: Default methods on Ecosystem trait (deduplicate LSP handlers) — done in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70
3. P0: Consolidate complete_package_names — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71
4. P1: Replace async_trait with BoxFuture pattern — done in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70
5. P1: Extract test helpers — centralized in refactor(core): remove legacy traits, add Ecosystem defaults, replace async_trait #70, ecosystem crates deferred
6. P1: Clean up ranges_overlap naming — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71
7. P2: Remove once_cell, seal Ecosystem, rename formatter method — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71
8. P3: Type-safe improvements — done in refactor(core): consolidate complete_package_names, seal Ecosystem, remove once_cell #71 (unified DependencySource enum)