refactor: Reduce reference indirection and type sizes; Cleanup doctests#257
refactor: Reduce reference indirection and type sizes; Cleanup doctests#257DanikVitek wants to merge 3 commits into
Conversation
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughThis refactoring removes unnecessary Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR removes unnecessary Confidence Score: 5/5Safe to merge — purely structural refactoring with no behavioural changes; only remaining finding is a one-line doc comment. All findings are P2. The refactoring is mechanically correct: dependi-lsp/src/registries/mod.rs — one-line doc comment needs updating.
|
| Filename | Overview |
|---|---|
| dependi-lsp/src/registries/mod.rs | Registry trait updated to return owned Client instead of Arc<Client>; module-level architecture doc block still shows the old Arc<Client> signature. |
| dependi-lsp/src/backend.rs | Parsers moved to Copy fields; registries now held directly (not wrapped in Arc); commented-out Arc::ptr_eq debug asserts correctly replaced with a TODO; all cloning updated to new model. |
| dependi-lsp/src/registries/http_client.rs | create_shared_client() now returns Client directly instead of Arc<Client> — straightforward and correct. |
| dependi-lsp/src/registries/crates_io.rs | CratesIoRegistry now derives Clone and holds reqwest::Client directly with the rate-limiter still wrapped in Arc<Mutex<_>>; base URL moved to a module-level const. |
| dependi-lsp/src/vulnerabilities/osv.rs | OsvClient already held its own reqwest::Client; Arc wrapper removed; dedicated 30s timeout client is intentional and unchanged in behaviour. |
| dependi-lsp/Cargo.toml | Adds clone_on_ref_ptr = "warn" to clippy lints, which is the motivating guardrail for the entire Arc-removal refactor. |
| dependi-lsp/src/parsers/cargo.rs | CargoParser is now #[derive(Debug, Clone, Copy, Default)] — a ZST; new() kept for API compatibility. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
direction LR
class DependiBackend {
+HttpClient http_client
+CratesIoRegistry crates_io
+PyPiRegistry pypi
+GoProxyRegistry go_proxy
+PackagistRegistry packagist
+PubDevRegistry pub_dev
+NuGetRegistry nuget
+RubyGemsRegistry rubygems
+Arc~NpmRegistry~ npm_registry
+Arc~MavenCentralRegistry~ maven_central
+OsvClient osv_client
}
class Registry {
<<trait>>
+get_version_info(name) VersionInfo
+http_client() Client
}
class CratesIoRegistry {
+Client client
+Arc~Mutex~RateLimiter~~ rate_limiter
}
class PyPiRegistry {
+Client client
}
class GoProxyRegistry {
+Client client
}
class OsvClient {
+Client client
}
note for CratesIoRegistry "Arc removed;\nClient cloned directly"
note for OsvClient "Own Client\n(30s timeout)"
Registry <|.. CratesIoRegistry
Registry <|.. PyPiRegistry
Registry <|.. GoProxyRegistry
DependiBackend --> CratesIoRegistry
DependiBackend --> PyPiRegistry
DependiBackend --> GoProxyRegistry
DependiBackend --> OsvClient
Prompt To Fix All With AI
This is a comment left during a code review.
Path: dependi-lsp/src/registries/mod.rs
Line: 28
Comment:
**Stale return type in Architecture doc block**
The `fn http_client` signature shown in the `//! ## Architecture` code block still shows `Arc<Client>` as the return type, but the actual trait now returns an owned `Client`.
```suggestion
//! fn http_client(&self) -> Client;
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (3): Last reviewed commit: "chore: Update `CHANGELOG.md`" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
DanikVitek
force-pushed
the
refactor/indirection
branch
2 times, most recently
from
57e05bf to
ee063c9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
dependi-lsp/src/registries/mod.rs (1)
25-29:⚠️ Potential issue | 🟡 MinorUpdate the stale
Registrydocumentation.The architecture snippet still advertises
Arc<Client>, and the method docs still say “reference” even thoughhttp_client()now returns a clonedClient.📝 Proposed doc fix
-//! ```ignore +//! ```no_run +//! use dependi_lsp::registries::VersionInfo; +//! use reqwest::Client; +//! //! pub trait Registry: Send + Sync { //! async fn get_version_info(&self, package_name: &str) -> anyhow::Result<VersionInfo>; -//! fn http_client(&self) -> Arc<Client>; +//! fn http_client(&self) -> Client; //! } //! ``` ... - /// Get a reference to the HTTP client used by this registry + /// Get a clone of the HTTP client used by this registry fn http_client(&self) -> Client;Also applies to: 218-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/registries/mod.rs` around lines 25 - 29, Update the stale Registry docs to reflect that http_client() returns a cloned reqwest::Client (not Arc<Client>): change the example code block to import reqwest::Client and VersionInfo, update the code fence to no_run, and show fn http_client(&self) -> Client; also update the method doc string for http_client (previously "Get a reference to the HTTP client used by this registry") to "Get a clone of the HTTP client used by this registry"; apply the same wording and signature correction wherever Registry::http_client appears (including the other doc occurrence around the http_client docs).dependi-lsp/src/backend.rs (1)
601-617:⚠️ Potential issue | 🟡 MinorStale doc comments: fields are no longer
Arc-wrapped.After the refactor,
config/documentsare stillArc, but the per-parser comment on line 607 ("Parsers (Arc-wrapped for sharing with debounce tasks)") is no longer accurate since parsers are now ownedCopyvalues. Same for the similar phrasing on lines 601/603 if it implies parsers/registries areArc. Also the comment on line 418 (// Clone Arc references for async tasks) is misleading now that most clones are plain value clones.📝 Suggested wording update
- /// Parsers (Arc-wrapped for sharing with debounce tasks) + /// Parsers (owned `Copy` ZSTs, copied into processing contexts) cargo_parser: CargoParser,- // Clone Arc references for async tasks + // Snapshot shared state for async tasks (cheap clones; reqwest::Client is internally Arc'd) let crates_io = self.crates_io.clone();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dependi-lsp/src/backend.rs` around lines 601 - 617, Update stale doc comments that claim parser fields are "Arc-wrapped" or shared via Arc: change the comment for the parser group (cargo_parser, npm_parser, python_parser, go_parser, php_parser, dart_parser, csharp_parser, ruby_parser, maven_parser) to reflect that parsers are now plain owned/Copy values and not Arc-wrapped, and remove or reword any wording that implies registries/parsers are Arc; also update the other misleading comment that says "// Clone Arc references for async tasks" to accurately describe what's being cloned (e.g., cloning Arc for config/documents only, and plain value clones for parsers) so comments match current ownership semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dependi-lsp/src/backend.rs`:
- Line 709: The OsvClient currently uses OsvClient::default() which calls
Self::new().expect(...) and creates its own reqwest::Client (30s timeout &
different UA), causing a panic path and breaking shared-client pooling; update
OsvClient to accept the shared reqwest::Client instance (the http_client created
earlier) instead of creating its own, remove or rework the Default/new panic
behavior to return Result or otherwise propagate creation errors, and change the
backend construction site that sets osv_client: OsvClient::default() to call the
new constructor (e.g., OsvClient::with_client(shared_client)) so the shared
timeout, pooling and user-agent are reused and startup no longer panics on
client build failures.
- Around line 1273-1284: Remove the dead/commented Arc::ptr_eq verification
block because Registry::http_client() now returns a Client by value (not
Arc<Client>) so ptr_eq is invalid; delete the entire commented section
referencing Arc::ptr_eq and the related lines that reference
self.crates_io.http_client(), self.npm_registry.read().await.http_client(),
self.pypi.http_client(), self.go_proxy.http_client(),
self.packagist.http_client(), self.pub_dev.http_client(),
self.nuget.http_client(), and self.rubygems.http_client(); rely on construction
guarantees (clients cloned from self.http_client) and replace any future
identity checks with functional tests exercising endpoints across registries
instead of pointer equality checks.
- Around line 664-667: The public API was accidentally changed by making
with_http_client accept Option<HttpClient> instead of Option<Arc<HttpClient>>,
breaking external callers that pass Arc<HttpClient>; revert the parameter type
to Option<Arc<HttpClient>> in DependiBackend::with_http_client and adapt the
internal unwrap_or_else to produce an Arc<HttpClient> (e.g., wrap the created
client in Arc::new or clone as appropriate), or provide an additional
constructor overload that accepts Option<HttpClient> while keeping the original
Option<Arc<HttpClient>> signature; update callers (e.g., DependiBackend::new)
and the public docs/release notes accordingly.
In `@dependi-lsp/src/registries/go_proxy.rs`:
- Around line 113-119: The rustdoc in the GoProxyRegistry constructor references
a non-existent `create_client`; update the intra-doc link to point to the actual
function `create_shared_client` so docs build correctly; locate the doc comment
above `GoProxyRegistry` and replace `[`create_client`]` with
`[`create_shared_client`]` (or remove the link and mention
`create_shared_client`) to match the imported/used function name.
In `@dependi-lsp/src/registries/pub_dev.rs`:
- Around line 100-104: The doc comment for PubDevRegistry incorrectly links to
create_client; update the intra-doc link to the actual function name
create_shared_client used in this module so the documentation points to the
correct symbol (replace references to create_client with create_shared_client in
the PubDevRegistry doc comment).
---
Outside diff comments:
In `@dependi-lsp/src/backend.rs`:
- Around line 601-617: Update stale doc comments that claim parser fields are
"Arc-wrapped" or shared via Arc: change the comment for the parser group
(cargo_parser, npm_parser, python_parser, go_parser, php_parser, dart_parser,
csharp_parser, ruby_parser, maven_parser) to reflect that parsers are now plain
owned/Copy values and not Arc-wrapped, and remove or reword any wording that
implies registries/parsers are Arc; also update the other misleading comment
that says "// Clone Arc references for async tasks" to accurately describe
what's being cloned (e.g., cloning Arc for config/documents only, and plain
value clones for parsers) so comments match current ownership semantics.
In `@dependi-lsp/src/registries/mod.rs`:
- Around line 25-29: Update the stale Registry docs to reflect that
http_client() returns a cloned reqwest::Client (not Arc<Client>): change the
example code block to import reqwest::Client and VersionInfo, update the code
fence to no_run, and show fn http_client(&self) -> Client; also update the
method doc string for http_client (previously "Get a reference to the HTTP
client used by this registry") to "Get a clone of the HTTP client used by this
registry"; apply the same wording and signature correction wherever
Registry::http_client appears (including the other doc occurrence around the
http_client docs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d4e97ff-9bbc-42c8-a605-3398d8fc0ac4
📒 Files selected for processing (25)
CHANGELOG.mddependi-lsp/Cargo.tomldependi-lsp/src/backend.rsdependi-lsp/src/parsers/cargo.rsdependi-lsp/src/parsers/csharp.rsdependi-lsp/src/parsers/dart.rsdependi-lsp/src/parsers/go.rsdependi-lsp/src/parsers/maven.rsdependi-lsp/src/parsers/npm.rsdependi-lsp/src/parsers/php.rsdependi-lsp/src/parsers/python.rsdependi-lsp/src/parsers/ruby.rsdependi-lsp/src/registries/cargo_sparse.rsdependi-lsp/src/registries/crates_io.rsdependi-lsp/src/registries/go_proxy.rsdependi-lsp/src/registries/http_client.rsdependi-lsp/src/registries/maven_central.rsdependi-lsp/src/registries/mod.rsdependi-lsp/src/registries/npm.rsdependi-lsp/src/registries/nuget.rsdependi-lsp/src/registries/packagist.rsdependi-lsp/src/registries/pub_dev.rsdependi-lsp/src/registries/pypi.rsdependi-lsp/src/registries/rubygems.rsdependi-lsp/src/vulnerabilities/osv.rs
| pub fn with_http_client(client: Client, http_client: Option<HttpClient>) -> Self { | ||
| let http_client = http_client.unwrap_or_else(|| { | ||
| create_shared_client().expect("Failed to create shared HTTP client") | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all call sites of with_http_client and confirm none still pass Arc<Client>.
rg -nP --type=rust -C2 '\bwith_http_client\s*\('
# Also scan for any remaining Arc<HttpClient> / Arc<Client> types that may be feeding in.
rg -nP --type=rust -C1 'Arc\s*<\s*(HttpClient|reqwest::Client|Client)\s*>'Repository: mpiton/zed-dependi
Length of output: 898
🏁 Script executed:
#!/bin/bash
# Check git log for changes to this function
git log --oneline -p -- dependi-lsp/src/backend.rs | grep -A 10 -B 5 "with_http_client" | head -50
# Also check if there's a diff showing the change
git diff HEAD~5 -- dependi-lsp/src/backend.rs | grep -A 10 -B 5 "with_http_client" || echo "No recent diff found"Repository: mpiton/zed-dependi
Length of output: 998
🏁 Script executed:
#!/bin/bash
# Try to see the actual git diff for this specific function
git diff HEAD -- dependi-lsp/src/backend.rs | grep -A 20 -B 5 "with_http_client" || echo "No HEAD diff"
# Check git status to see if files are modified
git status dependi-lsp/src/backend.rs
# Try showing the last commit that touched this file
git log -1 --oneline dependi-lsp/src/backend.rs
# Get the actual diff from the last commit
git show HEAD:dependi-lsp/src/backend.rs 2>/dev/null | grep -A 10 "with_http_client" || echo "Could not show previous version"Repository: mpiton/zed-dependi
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Get the previous commit and show the old version
git log --oneline dependi-lsp/src/backend.rs | head -5
# Show the version from 2 commits back
git show HEAD~1:dependi-lsp/src/backend.rs 2>/dev/null | grep -A 10 "with_http_client" || echo "Could not show HEAD~1"Repository: mpiton/zed-dependi
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Check if there are any recent commits with details about the Arc unwrapping
git log --all --oneline | head -10
# Try to look at the actual diff that was reviewed
# Search for any indication of Arc<HttpClient> in the codebase to see if it's completely removed
rg "Arc\s*<\s*HttpClient\s*>" dependi-lsp/src/
# Also check the CHANGELOG to see if there's info about the API change
grep -i "http_client\|Arc" dependi-lsp/CHANGELOG.md || echo "No relevant entries"Repository: mpiton/zed-dependi
Length of output: 772
🏁 Script executed:
#!/bin/bash
# Check the diff from the most relevant commit (the refactor one)
git show 2b95214 --stat | head -20
# Look for commits mentioning Arc or backend
git log --all --oneline --grep="Arc\|backend\|http" | head -10
# Check what actually changed in the backend.rs file in recent commits
git log -p --reverse -- dependi-lsp/src/backend.rs | grep -A 15 "with_http_client" | head -40Repository: mpiton/zed-dependi
Length of output: 1537
🏁 Script executed:
#!/bin/bash
# Let me check what HttpClient actually is
rg "type\s+HttpClient|struct\s+HttpClient" dependi-lsp/src/
# Check the imports and definitions
rg "HttpClient" dependi-lsp/src/backend.rs | head -20
# Let me check if there's a PR or branch context
git branch -a
# Check if there are any uncommitted changes in staging
git diff --cached dependi-lsp/src/backend.rsRepository: mpiton/zed-dependi
Length of output: 310
🏁 Script executed:
#!/bin/bash
# Check if there's a diff being reviewed. Let me see the main branch version
git show origin/main:dependi-lsp/src/backend.rs 2>/dev/null | grep -A 10 "with_http_client" | head -20 || echo "Could not access main"
# Also compare current HEAD vs main
git diff origin/main -- dependi-lsp/src/backend.rs | grep -A 15 -B 5 "with_http_client" || echo "No diff in with_http_client"
# Let's see what the actual PR context is - look at commit messages
git log --oneline origin/main..HEAD 2>/dev/null | head -10 || echo "HEAD is on main"Repository: mpiton/zed-dependi
Length of output: 1953
🏁 Script executed:
#!/bin/bash
# Perfect! Now I can see the actual change. Let me verify if this function is actually part of the public API
# by checking what's exported from the crate
# Check the lib.rs or crate root to see what's exported
rg "pub use|pub mod|pub struct DependiBackend" dependi-lsp/src/ | head -20
# Check if DependiBackend is exported
cat dependi-lsp/src/lib.rs 2>/dev/null | grep -i "dependibackend\|with_http_client" || echo "Checking main.rs"
# Look for any example code or tests that might show external usage
find dependi-lsp -name "*.rs" -exec grep -l "with_http_client" {} \;
# Check if there's a feature gate or public exports
rg "pub.*DependiBackend" dependi-lsp/src/Repository: mpiton/zed-dependi
Length of output: 1074
🏁 Script executed:
#!/bin/bash
# Check if with_http_client is used anywhere else, especially in tests or examples
rg "with_http_client" dependi-lsp/ --type=rust
# Check if there are any integration tests or examples
find dependi-lsp -name "*.rs" -path "*/examples/*" -o -path "*/tests/*" | head -10
# Look for any Arc<HttpClient> usage that might break
rg "Arc\s*<.*HttpClient" dependi-lsp/src/Repository: mpiton/zed-dependi
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Check the integration tests to see if they use with_http_client
grep -n "with_http_client\|DependiBackend" dependi-lsp/tests/integration_test.rs | head -30
# Also check fuzz tests
grep -n "with_http_client\|DependiBackend" dependi-lsp/tests/fuzz_regression.rs | head -30
# And check main.rs to see how DependiBackend is typically instantiated
rg "DependiBackend::" dependi-lsp/src/main.rs -A 2 -B 2Repository: mpiton/zed-dependi
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Check if this crate is published as a library with public API, or just a binary
cat dependi-lsp/Cargo.toml | grep -A 5 "\[package\]" | head -20
# Check lib.rs to see what's re-exported
cat dependi-lsp/src/lib.rs | head -50
# Double-check if there's a lib target
grep -A 10 "\[\[lib\]\]" dependi-lsp/Cargo.toml || echo "No explicit lib section"Repository: mpiton/zed-dependi
Length of output: 610
🏁 Script executed:
#!/bin/bash
# Check the integration test to see if it calls with_http_client
cat dependi-lsp/tests/integration_test.rs | head -100
# Check if there's any usage of Arc<HttpClient> in tests
rg "Arc.*HttpClient|with_http_client" dependi-lsp/tests/ --type=rust -B 2 -A 2Repository: mpiton/zed-dependi
Length of output: 2787
🏁 Script executed:
#!/bin/bash
# Let me verify the exact state one more time - compare the current code with what was shown before
echo "=== Current state on HEAD ==="
git show HEAD:dependi-lsp/src/backend.rs | grep -A 3 "pub fn with_http_client"
echo "=== State on origin/main ==="
git show origin/main:dependi-lsp/src/backend.rs | grep -A 3 "pub fn with_http_client"
# Verify this is indeed a public API by checking crates.io or docs
# Check if the crate publishes to crates.io
grep -i "repository\|homepage" dependi-lsp/Cargo.tomlRepository: mpiton/zed-dependi
Length of output: 601
Public API break: with_http_client signature changed from Option<Arc<HttpClient>> to Option<HttpClient>.
The function parameter type was simplified, removing the Arc wrapper. While the internal caller DependiBackend::new() passes None and avoids the issue, any external code (other crates, future consumers of the dependi-lsp library) that calls with_http_client with an Arc<HttpClient> will fail to compile. Worth documenting in release notes if this crate is published as a library.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/backend.rs` around lines 664 - 667, The public API was
accidentally changed by making with_http_client accept Option<HttpClient>
instead of Option<Arc<HttpClient>>, breaking external callers that pass
Arc<HttpClient>; revert the parameter type to Option<Arc<HttpClient>> in
DependiBackend::with_http_client and adapt the internal unwrap_or_else to
produce an Arc<HttpClient> (e.g., wrap the created client in Arc::new or clone
as appropriate), or provide an additional constructor overload that accepts
Option<HttpClient> while keeping the original Option<Arc<HttpClient>> signature;
update callers (e.g., DependiBackend::new) and the public docs/release notes
accordingly.
| http_client, | ||
| token_manager, | ||
| osv_client: Arc::new(OsvClient::default()), | ||
| osv_client: OsvClient::default(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for the Default impl (or derive) on OsvClient.
rg -nP --type=rust -C4 'impl\s+Default\s+for\s+OsvClient|#\[derive\([^)]*\bDefault\b[^)]*\)\][\s\S]{0,120}?struct\s+OsvClient'
# Inspect OsvClient::new / construction for comparison.
ast-grep --pattern $'impl OsvClient {
$$$
}'Repository: mpiton/zed-dependi
Length of output: 14841
🏁 Script executed:
sed -n '660,675p' dependi-lsp/src/backend.rsRepository: mpiton/zed-dependi
Length of output: 759
🏁 Script executed:
rg -n "create_shared_client\|fn create_shared_client" dependi-lsp/src/backend.rs -A 10Repository: mpiton/zed-dependi
Length of output: 44
🏁 Script executed:
rg -n "create_shared_client|fn create_shared_client" dependi-lsp/src/ -A 5Repository: mpiton/zed-dependi
Length of output: 20379
🏁 Script executed:
cat -n dependi-lsp/src/registries/http_client.rs | head -40Repository: mpiton/zed-dependi
Length of output: 1487
OsvClient::default() creates a latent panic path; reconsider accepting the shared HTTP client.
Default implements Self::new().expect("Failed to create OsvClient"), which panics at LSP startup if the reqwest::Client build fails. While Default correctly delegates to new() (no settings divergence), the implementation is inconsistent with the shared HTTP client pattern used throughout the codebase:
- Other registry clients (npm, cargo, crates.io, etc.) accept and reuse the shared client created on line 666, which uses a 10-second timeout and shared pooling.
OsvClientconstructs its own separatereqwest::Clientwith a 30-second timeout and a different user-agent format, defeating the connection pooling and configuration consistency of the shared client.
Consider refactoring OsvClient to accept the shared http_client: Client parameter, similar to other registries, to eliminate the redundant client creation and improve resilience (e.g., by gracefully handling client creation failures rather than panicking at startup).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dependi-lsp/src/backend.rs` at line 709, The OsvClient currently uses
OsvClient::default() which calls Self::new().expect(...) and creates its own
reqwest::Client (30s timeout & different UA), causing a panic path and breaking
shared-client pooling; update OsvClient to accept the shared reqwest::Client
instance (the http_client created earlier) instead of creating its own, remove
or rework the Default/new panic behavior to return Result or otherwise propagate
creation errors, and change the backend construction site that sets osv_client:
OsvClient::default() to call the new constructor (e.g.,
OsvClient::with_client(shared_client)) so the shared timeout, pooling and
user-agent are reused and startup no longer panics on client build failures.
There was a problem hiding this comment.
@mpiton What do you think? Also, I'd consider writing explicit with_defaults (or something like that) factory methods, rather than panicking inside the impl Defaults which should probably be known to be panic-safe
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Agree on the Default-safety point — an LSP that panics during startup because TLS init hiccuped is a bad trade.
For OsvClient specifically I'd go a bit further and drop Default entirely: make with_client(Client) the only constructor and accept the shared client from the backend. The 30s timeout was probably set because OSV batch queries can be slow, but that's better expressed as a per-request .timeout() on the RequestBuilder than as a separate client with its own pool. While we're in there, the UA string in new() still points at github.com/mathieu/zed-dependi which is stale.
On the broader with_defaults idea: happy to land it in this PR if you want to keep scope tight, or as a follow-up. If it's a follow-up, can you delete the commented-out Arc::ptr_eq block in backend.rs around line 1271 before merge? Leaving it as dead code bothers me more than losing the invariant check, and a functional test that hits one mock per registry would cover the shared-client property better than ptr identity anyway.
There was a problem hiding this comment.
Ok, I'll work on this.
Also, I was tinkering around with compile-time "branding" of the cloned reqwest::Clients, using lifetime parameters, just to see what this could look like. So far, this seems to require scoping the creation of DependiBackend with closures. Seems to make the factory a little complicated. I could try to do this later as a separate PR. I'm not sure if it's worth it to introduce the lifetime parameters into the codebase ATM, especially when there are mock tests.
There was a problem hiding this comment.
Thanks for trying it, that's a neat experiment. I've played with ghost-cell-style branded lifetimes in another project and hit exactly the pain you're describing: the scoped closure factory is workable, but every type that holds a registry has to propagate the 'brand parameter, which is about as viral as async once you're past a couple of layers.
For this codebase I don't think we'd get our money's worth. DependiBackend is a single long-lived instance per LSP session, so the construction-site mistake we'd be preventing at compile time is pretty narrow. wiremock tests would need their own scope per test, which works but adds noise. And dependi-zed targets WASM, I'd want to validate the brand pattern round-trips cleanly through wasm-bindgen before committing to it.
I'd keep this PR focused on the Default/with_client fix and park the branded-client idea for later. If we ever wire up a second client (say a different-timeout one dedicated to OSV) and find ourselves needing compile-time proof they don't cross-pollinate, that's the moment to revisit.
Agreed on dropping the invariant check for now. A functional test that fires one request per registry from a single scope would give us the same confidence without the type-system gymnastics.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 25 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="dependi-lsp/src/registries/pub_dev.rs">
<violation number="1" location="dependi-lsp/src/registries/pub_dev.rs:100">
P2: Rustdoc comment links to non-existent `create_client`; it should reference `create_shared_client`.</violation>
</file>
<file name="dependi-lsp/src/registries/mod.rs">
<violation number="1" location="dependi-lsp/src/registries/mod.rs:219">
P2: Registry documentation is stale: it still advertises `Arc<Client>`/reference semantics while the trait now returns owned `Client`.</violation>
</file>
<file name="dependi-lsp/src/registries/go_proxy.rs">
<violation number="1" location="dependi-lsp/src/registries/go_proxy.rs:115">
P2: Incorrect intra-doc link references `create_client`, which does not exist; docs should link to `create_shared_client`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
DanikVitek
force-pushed
the
refactor/indirection
branch
from
ee063c9 to
e7e9b6e
Compare
mpiton
left a comment
mpiton
left a comment
There was a problem hiding this comment.
Nice cleanup overall — the Arc<reqwest::Client> was genuinely double-Arc'ing, and the ZST parsers had no business sitting behind an Arc either. A few things I want to see before we merge:
Stale docs and dead code
registries/mod.rs(~25-29, ~218): the//! ## Architectureblock still showsfn http_client(&self) -> Arc<Client>, and the method doc says "Get a reference" — both need updating now that it returns an ownedClient.backend.rs~606:/// Parsers (Arc-wrapped for sharing with debounce tasks)is no longer true; they'reCopyZSTs now.backend.rs~1271-1284: please delete the commented-outArc::ptr_eqblock instead of leaving it. If we want the shared-client invariant back, a functional test hitting one mock per registry is more durable than ptr identity.go_proxy.rs~115 andpub_dev.rs~100: intra-doc links point tocreate_client, which doesn't exist — should becreate_shared_client.
OsvClient + Default
Already covered in the thread above, but to recap: drop Default for OsvClient, make with_client(Client) the only constructor, and pass the shared client in from the backend. The 30s timeout can become a per-request .timeout() on the RequestBuilder. Also the UA string in new() is stale (github.com/mathieu/zed-dependi).
API break
DependiBackend::with_http_client went from Option<Arc<HttpClient>> to Option<HttpClient>. That's a public signature change. If anything downstream (I'm mainly thinking of dependi-zed) holds an Arc<HttpClient> and hands it in, it'll break. Worth a quick check.
Lost test coverage
test_registries_share_client_instance got removed along with the Arc::as_ptr asserts. Fair, since ptr_eq isn't meaningful anymore — but we've lost the guarantee that every registry shares one pool. A wiremock-based test that fires one request from each registry and inspects the pool on a shared handle would cover it, or even just a doctest showing the intended wiring.
Security
No direct regression. The startup panic in Default is the only robustness concern, and you've already flagged it.
Don't forget to rebase on main before pushing fixes — we've landed #255 (lockfile scanning) and #260 (HTML output) since this branch was cut, and the merge is going to touch backend.rs pretty heavily.
2 participants
Summary
Type of Change
Related Issue
Closes #256
Changes Made
Arc-unwrapped ZSTParsers and made them CopyArc-unwrappedreqwest::ClientsArcs are goneArc-unwrappedRegistrys with constantbase_urls (moved URLs into privateconsts)impl Defaultsignored doc-tests to beno_runTesting
cargo testpassescargo clippy -- -D warningspassescargo fmt --checkpassesChecklist
Screenshots (if applicable)
Summary by cubic
Reduce reference indirection across parsers, registries, and OSV by removing extra
Arclayers and shrinking types. This simplifies cloning, reduces memory, and cleans up doctests and examples.Refactors
Clone + Copy; removedArcaround them.OsvClientnow hold plainreqwest::Client(notArc<Client>); key registries deriveClone.create_shared_client()returnsClient;Registry::http_client()returnsClient.consts (crates.io, PyPI, Go proxy, npm, NuGet, Packagist, pub.dev, RubyGems).Clientinstead ofArc.no_runand removed redundantDefaultexamples.clone_on_ref_ptr = "warn"; inlined formatting arguments in logs.Migration
with_client(Arc<Client>)/with_client_and_config(Arc<Client>, ...)withwith_client(Client)/with_client_and_config(Client, ...)across registries (CratesIoRegistry,PyPiRegistry,GoProxyRegistry,PackagistRegistry,PubDevRegistry,NuGetRegistry,RubyGemsRegistry,NpmRegistry,MavenCentralRegistry).CargoSparseRegistry::with_client_and_configto(Client, &str, Option<String>).Client::clone()instead ofArc::clone(); acceptClientfromcreate_shared_client().Written for commit e7e9b6e. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Documentation