From d3181a9a012805133d4b2808108c37cf4b23ea66 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Thu, 23 Jun 2022 17:19:56 +0200 Subject: [PATCH] rustdoc: censor certain complex unevaluated const exprs --- src/librustdoc/clean/utils.rs | 93 +++++++++++++++++-- src/librustdoc/html/render/mod.rs | 4 +- src/librustdoc/html/render/print_item.rs | 9 ++ src/test/rustdoc/assoc-consts.rs | 4 + src/test/rustdoc/const-value-display.rs | 4 +- ...ide-complex-unevaluated-const-arguments.rs | 82 ++++++++++++++++ .../hide-complex-unevaluated-consts.rs | 71 ++++++++++++++ src/test/rustdoc/show-const-contents.rs | 2 +- 8 files changed, 259 insertions(+), 10 deletions(-) create mode 100644 src/test/rustdoc/hide-complex-unevaluated-const-arguments.rs create mode 100644 src/test/rustdoc/hide-complex-unevaluated-consts.rs diff --git a/src/librustdoc/clean/utils.rs b/src/librustdoc/clean/utils.rs index 41ab0e9377d..3bd4d2f01f5 100644 --- a/src/librustdoc/clean/utils.rs +++ b/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: `>::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 --git a/src/librustdoc/html/render/mod.rs b/src/librustdoc/html/render/mod.rs index cb887d16906..7642473b6cd 100644 --- a/src/librustdoc/html/render/mod.rs +++ b/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())))); } } diff --git a/src/librustdoc/html/render/print_item.rs b/src/librustdoc/html/render/print_item.rs index d115185562c..d3e1bdc697f 100644 --- a/src/librustdoc/html/render/print_item.rs +++ b/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()); diff --git a/src/test/rustdoc/assoc-consts.rs b/src/test/rustdoc/assoc-consts.rs index 0ac6dc763df..a79e93145ba 100644 --- a/src/test/rustdoc/assoc-consts.rs +++ b/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 = \"markup\"" + pub const BAR_ESCAPED: &'static str = "markup"; } pub struct Baz<'a, U: 'a, T>(T, &'a [U]); diff --git a/src/test/rustdoc/const-value-display.rs b/src/test/rustdoc/const-value-display.rs index 0ae52592b64..5b2f3c48d57 100644 --- a/src/test/rustdoc/const-value-display.rs +++ b/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 --git a/src/test/rustdoc/hide-complex-unevaluated-const-arguments.rs b/src/test/rustdoc/hide-complex-unevaluated-const-arguments.rs new file mode 100644 index 00000000000..644a6e1cf33 --- /dev/null +++ b/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 `>::OUT`) but + // right now “safe is safe”. + // + // @has - '//*[@id="associatedconstant.PATH"]' \ + // 'const PATH: usize = _' + const PATH: usize = >::OUT; +} + +const fn compute(input: &str, extra: usize) -> usize { + input.len() + extra +} + +pub trait Helper { + const OUT: usize; +} + +impl Helper 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 {} + +pub struct Struct { private: () } + +impl Struct { + const fn new() -> Self { Self { private: () } } +} + +impl const std::ops::Add for Struct { + type Output = usize; + + fn add(self, _: usize) -> usize { 0 } +} diff --git a/src/test/rustdoc/hide-complex-unevaluated-consts.rs b/src/test/rustdoc/hide-complex-unevaluated-consts.rs new file mode 100644 index 00000000000..ba623246a01 --- /dev/null +++ b/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 --git a/src/test/rustdoc/show-const-contents.rs b/src/test/rustdoc/show-const-contents.rs index f5a356bcae6..48b60885974 100644 --- a/src/test/rustdoc/show-const-contents.rs +++ b/src/test/rustdoc/show-const-contents.rs @@ -21,7 +21,7 @@ // @!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;'