Skip to content

fix SCIP panicking due to salsa not attaching#20735

Merged
ShoyuVanilla merged 1 commit into
rust-lang:masterfrom
itsjunetime:fix_scip_salsa
Sep 25, 2025
Merged

fix SCIP panicking due to salsa not attaching#20735
ShoyuVanilla merged 1 commit into
rust-lang:masterfrom
itsjunetime:fix_scip_salsa

Conversation

@itsjunetime

Copy link
Copy Markdown
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 left a comment

Copy link
Copy Markdown
Contributor

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.

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Please squash.

@itsjunetime

Copy link
Copy Markdown
Contributor Author

squashed 👍

@ChayimFriedman2 ChayimFriedman2 left a comment

Copy link
Copy Markdown
Contributor

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
@lnicola

lnicola commented Oct 14, 2025

Copy link
Copy Markdown
Member

changelog fixup #20329

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.

5 participants