reduce false positives of tail-expr-drop-order from consumed values

Author: dingxiangfei2009

compiler/rustc_hir_typeck/src/expr_use_visitor.rs

@@ -150,6 +150,50 @@ pub trait TypeInformationCtxt<'tcx> {
     fn tcx(&self) -> TyCtxt<'tcx>;
 }
 
+impl<'tcx> TypeInformationCtxt<'tcx> for (TyCtxt<'tcx>, LocalDefId) {
+    type TypeckResults<'a> = &'tcx ty::TypeckResults<'tcx>
+    where
+        Self: 'a;
+
+    type Error = !;
+
+    fn typeck_results(&self) -> Self::TypeckResults<'_> {
+        self.0.typeck(self.1)
+    }
+
+    fn resolve_vars_if_possible<T: TypeFoldable<TyCtxt<'tcx>>>(&self, t: T) -> T {
+        t
+    }
+
+    fn try_structurally_resolve_type(&self, _span: Span, ty: Ty<'tcx>) -> Ty<'tcx> {
+        ty
+    }
+
+    fn report_error(&self, span: Span, msg: impl ToString) -> Self::Error {
+        span_bug!(span, "{}", msg.to_string())
+    }
+
+    fn error_reported_in_ty(&self, _ty: Ty<'tcx>) -> Result<(), Self::Error> {
+        Ok(())
+    }
+
+    fn tainted_by_errors(&self) -> Result<(), Self::Error> {
+        Ok(())
+    }
+
+    fn type_is_copy_modulo_regions(&self, ty: Ty<'tcx>) -> bool {
+        ty.is_copy_modulo_regions(self.0, self.0.param_env(self.1))
+    }
+
+    fn body_owner_def_id(&self) -> LocalDefId {
+        self.1
+    }
+
+    fn tcx(&self) -> TyCtxt<'tcx> {
+        self.0
+    }
+}
+
 impl<'tcx> TypeInformationCtxt<'tcx> for &FnCtxt<'_, 'tcx> {
     type TypeckResults<'a> = Ref<'a, ty::TypeckResults<'tcx>>
     where

compiler/rustc_hir_typeck/src/lib.rs

@@ -30,6 +30,7 @@ mod fallback;
 mod fn_ctxt;
 mod gather_locals;
 mod intrinsicck;
+mod lint;
 mod method;
 mod op;
 mod pat;
@@ -476,5 +477,6 @@ fn fatally_break_rust(tcx: TyCtxt<'_>, span: Span) -> ! {
 
 pub fn provide(providers: &mut Providers) {
     method::provide(providers);
+    lint::provide(providers);
     *providers = Providers { typeck, diagnostic_only_typeck, used_trait_imports, ..*providers };
 }

compiler/rustc_hir_typeck/src/lint.rs

@@ -0,0 +1,67 @@
+use rustc_hir::def_id::DefId;
+use rustc_hir::{HirId, HirIdSet};
+use rustc_middle::hir::place::{PlaceBase, PlaceWithHirId};
+use rustc_middle::mir::FakeReadCause;
+use rustc_middle::query::Providers;
+use rustc_middle::ty::{self, TyCtxt};
+use tracing::instrument;
+
+use crate::expr_use_visitor::{Delegate, ExprUseVisitor};
+
+#[derive(Default)]
+struct ExtractConsumingNodeDelegate {
+    nodes: HirIdSet,
+}
+
+impl<'tcx> Delegate<'tcx> for ExtractConsumingNodeDelegate {
+    #[instrument(level = "debug", skip(self))]
+    fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) {
+        match place_with_id.place.base {
+            PlaceBase::Rvalue => {
+                self.nodes.insert(place_with_id.hir_id);
+            }
+            PlaceBase::Local(id) => {
+                self.nodes.insert(id);
+            }
+            PlaceBase::Upvar(upvar) => {
+                self.nodes.insert(upvar.var_path.hir_id);
+            }
+            PlaceBase::StaticItem => {}
+        }
+    }
+
+    fn borrow(
+        &mut self,
+        _place_with_id: &PlaceWithHirId<'tcx>,
+        _diag_expr_id: HirId,
+        _bk: ty::BorrowKind,
+    ) {
+        // do nothing
+    }
+
+    fn mutate(&mut self, _assignee_place: &PlaceWithHirId<'tcx>, _diag_expr_id: HirId) {
+        // do nothing
+    }
+
+    fn fake_read(
+        &mut self,
+        _place_with_id: &PlaceWithHirId<'tcx>,
+        _cause: FakeReadCause,
+        _diag_expr_id: HirId,
+    ) {
+        // do nothing
+    }
+}
+
+fn extract_tail_expr_consuming_nodes<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx HirIdSet {
+    let hir = tcx.hir();
+    let body = hir.body_owned_by(def_id.expect_local());
+    let mut delegate = ExtractConsumingNodeDelegate::default();
+    let euv = ExprUseVisitor::new((tcx, def_id.expect_local()), &mut delegate);
+    let _ = euv.walk_expr(body.value);
+    tcx.arena.alloc(delegate.nodes)
+}
+
+pub(crate) fn provide(providers: &mut Providers) {
+    providers.extract_tail_expr_consuming_nodes = extract_tail_expr_consuming_nodes;
+}

compiler/rustc_lint/messages.ftl

@@ -767,8 +767,8 @@ lint_suspicious_double_ref_clone =
 lint_suspicious_double_ref_deref =
     using `.deref()` on a double reference, which returns `{$ty}` instead of dereferencing the inner type
 
-lint_tail_expr_drop_order = these values and local bindings have significant drop implementation that will have a different drop order from that of Edition 2021
-    .label = these values have significant drop implementation and will observe changes in drop order under Edition 2024
+lint_tail_expr_drop_order = this value of type `{$ty}` has significant drop implementation that will have a different drop order from that of Edition 2021
+    .label = these local bindings with significant drop implementation may observe changes in drop order under Edition 2024
 
 lint_trailing_semi_macro = trailing semicolon in macro used in expression position
     .note1 = macro invocations at the end of a block are treated as expressions

compiler/rustc_lint/src/tail_expr_drop_order.rs

@@ -3,13 +3,17 @@ use std::mem::swap;
 use rustc_ast::UnOp;
 use rustc_hir::def::Res;
 use rustc_hir::intravisit::{self, Visitor};
-use rustc_hir::{self as hir, Block, Expr, ExprKind, LetStmt, Pat, PatKind, QPath, StmtKind};
+use rustc_hir::{
+    self as hir, Block, Expr, ExprKind, HirIdMap, HirIdSet, LetStmt, Pat, PatKind, QPath, StmtKind,
+};
 use rustc_macros::LintDiagnostic;
-use rustc_middle::ty;
+use rustc_middle::hir::place::PlaceBase;
+use rustc_middle::ty::{self, Ty};
 use rustc_session::lint::FutureIncompatibilityReason;
 use rustc_session::{declare_lint, declare_lint_pass};
 use rustc_span::edition::Edition;
 use rustc_span::Span;
+use tracing::instrument;
 
 use crate::{LateContext, LateLintPass};
 
@@ -94,35 +98,41 @@ declare_lint! {
 declare_lint_pass!(TailExprDropOrder => [TAIL_EXPR_DROP_ORDER]);
 
 impl TailExprDropOrder {
+    #[instrument(level = "debug", skip(cx, body))]
     fn check_fn_or_closure<'tcx>(
         cx: &LateContext<'tcx>,
         fn_kind: hir::intravisit::FnKind<'tcx>,
         body: &'tcx hir::Body<'tcx>,
         def_id: rustc_span::def_id::LocalDefId,
     ) {
-        let mut locals = vec![];
+        let consumed = cx.tcx.extract_tail_expr_consuming_nodes(def_id.to_def_id());
+        let mut locals = HirIdMap::default();
         if matches!(fn_kind, hir::intravisit::FnKind::Closure) {
             for &capture in cx.tcx.closure_captures(def_id) {
                 if matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue)
                     && capture.place.ty().has_significant_drop(cx.tcx, cx.param_env)
                 {
-                    locals.push(capture.var_ident.span);
+                    let hir_id = match capture.place.base {
+                        PlaceBase::Local(id) => id,
+                        PlaceBase::Upvar(upvar) => upvar.var_path.hir_id,
+                        PlaceBase::Rvalue | PlaceBase::StaticItem => continue,
+                    };
+                    if consumed.contains(&hir_id) {
+                        continue;
+                    }
+                    locals.insert(hir_id, capture.var_ident.span);
                 }
             }
         }
         for param in body.params {
-            if cx
-                .typeck_results()
-                .node_type(param.hir_id)
-                .has_significant_drop(cx.tcx, cx.param_env)
-            {
-                locals.push(param.span);
-            }
+            LocalCollector { cx, locals: &mut locals, consumed: &consumed }.visit_pat(param.pat);
         }
         if let hir::ExprKind::Block(block, _) = body.value.kind {
-            LintVisitor { cx, locals }.check_block_inner(block);
+            LintVisitor { cx, locals, consumed }.check_block_inner(block);
         } else {
-            LintTailExpr { cx, locals: &locals, is_root_tail_expr: true }.visit_expr(body.value);
+            let locals: Vec<_> = locals.values().copied().collect();
+            LintTailExpr { cx, locals: &locals, consumed, is_root_tail_expr: true }
+                .visit_expr(body.value);
         }
     }
 }
