resolve: Unload speculatively resolved crates before freezing cstore #119592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
cc @surechen |
Yes, this happens and causes spurious knock-on errors in typeck. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
compiler-errors
approved these changes
Feb 5, 2024
compiler-errors
added
S-waiting-on-author
|
@bors r=compiler-errors |
bors
added
S-waiting-on-bors
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 7, 2024
…rors resolve: Unload speculatively resolved crates before freezing cstore Name resolution sometimes loads additional crates to improve diagnostics (e.g. suggest imports). Not all of these diagnostics result in errors, sometimes they are just warnings, like in rust-lang#117772. If additional crates loaded speculatively stay and gets listed by things like `query crates` then they may produce further errors like duplicated lang items, because lang items from speculatively loaded crates are as good as from non-speculatively loaded crates. They can probably do things like adding unintended impls from speculatively loaded crates to method resolution as well. The extra crates will also get into the crate's metadata as legitimate dependencies. In this PR I remove the speculative crates from cstore when name resolution is finished and cstore is frozen. This is better than e.g. filtering away speculative crates in `query crates` because things like `DefId`s referring to these crates and leaking to later compilation stages can produce ICEs much easier, allowing to detect them. The unloading could potentially be skipped if any errors were reported (to allow using `DefId`s from speculatively loaded crates for recovery), but I didn't do it in this PR because I haven't seen such cases of recovery. We can reconsider later if any relevant ICEs are reported. Unblocks rust-lang#117772.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 7, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#119162 (Add unstable `-Z direct-access-external-data` cmdline flag for `rustc`) - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore) - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation) - rust-lang#120455 ( Add FileCheck annotations to MIR-opt SROA tests) - rust-lang#120470 (Mark "unused binding" suggestion as maybe incorrect) - rust-lang#120619 (Assert that params with the same *index* have the same *name*) - rust-lang#120633 (pattern_analysis: gather up place-relevant info) - rust-lang#120726 (Don't use bashism in checktools.sh) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 7, 2024
…rors resolve: Unload speculatively resolved crates before freezing cstore Name resolution sometimes loads additional crates to improve diagnostics (e.g. suggest imports). Not all of these diagnostics result in errors, sometimes they are just warnings, like in rust-lang#117772. If additional crates loaded speculatively stay and gets listed by things like `query crates` then they may produce further errors like duplicated lang items, because lang items from speculatively loaded crates are as good as from non-speculatively loaded crates. They can probably do things like adding unintended impls from speculatively loaded crates to method resolution as well. The extra crates will also get into the crate's metadata as legitimate dependencies. In this PR I remove the speculative crates from cstore when name resolution is finished and cstore is frozen. This is better than e.g. filtering away speculative crates in `query crates` because things like `DefId`s referring to these crates and leaking to later compilation stages can produce ICEs much easier, allowing to detect them. The unloading could potentially be skipped if any errors were reported (to allow using `DefId`s from speculatively loaded crates for recovery), but I didn't do it in this PR because I haven't seen such cases of recovery. We can reconsider later if any relevant ICEs are reported. Unblocks rust-lang#117772.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 7, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore) - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation) - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages) - rust-lang#120688 (GVN: also turn moves into copies with projections) - rust-lang#120702 (docs: also check the inline stmt during redundant link check) - rust-lang#120739 (improve pretty printing for associated items in trait objects) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 8, 2024
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore) - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation) - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages) - rust-lang#120688 (GVN: also turn moves into copies with projections) - rust-lang#120702 (docs: also check the inline stmt during redundant link check) - rust-lang#120739 (improve pretty printing for associated items in trait objects) r? `@ghost` `@rustbot` modify labels: rollup
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 8, 2024
…rors resolve: Unload speculatively resolved crates before freezing cstore Name resolution sometimes loads additional crates to improve diagnostics (e.g. suggest imports). Not all of these diagnostics result in errors, sometimes they are just warnings, like in rust-lang#117772. If additional crates loaded speculatively stay and gets listed by things like `query crates` then they may produce further errors like duplicated lang items, because lang items from speculatively loaded crates are as good as from non-speculatively loaded crates. They can probably do things like adding unintended impls from speculatively loaded crates to method resolution as well. The extra crates will also get into the crate's metadata as legitimate dependencies. In this PR I remove the speculative crates from cstore when name resolution is finished and cstore is frozen. This is better than e.g. filtering away speculative crates in `query crates` because things like `DefId`s referring to these crates and leaking to later compilation stages can produce ICEs much easier, allowing to detect them. The unloading could potentially be skipped if any errors were reported (to allow using `DefId`s from speculatively loaded crates for recovery), but I didn't do it in this PR because I haven't seen such cases of recovery. We can reconsider later if any relevant ICEs are reported. Unblocks rust-lang#117772.
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 8, 2024
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore) - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation) - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120596 ([rustdoc] Correctly generate path for non-local items in source code pages) - rust-lang#120688 (GVN: also turn moves into copies with projections) - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered") - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.) - rust-lang#120739 (improve pretty printing for associated items in trait objects) r? `@ghost` `@rustbot` modify labels: rollup
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 8, 2024
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore) - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation) - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120688 (GVN: also turn moves into copies with projections) - rust-lang#120702 (docs: also check the inline stmt during redundant link check) - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered") - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.) - rust-lang#120739 (improve pretty printing for associated items in trait objects) r? `@ghost` `@rustbot` modify labels: rollup
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Feb 8, 2024
Rollup merge of rust-lang#119592 - petrochenkov:unload, r=compiler-errors resolve: Unload speculatively resolved crates before freezing cstore Name resolution sometimes loads additional crates to improve diagnostics (e.g. suggest imports). Not all of these diagnostics result in errors, sometimes they are just warnings, like in rust-lang#117772. If additional crates loaded speculatively stay and gets listed by things like `query crates` then they may produce further errors like duplicated lang items, because lang items from speculatively loaded crates are as good as from non-speculatively loaded crates. They can probably do things like adding unintended impls from speculatively loaded crates to method resolution as well. The extra crates will also get into the crate's metadata as legitimate dependencies. In this PR I remove the speculative crates from cstore when name resolution is finished and cstore is frozen. This is better than e.g. filtering away speculative crates in `query crates` because things like `DefId`s referring to these crates and leaking to later compilation stages can produce ICEs much easier, allowing to detect them. The unloading could potentially be skipped if any errors were reported (to allow using `DefId`s from speculatively loaded crates for recovery), but I didn't do it in this PR because I haven't seen such cases of recovery. We can reconsider later if any relevant ICEs are reported. Unblocks rust-lang#117772.
flip1995
pushed a commit
to flip1995/rust
that referenced
this pull request
Feb 26, 2024
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore) - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation) - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s) - rust-lang#120214 (match lowering: consistently lower bindings deepest-first) - rust-lang#120688 (GVN: also turn moves into copies with projections) - rust-lang#120702 (docs: also check the inline stmt during redundant link check) - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered") - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.) - rust-lang#120739 (improve pretty printing for associated items in trait objects) r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Name resolution sometimes loads additional crates to improve diagnostics (e.g. suggest imports).
Not all of these diagnostics result in errors, sometimes they are just warnings, like in #117772.
If additional crates loaded speculatively stay and gets listed by things like
query cratesthen they may produce further errors like duplicated lang items, because lang items from speculatively loaded crates are as good as from non-speculatively loaded crates.They can probably do things like adding unintended impls from speculatively loaded crates to method resolution as well.
The extra crates will also get into the crate's metadata as legitimate dependencies.
In this PR I remove the speculative crates from cstore when name resolution is finished and cstore is frozen.
This is better than e.g. filtering away speculative crates in
query cratesbecause things likeDefIds referring to these crates and leaking to later compilation stages can produce ICEs much easier, allowing to detect them.The unloading could potentially be skipped if any errors were reported (to allow using
DefIds from speculatively loaded crates for recovery), but I didn't do it in this PR because I haven't seen such cases of recovery. We can reconsider later if any relevant ICEs are reported.Unblocks #117772.