Auto merge of #98758 - nnethercote:more-derive-output-improvements, r=Mark-Simulacrum

More derive output improvements

This PR includes:
- Some test improvements.
- Some cosmetic changes to derive output that make the code look more like what a human would write.
- Some more fundamental improvements to `cmp` and `partial_cmp` generation.

r? `@Mark-Simulacrum`

Author: bors

compiler/rustc_ast/src/ast.rs

@@ -2036,6 +2036,14 @@ impl TyKind {
     pub fn is_unit(&self) -> bool {
         matches!(self, TyKind::Tup(tys) if tys.is_empty())
     }
+
+    pub fn is_simple_path(&self) -> Option<Symbol> {
+        if let TyKind::Path(None, Path { segments, .. }) = &self && segments.len() == 1 {
+            Some(segments[0].ident.name)
+        } else {
+            None
+        }
+    }
 }
 
 /// Syntax used to declare a trait object.

compiler/rustc_builtin_macros/src/deriving/clone.rs

@@ -2,8 +2,8 @@ use crate::deriving::generic::ty::*;
 use crate::deriving::generic::*;
 use crate::deriving::path_std;
 
-use rustc_ast::ptr::P;
-use rustc_ast::{self as ast, Expr, Generics, ItemKind, MetaItem, VariantData};
+use rustc_ast::{self as ast, Generics, ItemKind, MetaItem, VariantData};
+use rustc_data_structures::fx::FxHashSet;
 use rustc_expand::base::{Annotatable, ExtCtxt};
 use rustc_span::symbol::{kw, sym, Ident};
 use rustc_span::Span;
@@ -98,22 +98,31 @@ fn cs_clone_simple(
     trait_span: Span,
     substr: &Substructure<'_>,
     is_union: bool,
-) -> P<Expr> {
+) -> BlockOrExpr {
     let mut stmts = Vec::new();
+    let mut seen_type_names = FxHashSet::default();
     let mut process_variant = |variant: &VariantData| {
         for field in variant.fields() {
-            // let _: AssertParamIsClone<FieldTy>;
-            super::assert_ty_bounds(
-                cx,
-                &mut stmts,
-                field.ty.clone(),
-                field.span,
-                &[sym::clone, sym::AssertParamIsClone],
-            );
+            // This basic redundancy checking only prevents duplication of
+            // assertions like `AssertParamIsClone<Foo>` where the type is a
+            // simple name. That's enough to get a lot of cases, though.
+            if let Some(name) = field.ty.kind.is_simple_path() && !seen_type_names.insert(name) {
+                // Already produced an assertion for this type.
+            } else {
+                // let _: AssertParamIsClone<FieldTy>;
+                super::assert_ty_bounds(
+                    cx,
+                    &mut stmts,
+                    field.ty.clone(),
+                    field.span,
+                    &[sym::clone, sym::AssertParamIsClone],
+                );
+            }
         }
     };
 
     if is_union {
+        // Just a single assertion for unions, that the union impls `Copy`.
         // let _: AssertParamIsCopy<Self>;
         let self_ty = cx.ty_path(cx.path_ident(trait_span, Ident::with_dummy_span(kw::SelfUpper)));
         super::assert_ty_bounds(
@@ -139,16 +148,15 @@ fn cs_clone_simple(
             ),
         }
     }
-    stmts.push(cx.stmt_expr(cx.expr_deref(trait_span, cx.expr_self(trait_span))));
-    cx.expr_block(cx.block(trait_span, stmts))
+    BlockOrExpr::new_mixed(stmts, cx.expr_deref(trait_span, cx.expr_self(trait_span)))
 }
 
 fn cs_clone(
     name: &str,
     cx: &mut ExtCtxt<'_>,
     trait_span: Span,
     substr: &Substructure<'_>,
-) -> P<Expr> {
+) -> BlockOrExpr {
     let ctor_path;
     let all_fields;
     let fn_path = cx.std_path(&[sym::clone, sym::Clone, sym::clone]);
@@ -177,7 +185,7 @@ fn cs_clone(
         }
     }
 
