Rollup merge of #143088 - firefighterduck:improve-doc-discr-tag, r=RalfJung

Improve documentation of `TagEncoding`

This PR is follow-up from the [discussion here](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/.E2.9C.94.20VariantId.3DDiscriminant.20when.20tag.20is.20niche.20encoded.3F/with/524384295).

It aims at making the `TagEncoding` documentation less ambiguous and more detailed with references to relevant implementation sides. It especially clears up the ambiguous use of discriminant/variant index, which sparked the discussion referenced above.

PS: While working with layout data, I somehow ended up looking at the docs for `FakeBorrowKind` and noticed that the one example was not in a doc comment. I hope that this is minor enough of a fix for it to be okay in this otherwise unrelated PR.

4 files changed, 36 insertions, 30 deletions

diff --git a/compiler/rustc_abi/src/lib.rs b/compiler/rustc_abi/src/lib.rs
index 6d729b6919a..0df8921c9b7 100644
--- a/compiler/rustc_abi/src/lib.rs
+++ b/compiler/rustc_abi/src/lib.rs
@@ -1592,24 +1592,33 @@ pub enum TagEncoding<VariantIdx: Idx> {
     /// (so converting the tag to the discriminant can require sign extension).
     Direct,
 
-    /// Niche (values invalid for a type) encoding the discriminant:
-    /// Discriminant and variant index coincide.
+    /// Niche (values invalid for a type) encoding the discriminant.
+    /// Note that for this encoding, the discriminant and variant index of each variant coincide!
+    /// This invariant is codified as part of [`layout_sanity_check`](../rustc_ty_utils/layout/invariant/fn.layout_sanity_check.html).
+    ///
     /// The variant `untagged_variant` contains a niche at an arbitrary
-    /// offset (field `tag_field` of the enum), which for a variant with
-    /// discriminant `d` is set to
-    /// `(d - niche_variants.start).wrapping_add(niche_start)`
-    /// (this is wrapping arithmetic using the type of the niche field).
+    /// offset (field [`Variants::Multiple::tag_field`] of the enum).
+    /// For a variant with variant index `i`, such that `i != untagged_variant`,
+    /// the tag is set to `(i - niche_variants.start).wrapping_add(niche_start)`
+    /// (this is wrapping arithmetic using the type of the niche field, cf. the
+    /// [`tag_for_variant`](../rustc_const_eval/interpret/struct.InterpCx.html#method.tag_for_variant)
+    /// query implementation).
+    /// To recover the variant index `i` from a `tag`, the above formula has to be reversed,
+    /// i.e. `i = tag.wrapping_sub(niche_start) + niche_variants.start`. If `i` ends up outside
+    /// `niche_variants`, the tag must have encoded the `untagged_variant`.
     ///
-    /// For example, `Option<(usize, &T)>`  is represented such that
-    /// `None` has a null pointer for the second tuple field, and
-    /// `Some` is the identity function (with a non-null reference).
+    /// For example, `Option<(usize, &T)>`  is represented such that the tag for
+    /// `None` is the null pointer in the second tuple field, and
+    /// `Some` is the identity function (with a non-null reference)
+    /// and has no additional tag, i.e. the reference being non-null uniquely identifies this variant.
     ///
     /// Other variants that are not `untagged_variant` and that are outside the `niche_variants`
     /// range cannot be represented; they must be uninhabited.
+    /// Nonetheless, uninhabited variants can also fall into the range of `niche_variants`.
     Niche {
         untagged_variant: VariantIdx,
-        /// This range *may* contain `untagged_variant`; that is then just a "dead value" and
-        /// not used to encode anything.
+        /// This range *may* contain `untagged_variant` or uninhabited variants;
+        /// these are then just "dead values" and not used to encode anything.
         niche_variants: RangeInclusive<VariantIdx>,
         /// This is inbounds of the type of the niche field
         /// (not sign-extended, i.e., all bits beyond the niche field size are 0).
diff --git a/compiler/rustc_codegen_ssa/src/mir/operand.rs b/compiler/rustc_codegen_ssa/src/mir/operand.rs
index 99957c67708..da615cc9a00 100644
--- a/compiler/rustc_codegen_ssa/src/mir/operand.rs
+++ b/compiler/rustc_codegen_ssa/src/mir/operand.rs
@@ -479,17 +479,8 @@ impl<'a, 'tcx, V: CodegenObject> OperandRef<'tcx, V> {
                     _ => (tag_imm, bx.cx().immediate_backend_type(tag_op.layout)),
                 };
 
-                // Layout ensures that we only get here for cases where the discriminant
+                // `layout_sanity_check` ensures that we only get here for cases where the discriminant
                 // value and the variant index match, since that's all `Niche` can encode.
-                // But for emphasis and debugging, let's double-check one anyway.
-                debug_assert_eq!(
-                    self.layout
-                        .ty
-                        .discriminant_for_variant(bx.tcx(), untagged_variant)
-                        .unwrap()
-                        .val,
-                    u128::from(untagged_variant.as_u32()),
-                );
 
                 let relative_max = niche_variants.end().as_u32() - niche_variants.start().as_u32();
 
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
index c50a30cf7f3..92eefd89848 100644
--- a/compiler/rustc_middle/src/mir/syntax.rs
+++ b/compiler/rustc_middle/src/mir/syntax.rs
@@ -284,15 +284,15 @@ pub enum FakeBorrowKind {
     ///
     /// This is used when lowering deref patterns, where shallow borrows wouldn't prevent something
     /// like:
-    // ```compile_fail
-    // let mut b = Box::new(false);
-    // match b {
-    //     deref!(true) => {} // not reached because `*b == false`
-    //     _ if { *b = true; false } => {} // not reached because the guard is `false`
-    //     deref!(false) => {} // not reached because the guard changed it
-    //     // UB because we reached the unreachable.
-    // }
-    // ```
+    /// ```compile_fail
+    /// let mut b = Box::new(false);
+    /// match b {
+    ///     deref!(true) => {} // not reached because `*b == false`
+    ///     _ if { *b = true; false } => {} // not reached because the guard is `false`
+    ///     deref!(false) => {} // not reached because the guard changed it
+    ///     // UB because we reached the unreachable.
+    /// }
+    /// ```
     Deep,
 }
 
diff --git a/compiler/rustc_ty_utils/src/layout/invariant.rs b/compiler/rustc_ty_utils/src/layout/invariant.rs
index c929de11624..4b65c05d0e9 100644
--- a/compiler/rustc_ty_utils/src/layout/invariant.rs
+++ b/compiler/rustc_ty_utils/src/layout/invariant.rs
@@ -277,6 +277,12 @@ pub(super) fn layout_sanity_check<'tcx>(cx: &LayoutCx<'tcx>, layout: &TyAndLayou
                     if !variant.is_uninhabited() {
                         assert!(idx == *untagged_variant || niche_variants.contains(&idx));
                     }
+
+                    // Ensure that for niche encoded tags the discriminant coincides with the variant index.
+                    assert_eq!(
+                        layout.ty.discriminant_for_variant(tcx, idx).unwrap().val,
+                        u128::from(idx.as_u32()),
+                    );
                 }
             }
             for variant in variants.iter() {