-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix Issue#17414: needless_borrow do not contradict with `dangerous_implicit_aut…
#17433
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 all commits
32624cc
b9b401a
ad3781a
20c5fcc
5ab6541
888897f
f107b9c
4183282
db3ec1e
1ca481c
a74342f
2acdb17
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 |
|---|---|---|
|
|
@@ -6,19 +6,21 @@ use clippy_utils::ty::{ | |
| adjust_derefs_manually_drop, get_adt_inherent_method, implements_trait, is_manually_drop, peel_and_count_ty_refs, | ||
| }; | ||
| use clippy_utils::{ | ||
| DefinedTy, ExprUseNode, get_expr_use_site, get_parent_expr, is_block_like, is_from_proc_macro, is_lint_allowed, sym, | ||
| DefinedTy, ExprUseNode, expr_use_sites, get_expr_use_site, get_parent_expr, is_block_like, is_from_proc_macro, | ||
| is_lint_allowed, sym, | ||
| }; | ||
| use rustc_ast::util::parser::ExprPrecedence; | ||
| use rustc_data_structures::fx::FxIndexMap; | ||
| use rustc_errors::Applicability; | ||
| use rustc_hir::attrs::{AttributeKind, HasAttrs as _}; | ||
| use rustc_hir::def_id::DefId; | ||
| use rustc_hir::intravisit::{InferKind, Visitor, VisitorExt as _, walk_ty}; | ||
| use rustc_hir::{ | ||
| self as hir, AmbigArg, BindingMode, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, Item, MatchSource, Mutability, | ||
| Node, OwnerId, Pat, PatKind, Path, QPath, TyKind, UnOp, | ||
| self as hir, AmbigArg, Attribute, BindingMode, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, Item, MatchSource, | ||
| Mutability, Node, OwnerId, Pat, PatKind, Path, QPath, TyKind, UnOp, | ||
| }; | ||
| use rustc_lint::{LateContext, LateLintPass}; | ||
| use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; | ||
| use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability, DerefAdjustKind}; | ||
| use rustc_middle::ty::{self, AssocTag, Ty, TyCtxt, TypeVisitableExt as _, TypeckResults, Unnormalized}; | ||
| use rustc_session::impl_lint_pass; | ||
| use rustc_span::{Span, Symbol, SyntaxContext}; | ||
|
|
@@ -275,6 +277,10 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { | |
| return; | ||
| } | ||
|
|
||
| if need_explicit_ref(cx, typeck, expr) { | ||
| return; | ||
| } | ||
|
|
||
| match (self.state.take(), kind) { | ||
| (None, kind) => { | ||
| let expr_ty = typeck.expr_ty(expr); | ||
|
|
@@ -441,6 +447,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { | |
| true | ||
| } | ||
| }, | ||
| ExprUseNode::Index(base, _) if expr.hir_id == base.hir_id => true, | ||
| _ => false, | ||
| }; | ||
|
|
||
|
|
@@ -732,6 +739,96 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { | |
| } | ||
| } | ||
|
|
||
| // Issue: https://github.com/rust-lang/rust-clippy/issues/17414 | ||
| // Related: https://doc.rust-lang.org/beta/nightly-rustc/rustc_lint/autorefs/static.DANGEROUS_IMPLICIT_AUTOREFS.html | ||
| // A raw pointer need explicit ref to prevent from dangerous_implicit_autorefs. | ||
| // Recognize whether the ref of expr must be explicit. Similar Logic to `rustc_lint::autorefs` . | ||
| // 1. expr is a AddrOf Ref. | ||
| // 2. Loop sub_expr of expr if sub_expr is index/field, and is a deref of a raw ptr at the end. | ||
| // 3. Not all raw ptr need explicit ref. Only if it is in a danger invoke chain. Will check in | ||
| // method check_invoke_chain_danger. | ||
| fn need_explicit_ref<'tcx>(cx: &LateContext<'tcx>, typeck: &'tcx TypeckResults<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { | ||
|
skiefucker marked this conversation as resolved.
Comment on lines
+742
to
+750
Contributor
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. Please reword the code comment to
|
||
| match expr.kind { | ||
| ExprKind::AddrOf(BorrowKind::Ref, _, mut sub_expr) => { | ||
| typeck | ||
| .expr_ty(loop { | ||
| match sub_expr.kind { | ||
| ExprKind::Index(e, _, _) | ExprKind::Field(e, _) => sub_expr = e, | ||
| ExprKind::Unary(UnOp::Deref, e) => break e, | ||
| _ => break sub_expr, | ||
| } | ||
| }) | ||
| .is_raw_ptr() | ||
| && check_invoke_chain_danger(cx, typeck, expr) | ||
| }, | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| // Check expr whether in a danger invoke chain. | ||
| // 1. Loop use site of expr. | ||
| // 2. Get adjustments. | ||
| // 3. If adjs list pattern [N * Deref Adjust, 1 * AutoBorrow Adjust], means auto ref exist. | ||
| // 3. Get use node. | ||
| // 4. If node is field or index and auto ref exist, return true, else continue loop. | ||
| // 5. If method call, check method has `RustcNoImplicitAutorefs` attr. | ||
| fn check_invoke_chain_danger<'tcx>( | ||
|
Comment on lines
+768
to
+775
Contributor
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. these code comments are super unclear given that it currently seems to be a kind of verbatim retelling of the code. |
||
| cx: &LateContext<'tcx>, | ||
| typeck: &'tcx TypeckResults<'tcx>, | ||
| mut expr: &'tcx Expr<'tcx>, | ||
| ) -> bool { | ||
| while let Some(use_site) = expr_use_sites(cx.tcx, typeck, SyntaxContext::root(), expr).next() { | ||
| let adjs = peel_derefs_adjustments(use_site.adjustments); | ||
| let auto_ref = if let [adj] = adjs { | ||
| matches!( | ||
| adj.kind, | ||
| Adjust::Deref(DerefAdjustKind::Overloaded(_)) | Adjust::Borrow(AutoBorrow::Ref(_)) | ||
| ) | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| match use_site.node { | ||
| Node::Expr(use_expr) => match use_expr.kind { | ||
| ExprKind::Field(..) | ExprKind::Index(..) => { | ||
| if auto_ref { | ||
| return true; | ||
| } | ||
| expr = use_expr; | ||
| }, | ||
| ExprKind::MethodCall(..) => { | ||
| return auto_ref | ||
| && typeck.type_dependent_def_id(use_expr.hir_id).is_some_and(|fn_id| { | ||
| fn_id | ||
| .get_attrs(&cx.tcx) // can not use private macro find_attr!() | ||
| .iter() | ||
| .any(|attr| matches!(attr, Attribute::Parsed(AttributeKind::RustcNoImplicitAutorefs))) | ||
| }); | ||
| }, | ||
| _ => return false, | ||
| }, | ||
| _ => return false, | ||
| } | ||
| } | ||
|
|
||
| false | ||
| } | ||
|
|
||
| // same code from rustc_lint:auto_ref.rs | ||
| fn peel_derefs_adjustments<'a>(mut adjs: &'a [Adjustment<'a>]) -> &'a [Adjustment<'a>] { | ||
| while let [ | ||
| Adjustment { | ||
| kind: Adjust::Deref(_), .. | ||
| }, | ||
| end @ .., | ||
| ] = adjs | ||
| && !end.is_empty() | ||
| { | ||
| adjs = end; | ||
| } | ||
| adjs | ||
| } | ||
|
|
||
| fn is_deref_or_derefmut_impl(cx: &LateContext<'_>, item: &Item<'_>) -> bool { | ||
| if let hir::ItemKind::Impl(impl_) = item.kind | ||
| && let Some(of_trait) = impl_.of_trait | ||
|
|
@@ -1153,15 +1250,6 @@ impl<'tcx> Dereferencing<'tcx> { | |
| ); | ||
| }, | ||
| State::DerefedBorrow(state) => { | ||
| // Do not suggest removing a non-mandatory `&` in `&*rawptr` in an `unsafe` context, | ||
| // as this may make rustc trigger its `dangerous_implicit_autorefs` lint. | ||
| if let ExprKind::AddrOf(BorrowKind::Ref, _, subexpr) = data.first_expr.kind | ||
| && let ExprKind::Unary(UnOp::Deref, subsubexpr) = subexpr.kind | ||
| && cx.typeck_results().expr_ty_adjusted(subsubexpr).is_raw_ptr() | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| let mut app = Applicability::MachineApplicable; | ||
| let (snip, snip_is_macro) = | ||
| snippet_with_context(cx, expr.span, data.first_expr.span.ctxt(), "..", &mut app); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |
| clippy::unnecessary_mut_passed | ||
| )] | ||
|
|
||
| use std::ops::Index; | ||
|
|
||
| fn main() { | ||
| let a = 5; | ||
| let ref_a = &a; | ||
|
|
@@ -294,3 +296,79 @@ fn issue_14743<T>(slice: &[T]) { | |
| #[expect(dangerous_implicit_autorefs)] | ||
| let _ = unsafe { (*slice).len() }; | ||
| } | ||
|
|
||
| fn issue_17414( | ||
| slice: &(i32, (i32, [usize])), | ||
| tuple_slice: *const (i32, [usize]), | ||
| nested_tuple: *const (i32, (i32, [usize])), | ||
| ) { | ||
| let _ = (&slice).1.1.len(); | ||
| //~^ needless_borrow | ||
| let _ = (&slice.1).1.len(); | ||
| //~^ needless_borrow | ||
| let _ = (&slice.1.1).len(); | ||
| //~^ needless_borrow | ||
|
|
||
| unsafe { | ||
| // Rule: `rustc_lint:autoref.rs` require explicit ref to arg from a [inner] derefed raw pointer | ||
| // when calling a #[rustc_no_implicit_autorefs] method. | ||
|
|
||
| // 1. Issue 17414 case: deref tuple, get slice field, call [].len() method. | ||
| let _ = (&(*tuple_slice).1).len(); // necessary borrow, no lint | ||
|
|
||
| // 2. simple slice | ||
| let a = [0, 1, 2]; | ||
| let b = &raw const a[0..2]; // same with raw const or raw mut | ||
| let _ = (&*b).len(); // necessary | ||
| let _ = (&*b).index(0); // necessary | ||
| let _: i32 = (*b)[0]; // success | ||
|
|
||
| let _: i32 = (&*b)[0]; | ||
| //~^ needless_borrow | ||
|
|
||
| // 3. nested tuple | ||
| let _ = (&*nested_tuple).1.1.len(); // necessary | ||
| let _ = (&(*nested_tuple).1).1.len(); // necessary | ||
| let _ = (&(*nested_tuple).1.1).len(); // necessary | ||
|
|
||
| let _ = (*nested_tuple).1.1.first(); // success | ||
|
|
||
| let _ = (&*nested_tuple).1.1.first(); | ||
| //~^ needless_borrow | ||
| let _ = (&(*nested_tuple).1).1.first(); | ||
| //~^ needless_borrow | ||
| let _ = (&(*nested_tuple).1.1).first(); | ||
| //~^ needless_borrow | ||
|
|
||
| // 4. array | ||
| let a = [1, 2, 3]; | ||
| let b: *const [i32; 3] = &raw const a; | ||
| // let _ = (*b).index(0); // fail | ||
| let _ = (&*b).index(0); // necessary | ||
|
|
||
| let _ = (*b)[0]; // ok | ||
| let _ = (&*b)[0]; | ||
| //~^ needless_borrow | ||
|
|
||
| // 5. trait | ||
| trait T: Index<usize, Output = i32> {} | ||
| struct S {} | ||
| impl Index<usize> for S { | ||
| type Output = i32; | ||
| // no attr, defined in super.index | ||
| fn index(&self, _: usize) -> &i32 { | ||
| &42 | ||
| } | ||
| } | ||
| impl T for S {} | ||
|
|
||
| let s = S {}; | ||
| let t: &dyn T = &s; | ||
| let ptr: *const dyn T = t; | ||
|
|
||
| // let _ = (*ptr)[0]; // fail | ||
| let _ = (&*ptr)[0]; // necessary | ||
| // let _ = (*ptr).index(0); // fail | ||
|
Comment on lines
+369
to
+371
Contributor
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. are |
||
| let _ = (&*ptr).index(0); // necessary | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Is there a reason why you put this guard here instead of where the old guard was? This would now skip handling other diagnostics for the expression not just
needless_borrow.View changes since the review
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.
@Gri-ffin
My reasons :
need_explicit_refreturns true only when the explicit ref is from a raw pointer deref (maybe index/field operation in the middle), so it will not skip other diagnostics, all tests passed confirms this.dereference.rswhen we already know the lint should be suppressed. The old guard was placed later after many calculations.Let me know if you have other ideas !!!