-    match *vdata {
+    let expr = match *vdata {
         VariantData::Struct(..) => {
             let fields = all_fields
                 .iter()
@@ -201,5 +209,6 @@ fn cs_clone(
             cx.expr_call(trait_span, path, subcalls)
         }
         VariantData::Unit(..) => cx.expr_path(ctor_path),
-    }
+    };
+    BlockOrExpr::new_expr(expr)
 }

compiler/rustc_builtin_macros/src/deriving/cmp/eq.rs

@@ -2,8 +2,8 @@ use crate::deriving::generic::ty::*;
 use crate::deriving::generic::*;
 use crate::deriving::path_std;
 
-use rustc_ast::ptr::P;
-use rustc_ast::{self as ast, Expr, MetaItem};
+use rustc_ast::{self as ast, MetaItem};
+use rustc_data_structures::fx::FxHashSet;
 use rustc_expand::base::{Annotatable, ExtCtxt};
 use rustc_span::symbol::{sym, Ident};
 use rustc_span::Span;
@@ -52,18 +52,26 @@ fn cs_total_eq_assert(
     cx: &mut ExtCtxt<'_>,
     trait_span: Span,
     substr: &Substructure<'_>,
-) -> P<Expr> {
+) -> BlockOrExpr {
     let mut stmts = Vec::new();
+    let mut seen_type_names = FxHashSet::default();
     let mut process_variant = |variant: &ast::VariantData| {
         for field in variant.fields() {
-            // let _: AssertParamIsEq<FieldTy>;
-            super::assert_ty_bounds(
-                cx,
-                &mut stmts,
-                field.ty.clone(),
-                field.span,
-                &[sym::cmp, sym::AssertParamIsEq],
-            );
+            // This basic redundancy checking only prevents duplication of
+            // assertions like `AssertParamIsEq<Foo>` where the type is a
+            // simple name. That's enough to get a lot of cases, though.
+            if let Some(name) = field.ty.kind.is_simple_path() && !seen_type_names.insert(name) {
+                // Already produced an assertion for this type.
+            } else {
+                // let _: AssertParamIsEq<FieldTy>;
+                super::assert_ty_bounds(
+                    cx,
+                    &mut stmts,
+                    field.ty.clone(),
+                    field.span,
+                    &[sym::cmp, sym::AssertParamIsEq],
+                );
+            }
         }
     };
 
@@ -78,5 +86,5 @@ fn cs_total_eq_assert(
         }
         _ => cx.span_bug(trait_span, "unexpected substructure in `derive(Eq)`"),
     }
-    cx.expr_block(cx.block(trait_span, stmts))
+    BlockOrExpr::new_stmts(stmts)
 }

compiler/rustc_builtin_macros/src/deriving/cmp/ord.rs

@@ -3,7 +3,7 @@ use crate::deriving::generic::*;
 use crate::deriving::path_std;
 
 use rustc_ast::ptr::P;
-use rustc_ast::{self as ast, Expr, MetaItem};
+use rustc_ast::{self as ast, MetaItem};
 use rustc_expand::base::{Annotatable, ExtCtxt};
 use rustc_span::symbol::{sym, Ident};
 use rustc_span::Span;
@@ -51,7 +51,7 @@ pub fn ordering_collapsed(
     cx.expr_call_global(span, fn_cmp_path, vec![lft, rgt])
 }
 
-pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
+pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
     let test_id = Ident::new(sym::cmp, span);
     let equals_path = cx.path_global(span, cx.std_path(&[sym::cmp, sym::Ordering, sym::Equal]));
 
@@ -70,7 +70,7 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<
     // cmp => cmp
     // }
     //
-    cs_fold(
+    let expr = cs_fold(
         // foldr nests the if-elses correctly, leaving the first field
         // as the outermost one, and the last as the innermost.
         false,
@@ -79,15 +79,12 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<
             //     ::std::cmp::Ordering::Equal => old,
             //     cmp => cmp
             // }
-
             let new = {
                 let [other_f] = other_fs else {
                     cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`");
                 };
-
                 let args =
                     vec![cx.expr_addr_of(span, self_f), cx.expr_addr_of(span, other_f.clone())];
-
                 cx.expr_call_global(span, cmp_path.clone(), args)
             };
 
@@ -96,7 +93,21 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<
 
             cx.expr_match(span, new, vec![eq_arm, neq_arm])
         },
-        cx.expr_path(equals_path.clone()),
+        |cx, args| match args {
+            Some((span, self_f, other_fs)) => {
+                let new = {
+                    let [other_f] = other_fs else {
+                            cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`");
+                        };
+                    let args =
+                        vec![cx.expr_addr_of(span, self_f), cx.expr_addr_of(span, other_f.clone())];
+                    cx.expr_call_global(span, cmp_path.clone(), args)
+                };
+
+                new
+            }
+            None => cx.expr_path(equals_path.clone()),
+        },
         Box::new(|cx, span, tag_tuple| {
             if tag_tuple.len() != 2 {
                 cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`")
@@ -107,5 +118,6 @@ pub fn cs_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<
         cx,
         span,
         substr,
-    )
+    );
+    BlockOrExpr::new_expr(expr)
 }

compiler/rustc_builtin_macros/src/deriving/cmp/partial_eq.rs

@@ -15,16 +15,14 @@ pub fn expand_deriving_partial_eq(
     item: &Annotatable,
     push: &mut dyn FnMut(Annotatable),
 ) {
-    // structures are equal if all fields are equal, and non equal, if
-    // any fields are not equal or if the enum variants are different
     fn cs_op(
         cx: &mut ExtCtxt<'_>,
         span: Span,
         substr: &Substructure<'_>,
         op: BinOpKind,
         combiner: BinOpKind,
         base: bool,
-    ) -> P<Expr> {
+    ) -> BlockOrExpr {
         let op = |cx: &mut ExtCtxt<'_>, span: Span, self_f: P<Expr>, other_fs: &[P<Expr>]| {
             let [other_f] = other_fs else {
                 cx.span_bug(span, "not exactly 2 arguments in `derive(PartialEq)`");
@@ -33,7 +31,7 @@ pub fn expand_deriving_partial_eq(
             cx.expr_binary(span, op, self_f, other_f.clone())
         };
 
-        cs_fold1(
+        let expr = cs_fold(
             true, // use foldl
             |cx, span, subexpr, self_f, other_fs| {
                 let eq = op(cx, span, self_f, other_fs);
@@ -52,13 +50,14 @@ pub fn expand_deriving_partial_eq(
             cx,
             span,
             substr,
-        )
+        );
+        BlockOrExpr::new_expr(expr)
     }
 
-    fn cs_eq(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
+    fn cs_eq(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
         cs_op(cx, span, substr, BinOpKind::Eq, BinOpKind::And, true)
     }
-    fn cs_ne(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
+    fn cs_ne(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
         cs_op(cx, span, substr, BinOpKind::Ne, BinOpKind::Or, false)
     }
 

compiler/rustc_builtin_macros/src/deriving/cmp/partial_ord.rs

@@ -48,11 +48,10 @@ pub fn expand_deriving_partial_ord(
     trait_def.expand(cx, mitem, item, push)
 }
 
-pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
+pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
     let test_id = Ident::new(sym::cmp, span);
     let ordering = cx.path_global(span, cx.std_path(&[sym::cmp, sym::Ordering, sym::Equal]));
     let ordering_expr = cx.expr_path(ordering.clone());
-    let equals_expr = cx.expr_some(span, ordering_expr);
 
     let partial_cmp_path = cx.std_path(&[sym::cmp, sym::PartialOrd, sym::partial_cmp]);
 
@@ -69,7 +68,7 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
     // cmp => cmp
     // }
     //
-    cs_fold(
+    let expr = cs_fold(
         // foldr nests the if-elses correctly, leaving the first field
         // as the outermost one, and the last as the innermost.
         false,
@@ -95,7 +94,21 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
 
             cx.expr_match(span, new, vec![eq_arm, neq_arm])
         },
-        equals_expr,
+        |cx: &mut ExtCtxt<'_>, args: Option<(Span, P<Expr>, &[P<Expr>])>| match args {
+            Some((span, self_f, other_fs)) => {
+                let new = {
+                    let [other_f] = other_fs else {
+                            cx.span_bug(span, "not exactly 2 arguments in `derive(Ord)`");
+                        };
+                    let args =
+                        vec![cx.expr_addr_of(span, self_f), cx.expr_addr_of(span, other_f.clone())];
+                    cx.expr_call_global(span, partial_cmp_path.clone(), args)
+                };
+
+                new
+            }
+            None => cx.expr_some(span, ordering_expr.clone()),
+        },
         Box::new(|cx, span, tag_tuple| {
             if tag_tuple.len() != 2 {
                 cx.span_bug(span, "not exactly 2 arguments in `derive(PartialOrd)`")
@@ -110,5 +123,6 @@ pub fn cs_partial_cmp(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_
         cx,
         span,
         substr,
-    )
+    );
+    BlockOrExpr::new_expr(expr)
 }

compiler/rustc_builtin_macros/src/deriving/debug.rs

@@ -2,8 +2,7 @@ use crate::deriving::generic::ty::*;
 use crate::deriving::generic::*;
 use crate::deriving::path_std;
 
-use rustc_ast::ptr::P;
-use rustc_ast::{self as ast, Expr, MetaItem};
+use rustc_ast::{self as ast, MetaItem};
 use rustc_expand::base::{Annotatable, ExtCtxt};
 use rustc_span::symbol::{sym, Ident, Symbol};
 use rustc_span::Span;
@@ -42,7 +41,7 @@ pub fn expand_deriving_debug(
     trait_def.expand(cx, mitem, item, push)
 }
 
-fn show_substructure(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> P<Expr> {
+fn show_substructure(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>) -> BlockOrExpr {
     let (ident, vdata, fields) = match substr.fields {
         Struct(vdata, fields) => (substr.type_ident, *vdata, fields),
         EnumMatching(_, _, v, fields) => (v.ident, &v.data, fields),
@@ -74,7 +73,8 @@ fn show_substructure(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>
     if fields.is_empty() {
         // Special case for no fields.
         let fn_path_write_str = cx.std_path(&[sym::fmt, sym::Formatter, sym::write_str]);
-        cx.expr_call_global(span, fn_path_write_str, vec![fmt, name])
+        let expr = cx.expr_call_global(span, fn_path_write_str, vec![fmt, name]);
+        BlockOrExpr::new_expr(expr)
     } else if fields.len() <= CUTOFF {
         // Few enough fields that we can use a specific-length method.
         let debug = if is_struct {
@@ -100,7 +100,8 @@ fn show_substructure(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>
             let field = cx.expr_addr_of(field.span, field);
             args.push(field);
         }
-        cx.expr_call_global(span, fn_path_debug, args)
+        let expr = cx.expr_call_global(span, fn_path_debug, args);
+        BlockOrExpr::new_expr(expr)
     } else {
         // Enough fields that we must use the any-length method.
         let mut name_exprs = Vec::with_capacity(fields.len());
@@ -176,8 +177,6 @@ fn show_substructure(cx: &mut ExtCtxt<'_>, span: Span, substr: &Substructure<'_>
             stmts.push(names_let.unwrap());
         }
         stmts.push(values_let);
-        stmts.push(cx.stmt_expr(expr));
-
-        cx.expr_block(cx.block(span, stmts))
+        BlockOrExpr::new_mixed(stmts, expr)
     }
 }