Rollup merge of #98814 - fmease:minimal-fix-for-issue-97933, r=GuillaumeGomez

rustdoc: Censor certain complex unevaluated const exprs

Fixes #97933.

This is more of a hotfix for the aforementioned issue. By that, I mean that my proposed patch is
not the best solution but one that does not change as much existing code.
It treats symptoms rather than the root cause.

This PR “censors” certain complex unevaluated constant expressions like `match`es, blocks, function calls, struct literals etc. by pretty-printing them as `_` / `{ _ }` (number and string literals, paths and `()` are still printed as one would expect).
Resorting to this placeholder is preferable to printing the full expression verbatim since
they can be quite large and verbose resulting in an unreadable mess in the generated documentation.
Further, mindlessly printing the const would leak private and `doc(hidden)` struct fields (#97933), at least in the current
stable & nightly implementations which rely on `span_to_snippet` (!) and `rustc_hir_pretty::id_to_string`.

The censoring of _verbose_ expressions is probably going to stay longer term.
However, in regards to private and `doc(hidden)` struct fields, I have a more proper fix in mind
which I have already partially implemented locally and for which I am going to open a separate PR sometime soon.
For that, I was already in contact with `@GuillaumeGomez.`
The proper fix involves rustdoc not falling back on pretty-printing unevaluated consts so easily (what this PR is concerned about)
and instead preferring to print evaluated consts which contain more information allowing it to selectively hide private and `doc(hidden)` fields, create hyperlinks etc. generally making the output more granular and precise (compared to the brutal `_` placeholder).

Unfortunately, I was a bit too late and the issue just hit stable (1.62).
Should this be backported to beta or even a potential 1.62.1?

r? `@GuillaumeGomez`

Author: matthiaskrgr

Diff for: src/librustdoc/clean/utils.rs

@@ -340,17 +340,98 @@ pub(crate) fn is_literal_expr(tcx: TyCtxt<'_>, hir_id: hir::HirId) -> bool {
     false
 }
 
+/// Build a textual representation of an unevaluated constant expression.
+///
+/// If the const expression is too complex, an underscore `_` is returned.
+/// For const arguments, it's `{ _ }` to be precise.
+/// This means that the output is not necessarily valid Rust code.
+///
+/// Currently, only
+///
+/// * literals (optionally with a leading `-`)
+/// * unit `()`
+/// * blocks (`{ … }`) around simple expressions and
+/// * paths without arguments
+///
+/// are considered simple enough. Simple blocks are included since they are
+/// necessary to disambiguate unit from the unit type.
+/// This list might get extended in the future.
+///
+/// Without this censoring, in a lot of cases the output would get too large
+/// and verbose. Consider `match` expressions, blocks and deeply nested ADTs.
+/// Further, private and `doc(hidden)` fields of structs would get leaked
+/// since HIR datatypes like the `body` parameter do not contain enough
+/// semantic information for this function to be able to hide them –
+/// at least not without significant performance overhead.
+///
+/// Whenever possible, prefer to evaluate the constant first and try to
+/// use a different method for pretty-printing. Ideally this function
+/// should only ever be used as a fallback.
 pub(crate) fn print_const_expr(tcx: TyCtxt<'_>, body: hir::BodyId) -> String {
     let hir = tcx.hir();
     let value = &hir.body(body).value;
 
-    let snippet = if !value.span.from_expansion() {
-        tcx.sess.source_map().span_to_snippet(value.span).ok()
-    } else {
-        None
-    };
+    #[derive(PartialEq, Eq)]
+    enum Classification {
+        Literal,
+        Simple,
+        Complex,
+    }
 
-    snippet.unwrap_or_else(|| rustc_hir_pretty::id_to_string(&hir, body.hir_id))
+    use Classification::*;
+
+    fn classify(expr: &hir::Expr<'_>) -> Classification {
+        match &expr.kind {
+            hir::ExprKind::Unary(hir::UnOp::Neg, expr) => {
+                if matches!(expr.kind, hir::ExprKind::Lit(_)) { Literal } else { Complex }
+            }
+            hir::ExprKind::Lit(_) => Literal,
+            hir::ExprKind::Tup([]) => Simple,
+            hir::ExprKind::Block(hir::Block { stmts: [], expr: Some(expr), .. }, _) => {
+                if classify(expr) == Complex { Complex } else { Simple }
+            }
+            // Paths with a self-type or arguments are too “complex” following our measure since
+            // they may leak private fields of structs (with feature `adt_const_params`).
+            // Consider: `<Self as Trait<{ Struct { private: () } }>>::CONSTANT`.
+            // Paths without arguments are definitely harmless though.
+            hir::ExprKind::Path(hir::QPath::Resolved(_, hir::Path { segments, .. })) => {
+                if segments.iter().all(|segment| segment.args.is_none()) { Simple } else { Complex }
+            }
+            // FIXME: Claiming that those kinds of QPaths are simple is probably not true if the Ty
+            //        contains const arguments. Is there a *concise* way to check for this?
+            hir::ExprKind::Path(hir::QPath::TypeRelative(..)) => Simple,
+            // FIXME: Can they contain const arguments and thus leak private struct fields?
+            hir::ExprKind::Path(hir::QPath::LangItem(..)) => Simple,
+            _ => Complex,
+        }
+    }
+
+    let classification = classify(value);
+
+    if classification == Literal
+    && !value.span.from_expansion()
+    && let Ok(snippet) = tcx.sess.source_map().span_to_snippet(value.span) {
+        // For literals, we avoid invoking the pretty-printer and use the source snippet instead to
+        // preserve certain stylistic choices the user likely made for the sake legibility like
+        //
+        // * hexadecimal notation
+        // * underscores
+        // * character escapes
+        //
+        // FIXME: This passes through `-/*spacer*/0` verbatim.
+        snippet
+    } else if classification == Simple {
+        // Otherwise we prefer pretty-printing to get rid of extraneous whitespace, comments and
+        // other formatting artifacts.
+        rustc_hir_pretty::id_to_string(&hir, body.hir_id)
+    } else if tcx.def_kind(hir.body_owner_def_id(body).to_def_id()) == DefKind::AnonConst {
+        // FIXME: Omit the curly braces if the enclosing expression is an array literal
+        //        with a repeated element (an `ExprKind::Repeat`) as in such case it
+        //        would not actually need any disambiguation.
+        "{ _ }".to_owned()
+    } else {
+        "_".to_owned()
+    }
 }
 
 /// Given a type Path, resolve it to a Type using the TyCtxt

Diff for: src/librustdoc/html/render/mod.rs

@@ -716,12 +716,14 @@ fn assoc_const(
         ty = ty.print(cx),
     );
     if let Some(default) = default {
+        write!(w, " = ");
+
         // FIXME: `.value()` uses `clean::utils::format_integer_with_underscore_sep` under the
         //        hood which adds noisy underscores and a type suffix to number literals.
         //        This hurts readability in this context especially when more complex expressions
         //        are involved and it doesn't add much of value.
         //        Find a way to print constants here without all that jazz.
-        write!(w, " = {}", default.value(cx.tcx()).unwrap_or_else(|| default.expr(cx.tcx())));
+        write!(w, "{}", Escape(&default.value(cx.tcx()).unwrap_or_else(|| default.expr(cx.tcx()))));
     }
 }
 

Diff for: src/librustdoc/html/render/print_item.rs

@@ -1360,6 +1360,15 @@ fn item_constant(w: &mut Buffer, cx: &mut Context<'_>, it: &clean::Item, c: &cle
                 typ = c.type_.print(cx),
             );
 
+            // FIXME: The code below now prints
+            //            ` = _; // 100i32`
+            //        if the expression is
+            //            `50 + 50`
+            //        which looks just wrong.
+            //        Should we print
+            //            ` = 100i32;`
+            //        instead?
+
             let value = c.value(cx.tcx());
             let is_literal = c.is_literal(cx.tcx());
             let expr = c.expr(cx.tcx());

Diff for: src/test/rustdoc/assoc-consts.rs

@@ -27,6 +27,10 @@ impl Bar {
     // @has assoc_consts/struct.Bar.html '//*[@id="associatedconstant.BAR"]' \
     //      'const BAR: usize'
     pub const BAR: usize = 3;
+
+    // @has - '//*[@id="associatedconstant.BAR_ESCAPED"]' \
+    //      "const BAR_ESCAPED: &'static str = \"<em>markup</em>\""
+    pub const BAR_ESCAPED: &'static str = "<em>markup</em>";
 }
 
 pub struct Baz<'a, U: 'a, T>(T, &'a [U]);

Diff for: src/test/rustdoc/const-value-display.rs

@@ -1,9 +1,9 @@
 #![crate_name = "foo"]
 
 // @has 'foo/constant.HOUR_IN_SECONDS.html'
-// @has - '//*[@class="docblock item-decl"]//code' 'pub const HOUR_IN_SECONDS: u64 = 60 * 60; // 3_600u64'
+// @has - '//*[@class="docblock item-decl"]//code' 'pub const HOUR_IN_SECONDS: u64 = _; // 3_600u64'
 pub const HOUR_IN_SECONDS: u64 = 60 * 60;
 
 // @has 'foo/constant.NEGATIVE.html'
-// @has - '//*[@class="docblock item-decl"]//code' 'pub const NEGATIVE: i64 = -60 * 60; // -3_600i64'
+// @has - '//*[@class="docblock item-decl"]//code' 'pub const NEGATIVE: i64 = _; // -3_600i64'
 pub const NEGATIVE: i64 = -60 * 60;

Diff for: src/test/rustdoc/hide-complex-unevaluated-const-arguments.rs

@@ -0,0 +1,82 @@
+// Test that certain unevaluated constant expression arguments that are
+// deemed too verbose or complex and that may leak private or
+// `doc(hidden)` struct fields are not displayed in the documentation.
+//
+// Read the documentation of `rustdoc::clean::utils::print_const_expr`
+// for further details.
+#![feature(const_trait_impl, generic_const_exprs)]
+#![allow(incomplete_features)]
+
+// @has hide_complex_unevaluated_const_arguments/trait.Stage.html
+pub trait Stage {
+    // A helper constant that prevents const expressions containing it
+    // from getting fully evaluated since it doesn't have a body and
+    // thus is non-reducible. This allows us to specifically test the
+    // pretty-printing of *unevaluated* consts.
+    const ABSTRACT: usize;
+
+    // Currently considered "overly complex" by the `generic_const_exprs`
+    // feature. If / once this expression kind gets supported, this
+    // unevaluated const expression could leak the private struct field.
+    //
+    // FIXME: Once the line below compiles, make this a test that
+    //        ensures that the private field is not printed.
+    //
+    //const ARRAY0: [u8; Struct { private: () } + Self::ABSTRACT];
+
+    // This assoc. const could leak the private assoc. function `Struct::new`.
+    // Ensure that this does not happen.
+    //
+    // @has - '//*[@id="associatedconstant.ARRAY1"]' \
+    //        'const ARRAY1: [u8; { _ }]'
+    const ARRAY1: [u8; Struct::new(/* ... */) + Self::ABSTRACT * 1_000];
+
+    // @has - '//*[@id="associatedconstant.VERBOSE"]' \
+    //        'const VERBOSE: [u16; { _ }]'
+    const VERBOSE: [u16; compute("thing", 9 + 9) * Self::ABSTRACT];
+
+    // Check that we do not leak the private struct field contained within
+    // the path. The output could definitely be improved upon
+    // (e.g. printing sth. akin to `<Self as Helper<{ _ }>>::OUT`) but
+    // right now “safe is safe”.
+    //
+    // @has - '//*[@id="associatedconstant.PATH"]' \
+    //        'const PATH: usize = _'
+    const PATH: usize = <Self as Helper<{ Struct { private: () } }>>::OUT;
+}
+
+const fn compute(input: &str, extra: usize) -> usize {
+    input.len() + extra
+}
+
+pub trait Helper<const S: Struct> {
+    const OUT: usize;
+}
+
+impl<const S: Struct, St: Stage + ?Sized> Helper<S> for St {
+    const OUT: usize = St::ABSTRACT;
+}
+
+// Currently in rustdoc, const arguments are not evaluated in this position
+// and therefore they fall under the realm of `print_const_expr`.
+// If rustdoc gets patched to evaluate const arguments, it is fine to replace
+// this test as long as one can ensure that private fields are not leaked!
+//
+// @has hide_complex_unevaluated_const_arguments/trait.Sub.html \
+//      '//*[@class="rust trait"]' \
+//      'pub trait Sub: Sup<{ _ }, { _ }> { }'
+pub trait Sub: Sup<{ 90 * 20 * 4 }, { Struct { private: () } }> {}
+
+pub trait Sup<const N: usize, const S: Struct> {}
+
+pub struct Struct { private: () }
+
+impl Struct {
+    const fn new() -> Self { Self { private: () } }
+}
+
+impl const std::ops::Add<usize> for Struct {
+    type Output = usize;
+
+    fn add(self, _: usize) -> usize { 0 }
+}

Diff for: src/test/rustdoc/hide-complex-unevaluated-consts.rs

@@ -0,0 +1,71 @@
+// Regression test for issue #97933.
+//
+// Test that certain unevaluated constant expressions that are
+// deemed too verbose or complex and that may leak private or
+// `doc(hidden)` struct fields are not displayed in the documentation.
+//
+// Read the documentation of `rustdoc::clean::utils::print_const_expr`
+// for further details.
+
+// @has hide_complex_unevaluated_consts/trait.Container.html
+pub trait Container {
+    // A helper constant that prevents const expressions containing it
+    // from getting fully evaluated since it doesn't have a body and
+    // thus is non-reducible. This allows us to specifically test the
+    // pretty-printing of *unevaluated* consts.
+    const ABSTRACT: i32;
+
+    // Ensure that the private field does not get leaked:
+    //
+    // @has - '//*[@id="associatedconstant.STRUCT0"]' \
+    //        'const STRUCT0: Struct = _'
+    const STRUCT0: Struct = Struct { private: () };
+
+    // @has - '//*[@id="associatedconstant.STRUCT1"]' \
+    //        'const STRUCT1: (Struct,) = _'
+    const STRUCT1: (Struct,) = (Struct{private: /**/()},);
+
+    // Although the struct field is public here, check that it is not
+    // displayed. In a future version of rustdoc, we definitely want to
+    // show it. However for the time being, the printing logic is a bit
+    // conservative.
+    //
+    // @has - '//*[@id="associatedconstant.STRUCT2"]' \
+    //        'const STRUCT2: Record = _'
+    const STRUCT2: Record = Record { public: 5 };
+
+    // Test that we do not show the incredibly verbose match expr:
+    //
+    // @has - '//*[@id="associatedconstant.MATCH0"]' \
+    //        'const MATCH0: i32 = _'
+    const MATCH0: i32 = match 234 {
+        0 => 1,
+        _ => Self::ABSTRACT,
+    };
+
+    // @has - '//*[@id="associatedconstant.MATCH1"]' \
+    //        'const MATCH1: bool = _'
+    const MATCH1: bool = match Self::ABSTRACT {
+        _ => true,
+    };
+
+    // Check that we hide complex (arithmetic) operations.
+    // In this case, it is a bit unfortunate since the expression
+    // is not *that* verbose and it might be quite useful to the reader.
+    //
+    // However in general, the expression might be quite large and
+    // contain match expressions and structs with private fields.
+    // We would need to recurse over the whole expression and even more
+    // importantly respect operator precedence when pretty-printing
+    // the potentially partially censored expression.
+    // For now, the implementation is quite simple and the choices
+    // rather conservative.
+    //
+    // @has - '//*[@id="associatedconstant.ARITH_OPS"]' \
+    //        'const ARITH_OPS: i32 = _'
+    const ARITH_OPS: i32 = Self::ABSTRACT * 2 + 1;
+}
+
+pub struct Struct { private: () }
+
+pub struct Record { pub public: i32 }

Diff for: src/test/rustdoc/show-const-contents.rs

@@ -21,7 +21,7 @@ pub const CONST_NEG_I32: i32 = -42;
 // @!has show_const_contents/constant.CONST_EQ_TO_VALUE_I32.html '// 42i32'
 pub const CONST_EQ_TO_VALUE_I32: i32 = 42i32;
 
-// @has show_const_contents/constant.CONST_CALC_I32.html '= 42 + 1; // 43i32'
+// @has show_const_contents/constant.CONST_CALC_I32.html '= _; // 43i32'
 pub const CONST_CALC_I32: i32 = 42 + 1;
 
 // @!has show_const_contents/constant.CONST_REF_I32.html '= &42;'