Skip to content

Extend QueryStability to handle IntoIterator implementations #139345

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 2 additions & 2 deletions compiler/rustc_errors/src/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use std::path::Path;
use std::sync::Arc;

use derive_setters::Setters;
use rustc_data_structures::fx::{FxHashMap, FxIndexMap, FxIndexSet};
use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
use rustc_data_structures::sync::{DynSend, IntoDynSyncSend};
use rustc_error_messages::{FluentArgs, SpanLabel};
use rustc_lexer;
Expand Down Expand Up @@ -1840,7 +1840,7 @@ impl HumanEmitter {
!is_cont && line_idx + 1 == annotated_file.lines.len(),
);

let mut to_add = FxHashMap::default();
let mut to_add = FxIndexMap::default();

for (depth, style) in depths {
// FIXME(#120456) - is `swap_remove` correct?
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_interface/src/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,9 @@ pub(crate) fn parse_check_cfg(dcx: DiagCtxtHandle<'_>, specs: Vec<String>) -> Ch
.expecteds
.entry(name.name)
.and_modify(|v| match v {
ExpectedValues::Some(v) if !values_any_specified => {
ExpectedValues::Some(v) if !values_any_specified =>
{
#[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.

I'm torn between comment vs. no comment. I've tracked down all uses of ExpectedValues::Some (etc.) and have learnt that we sort unstably (by &str) everywhere before outputting the list, so all is well. Even without that context, this potential instability is transitive anyway, so a comment is probably superfluous.

(At some point in the future I might experiment with using UnordMap for psess.check_cfg.expecteds and UnordSet for ExpectedValues::Some leveraging the fact that Symbol impls ToStableHashKey1 allowing us to use to_sorted which would arguably make it easier to see that there's no risk of instability)

Footnotes

  1. Well, it's KeyType is String, not <'call> &'call str which may impact performance, so we might need to GAT-ify KeyType to type KeyType<'a>: …; and so on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm torn between comment vs. no comment.

The comment would be "FIXME" or something like that?

It looks like ExpectedValues could be made to contain an FxIndexMap (and I don't know why I didn't make that change). Should I just make that change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why I didn't make that change

I think I was trying to change only the data structure I had initially set out out to.

v.extend(values.clone())
}
ExpectedValues::Some(_) => *v = ExpectedValues::Any,
Expand Down
168 changes: 123 additions & 45 deletions compiler/rustc_lint/src/internal.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
//! Some lints that are only useful in the compiler or crates that use compiler internals, such as
//! Clippy.

use rustc_hir::HirId;
use rustc_hir::def::Res;
use rustc_hir::def_id::DefId;
use rustc_middle::ty::{self, GenericArgsRef, Ty as MiddleTy};
use rustc_hir::{Expr, ExprKind, HirId};
use rustc_middle::ty::{
self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty as MiddleTy,
};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::hygiene::{ExpnKind, MacroKind};
use rustc_span::{Span, sym};
Expand Down Expand Up @@ -56,25 +58,6 @@ impl LateLintPass<'_> for DefaultHashTypes {
}
}

/// Helper function for lints that check for expressions with calls and use typeck results to
/// get the `DefId` and `GenericArgsRef` of the function.
fn typeck_results_of_method_fn<'tcx>(
cx: &LateContext<'tcx>,
expr: &hir::Expr<'_>,
) -> Option<(Span, DefId, ty::GenericArgsRef<'tcx>)> {
match expr.kind {
hir::ExprKind::MethodCall(segment, ..)
if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) =>
{
Some((segment.ident.span, def_id, cx.typeck_results().node_args(expr.hir_id)))
}
_ => match cx.typeck_results().node_type(expr.hir_id).kind() {
&ty::FnDef(def_id, args) => Some((expr.span, def_id, args)),
_ => None,
},
}
}

declare_tool_lint! {
/// The `potential_query_instability` lint detects use of methods which can lead to
/// potential query instability, such as iterating over a `HashMap`.
Expand All @@ -101,10 +84,12 @@ declare_tool_lint! {

declare_lint_pass!(QueryStability => [POTENTIAL_QUERY_INSTABILITY, UNTRACKED_QUERY_INFORMATION]);

impl LateLintPass<'_> for QueryStability {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) {
let Some((span, def_id, args)) = typeck_results_of_method_fn(cx, expr) else { return };
if let Ok(Some(instance)) = ty::Instance::try_resolve(cx.tcx, cx.typing_env(), def_id, args)
impl<'tcx> LateLintPass<'tcx> for QueryStability {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
if let Some((def_id, span, generic_args, _recv, _args)) =
get_callee_span_generic_args_and_args(cx, expr)
&& let Ok(Some(instance)) =
ty::Instance::try_resolve(cx.tcx, cx.typing_env(), def_id, generic_args)
{
let def_id = instance.def_id();
if cx.tcx.has_attr(def_id, sym::rustc_lint_query_instability) {
Expand All @@ -122,9 +107,113 @@ impl LateLintPass<'_> for QueryStability {
);
}
}
check_into_iter_stability(cx, expr);
}
}

fn check_into_iter_stability<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
let Some(into_iterator_def_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) else {
Copy link
Member

Choose a reason for hiding this comment

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

The base/existing impl never hard-codes any specific items and looks anything labeled #[rustc_lint_query_instability]. It would be very nice if we could continue doing so 🤔

We could of course mark trait IntoIterator in the stdlib with #[cfg_attr(not(boostrap), rustc_lint_query_instability)] and then look for any traits with this attr but I guess the rest of the code is quite tailored to IntoIterator..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and then look for any traits with this attr

To be clear, this would be an enhancement to the existing rustc_lint_query_instability detection logic?

I could make this change if you think it would be best.

return;
};
let Some(into_iter_fn_def_id) = cx.tcx.lang_items().into_iter_fn() else {
return;
};
if expr.span.from_expansion() {
Copy link
Member

@fmease fmease May 27, 2025

Choose a reason for hiding this comment

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

This suppresses the lint from firing for code like this?

fn iter<T>(x: impl IntoIterator<Item = T>) = impl Iterator<Item = T> { x.into_iter() }
macro_rules! iter { ($e:expr) => { iter($e) } }
fn take(map: std::collections::HashMap<i32, i32>) { _ = iter!(map); }

I think we should fire regardless. Internal lints can be a lot more aggressive than Clippy lints. There's a reason why rustc::potential_query_instability is marked report_in_external_macro: true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without that condition, an additional warning is produced here:

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

+       error: using `into_iter` can result in unstable query results
+         --> $DIR/query_stability.rs:22:14
+          |
+       LL |     for _ in x {}
+          |              ^
+          |
+          = note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

Not linting expanded code seemed like the most straightforward way of avoiding the duplicate warnings.

Would you prefer that the lint check the context in which the expression appears, e.g., something along these lines? https://doc.rust-lang.org/beta/nightly-rustc/src/clippy_utils/higher.rs.html#34-54

return;
};
// Is `expr` a function or method call?
let Some((callee_def_id, _span, generic_args, recv, args)) =
get_callee_span_generic_args_and_args(cx, expr)
else {
return;
};
let fn_sig = cx.tcx.fn_sig(callee_def_id).instantiate_identity().skip_binder();
for (arg_index, &input) in fn_sig.inputs().iter().enumerate() {
Copy link
Member

@fmease fmease May 27, 2025

Choose a reason for hiding this comment

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

In general, this implementations seems overly complex to me for what it's checking. You're basically re-implementing a very limited ad-hoc trait solver:

  1. going through all predicates in the param env,
  2. eliminating uneligible candidates,
  3. going through all impls,
  4. eliminating impls where the self ty doesn't match (based on Ty's == instead of normalizing + performing semantic type relating via type unification)

and all of that in ~O(n^3) but n is prolly small so idk

I wonder we could utilize some existing trait solving methods 🤔 I need to think about it. Sorry, it's not very actionable I g2g

Copy link
Contributor Author

@smoelius smoelius May 27, 2025

Choose a reason for hiding this comment

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

normalizing + performing semantic type relating via type unification

Do you have an example of something you think this code should resemble?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right: the lint has a much simpler implementation using Instance::try_resolve. I have verified that the new implementation necessitates all of the changes the previous one did.

let &ty::Param(ParamTy { index: param_index, .. }) = input.kind() else {
Copy link
Member

Choose a reason for hiding this comment

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

Right, this has a bunch of false negatives. E.g., it won't consider Option<T>, (impl IntoIterator,) etc. but I guess it's still better than the status quo.

You could create a custom TypeVisitor to find all containing type params in a Ty but that's probably overkill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to ignore this unless you tell me to act on it.

continue;
};
let (trait_predicates, _) = get_input_traits_and_projections(cx, callee_def_id, input);
for TraitPredicate { trait_ref, .. } in trait_predicates {
// Does the function or method require any of its arguments to implement `IntoIterator`?
if trait_ref.def_id != into_iterator_def_id {
continue;
}
let self_ty_generic_arg = generic_args[param_index as usize];
let Ok(Some(instance)) = ty::Instance::try_resolve(
cx.tcx,
cx.typing_env(),
into_iter_fn_def_id,
cx.tcx.mk_args(&[self_ty_generic_arg]),
) else {
continue;
};
// Does the input type's `IntoIterator` implementation have the
// `rustc_lint_query_instability` attribute on its `into_iter` method?
if !cx.tcx.has_attr(instance.def_id(), sym::rustc_lint_query_instability) {
return;
}
let span = if let Some(recv) = recv {
if arg_index == 0 { recv.span } else { args[arg_index - 1].span }
} else {
args[arg_index].span
};
cx.emit_span_lint(
POTENTIAL_QUERY_INSTABILITY,
span,
QueryInstability { query: cx.tcx.item_name(instance.def_id()) },
);
}
}
}

