-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Audit and handle remaining cases of rustc::potential_query_instability
lint (iterating HashMap
s)
#84447
Comments
#64131 added the |
The lint exists since #89558, and has allowed on all compiler crates as Steps, for each crate except
Please reach out on zulip if there are any questions. |
It seems like no one is assigned to this and I'd like to give it a try so assigning myself. @rustbot claim |
…etrochenkov remove allow(rustc::potential_query_instability) in rustc_span Also, avoid sorting before debug output as iteration order can now be relied upon. Related rust-lang#84447
…ompiler-errors rustc_expand: Switch FxHashMap to FxIndexMap where iteration is used Relates rust-lang#84447
I was hoping to get back to this after vacation but I haven't had the time and I likely won't in the near future. I've converted some of the modules to use the stable hash containers but there is a decent amount left so it is probably possible to have multiple people working on this, assuming some coordination. There are two outstanding PRs from me that I will finish up. The first is #99334 and seems ready to merge. Second one is #99065 which requires the @rustbot release-assignment |
…oli-obk rustc_error, rustc_private: Switch to stable hash containers Relates rust-lang#84447
rustc_error, rustc_private: Switch to stable hash containers Relates rust-lang/rust#84447
I'm gonna give this a try |
Rollup merge of rust-lang#118865 - Enselic:rustc_codegen_llvm-lint-fix, r=petrochenkov rustc_codegen_llvm: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of `rustc_codegen_llvm` and instead allow it on a case-by-case basis if it is safe to do so. In this case, all 2 instances are safe to allow. Part of rust-lang#84447 which is E-help-wanted.
…ty, r=compiler-errors rustc_passes: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` in all of `rustc_passes` and instead allow it on a case-by-case basis if it is safe. In this case, all instances of the lint are safe to allow. Part of rust-lang#84447 which is E-help-wanted.
…, r=compiler-errors rustc_passes: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` in all of `rustc_passes` and instead allow it on a case-by-case basis if it is safe. In this case, all instances of the lint are safe to allow. Part of rust-lang#84447 which is E-help-wanted.
…ler-errors rustc_passes: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` in all of `rustc_passes` and instead allow it on a case-by-case basis if it is safe. In this case, all instances of the lint are safe to allow. Part of rust-lang/rust#84447 which is E-help-wanted.
…ility, r=michaelwoerister rustc_mir_build: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of `rustc_mir_build` and instead allow it on a case-by-case basis if it is safe to do so. In this crate there was only one instance of the lint, and it was safe to allow. Part of rust-lang#84447 which is E-help-wanted.
Rollup merge of rust-lang#118863 - Enselic:rustc_mir-build-query-stability, r=michaelwoerister rustc_mir_build: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of `rustc_mir_build` and instead allow it on a case-by-case basis if it is safe to do so. In this crate there was only one instance of the lint, and it was safe to allow. Part of rust-lang#84447 which is E-help-wanted.
…r=cjgillot rustc_lint: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of `rustc_lint` and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all lints were safe to allow. Part of rust-lang#84447 which is E-help-wanted.
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? ```@cjgillot``` who is reviewer for the highly related PR rust-lang#119252.
…ble, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
Rollup merge of rust-lang#119591 - Enselic:DestinationPropagation-stable, r=cjgillot rustc_mir_transform: Make DestinationPropagation stable for queries By using `FxIndexMap` instead of `FxHashMap`, so that the order of visiting of locals is deterministic. We also need to bless `copy_propagation_arg.foo.DestinationPropagation.panic*.diff`. Do not review the diff of the diff. Instead look at the diff files before and after this commit. Both before and after this commit, 3 statements are replaced with nop. It's just that due to change in ordering, different statements are replaced. But the net result is the same. In other words, compare this diff (before fix): * https://github.com/rust-lang/rust/blob/090d5eac722000906cc00d991f2bf052b0e388c3/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff With this diff (after fix): * https://github.com/rust-lang/rust/blob/f603babd63a607e155609dc0277806e559626ea0/tests/mir-opt/dest-prop/copy_propagation_arg.foo.DestinationPropagation.panic-unwind.diff and you can see that both before and after the fix, we replace 3 statements with `nop`s. I find it _slightly_ surprising that the test this PR affects did not previously fail spuriously due to the indeterminism of `FxHashMap`, but I guess in can be explained with the predictability of small `FxHashMap`s with `usize` (`Local`) keys, or something along those lines. This should fix [this](rust-lang#119252 (comment)) comment, but I wanted to make a separate PR for this fix for a simpler development and review process. Part of rust-lang#84447 which is E-help-wanted. r? `@cjgillot` who is reviewer for the highly related PR rust-lang#119252.
…stability, r=cjgillot rustc_mir_transform: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of rustc_mir_transform and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all instances were safe to allow. Part of rust-lang#84447 which is E-help-wanted.
Rollup merge of rust-lang#119252 - Enselic:rustc_mir_transform-query-stability, r=cjgillot rustc_mir_transform: Enforce `rustc::potential_query_instability` lint Stop allowing `rustc::potential_query_instability` on all of rustc_mir_transform and instead allow it on a case-by-case basis if it is safe to do so. In this particular crate, all instances were safe to allow. Part of rust-lang#84447 which is E-help-wanted.
…bility, r=Nilstrieb rustc_codegen_ssa: Enforce `rustc::potential_query_instability` lint Part of rust-lang#84447.
…=Nilstrieb rustc_codegen_ssa: Enforce `rustc::potential_query_instability` lint Part of rust-lang/rust#84447.
…=Nilstrieb rustc_codegen_ssa: Enforce `rustc::potential_query_instability` lint Part of rust-lang/rust#84447.
…=Nilstrieb rustc_codegen_ssa: Enforce `rustc::potential_query_instability` lint Part of rust-lang/rust#84447.
There may be more need to clean up, for anyone maybe interested can refer to #120485 |
… r=<try> Stabilize order of MonoItems in CGUs and disallow query_instability lint for rustc_monomorphize The HashStable impl for `CodegenUnit` was incorrect as described in [MCP 533](rust-lang/compiler-team#533). This PR removes any indeterminism from the way codegen units are built. The changes are pretty straightforward. Part of rust-lang#84447 and [MCP 533](rust-lang/compiler-team#533).
… r=oli-obk Stabilize order of MonoItems in CGUs and disallow query_instability lint for rustc_monomorphize The HashStable impl for `CodegenUnit` was incorrect as described in [MCP 533](rust-lang/compiler-team#533). This PR removes any indeterminism from the way codegen units are built. The changes are pretty straightforward. Part of rust-lang#84447 and [MCP 533](rust-lang/compiler-team#533).
Stabilize order of MonoItems in CGUs and disallow query_instability lint for rustc_monomorphize The HashStable impl for `CodegenUnit` was incorrect as described in [MCP 533](rust-lang/compiler-team#533). This PR removes any indeterminism from the way codegen units are built. The changes are pretty straightforward. Part of rust-lang/rust#84447 and [MCP 533](rust-lang/compiler-team#533).
Stabilize order of MonoItems in CGUs and disallow query_instability lint for rustc_monomorphize The HashStable impl for `CodegenUnit` was incorrect as described in [MCP 533](rust-lang/compiler-team#533). This PR removes any indeterminism from the way codegen units are built. The changes are pretty straightforward. Part of rust-lang/rust#84447 and [MCP 533](rust-lang/compiler-team#533).
The iteration order of a
HashMap
depends on theHash
value of the keys. In rustc, theHash
value of a key can change between compilation sessions, even if theHashStable
value remains the same (e.g.DefId
,Symbol
, etc.). This means that iterating over aHashMap
can cause a query to produce different results given the same (as determined byHashStable
) input. This can be quite subtle, as demonstrated by #82272 (comment)To eliminate this source of issues, we should make an internal rustc-only lint for iterating over a
HashMap
in any way. This will need to cover explicit method calls (e.g..keys()
and.values()
), as well as any usage of theIntoIterator
impl forHashMap
.Note that we don't want to ban the usage of
HashMap
s with unstable keys - as long as they're only used for lookup (and not iteration), it's much more efficient than repeatedly converting to the stable form of a key (e.g.DefId
->DefPathHash
orSymbol
->String
).The text was updated successfully, but these errors were encountered: