Skip to content

Conversation

@ChayimFriedman2
Copy link
Contributor

rust-analyzer needs this.

… current attached one

rust-analyzer needs this.
@netlify
Copy link

netlify bot commented Sep 17, 2025

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 678a650
🔍 Latest deploy log https://app.netlify.com/projects/salsa-rs/deploys/68cbc75f28484c0008459e98

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 17, 2025

CodSpeed Performance Report

Merging #992 will not alter performance

Comparing ChayimFriedman2:attach-allow-change (678a650) with master (3713cd7)

Summary

✅ 12 untouched

@ChayimFriedman2
Copy link
Contributor Author

The Clippy failure seems unrelated?

@MichaReiser
Copy link
Contributor

Can you say more why RA needs this? Isn't this technically unsound because it means that the outer query won't capture all dependencies?

@Veykril
Copy link
Member

Veykril commented Sep 17, 2025

We currently create sub databases to speculatively do highlighting in doc strings and the like. We already take care of not intermixing queries between these, but we are now in a situation where we create these temporary sub databases within query calls (we still don't mix things though) and that now breaks due to the checks salsa does.

I do think we should mark this unsafe or give it a dummy param to mark it as problematic if misused (if it is not unsound, unsure if this is a soundness problem or not given you can already smuggle IDs between databases).

@MichaReiser
Copy link
Contributor

Funny, Niko and I just talked about some changes to fixpoint iteration that might also allow speculative execution

@ChayimFriedman2
Copy link
Contributor Author

I don't think this is unsound? First, you cannot avoid recording query deps with this, as you can't call queries on the old db (they will attach it again), and second, even if you could I don't think not tracking some inputs is unsound.

@ChayimFriedman2
Copy link
Contributor Author

Funny, Niko and I just talked about some changes to fixpoint iteration that might also allow speculative execution

We do like speculative execution, but the speculative execution that @Veykril talked about is of another kind and unrelated to Salsa.

@ChayimFriedman2
Copy link
Contributor Author

What's up with the test?

@MichaReiser
Copy link
Contributor

The output is different based on compiler version. You need to gate this test to only run for Rust version >= whatever version beta is

@ChayimFriedman2
Copy link
Contributor Author

Locally it outputs what I pushed now. But then it errors at CI. And if I update to the CI version it errors again.

@Veykril
Copy link
Member

Veykril commented Sep 18, 2025

The compile fail tests are gated by the persistence feature which is disabled by default, so they don't run with a plain cargo test

@Veykril Veykril enabled auto-merge September 18, 2025 08:49
@Veykril Veykril added this pull request to the merge queue Sep 18, 2025
Merged via the queue into salsa-rs:master with commit e257df1 Sep 18, 2025
12 checks passed
@github-actions github-actions bot mentioned this pull request Sep 10, 2025
@ChayimFriedman2 ChayimFriedman2 deleted the attach-allow-change branch September 20, 2025 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants