Extend `QueryStability` to handle `IntoIterator` implementations #139345

smoelius · 2025-04-04T01:48:59Z

This PR extends the rustc::potential_query_instability lint to check values passed as IntoIterator implementations.

Full disclosure: I want the lint to warn about this line (please see #138871 for why):

Line 261 in aa8f0fd

.chain(&self.cache.external_paths)

However, the lint warns about several other lines as well.

Final note: the functions get_callee_generic_args_and_args and get_input_traits_and_projections were copied directly from Clippy's source code.

rustbot · 2025-04-04T01:49:03Z

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

smoelius · 2025-04-04T10:35:45Z

Apologies, I was not aware of the tests/ui-fulldeps/internal-lints/query_stability.rs test. I will refactor my changes to eliminate the duplicate warning.

But in your opinion, @fmease, should I keep the tests/ui/internal-lints/query_stability_into_iter.rs test?

fmease · 2025-04-04T10:54:11Z

I think it would make sense to merge tests/ui/internal-lints/query_stability_into_iter.rs into the preexisting tests/ui-fulldeps/internal-lints/query_stability.rs (and make your new test case use FxHash{Map,Set}).

Note that tests/ui-fulldeps/ tests require a stage2 compiler which takes extra compute power and time to build locally. Just as a heads up

smoelius · 2025-04-04T21:28:05Z

I think this is good to go, @fmease. Thank you for your suggestions.

EDIT: Nits are welcome, BTW.

smoelius · 2025-04-30T11:52:00Z

dfec3c0 is a small simplification.

@fmease Please let me know if you have any questions, or if I can do anything to make this PR easier to review.

smoelius · 2025-05-27T09:27:14Z

@fmease Are there any questions I could answer about this PR?

fmease

Sorry for the delay. I've had an initial scan. Not all of my comments are actionable unfortunately, I'll try to come back to it later.

fmease · 2025-05-27T11:51:25Z

compiler/rustc_interface/src/interface.rs

-                        ExpectedValues::Some(v) if !values_any_specified => {
+                        ExpectedValues::Some(v) if !values_any_specified =>
+                        {
+                            #[allow(rustc::potential_query_instability)]


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 ToStableHashKey¹ allowing us to use to_sorted which would arguably make it easier to see that there's no risk of instability)

Footnotes

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 ↩

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?

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.

fmease · 2025-05-27T11:58:56Z

compiler/rustc_lint/src/internal.rs

+}
+
+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 {


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..

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.

fmease · 2025-05-27T12:03:07Z

compiler/rustc_lint/src/internal.rs

+    let Some(into_iterator_def_id) = cx.tcx.get_diagnostic_item(sym::IntoIterator) else {
+        return;
+    };
+    if expr.span.from_expansion() {


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.

Without that condition, an additional warning is produced here:

rust/tests/ui-fulldeps/internal-lints/query_stability.rs

Lines 22 to 23 in dfec3c0

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

I think we should lint in expansions (generally), however, we could avoid looking for IntoITerator trait builds in case the lint already fired for the method call itself. i.e. if wee lint because we resolved to a method with the attribute, don't check whether that method also has a relevant IntoIterator bound

also, i think we can merge this lint into one

if let Some((callee_def_id, span, generic_args, _recv, _args)) = get_callee_span_generic_args_and_args(cx, expr)

first checking whether the method/call itself is marked as query unstable, then check whether it has a where-bound which relies on an unstable trait impl

Sorry, I'm not following. What do you mean by "merge this lint into one"?

rn u lint in two cases

the expr is a method/call

we can resolve it to a method with a #[rustc_lint_query_instability] attribute

the expr is a method

the method has a where-bound which is proven by an impl with a #[rustc_lint_query_instability] attribute.

Why not do

the expr is a method/call

we can resolve it to a method with a #[rustc_lint_query_instability] attribute

the method has a where-bound which is proven by an impl with a #[rustc_lint_query_instability] attribute.

Please tell me if what I pushed is what you had in mind.

tests/ui-fulldeps/internal-lints/query_stability.rs

compiler/rustc_lint/src/internal.rs

fmease · 2025-05-27T12:30:39Z

compiler/rustc_lint/src/internal.rs

-        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((span, def_id, args)) = typeck_results_of_method_fn(cx, expr)


It would be awesome if we could somehow unify typeck_results_of_method_fn and get_callee_generic_args_and_args.

Done in commit ae437dd.

compiler/rustc_lint/src/internal.rs

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

smoelius · 2025-05-28T22:22:02Z

Apologies. The force push was to remove an unnecessary change I included accidentally.

EDIT: Note that another commit (085312b re #139345 (comment)) has been since been pushed.

wesleywiser · 2025-06-26T14:09:30Z

Hi @fmease, it looks like the author has responded to all of your review feedback so this is ready for another look when you get a chance. Thanks!

wesleywiser · 2025-07-31T14:17:44Z

r? rust-lang/compiler

lcnr · 2025-08-07T10:47:45Z

compiler/rustc_interface/src/interface.rs

+                        ExpectedValues::Some(v) if !values_any_specified =>
+                        {


Suggested change

ExpectedValues::Some(v) if !values_any_specified =>

{

ExpectedValues::Some(v) if !values_any_specified => {

why does this formatting change happen?

I don't know precisely, but it seems to have something to do with the attribute on line 280. With the attribute removed, the left bracket is returned to line 278; with the attribute present, the left bracket is placed on line 279.

compiler/rustc_resolve/src/late.rs

lcnr · 2025-08-07T10:53:36Z

compiler/rustc_lint/src/internal.rs

-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,


Suggested change

self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty as MiddleTy,

self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty,

please avoid importing hir::Ty instead. We generally always uses Ty for ty::Ty

That makes sense to me.

lcnr · 2025-08-07T11:04:08Z

compiler/rustc_lint/src/internal.rs

+    else {
+        return;
+    };
+    let fn_sig = cx.tcx.fn_sig(callee_def_id).instantiate_identity().skip_binder();


this feels fairly iffy to me 🤔 we're doing this check in the context of the called functions definition (we're using instantiate_identity and I feel like we shouldn't 🤔)

what we could do instead is:

get the instantiated predicates of the function predicates_of(callee_def_id).instantiate(generic_args)

search for a predicate wahtever: IntoIterator predicate

resolve the instance for that predicat eand look for the rustc_lint_query_instability attr

if so, lint.

i personally wouldn't do any manually try to match function arg to predicate lookup, but instead print the type whose IntoIterator impl is not stable

my general perspective if that such sort of manual lookup is fairly undesirable and has a fairly high cost, so even if an alternative has slightly worse output, it's still worth it for the sake of maintainability

Please tell me if what I pushed is what you had in mind.

Note that by not matching the function argument, I wasn't sure what to use as the span. So I use the entire function or method call, e.g.:

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

rustbot · 2025-08-14T00:32:29Z

rustc_errors::emitter was changed

cc @Muscraft

…nts`

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

smoelius · 2025-08-14T01:29:54Z

I'm trying to rebase onto master to eliminate the build failure. Please forgive the impending churn.

…ebase

rustbot · 2025-08-14T21:28:55Z

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

rustbot assigned fmease Apr 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 4, 2025

This comment has been minimized.

Sign in to view

smoelius mentioned this pull request Apr 4, 2025

Change one FxHashMap to FxIndexMap in librustdoc #138871

Draft

1 task

smoelius force-pushed the into-iter-stability branch from 7c51ef9 to 1a2c5a3 Compare April 4, 2025 02:03

This comment has been minimized.

Sign in to view

smoelius marked this pull request as draft April 4, 2025 10:36

smoelius marked this pull request as ready for review April 4, 2025 21:27

fmease reviewed May 27, 2025

View reviewed changes

smoelius added a commit to smoelius/rust that referenced this pull request May 28, 2025

Address two review comments

2a4d30f

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

This comment has been minimized.

Sign in to view

smoelius added a commit to smoelius/rust that referenced this pull request May 28, 2025

Address two review comments

ae437dd

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

smoelius force-pushed the into-iter-stability branch from 2a4d30f to ae437dd Compare May 28, 2025 22:21

This comment has been minimized.

Sign in to view

rustbot assigned lcnr and unassigned fmease Jul 31, 2025

lcnr reviewed Aug 7, 2025

View reviewed changes

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved

lcnr reviewed Aug 7, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

smoelius force-pushed the into-iter-stability branch 2 times, most recently from 610cb4d to cf80434 Compare August 14, 2025 00:56

This comment has been minimized.

Sign in to view

smoelius and others added 8 commits August 13, 2025 21:27

Extend QueryStability to handle IntoIterator implementations

08ce2d0

Fix adjacent code

15866b6

Fix duplicate warning; merge test into `tests/ui-fulldeps/internal-li…

c102394

…nts`

Use rustc_middle::ty::FnSig::inputs

af5e468

Address two review comments

5fa3384

- rust-lang#139345 (comment) - rust-lang#139345 (comment)

Use Instance::try_resolve

fe2458b

Import rustc_middle::ty::Ty as Ty rather than MiddleTy

76d82dc

Simplify predicate handling

e8b2a43

smoelius added 3 commits August 14, 2025 08:52

Add more #[allow(rustc::potential_query_instability)] following rebase

cdcc78a

Remove two #[allow(rustc::potential_query_instability)] following r…

cd4f539

…ebase

Address review comment

ba9c93e

smoelius force-pushed the into-iter-stability branch from cf80434 to ba9c93e Compare August 14, 2025 21:28

	ExpectedValues::Some(v) if !values_any_specified =>
	{
	ExpectedValues::Some(v) if !values_any_specified => {

	self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty as MiddleTy,
	self, ClauseKind, GenericArgsRef, ParamTy, ProjectionPredicate, TraitPredicate, Ty,

Extend QueryStability to handle IntoIterator implementations #139345

Are you sure you want to change the base?

Extend QueryStability to handle IntoIterator implementations #139345

Conversation

smoelius commented Apr 4, 2025

Uh oh!

rustbot commented Apr 4, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

smoelius commented Apr 4, 2025

Uh oh!

fmease commented Apr 4, 2025

Uh oh!

smoelius commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smoelius commented Apr 30, 2025

Uh oh!

smoelius commented May 27, 2025

Uh oh!

fmease left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fmease May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmease May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smoelius May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

smoelius commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesleywiser commented Jun 26, 2025

Uh oh!

wesleywiser commented Jul 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Extend `QueryStability` to handle `IntoIterator` implementations #139345

Extend `QueryStability` to handle `IntoIterator` implementations #139345

smoelius commented Apr 4, 2025 •

edited

Loading

fmease left a comment •

edited

Loading

fmease May 27, 2025 •

edited

Loading

fmease May 27, 2025 •

edited

Loading

smoelius May 28, 2025 •

edited

Loading

smoelius commented May 28, 2025 •

edited

Loading