Skip to content
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

add missing potential_query_instability for keys and values in hashmap #120485

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_codegen_llvm/src/coverageinfo/mapgen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ fn codegenned_and_inlined_items(tcx: TyCtxt<'_>) -> DefIdSet {
let mut result = items.clone();

for cgu in cgus {
#[allow(rustc::potential_query_instability)]
Copy link
Member

Choose a reason for hiding this comment

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

Can you try turning the items field in CodegenUnit into an FxIndexMap. That should make iteration stable (if the collection is built in a deterministic order). An UnordMap would be even better, but I suspect that that would be more work.

for item in cgu.items().keys() {
if let mir::mono::MonoItem::Fn(ref instance) = item {
let did = instance.def_id();
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/assert_module_sources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ impl CguReuseTracker {

fn check_expected_reuse(&self, sess: &Session) {
if let Some(ref data) = self.data {
#[allow(rustc::potential_query_instability)]
let mut keys = data.expected_reuse.keys().collect::<Vec<_>>();
Copy link
Member

Choose a reason for hiding this comment

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

We can fix this properly by using UnordMap instead of FxHashMap for the actual_reuse and expected_reuse fields in TrackerData. That has the to_sorted_stable_ord method that does exactly what we want.

keys.sort_unstable();
for cgu_name in keys {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/back/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,7 @@ fn link_dwarf_object<'a>(
}

// Input rlibs contain .o/.dwo files from dependencies.
#[allow(rustc::potential_query_instability)]
let input_rlibs = cg_results
.crate_info
.used_crate_source
Copy link
Member

Choose a reason for hiding this comment

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

Can you turn the used_crate_source field into an UnordMap? Then you should be able to do this:

// Input rlibs contain .o/.dwo files from dependencies.
let input_rlibs = cg_results
    .crate_info
    .used_crate_source
    .items()
    .filter_map(|(_, csource)| csource.rlib.as_ref())
    .map(|(path, _)| path)
    .into_sorted_stable_ord();

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1950,6 +1950,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
) {
let len = remaining_fields.len();

#[allow(rustc::potential_query_instability)]
Copy link
Member

Choose a reason for hiding this comment

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

It should be straightforward to make remaining_fields and UnordMap.

let mut displayable_field_names: Vec<&str> =
remaining_fields.keys().map(|ident| ident.as_str()).collect();
// sorting &str primitives here, sort_unstable is ok
Expand Down
14 changes: 8 additions & 6 deletions compiler/rustc_lint/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,11 +326,9 @@ impl LintStore {

/// True if this symbol represents a lint group name.
pub fn is_lint_group(&self, lint_name: Symbol) -> bool {
debug!(
"is_lint_group(lint_name={:?}, lint_groups={:?})",
lint_name,
self.lint_groups.keys().collect::<Vec<_>>()
);
#[allow(rustc::potential_query_instability)]
let lint_groups = self.lint_groups.keys().collect::<Vec<_>>();
debug!("is_lint_group(lint_name={:?}, lint_groups={:?})", lint_name, lint_groups);
Comment on lines -329 to +331
Copy link
Contributor

@klensy klensy Jan 30, 2024

Choose a reason for hiding this comment

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

Will collect things on release now, instead of debug only, next one too. cfg(debug_assertions)?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, I will clean this up when I replace HashMap.

let lint_name_str = lint_name.as_str();
self.lint_groups.contains_key(lint_name_str) || {
let warnings_name_str = crate::WARNINGS.name_lower();
Expand Down Expand Up @@ -374,8 +372,12 @@ impl LintStore {
None => {
// 1. The tool is currently running, so this lint really doesn't exist.
// FIXME: should this handle tools that never register a lint, like rustfmt?
debug!("lints={:?}", self.by_name.keys().collect::<Vec<_>>());
#[allow(rustc::potential_query_instability)]
let lints = self.by_name.keys().collect::<Vec<_>>();
debug!("lints={:?}", lints);
let tool_prefix = format!("{tool_name}::");

#[allow(rustc::potential_query_instability)]
return if self.by_name.keys().any(|lint| lint.starts_with(&tool_prefix)) {
self.no_lint_suggestion(&complete_name, tool_name.as_str())
} else {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_lint/src/context/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ pub(super) fn builtin(
db.note("see the asm section of Rust By Example <https://doc.rust-lang.org/nightly/rust-by-example/unsafe/asm.html#labels> for more information");
}
BuiltinLintDiagnostics::UnexpectedCfgName((name, name_span), value) => {
#[allow(rustc::potential_query_instability)]
let possibilities: Vec<Symbol> =
sess.parse_sess.check_config.expecteds.keys().copied().collect();
let is_from_cargo = std::env::var_os("CARGO").is_some();
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_trait_selection/src/traits/auto_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ impl<'tcx> AutoTraitFinder<'tcx> {
}

while !vid_map.is_empty() {
#[allow(rustc::potential_query_instability)]
let target = *vid_map.keys().next().expect("Keys somehow empty");
let deps = vid_map.remove(&target).expect("Entry somehow missing");

Expand Down
3 changes: 3 additions & 0 deletions library/std/src/collections/hash/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ impl<K, V, S> HashMap<K, V, S> {
///
/// In the current implementation, iterating over keys takes O(capacity) time
/// instead of O(len) because it internally visits empty buckets too.
#[rustc_lint_query_instability]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn keys(&self) -> Keys<'_, K, V> {
Keys { inner: self.iter() }
Expand Down Expand Up @@ -417,6 +418,7 @@ impl<K, V, S> HashMap<K, V, S> {
///
/// In the current implementation, iterating over values takes O(capacity) time
/// instead of O(len) because it internally visits empty buckets too.
#[rustc_lint_query_instability]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn values(&self) -> Values<'_, K, V> {
Values { inner: self.iter() }
Expand Down Expand Up @@ -449,6 +451,7 @@ impl<K, V, S> HashMap<K, V, S> {
///
/// In the current implementation, iterating over values takes O(capacity) time
/// instead of O(len) because it internally visits empty buckets too.
#[rustc_lint_query_instability]
#[stable(feature = "map_values_mut", since = "1.10.0")]
pub fn values_mut(&mut self) -> ValuesMut<'_, K, V> {
ValuesMut { inner: self.iter_mut() }
Expand Down
13 changes: 13 additions & 0 deletions tests/ui-fulldeps/internal-lints/query_stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,17 @@ fn main() {

for _ in x {}
//~^ ERROR using `into_iter`

let x = FxHashMap::<u32, i32>::default();
let _ = x.keys();
//~^ ERROR using `keys` can result in unstable query results

let _ = x.values();
//~^ ERROR using `values` can result in unstable query results

let mut x = FxHashMap::<u32, i32>::default();
for val in x.values_mut() {
//~^ ERROR using `values_mut` can result in unstable query results
*val = *val + 10;
}
}
26 changes: 25 additions & 1 deletion tests/ui-fulldeps/internal-lints/query_stability.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,29 @@ LL | for _ in x {}
|
= note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: aborting due to 4 previous errors
error: using `keys` can result in unstable query results
--> $DIR/query_stability.rs:26:15
|
LL | let _ = x.keys();
| ^^^^
|
= note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `values` can result in unstable query results
--> $DIR/query_stability.rs:29:15
|
LL | let _ = x.values();
| ^^^^^^
|
= note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: using `values_mut` can result in unstable query results
--> $DIR/query_stability.rs:33:18
|
LL | for val in x.values_mut() {
| ^^^^^^^^^^
|
= note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

error: aborting due to 7 previous errors

Loading