coverage: Switch over to hybrid HIR-aware coverage spans

compiler/rustc_codegen_llvm/src/coverageinfo/mod.rs

@@ -111,7 +111,7 @@ impl<'tcx> CoverageInfoBuilderMethods<'tcx> for Builder<'_, '_, 'tcx> {
         };
 
         match *kind {
-            CoverageKind::SpanMarker | CoverageKind::BlockMarker { .. } => unreachable!(
+            CoverageKind::Point { .. } | CoverageKind::BlockMarker { .. } => unreachable!(
                 "marker statement {kind:?} should have been removed by CleanupPostBorrowck"
             ),
             CoverageKind::VirtualCounter { bcb }

compiler/rustc_middle/src/mir/coverage.rs

@@ -3,6 +3,7 @@
 use std::fmt::{self, Debug, Formatter};
 
 use rustc_data_structures::fx::FxIndexMap;
+use rustc_hir::HirId;
 use rustc_index::{Idx, IndexVec};
 use rustc_macros::{StableHash, TyDecodable, TyEncodable};
 use rustc_span::Span;
@@ -70,13 +71,24 @@ impl Debug for CovTerm {
     }
 }
 
+/// The specific relationship between [`CoverageKind::Point`] and its [`HirId`].
+#[derive(Clone, Copy, Debug, PartialEq, TyEncodable, TyDecodable, StableHash)]
+pub enum PointKind {
+    /// Inserted just before evaluating an expression.
+    Expr,
+    /// Inserted when a one-sided `if` expression generates its synthetic `else {}`.
+    /// The absent `else` has no node, so [`HirId`] is the `if` expression.
+    ImplicitElse,
+    /// Inserted at the end of a function's body. [`HirId`] is the function itself.
+    FunctionEnd,
+}
+
 #[derive(Clone, PartialEq, TyEncodable, TyDecodable, StableHash)]
 pub enum CoverageKind {
-    /// Marks a span that might otherwise not be represented in MIR, so that
-    /// coverage instrumentation can associate it with its enclosing block/BCB.
-    ///
-    /// Should be erased before codegen (at some point after `InstrumentCoverage`).
-    SpanMarker,
+    /// Associates a HIR node (such as an expression) with a particular point in
+    /// MIR control-flow. The relationship between the node and the point is
+    /// indicated by [`PointKind`]. Injected during MIR building.
+    Point { point_kind: PointKind, hir_id: HirId },
 
     /// Marks its enclosing basic block with an ID that can be referred to by
     /// side data in [`CoverageEarlyInfo`].
@@ -94,11 +106,24 @@ pub enum CoverageKind {
 
 impl Debug for CoverageKind {
     fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
-        use CoverageKind::*;
         match self {
-            SpanMarker => write!(fmt, "SpanMarker"),
-            BlockMarker { id } => write!(fmt, "BlockMarker({:?})", id.index()),
-            VirtualCounter { bcb } => write!(fmt, "VirtualCounter({bcb:?})"),
+            CoverageKind::Point { point_kind, hir_id } => {
+                write!(fmt, "Point({point_kind:?}, {hir_id:?}")
+            }
+            CoverageKind::BlockMarker { id } => write!(fmt, "BlockMarker({:?})", id.index()),
+            CoverageKind::VirtualCounter { bcb } => write!(fmt, "VirtualCounter({bcb:?})"),
+        }
+    }
+}
+
+impl CoverageKind {
+    /// Returns true if this kind of coverage statement is a marker inserted during
+    /// MIR building, for use by analysis in the `InstrumentCoverage` pass, and is
+    /// no longer needed after that pass.
+    pub fn is_removed_after_analysis(&self) -> bool {
+        match self {
+            CoverageKind::Point { .. } | CoverageKind::BlockMarker { .. } => true,
+            CoverageKind::VirtualCounter { .. } => false,
         }
     }
 }

compiler/rustc_middle/src/mir/syntax.rs

@@ -104,13 +104,14 @@ pub enum AnalysisPhase {
     /// * [`TerminatorKind::FalseEdge`]
     /// * [`StatementKind::FakeRead`]
     /// * [`StatementKind::AscribeUserType`]
-    /// * [`StatementKind::Coverage`] with [`CoverageKind::BlockMarker`] or
-    ///   [`CoverageKind::SpanMarker`]
+    /// * [`StatementKind::Coverage`] with [`CoverageKind::is_removed_after_analysis`]
     /// * [`Rvalue::Ref`] with `BorrowKind::Fake`
     /// * [`CastKind::PointerCoercion`] with any of the following:
     ///   * [`PointerCoercion::ArrayToPointer`]
     ///   * [`PointerCoercion::MutToConstPointer`]
     ///
+    /// [`CoverageKind::is_removed_after_analysis`]: crate::mir::coverage::CoverageKind::is_removed_after_analysis
+    ///
     /// Furthermore, `Deref` projections must be the first projection within any place (if they
     /// appear at all)
     PostCleanup = 1,

compiler/rustc_middle/src/thir.rs

@@ -257,6 +257,9 @@ pub struct Expr<'tcx> {
 
     /// The id of the HIR expression whose [temporary scope] should be used for this expression.
     ///
+    /// Also used by coverage instrumentation to recover the HIR node that corresponds to a THIR
+    /// expression node.
+    ///
     /// [temporary scope]: https://doc.rust-lang.org/reference/destructors.html#temporary-scopes
     pub temp_scope_id: hir::ItemLocalId,
 

compiler/rustc_mir_build/src/builder/cfg.rs

@@ -107,17 +107,6 @@ impl<'tcx> CFG<'tcx> {
         self.push(block, stmt);
     }
 
-    /// Adds a dummy statement whose only role is to associate a span with its
-    /// enclosing block for the purposes of coverage instrumentation.
-    ///
-    /// This results in more accurate coverage reports for certain kinds of
-    /// syntax (e.g. `continue` or `if !`) that would otherwise not appear in MIR.
-    pub(crate) fn push_coverage_span_marker(&mut self, block: BasicBlock, source_info: SourceInfo) {
-        let kind = StatementKind::Coverage(coverage::CoverageKind::SpanMarker);
-        let stmt = Statement::new(source_info, kind);
-        self.push(block, stmt);
-    }
-
     pub(crate) fn terminate(
         &mut self,
         block: BasicBlock,

compiler/rustc_mir_build/src/builder/coverageinfo.rs

@@ -2,16 +2,22 @@ use std::assert_matches;
 use std::collections::hash_map::Entry;
 
 use rustc_data_structures::fx::FxHashMap;
-use rustc_middle::mir::coverage::{BlockMarkerId, BranchSpan, CoverageEarlyInfo, CoverageKind};
-use rustc_middle::mir::{self, BasicBlock, SourceInfo, UnOp};
-use rustc_middle::thir::{ExprId, ExprKind, Pat, Thir};
+use rustc_hir::HirId;
+use rustc_middle::mir::coverage::{
+    BlockMarkerId, BranchSpan, CoverageEarlyInfo, CoverageKind, PointKind,
+};
+use rustc_middle::mir::{self, BasicBlock, SourceInfo, Statement, UnOp};
+use rustc_middle::thir::{self, ExprId, ExprKind, Pat, Thir};
 use rustc_middle::ty::TyCtxt;
 use rustc_span::def_id::LocalDefId;
 
 use crate::builder::{Builder, CFG};
 
 /// Collects coverage-related information during MIR building, to eventually be
 /// turned into a function's [`CoverageEarlyInfo`] when MIR building is complete.
+///
+/// FIXME(Zalathar): Now that we have [`CoverageKind::Point`], we should be able
+/// to remove this and perform HIR-aware analysis during instrumentation instead.
 pub(crate) struct CoverageInfoBuilder {
     /// Maps condition expressions to their enclosing `!`, for better instrumentation.
     nots: FxHashMap<ExprId, NotInfo>,
@@ -175,6 +181,73 @@ impl CoverageInfoBuilder {
 }
 
 impl<'tcx> Builder<'_, 'tcx> {
+    /// Does nothing if `-Cinstrument-coverage` is not enabled.
+    ///
+    /// Otherwise, pushes a marker statement to `block` indicating that this is where
+    /// the HIR expression `hir_id` is being evaluated.
+    pub(crate) fn push_coverage_point_for_expr(
+        &mut self,
+        block: BasicBlock,
+        source_info: SourceInfo,
+        hir_id: HirId,
+    ) {
+        if !self.tcx.sess.instrument_coverage() {
+            return;
+        }
+        self.push_coverage_point_inner(block, source_info, PointKind::Expr, hir_id);
+    }
+
+    /// Does nothing if `-Cinstrument-coverage` is not enabled.
+    ///
+    /// Otherwise, pushes a marker statement to `block` indicating that this is where
+    /// the one-sided if-expression `if_expr` will generate its synthetic `else {}`
+    /// path, since it lacks an explicit `else` block.
+    pub(crate) fn push_coverage_point_for_implicit_else(
+        &mut self,
+        block: BasicBlock,
+        source_info: SourceInfo,
+        if_expr: &thir::Expr<'tcx>,
+    ) {
+        if !self.tcx.sess.instrument_coverage() {
+            return;
+        }
+        // Recover the full HirId by combining a local ID with the function's owner ID.
+        let hir_id = HirId { owner: self.hir_id.owner, local_id: if_expr.temp_scope_id };
+        self.push_coverage_point_inner(block, source_info, PointKind::ImplicitElse, hir_id);
+    }
+
+    /// Does nothing if `-Cinstrument-coverage` is not enabled.
+    ///
+    /// Otherwise, pushes a marker statement to `block` indicating that this is where
+    /// the function `fn_hir_id` would implicitly return at the end of its body.
+    pub(crate) fn push_coverage_point_for_fn_end(
+        &mut self,
+        block: BasicBlock,
+        source_info: SourceInfo,
+        fn_hir_id: HirId,
+    ) {
+        if !self.tcx.sess.instrument_coverage() {
+            return;
+        }
+        self.push_coverage_point_inner(block, source_info, PointKind::FunctionEnd, fn_hir_id);
+    }
+
+    fn push_coverage_point_inner(
+        &mut self,
+        block: BasicBlock,
+        source_info: SourceInfo,
+        point_kind: PointKind,
+        hir_id: HirId,
+    ) {
+        assert!(self.tcx.sess.instrument_coverage());
+
+        let stmt = Statement::new(
+            source_info,
+            mir::StatementKind::Coverage(CoverageKind::Point { point_kind, hir_id }),
+        );
+        self.cfg.push(block, stmt);
+    }
+
     /// If condition coverage is enabled, inject extra blocks and marker statements
     /// that will let us track the value of the condition in `place`.
     pub(crate) fn visit_coverage_standalone_condition(

compiler/rustc_mir_build/src/builder/expr/as_operand.rs

@@ -127,6 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             let source_info = this.source_info(expr.span);
             let region_scope = (region_scope, source_info);
             return this.in_scope(region_scope, LintLevel::Explicit(hir_id), |this| {
+                this.push_coverage_point_for_expr(block, source_info, hir_id);
                 this.as_operand(block, scope, value, local_info, needs_temporary)
             });
         }
@@ -170,6 +171,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             let source_info = this.source_info(expr.span);
             let region_scope = (region_scope, source_info);
             return this.in_scope(region_scope, LintLevel::Explicit(hir_id), |this| {
+                this.push_coverage_point_for_expr(block, source_info, hir_id);
                 this.as_call_operand(block, scope, value)
             });
         }

compiler/rustc_mir_build/src/builder/expr/as_place.rs

@@ -430,6 +430,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         match expr.kind {
             ExprKind::Scope { region_scope, hir_id, value } => {
                 this.in_scope((region_scope, source_info), LintLevel::Explicit(hir_id), |this| {
+                    this.push_coverage_point_for_expr(block, source_info, hir_id);
                     this.expr_as_place(block, value, mutability, fake_borrow_temps)
                 })
             }

compiler/rustc_mir_build/src/builder/expr/as_rvalue.rs

@@ -58,6 +58,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             ExprKind::Scope { region_scope, hir_id, value } => {
                 let region_scope = (region_scope, source_info);
                 this.in_scope(region_scope, LintLevel::Explicit(hir_id), |this| {
+                    this.push_coverage_point_for_expr(block, source_info, hir_id);
                     this.as_rvalue(block, scope, value)
                 })
             }

compiler/rustc_mir_build/src/builder/expr/as_temp.rs

@@ -29,7 +29,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             return this.in_scope(
                 (region_scope, source_info),
                 LintLevel::Explicit(hir_id),
-                |this| this.as_temp(block, temp_lifetime, value, mutability),
+                |this| {
+                    this.push_coverage_point_for_expr(block, source_info, hir_id);
+                    this.as_temp(block, temp_lifetime, value, mutability)
+                },
             );
         }
 

compiler/rustc_mir_build/src/builder/expr/into.rs

@@ -48,6 +48,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             ExprKind::Scope { region_scope, hir_id, value } => {
                 let region_scope = (region_scope, source_info);
                 this.in_scope(region_scope, LintLevel::Explicit(hir_id), |this| {
+                    this.push_coverage_point_for_expr(block, source_info, hir_id);
                     this.expr_into_dest(destination, block, value)
                 })
             }
@@ -114,6 +115,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     // There is no `else` arm, so we know both arms have type `()`.
                     // Generate the implicit `else {}` by assigning unit.
                     let correct_si = this.source_info(expr_span.shrink_to_hi());
+                    this.push_coverage_point_for_implicit_else(else_blk, correct_si, expr);
                     this.cfg.push_assign_unit(else_blk, correct_si, destination, this.tcx);
                 }
 

compiler/rustc_mir_build/src/builder/expr/stmt.rs

@@ -27,6 +27,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         match expr.kind {
             ExprKind::Scope { region_scope, hir_id, value } => {
                 this.in_scope((region_scope, source_info), LintLevel::Explicit(hir_id), |this| {
+                    this.push_coverage_point_for_expr(block, source_info, hir_id);
                     this.stmt_expr(block, value, statement_scope)
                 })
             }

compiler/rustc_mir_build/src/builder/matches/mod.rs

@@ -164,11 +164,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 let local_scope = this.local_scope();
                 let (success_block, failure_block) =
                     this.in_if_then_scope(local_scope, expr_span, |this| {
-                        // Help out coverage instrumentation by injecting a dummy statement with
-                        // the original condition's span (including `!`). This fixes #115468.
-                        if this.tcx.sess.instrument_coverage() {
-                            this.cfg.push_coverage_span_marker(block, this.source_info(expr_span));
-                        }
                         this.then_else_break_inner(
                             block,
                             arg,
@@ -182,8 +177,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 failure_block.unit()
             }
             ExprKind::Scope { region_scope, hir_id, value } => {
-                let region_scope = (region_scope, this.source_info(expr_span));
-                this.in_scope(region_scope, LintLevel::Explicit(hir_id), |this| {
+                let source_info = this.source_info(expr_span);
+                this.in_scope((region_scope, source_info), LintLevel::Explicit(hir_id), |this| {
+                    this.push_coverage_point_for_expr(block, source_info, hir_id);
                     this.then_else_break_inner(block, value, args)
                 })
             }

compiler/rustc_mir_build/src/builder/mod.rs

@@ -548,6 +548,7 @@ fn construct_fn<'tcx>(
             })
             .into_block();
         let source_info = builder.source_info(fn_end);
+        builder.push_coverage_point_for_fn_end(return_block, source_info, fn_id);
         builder.cfg.terminate(return_block, source_info, TerminatorKind::Return);
         builder.build_drop_trees();
         return_block.unit()

compiler/rustc_mir_build/src/builder/scope.rs

@@ -791,15 +791,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             (None, Some(_)) => {
                 panic!("`return`, `become` and `break` with value and must have a destination")
             }
-            (None, None) => {
-                if self.tcx.sess.instrument_coverage() {
-                    // Normally we wouldn't build any MIR in this case, but that makes it
-                    // harder for coverage instrumentation to extract a relevant span for
-                    // `continue` expressions. So here we inject a dummy statement with the
-                    // desired span.
-                    self.cfg.push_coverage_span_marker(block, source_info);
-                }
-            }
+            (None, None) => {}
         }
 
         let region_scope = self.scopes.breakable_scopes[break_index].region_scope;

compiler/rustc_mir_build/src/thir/cx/expr.rs

@@ -134,6 +134,9 @@ impl<'tcx> ThirBuildCx<'tcx> {
         }
 
         // Finally, wrap this up in the expr's scope.
+        //
+        // (In addition to marking scope, coverage instrumentation also uses this node
+        // to help mark the point in MIR where an expression is about to be evaluated.)
         expr = Expr {
             temp_scope_id: expr_scope.local_id,
             ty,

compiler/rustc_mir_transform/src/cleanup_post_borrowck.rs

@@ -5,18 +5,16 @@
 //!   - [`AscribeUserType`]
 //!   - [`FakeRead`]
 //!   - [`Assign`] statements with a [`Fake`] borrow
-//!   - [`Coverage`] statements of kind [`BlockMarker`] or [`SpanMarker`]
+//!   - [`Coverage`] statements that are not needed after the [`InstrumentCoverage`] pass
 //!
 //! [`AscribeUserType`]: rustc_middle::mir::StatementKind::AscribeUserType
 //! [`Assign`]: rustc_middle::mir::StatementKind::Assign
 //! [`FakeRead`]: rustc_middle::mir::StatementKind::FakeRead
 //! [`Nop`]: rustc_middle::mir::StatementKind::Nop
 //! [`Fake`]: rustc_middle::mir::BorrowKind::Fake
 //! [`Coverage`]: rustc_middle::mir::StatementKind::Coverage
-//! [`BlockMarker`]: rustc_middle::mir::coverage::CoverageKind::BlockMarker
-//! [`SpanMarker`]: rustc_middle::mir::coverage::CoverageKind::SpanMarker
+//! [`InstrumentCoverage`]: crate::coverage::InstrumentCoverage
 
-use rustc_middle::mir::coverage::CoverageKind;
 use rustc_middle::mir::*;
 use rustc_middle::ty::TyCtxt;
 use rustc_middle::ty::adjustment::PointerCoercion;
@@ -34,15 +32,13 @@ impl<'tcx> crate::MirPass<'tcx> for CleanupPostBorrowck {
                 match statement.kind {
                     StatementKind::AscribeUserType(..)
                     | StatementKind::Assign((_, Rvalue::Ref(_, BorrowKind::Fake(_), _)))
-                    | StatementKind::Coverage(
-                        // These kinds of coverage statements are markers inserted during
-                        // MIR building, and are not needed after InstrumentCoverage.
-                        CoverageKind::BlockMarker { .. } | CoverageKind::SpanMarker { .. },
-                    )
                     | StatementKind::FakeRead(..)
                     | StatementKind::BackwardIncompatibleDropHint { .. } => {
                         statement.make_nop(true)
                     }
+                    StatementKind::Coverage(ref kind) if kind.is_removed_after_analysis() => {
+                        statement.make_nop(true)
+                    }
                     StatementKind::Assign((
                         _,
                         Rvalue::Cast(

compiler/rustc_mir_transform/src/coverage/expansion.rs

@@ -103,7 +103,7 @@ pub(crate) fn build_expn_tree(
     hir_info: &ExtractedHirInfo,
     graph: &CoverageGraph,
 ) -> Result<ExpnTree, MappingsError> {
-    let raw_spans = from_mir::extract_raw_spans_from_mir(mir_body, graph);
+    let raw_spans = from_mir::extract_raw_spans_from_mir(mir_body, hir_info, graph);
 
     let mut nodes = FxIndexMap::default();
     let new_node = |&context: &SyntaxContext| ExpnNode::for_context(context);