/// Checks whether an expression is a function or method call and, if so, returns its `DefId`,
/// `Span`, `GenericArgs`, and arguments. This is a slight augmentation of a similarly named Clippy
/// function, `get_callee_generic_args_and_args`.
fn get_callee_span_generic_args_and_args<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'tcx>,
) -> Option<(DefId, Span, GenericArgsRef<'tcx>, Option<&'tcx Expr<'tcx>>, &'tcx [Expr<'tcx>])> {
if let ExprKind::Call(callee, args) = expr.kind
&& let callee_ty = cx.typeck_results().expr_ty(callee)
&& let ty::FnDef(callee_def_id, generic_args) = callee_ty.kind()
{
return Some((*callee_def_id, callee.span, generic_args, None, args));
}
if let ExprKind::MethodCall(segment, recv, args, _) = expr.kind
&& let Some(method_def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
{
let generic_args = cx.typeck_results().node_args(expr.hir_id);
return Some((method_def_id, segment.ident.span, generic_args, Some(recv), args));
}
None
}

/// Returns the `TraitPredicate`s and `ProjectionPredicate`s for a function's input type.
fn get_input_traits_and_projections<'tcx>(
cx: &LateContext<'tcx>,
callee_def_id: DefId,
input: MiddleTy<'tcx>,
) -> (Vec<TraitPredicate<'tcx>>, Vec<ProjectionPredicate<'tcx>>) {
let mut trait_predicates = Vec::new();
let mut projection_predicates = Vec::new();
for predicate in cx.tcx.param_env(callee_def_id).caller_bounds() {
match predicate.kind().skip_binder() {
ClauseKind::Trait(trait_predicate) => {
if trait_predicate.trait_ref.self_ty() == input {
trait_predicates.push(trait_predicate);
}
}
ClauseKind::Projection(projection_predicate) => {
if projection_predicate.projection_term.self_ty() == input {
projection_predicates.push(projection_predicate);
}
}
_ => {}
}
}
(trait_predicates, projection_predicates)
}

declare_tool_lint! {
/// The `usage_of_ty_tykind` lint detects usages of `ty::TyKind::<kind>`,
/// where `ty::<kind>` would suffice.
Expand Down Expand Up @@ -435,33 +524,22 @@ declare_tool_lint! {
declare_lint_pass!(Diagnostics => [UNTRANSLATABLE_DIAGNOSTIC, DIAGNOSTIC_OUTSIDE_OF_IMPL]);

impl LateLintPass<'_> for Diagnostics {
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &hir::Expr<'_>) {
fn check_expr<'tcx>(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) {
let collect_args_tys_and_spans = |args: &[hir::Expr<'_>], reserve_one_extra: bool| {
let mut result = Vec::with_capacity(args.len() + usize::from(reserve_one_extra));
result.extend(args.iter().map(|arg| (cx.typeck_results().expr_ty(arg), arg.span)));
result
};
// Only check function calls and method calls.
let (span, def_id, fn_gen_args, arg_tys_and_spans) = match expr.kind {
hir::ExprKind::Call(callee, args) => {
match cx.typeck_results().node_type(callee.hir_id).kind() {
&ty::FnDef(def_id, fn_gen_args) => {
(callee.span, def_id, fn_gen_args, collect_args_tys_and_spans(args, false))
}
_ => return, // occurs for fns passed as args
}
}
hir::ExprKind::MethodCall(_segment, _recv, args, _span) => {
let Some((span, def_id, fn_gen_args)) = typeck_results_of_method_fn(cx, expr)
else {
return;
};
let mut args = collect_args_tys_and_spans(args, true);
args.insert(0, (cx.tcx.types.self_param, _recv.span)); // dummy inserted for `self`
(span, def_id, fn_gen_args, args)
}
_ => return,
let Some((def_id, span, fn_gen_args, recv, args)) =
get_callee_span_generic_args_and_args(cx, expr)
else {
return;
};
let mut arg_tys_and_spans = collect_args_tys_and_spans(args, recv.is_some());
if let Some(recv) = recv {
arg_tys_and_spans.insert(0, (cx.tcx.types.self_param, recv.span)); // dummy inserted for `self`
}

Self::diagnostic_outside_of_impl(cx, span, expr.hir_id, def_id, fn_gen_args);
Self::untranslatable_diagnostic(cx, def_id, &arg_tys_and_spans);
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_resolve/src/late.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3934,12 +3934,14 @@ impl<'a, 'ast, 'ra: 'ast, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
// Move up the non-overlapping bindings to the or-pattern.
// Existing bindings just get "merged".
let collected = bindings.pop().unwrap().1;
#[allow(rustc::potential_query_instability)] // FIXME
bindings.last_mut().unwrap().1.extend(collected);
}
// This or-pattern itself can itself be part of a product,
// e.g. `(V1(a) | V2(a), a)` or `(a, V1(a) | V2(a))`.
// Both cases bind `a` again in a product pattern and must be rejected.
let collected = bindings.pop().unwrap().1;
#[allow(rustc::potential_query_instability)] // FIXME
bindings.last_mut().unwrap().1.extend(collected);

// Prevent visiting `ps` as we've already done so above.
Expand Down
2 changes: 1 addition & 1 deletion src/librustdoc/formats/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ pub(crate) struct Cache {

/// Similar to `paths`, but only holds external paths. This is only used for
/// generating explicit hyperlinks to other crates.
pub(crate) external_paths: FxHashMap<DefId, (Vec<Symbol>, ItemType)>,
pub(crate) external_paths: FxIndexMap<DefId, (Vec<Symbol>, ItemType)>,

/// Maps local `DefId`s of exported types to fully qualified paths.
/// Unlike 'paths', this mapping ignores any renames that occur
Expand Down
12 changes: 12 additions & 0 deletions tests/ui-fulldeps/internal-lints/query_stability.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,16 @@ fn main() {
//~^ ERROR using `values_mut` can result in unstable query results
*val = *val + 10;
}

FxHashMap::<u32, i32>::default().extend(x);
//~^ ERROR using `into_iter` can result in unstable query results
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a test like

fn hide_into_iter<T>(x: impl IntoIterator<Item = T>) = impl Iterator<Item = T> { x.into_iter() }
fn take(map: std::collections::HashMap<i32, i32>) { _ = hide_into_iter(map); }

Copy link
Contributor Author

@smoelius smoelius May 28, 2025

Choose a reason for hiding this comment

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

Done in commit ae437dd.

It adds the following warning. Is this what you were expecting?

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

}

fn hide_into_iter<T>(x: impl IntoIterator<Item = T>) -> impl Iterator<Item = T> {
x.into_iter()
}

fn take(map: std::collections::HashMap<i32, i32>) {
_ = hide_into_iter(map);
//~^ ERROR using `into_iter` can result in unstable query results
}
18 changes: 17 additions & 1 deletion tests/ui-fulldeps/internal-lints/query_stability.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,21 @@ 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
error: using `into_iter` can result in unstable query results
--> $DIR/query_stability.rs:38:45
|
LL | FxHashMap::<u32, i32>::default().extend(x);
| ^
|
= note: if you believe this case to be fine, allow this lint and add a comment explaining your rationale

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

error: aborting due to 9 previous errors

Loading