fix Treat doctests as rust code #4170#21949
fix Treat doctests as rust code #4170#21949asukaminato0721 wants to merge 1 commit intorust-lang:masterfrom
Conversation
asukaminato0721
force-pushed
the
4170
branch
2 times, most recently
from
c5c332f to
6397575
Compare
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
Pull request overview
This PR adds Rust completion support inside Rust doctest code fences embedded in doc comments by injecting the doctest snippet into a temporary wrapper function and mapping completion edits/ranges back to the original file.
Changes:
- Adds a doctest-aware completion fast-path that runs before normal completion.
- Introduces a new
completions::doctestmodule that builds an injected view of the file and upmaps completion results. - Adds a regression test ensuring
hel$0in a/// ```rustfenced block completeshelper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/ide-completion/src/lib.rs | Routes completion requests through doctest completion first. |
| crates/ide-completion/src/completions.rs | Exposes the new doctest completion module. |
| crates/ide-completion/src/completions/doctest.rs | Implements doctest fence detection, injection/mapping, and completion dispatch. |
| crates/ide-completion/src/tests.rs | Adds a doctest code fence completion test and updates expect-test imports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn build_doctest_db( | ||
| db: &RootDatabase, | ||
| changed_file_id: ide_db::FileId, | ||
| changed_file_text: String, | ||
| ) -> RootDatabase { | ||
| let mut doctest_db = RootDatabase::new(None); | ||
| if db.expand_proc_attr_macros() { | ||
| doctest_db.enable_proc_attr_macros(); | ||
| } | ||
|
|
||
| let roots = source_roots(db); | ||
| let mut change = ChangeWithProcMacros::default(); | ||
| change.set_roots(roots.clone()); | ||
| for root in &roots { | ||
| for file_id in root.iter() { | ||
| let text = if file_id == changed_file_id { | ||
| changed_file_text.clone() | ||
| } else { | ||
| db.file_text(file_id).text(db).to_string() | ||
| }; | ||
| change.change_file(file_id, Some(text)); | ||
| } | ||
| } | ||
| change.set_crate_graph(copy_crate_graph(db)); | ||
| doctest_db.apply_change(change); | ||
| doctest_db | ||
| } |
There was a problem hiding this comment.
build_doctest_db rebuilds a fresh RootDatabase, re-sets all source roots, and copies every file’s text + reconstructs the crate graph on each completion request. This is O(workspace size) work in a very hot path and is likely to cause severe completion latency/memory churn on medium/large projects. Prefer cloning the existing RootDatabase (db.clone()) and applying a ChangeWithProcMacros that only updates changed_file_id’s text (no set_roots / no crate-graph rebuild), so salsa can reuse existing inputs/caches and only re-analyze what’s needed.
There was a problem hiding this comment.
this is a bottleneck but can't figure out it for now.
There was a problem hiding this comment.
This is completely unacceptable as it's now.
|
I haven't looked at the code yet, but this is likely to need design discussions to meet expected performance. Just opening a PR is likely not going to go well. |
|
We already have a fine injection system for fixtures. We can reuse that, however we need to think how to represent crate dependencies. |
rustbot
removed
the
S-waiting-on-review
|
@asukaminato0721 What's the status of this? Do you intend to complete this? |
Stuck on how to represent crate dependencies. yeah, if possible. |
|
This is indeed a problem. If we create a new I don't have a solution in my head for that (perhaps speculative execution will help, once we have that in Salsa). @rust-lang/rust-analyzer do you have any idea? |
|
We might be able to go by swapping So, if you want to try this, please open a Zulip discussion. |
fix #4170
I use this method:
inject a function
So the completion works, then maintain a map between the real code and inject function.
current limitation: proc-macro is not well supported.