rustdoc: censor certain complex unevaluated const exprs

Author: fmease

src/librustdoc/clean/utils.rs

@@ -343,17 +343,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

src/librustdoc/html/render/mod.rs

@@ -707,12 +707,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()))));
     }
 }
 

src/librustdoc/html/render/print_item.rs

@@ -1356,6 +1356,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());

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]);

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;

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 }
+}

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 }

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;'