Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix a case where completion was unable to expand a macro #18723

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

ChayimFriedman2
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 commented Dec 20, 2024

Which caused the macros of the popular tracing crate to not offer completions.

The reason is rather complicated: it boils down to macro ignoring their input and completion always choosing the first expansion.

What started as a fairly simple feature request turned out to be a nightmary-ish group of changes on top of something that resembles a pile of hacks more than anything else (completions in IDEs). It turns out, well, this is not so simple. But at least it is fixed now, and tracing finally offers delicious completions!

Recording.2024-12-20.023324.mp4

Fixes #18719.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2024
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sure can't wait for salsa 3 and its promised database forking so that we can get rid of the speculative stuff

crates/parser/src/grammar/items.rs Outdated Show resolved Hide resolved
crates/parser/src/grammar/items.rs Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Outdated Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Outdated Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Outdated Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Outdated Show resolved Hide resolved
crates/ide-completion/src/context/analysis.rs Outdated Show resolved Hide resolved
@ChayimFriedman2 ChayimFriedman2 force-pushed the tracing-complete branch 4 times, most recently from 13d4fed to 585ecfc Compare December 20, 2024 10:26
@ChayimFriedman2
Copy link
Contributor Author

Does salsa 3 already have db forking or it's only planned?

@Veykril
Copy link
Member

Veykril commented Dec 20, 2024

Does salsa 3 already have db forking or it's only planned?

I think some very rudimentary forking is implemented, but the stuff that we need specifically (the fork having copy on write semantics aka speculative execution) is still missing

@Veykril Veykril added this pull request to the merge queue Dec 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2024
Which caused the macros of the popular `tracing` crate to not offer completions.

The reason is rather complicated: it boils down to macro ignoring their input and completion always choosing the first expansion.
@ChayimFriedman2
Copy link
Contributor Author

Needed to rebase and bless - @Veykril can you re-approve? Thanks.

@Veykril Veykril added this pull request to the merge queue Dec 20, 2024
Merged via the queue into rust-lang:master with commit 15d2d50 Dec 20, 2024
9 checks passed
@ChayimFriedman2 ChayimFriedman2 deleted the tracing-complete branch December 20, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor completion in tracing crate macros
3 participants