Extend QueryStability to handle IntoIterator implementations by smoelius · Pull Request #139345 · rust-lang/rust (original) (raw)

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

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

rn u lint in two cases

Why not do

Choose a reason for hiding this comment

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

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