-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
ddf7f18
1a2c5a3
420b986
dfec3c0
ae437dd
085312b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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}; | ||||||
|
@@ -101,10 +103,11 @@ 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((span, def_id, args)) = typeck_results_of_method_fn(cx, expr) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be awesome if we could somehow unify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in commit ae437dd. |
||||||
&& let Ok(Some(instance)) = | ||||||
ty::Instance::try_resolve(cx.tcx, cx.typing_env(), def_id, args) | ||||||
{ | ||||||
let def_id = instance.def_id(); | ||||||
if cx.tcx.has_attr(def_id, sym::rustc_lint_query_instability) { | ||||||
|
@@ -122,7 +125,121 @@ 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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 We could of course mark There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
To be clear, this would be an enhancement to the existing I could make this change if you think it would be best. |
||||||
return; | ||||||
}; | ||||||
if expr.span.from_expansion() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without that condition, an additional warning is produced here: rust/tests/ui-fulldeps/internal-lints/query_stability.rs Lines 22 to 23 in dfec3c0
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, generic_args, recv, args)) = | ||||||
get_callee_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() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You were right: the lint has a much simpler implementation using |
||||||
let &ty::Param(ParamTy { index: param_index, .. }) = input.kind() else { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 You could create a custom There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_args[param_index as usize].expect_ty(); | ||||||
let Some(self_ty_adt_def) = self_ty.peel_refs().ty_adt_def() else { | ||||||
return; | ||||||
}; | ||||||
cx.tcx.for_each_relevant_impl(into_iterator_def_id, self_ty, |impl_id| { | ||||||
let impl_ty = cx.tcx.type_of(impl_id).skip_binder(); | ||||||
let Some(impl_ty_adt_def) = impl_ty.peel_refs().ty_adt_def() else { | ||||||
return; | ||||||
}; | ||||||
// To reduce false positives, verify that `self_ty` and `impl_ty` refer to the same ADT. | ||||||
if self_ty_adt_def != impl_ty_adt_def { | ||||||
return; | ||||||
} | ||||||
let Some(into_iter_item) = cx | ||||||
.tcx | ||||||
.associated_items(impl_id) | ||||||
.filter_by_name_unhygienic(sym::into_iter) | ||||||
.next() | ||||||
else { | ||||||
return; | ||||||
}; | ||||||
// Does the input type's `IntoIterator` implementation have the | ||||||
// `rustc_lint_query_instability` attribute on its `into_iter` method? | ||||||
if !cx.tcx.has_attr(into_iter_item.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(into_iter_item.def_id) }, | ||||||
); | ||||||
}); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
/// Checks whether an expression is a function or method call and, if so, returns its `DefId`, | ||||||
/// `GenericArgs`, and arguments. | ||||||
fn get_callee_generic_args_and_args<'tcx>( | ||||||
cx: &LateContext<'tcx>, | ||||||
expr: &'tcx Expr<'tcx>, | ||||||
) -> Option<(DefId, 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, _) = callee_ty.kind() | ||||||
{ | ||||||
let generic_args = cx.typeck_results().node_args(callee.hir_id); | ||||||
return Some((*callee_def_id, generic_args, None, args)); | ||||||
} | ||||||
if let ExprKind::MethodCall(_, 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, 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! { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,4 +34,7 @@ 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); } There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
|
||
} |
There was a problem hiding this comment.
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
forpsess.check_cfg.expecteds
andUnordSet
forExpectedValues::Some
leveraging the fact thatSymbol
implsToStableHashKey
1 allowing us to useto_sorted
which would arguably make it easier to see that there's no risk of instability)Footnotes
Well, it's
KeyType
isString
, not<'call> &'call str
which may impact performance, so we might need to GAT-ifyKeyType
totype KeyType<'a>: …;
and so on ↩There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment would be "FIXME" or something like that?
It looks like
ExpectedValues
could be made to contain anFxIndexMap
(and I don't know why I didn't make that change). Should I just make that change?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was trying to change only the data structure I had initially set out out to.