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

…nts`

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

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)

rustbot · 2025-05-28T22:21:14Z

⚠️ Warning ⚠️

This PR is based on an upstream commit that is 55 days old.

It's recommended to update your branch according to the rustc-dev-guide.

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?

lcnr · 2025-08-07T10:52:06Z

compiler/rustc_resolve/src/late.rs

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


How much effort is it to change bindings to &mut SmallVec<[(PatBoundCtx, UnordSet<Ident>); 1]>?

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

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

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 added 2 commits April 3, 2025 22:03

Extend QueryStability to handle IntoIterator implementations

ddf7f18

Fix adjacent code

1a2c5a3

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

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

420b986

…nts`

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

Use rustc_middle::ty::FnSig::inputs

dfec3c0

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

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

Use Instance::try_resolve

085312b

rustbot assigned lcnr and unassigned fmease Jul 31, 2025

lcnr reviewed Aug 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

smoelius commented Apr 4, 2025

rustbot commented Apr 4, 2025

This comment has been minimized.

This comment has been minimized.

smoelius commented Apr 4, 2025

fmease commented Apr 4, 2025

smoelius commented Apr 4, 2025 •

edited

Loading

smoelius commented Apr 30, 2025

smoelius commented May 27, 2025

fmease left a comment •

edited

Loading

fmease May 27, 2025

smoelius May 27, 2025

smoelius May 29, 2025

fmease May 27, 2025

smoelius May 27, 2025

fmease May 27, 2025 •

edited

Loading

smoelius May 27, 2025

Uh oh!

Uh oh!

fmease May 27, 2025 •

edited

Loading

smoelius May 28, 2025 •

edited

Loading

Uh oh!

This comment has been minimized.

rustbot commented May 28, 2025

smoelius commented May 28, 2025 •

edited

Loading

wesleywiser commented Jun 26, 2025

wesleywiser commented Jul 31, 2025

lcnr Aug 7, 2025

lcnr Aug 7, 2025

lcnr Aug 7, 2025

lcnr Aug 7, 2025

	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

rustbot commented Apr 4, 2025

This comment has been minimized.

This comment has been minimized.

smoelius commented Apr 4, 2025

fmease commented Apr 4, 2025

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

Uh oh!

smoelius commented Apr 30, 2025

smoelius commented May 27, 2025

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

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

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!

This comment has been minimized.

rustbot commented May 28, 2025

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

Uh oh!

wesleywiser commented Jun 26, 2025

wesleywiser commented Jul 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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