8000 fix SCIP panicking due to salsa not attaching by itsjunetime · Pull Request #20735 · rust-lang/rust-analyzer · GitHub
[go: up one dir, main page]
Skip to content

fix SCIP panicking due to salsa not attaching #20735

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 25, 2025
Merged

fix SCIP panicking due to salsa not attaching #20735

merged 1 commit into from
Sep 25, 2025

Conversation

itsjunetime
Copy link
Contributor

On main, trying to run rust-analyzer scip $(pwd) --output $(pwd)/index.scip (within any repo I've tried) results in the following panic:

thread 'main' (1558669) panicked at crates/hir-ty/src/next_solver/interner.rs:295:10:
db is expected to be attached
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::option::expect_failed
   3: hir_ty::next_solver::interner::DbInterner::conjure
   4: hir_ty::traits::TraitEnvironment::empty
   5: hir::Type::new_with_resolver_inner
   6: hir::source_analyzer::SourceAnalyzer::resolve_record_field
   7: hir::semantics::SemanticsImpl::resolve_record_field_with_substitution
   8: ide_db::defs::NameRefClass::classify
   9: ide_db::defs::IdentClass::classify_node
  10: ide_db::defs::IdentClass::classify_token
  11: ide::static_index::StaticIndex::compute
  12: rust_analyzer::cli::scip::<impl rust_analyzer::cli::flags::Scip>::run
  13: rust_analyzer::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

This PR simply attaches the salsa db before calling into any code that needs it (but also after anything else that attaches it so we don't get double-attaching panics).

I also added a test here to make sure we can do a full scip run with the smallest crate in this workspace (edition), but that takes a while to run - significantly longer than all other tests (but I feel it's very useful since it would've caught this issue). On my machine, it's much faster to compile & run that specific test with --release than without (even including compile time), so maybe that's something that should be done to keep CI passing in a reasonable amount of time if we want to keep this test in?

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 24, 2025
ChayimFriedman2
ChayimFriedman2 requested changes Sep 24, 2025
View reviewed changes
Copy link
Contributor
@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

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

Please remove the test and the dependency. The change itself LGTM.

crates/rust-analyzer/Cargo.toml Outdated
test-utils.workspace = true
test-fixture.workspace = true
syntax-bridge.workspace = true
mktemp = "0.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the dependency is worth the test. We also generally don't test r-a commands this way, by running them externally. Yes, this can be problems from this, but the cost of testing this is too high.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense - I'll remove it.

@ChayimFriedman2
Copy link
Contributor

Please squash.

@itsjunetime
Copy link
Contributor Author

squashed 👍

ChayimFriedman2
ChayimFriedman2 approved these changes Sep 25, 2025
View reviewed changes
Copy link
Contributor
@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ChayimFriedman2 ChayimFriedman2 added this pull request to the merge queue Sep 25, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 25, 2025
@ShoyuVanilla ShoyuVanilla added this pull request to the merge queue Sep 25, 2025
Merged via the queue into rust-lang:master with commit 7ae5f2d Sep 25, 2025
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChayimFriedman2 ChayimFriedman2 ChayimFriedman2 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@itsjunetime @ChayimFriedman2 @ShoyuVanilla @rustbot
0