@@ -146,21 +156,26 @@ impl<'tcx> LateLintPass<'tcx> for TailExprDropOrder {
 struct LintVisitor<'tcx, 'a> {
     cx: &'a LateContext<'tcx>,
     // We only record locals that have significant drops
-    locals: Vec<Span>,
+    locals: HirIdMap<Span>,
+    consumed: &'a HirIdSet,
 }
 
 struct LocalCollector<'tcx, 'a> {
     cx: &'a LateContext<'tcx>,
-    locals: &'a mut Vec<Span>,
+    locals: &'a mut HirIdMap<Span>,
+    consumed: &'a HirIdSet,
 }
 
 impl<'tcx, 'a> Visitor<'tcx> for LocalCollector<'tcx, 'a> {
     type Result = ();
     fn visit_pat(&mut self, pat: &'tcx Pat<'tcx>) {
         if let PatKind::Binding(_binding_mode, id, ident, pat) = pat.kind {
+            if self.consumed.contains(&id) {
+                return;
+            }
             let ty = self.cx.typeck_results().node_type(id);
             if ty.has_significant_drop(self.cx.tcx, self.cx.param_env) {
-                self.locals.push(ident.span);
+                self.locals.insert(id, ident.span);
             }
             if let Some(pat) = pat {
                 self.visit_pat(pat);
@@ -179,7 +194,8 @@ impl<'tcx, 'a> Visitor<'tcx> for LintVisitor<'tcx, 'a> {
         swap(&mut locals, &mut self.locals);
     }
     fn visit_local(&mut self, local: &'tcx LetStmt<'tcx>) {
-        LocalCollector { cx: self.cx, locals: &mut self.locals }.visit_local(local);
+        LocalCollector { cx: self.cx, locals: &mut self.locals, consumed: &self.consumed }
+            .visit_local(local);
     }
 }
 
@@ -200,19 +216,26 @@ impl<'tcx, 'a> LintVisitor<'tcx, 'a> {
         if self.locals.is_empty() {
             return;
         }
-        LintTailExpr { cx: self.cx, locals: &self.locals, is_root_tail_expr: true }
-            .visit_expr(tail_expr);
+        let locals: Vec<_> = self.locals.values().copied().collect();
+        LintTailExpr {
+            cx: self.cx,
+            locals: &locals,
+            is_root_tail_expr: true,
+            consumed: self.consumed,
+        }
+        .visit_expr(tail_expr);
     }
 }
 
 struct LintTailExpr<'tcx, 'a> {
     cx: &'a LateContext<'tcx>,
     is_root_tail_expr: bool,
     locals: &'a [Span],
+    consumed: &'a HirIdSet,
 }
 
 impl<'tcx, 'a> LintTailExpr<'tcx, 'a> {
-    fn expr_eventually_point_into_local(mut expr: &Expr<'tcx>) -> bool {
+    fn expr_eventually_point_into_local(&self, mut expr: &Expr<'tcx>) -> bool {
         loop {
             match expr.kind {
                 ExprKind::Index(access, _, _) | ExprKind::Field(access, _) => expr = access,
@@ -231,24 +254,28 @@ impl<'tcx, 'a> LintTailExpr<'tcx, 'a> {
         }
     }
 
-    fn expr_generates_nonlocal_droppy_value(&self, expr: &Expr<'tcx>) -> bool {
-        if Self::expr_eventually_point_into_local(expr) {
-            return false;
+    fn expr_generates_nonlocal_droppy_value(&self, expr: &Expr<'tcx>) -> Option<Ty<'tcx>> {
+        if self.expr_eventually_point_into_local(expr) {
+            return None;
+        }
+        if self.consumed.contains(&expr.hir_id) {
+            return None;
         }
-        self.cx.typeck_results().expr_ty(expr).has_significant_drop(self.cx.tcx, self.cx.param_env)
+        let ty = self.cx.typeck_results().expr_ty(expr);
+        if ty.has_significant_drop(self.cx.tcx, self.cx.param_env) { Some(ty) } else { None }
     }
 }
 
 impl<'tcx, 'a> Visitor<'tcx> for LintTailExpr<'tcx, 'a> {
     fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
         if self.is_root_tail_expr {
             self.is_root_tail_expr = false;
-        } else if self.expr_generates_nonlocal_droppy_value(expr) {
+        } else if let Some(ty) = self.expr_generates_nonlocal_droppy_value(expr) {
             self.cx.tcx.emit_node_span_lint(
                 TAIL_EXPR_DROP_ORDER,
                 expr.hir_id,
                 expr.span,
-                TailExprDropOrderLint { spans: self.locals.to_vec() },
+                TailExprDropOrderLint { spans: self.locals.to_vec(), ty },
             );
             return;
         }
@@ -294,13 +321,15 @@ impl<'tcx, 'a> Visitor<'tcx> for LintTailExpr<'tcx, 'a> {
         }
     }
     fn visit_block(&mut self, block: &'tcx Block<'tcx>) {
-        LintVisitor { cx: self.cx, locals: <_>::default() }.check_block_inner(block);
+        LintVisitor { cx: self.cx, locals: <_>::default(), consumed: &<_>::default() }
+            .check_block_inner(block);
     }
 }
 
 #[derive(LintDiagnostic)]
 #[diag(lint_tail_expr_drop_order)]
-struct TailExprDropOrderLint {
+struct TailExprDropOrderLint<'a> {
     #[label]
     pub spans: Vec<Span>,
+    pub ty: Ty<'a>,
 }

compiler/rustc_middle/src/arena.rs

@@ -114,6 +114,7 @@ macro_rules! arena_types {
             [decode] specialization_graph: rustc_middle::traits::specialization_graph::Graph,
             [] crate_inherent_impls: rustc_middle::ty::CrateInherentImpls,
             [] hir_owner_nodes: rustc_hir::OwnerNodes<'tcx>,
+            [] hir_id_set: rustc_hir::HirIdSet,
         ]);
     )
 }

compiler/rustc_middle/src/query/mod.rs

@@ -26,7 +26,7 @@ use rustc_hir::def_id::{
     CrateNum, DefId, DefIdMap, DefIdSet, LocalDefId, LocalDefIdMap, LocalDefIdSet, LocalModDefId,
 };
 use rustc_hir::lang_items::{LangItem, LanguageItems};
-use rustc_hir::{Crate, ItemLocalId, ItemLocalMap, TraitCandidate};
+use rustc_hir::{Crate, HirIdSet, ItemLocalId, ItemLocalMap, TraitCandidate};
 use rustc_index::IndexVec;
 use rustc_macros::rustc_queries;
 use rustc_query_system::ich::StableHashingContext;
@@ -2279,6 +2279,10 @@ rustc_queries! {
         desc { "whether the item should be made inlinable across crates" }
         separate_provide_extern
     }
+
+    query extract_tail_expr_consuming_nodes(def_id: DefId) -> &'tcx HirIdSet {
+        desc { "extract hir nodes that consumes value" }
+    }
 }
 
 rustc_query_append! { define_callbacks! }