0xPlaygrounds / 0xPlaygrounds/rig
Resolve two core API naming/prominence smells: duplicate `Document` type and `ToolDyn` method overlap
- Vorherrschende Sprache
- Rust
- Sterne
- 8.6k
- Forks
- 959
- Ø Merge
- 4 Std. 32 Min.
- Gemergte PRs (30 T.)
- 117
Beschreibung
Two related API smells in `rig-core` surfaced while expanding `rig::prelude` (#2044 / #2057). Both were *worked around* there (left out of the prelude, imported explicitly), but the underlying issues are worth fixing at the type level because they confuse users independent of the prelude.
## 1. Two public types named `Document`
There are two distinct public `Document` structs:
- **`rig::completion::Document`** (`crates/rig-core/src/completion/request.rs`) — a **RAG / context document**: `{ id: String, text: String, additional_props: HashMap }`. This is what you attach as retrieval/static context for a completion.
- **`rig::message::Document`** (`crates/rig-core/src/completion/message.rs`) — a **message content block**: `{ data: DocumentSourceKind, media_type: Option, additional_params: Option }`. This is a piece of multimodal message content.
Same name, completely different shape and purpose. Consequences:
- You cannot glob-import both. When `use rig::prelude::*` brings one in, code that meant the *other* fails with confusing errors (`no field named \`data\``, `From not satisfied`, etc.). We hit exactly this migrating examples and had to leave `Document` out of the prelude entirely.
- Even outside the prelude, `use rig::completion::Document;` vs `use rig::message::Document;` is an easy footgun — the compiler error points at fields, not at the wrong import.
- Rust-analyzer's "import this" suggestions list *both* (plus provider-level `Document` variants), so autocomplete doesn't disambiguate either.
### Proposal
Rename one of them so the two concepts read differently. Options (bikeshed welcome):
- Rename the **message content** one → `DocumentContent` (parallels the surrounding `…Content` content blocks) or `DocumentBlock`; keep `completion::Document` as the RAG doc. **Recommended** — the content-block naming is the odd one out relative to its siblings, and the RAG `Document` is the more commonly-typed name in user code.
- Or rename the **RAG** one → `ContextDocument` / `ContextChunk`, keeping `message::Document`.
This is a **breaking change**, so it needs a deprecation path (re-export the old name with `#[deprecated]` for a release) and a CHANGELOG migration note. Once done, the renamed type is safe to add to `rig::prelude`, removing the current exception.
### Acceptance criteria
- [ ] The two document concepts have distinct type names.
- [ ] Old name(s) re-exported as `#[deprecated]` aliases for one release, with a migration note in `crates/rig-core/CHANGELOG.md`.
- [ ] The everyday one is added to `rig::prelude` (the `Document` prelude exception in #2057 can be removed).
## 2. `ToolDyn` prominence and method overlap with `Tool`
`rig::tool::ToolDyn` (`crates/rig-core/src/tool/mod.rs`) is the type-erased dispatch trait, with a blanket `impl ToolDyn for T`. It shares method names with `Tool` — `name`, `call`, `call_with_extensions`, `call_structured` — so when **both** traits are in scope, those calls are ambiguous (`E0034: multiple applicable items in scope`). This is why `ToolDyn` also had to be kept out of the prelude (adding it broke an existing example that called `.name()` on a tool).
Two things worth deciding:
- **Prominence.** `ToolDyn` is an internal-ish dispatch trait that the vast majority of users never name directly — they write `Tool` and let the blanket impl + `ToolSet` handle erasure. Does it need to be as public/top-level as `Tool`? Consider moving it to a `rig::tool::dyn`/`::erased` submodule, or documenting it clearly as "you usually want `Tool`."
- **Method overlap.** Independent of the prelude, having two in-scope traits with identically-named methods on the same receiver is a latent ergonomic hazard. Worth deciding whether the dyn-dispatch methods should be named distinctly (e.g. `call_dyn`) or whether the overlap is acceptable given `ToolDyn` is rarely imported alongside `Tool`.
Keeping `ToolDyn` out of the prelude is correct **regardless** of what's decided here — this issue is about whether the underlying prominence/overlap is worth changing.
### Acceptance criteria
- [ ] Decision recorded on whether `ToolDyn` stays a top-level `rig::tool` export or moves/downplays.
- [ ] Decision recorded on whether the `Tool`/`ToolDyn` method-name overlap is resolved or intentionally kept (with rationale).
---
Context: both were surfaced and worked around in #2057 (prelude expansion). This issue tracks the *proper* type-level fixes so those prelude exceptions can eventually go away.
Beitragsleitfaden
Rechercherichtung
The issue involves two distinct API naming problems in the rig-core crate. First, examine the two Document types in crates/rig-core/src/completion/request.rs and crates/rig-core/src/completion/message.rs. Second, look at the ToolDyn trait in crates/rig-core/src/tool/mod.rs and its overlap with the Tool trait. Start by reading the existing code and the prelude changes in PR #2057. The work requires understanding Rust's module system, trait design, and breaking change deprecation strategies. 'Done' means distinct type names, deprecation aliases, updated prelude, and documented decisions on ToolDyn.
Vom Indexierungsmodell aus dem Issue-Text verfasst.
Bewertung
- Tech-Stack
- rust
- Bereich
- backend-api-design
- Issue-Typ
- Refactoring
- Schwierigkeit
- 4/5
- Geschätzter Aufwand
- 3-5 Tage
- Aktivitätsstatus
- Ruhig
- Klarheit
- Klar beschrieben
- Anfängerfreundlichkeit
- 35/100