Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pretty-print macro matchers instead of using source code #86282

Merged
merged 3 commits into from
Jul 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions compiler/rustc_ast_pretty/src/pprust/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,11 @@ pub fn print_crate<'a>(
s.s.eof()
}

// This makes printed token streams look slightly nicer,
// and also addresses some specific regressions described in #63896 and #73345.
/// This makes printed token streams look slightly nicer,
/// and also addresses some specific regressions described in #63896 and #73345.
fn tt_prepend_space(tt: &TokenTree, prev: &TokenTree) -> bool {
if let TokenTree::Token(token) = prev {
if matches!(token.kind, token::Dot) {
if matches!(token.kind, token::Dot | token::Dollar) {
return false;
}
if let token::DocComment(comment_kind, ..) = token.kind {
Expand Down
28 changes: 12 additions & 16 deletions src/librustdoc/clean/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,9 @@ use rustc_metadata::creader::LoadedMacro;
use rustc_middle::ty::{self, TyCtxt};
use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::Span;

use crate::clean::{
self, Attributes, AttributesExt, FakeDefId, GetDefId, NestedAttributesExt, ToSource, Type,
self, utils, Attributes, AttributesExt, FakeDefId, GetDefId, NestedAttributesExt, Type,
};
use crate::core::DocContext;
use crate::formats::item_type::ItemType;
Expand Down Expand Up @@ -547,23 +546,20 @@ fn build_macro(cx: &mut DocContext<'_>, did: DefId, name: Symbol) -> clean::Item
let imported_from = cx.tcx.crate_name(did.krate);
match cx.enter_resolver(|r| r.cstore().load_macro_untracked(did, cx.sess())) {
LoadedMacro::MacroDef(def, _) => {
let matchers: Vec<Span> = if let ast::ItemKind::MacroDef(ref def) = def.kind {
if let ast::ItemKind::MacroDef(ref def) = def.kind {
let tts: Vec<_> = def.body.inner_tokens().into_trees().collect();
tts.chunks(4).map(|arm| arm[0].span()).collect()
} else {
unreachable!()
};
let matchers = tts.chunks(4).map(|arm| &arm[0]);

let source = format!(
"macro_rules! {} {{\n{}}}",
name.clean(cx),
matchers
.iter()
.map(|span| { format!(" {} => {{ ... }};\n", span.to_src(cx)) })
.collect::<String>()
);
let source = format!(
"macro_rules! {} {{\n{}}}",
name.clean(cx),
utils::render_macro_arms(matchers, ";")
);

clean::MacroItem(clean::Macro { source, imported_from: Some(imported_from) })
clean::MacroItem(clean::Macro { source, imported_from: Some(imported_from) })
} else {
unreachable!()
}
}
LoadedMacro::ProcMacro(ext) => clean::ProcMacroItem(clean::ProcMacro {
kind: ext.macro_kind(),
Expand Down
21 changes: 6 additions & 15 deletions src/librustdoc/clean/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2172,17 +2172,11 @@ impl Clean<Item> for (&hir::MacroDef<'_>, Option<Symbol>) {
let (item, renamed) = self;
let name = renamed.unwrap_or(item.ident.name);
let tts = item.ast.body.inner_tokens().trees().collect::<Vec<_>>();
// Extract the spans of all matchers. They represent the "interface" of the macro.
let matchers = tts.chunks(4).map(|arm| arm[0].span()).collect::<Vec<_>>();
// Extract the macro's matchers. They represent the "interface" of the macro.
let matchers = tts.chunks(4).map(|arm| &arm[0]);

let source = if item.ast.macro_rules {
format!(
"macro_rules! {} {{\n{}}}",
name,
matchers
.iter()
.map(|span| { format!(" {} => {{ ... }};\n", span.to_src(cx)) })
.collect::<String>(),
)
format!("macro_rules! {} {{\n{}}}", name, render_macro_arms(matchers, ";"))
} else {
let vis = item.vis.clean(cx);
let def_id = item.def_id.to_def_id();
Expand All @@ -2192,17 +2186,14 @@ impl Clean<Item> for (&hir::MacroDef<'_>, Option<Symbol>) {
"{}macro {}{} {{\n ...\n}}",
vis.to_src_with_space(cx.tcx, def_id),
name,
matchers.iter().map(|span| span.to_src(cx)).collect::<String>(),
matchers.map(render_macro_matcher).collect::<String>(),
)
} else {
format!(
"{}macro {} {{\n{}}}",
vis.to_src_with_space(cx.tcx, def_id),
name,
matchers
.iter()
.map(|span| { format!(" {} => {{ ... }},\n", span.to_src(cx)) })
.collect::<String>(),
render_macro_arms(matchers, ","),
)
}
};
Expand Down
37 changes: 21 additions & 16 deletions src/librustdoc/clean/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ use crate::clean::{
use crate::core::DocContext;
use crate::formats::item_type::ItemType;

use rustc_ast::tokenstream::TokenTree;
use rustc_hir as hir;
use rustc_hir::def::{DefKind, Res};
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_middle::mir::interpret::ConstValue;
use rustc_middle::ty::subst::{GenericArgKind, SubstsRef};
use rustc_middle::ty::{self, DefIdTree, TyCtxt};
use rustc_span::symbol::{kw, sym, Symbol};
use std::fmt::Write as _;
use std::mem;

#[cfg(test)]
Expand Down Expand Up @@ -248,22 +250,6 @@ crate fn build_deref_target_impls(cx: &mut DocContext<'_>, items: &[Item], ret:
}
}

crate trait ToSource {
fn to_src(&self, cx: &DocContext<'_>) -> String;
}

impl ToSource for rustc_span::Span {
fn to_src(&self, cx: &DocContext<'_>) -> String {
debug!("converting span {:?} to snippet", self);
let sn = match cx.sess().source_map().span_to_snippet(*self) {
Ok(x) => x,
Err(_) => String::new(),
};
debug!("got snippet {}", sn);
sn
}
}

crate fn name_from_pat(p: &hir::Pat<'_>) -> Symbol {
use rustc_hir::*;
debug!("trying to get a name from pattern: {:?}", p);
Expand Down Expand Up @@ -572,3 +558,22 @@ crate fn has_doc_flag(attrs: ty::Attributes<'_>, flag: Symbol) -> bool {
///
/// Set by `bootstrap::Builder::doc_rust_lang_org_channel` in order to keep tests passing on beta/stable.
crate const DOC_RUST_LANG_ORG_CHANNEL: &'static str = env!("DOC_RUST_LANG_ORG_CHANNEL");

/// Render a sequence of macro arms in a format suitable for displaying to the user
/// as part of an item declaration.
pub(super) fn render_macro_arms<'a>(
matchers: impl Iterator<Item = &'a TokenTree>,
arm_delim: &str,
) -> String {
let mut out = String::new();
for matcher in matchers {
writeln!(out, " {} => {{ ... }}{}", render_macro_matcher(matcher), arm_delim).unwrap();
}
out
}

/// Render a macro matcher in a format suitable for displaying to the user
/// as part of an item declaration.
pub(super) fn render_macro_matcher(matcher: &TokenTree) -> String {
rustc_ast_pretty::pprust::tt_to_string(matcher)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have this as a separate function? Just use tt_to_string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may not use tt_to_string in the future, or we may have additional logic, so I'd rather err on the side of having an extra function. But I can remove it if you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove it until and unless we add more logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, render_macro_matcher is used in another place, for rendering macros 2.0. I'd really rather keep it so there aren't multiple places to update later on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you think this would ever change. Rustdoc uses rustc in lots of places, it doesn't make sense to wrap every single one in our own function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, this is the function you mentioned above for removing the whitespace in the matcher. I think we should keep the function if we keep your special-casing, and remove it if not. And I do like that the special-casing makes it prettier. So I guess I'm in favor of keeping it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also the discussion in rust-lang/compiler-team#403

2 changes: 1 addition & 1 deletion src/test/pretty/cast-lt.pp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@
// pretty-mode:expanded
// pp-exact:cast-lt.pp

macro_rules! negative { ($ e : expr) => { $ e < 0 } }
macro_rules! negative { ($e : expr) => { $e < 0 } }

fn main() { (1 as i32) < 0; }
2 changes: 1 addition & 1 deletion src/test/pretty/delimited-token-groups.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#![feature(rustc_attrs)]

macro_rules! mac { ($ ($ tt : tt) *) => () }
macro_rules! mac { ($($tt : tt) *) => () }

mac! {
struct S { field1 : u8, field2 : u16, } impl Clone for S
Expand Down
2 changes: 1 addition & 1 deletion src/test/pretty/macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@

#![feature(decl_macro)]

pub(crate) macro mac { ($ arg : expr) => { $ arg + $ arg } }
pub(crate) macro mac { ($arg : expr) => { $arg + $arg } }

fn main() { }
6 changes: 3 additions & 3 deletions src/test/pretty/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ macro_rules! matcher_brackets {
}

macro_rules! all_fragments {
($ b : block, $ e : expr, $ i : ident, $ it : item, $ l : lifetime, $ lit
: literal, $ m : meta, $ p : pat, $ pth : path, $ s : stmt, $ tt : tt, $
ty : ty, $ vis : vis) => { } ;
($b : block, $e : expr, $i : ident, $it : item, $l : lifetime, $lit :
literal, $m : meta, $p : pat, $pth : path, $s : stmt, $tt : tt, $ty : ty,
$vis : vis) => { } ;
}

fn main() { }
12 changes: 6 additions & 6 deletions src/test/rustdoc/decl_macro.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ pub macro my_macro() {

}

// @has decl_macro/macro.my_macro_2.html //pre 'pub macro my_macro_2($($tok:tt)*) {'
// @has decl_macro/macro.my_macro_2.html //pre 'pub macro my_macro_2($($tok : tt) *) {'
// @has - //pre '...'
// @has - //pre '}'
pub macro my_macro_2($($tok:tt)*) {
Expand All @@ -18,8 +18,8 @@ pub macro my_macro_2($($tok:tt)*) {

// @has decl_macro/macro.my_macro_multi.html //pre 'pub macro my_macro_multi {'
// @has - //pre '(_) => { ... },'
// @has - //pre '($foo:ident . $bar:expr) => { ... },'
// @has - //pre '($($foo:literal),+) => { ... }'
// @has - //pre '($foo : ident.$bar : expr) => { ... },'
// @has - //pre '($($foo : literal), +) => { ... },'
// @has - //pre '}'
pub macro my_macro_multi {
(_) => {
Expand All @@ -33,7 +33,7 @@ pub macro my_macro_multi {
}
}

// @has decl_macro/macro.by_example_single.html //pre 'pub macro by_example_single($foo:expr) {'
// @has decl_macro/macro.by_example_single.html //pre 'pub macro by_example_single($foo : expr) {'
// @has - //pre '...'
// @has - //pre '}'
pub macro by_example_single {
Expand All @@ -42,12 +42,12 @@ pub macro by_example_single {

mod a {
mod b {
// @has decl_macro/a/b/macro.by_example_vis.html //pre 'pub(super) macro by_example_vis($foo:expr) {'
// @has decl_macro/a/b/macro.by_example_vis.html //pre 'pub(super) macro by_example_vis($foo : expr) {'
pub(in super) macro by_example_vis {
($foo:expr) => {}
}
mod c {
// @has decl_macro/a/b/c/macro.by_example_vis_named.html //pre 'pub(in a) macro by_example_vis_named($foo:expr) {'
// @has decl_macro/a/b/c/macro.by_example_vis_named.html //pre 'pub(in a) macro by_example_vis_named($foo : expr) {'
pub(in a) macro by_example_vis_named {
($foo:expr) => {}
}
Expand Down
45 changes: 45 additions & 0 deletions src/test/rustdoc/macro_rules-matchers.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// This is a regression test for issue #86208.
// It is also a general test of macro_rules! display.

#![crate_name = "foo"]

// @has 'foo/macro.todo.html'
// @has - '//span[@class="macro"]' 'macro_rules!'
// @has - '//span[@class="ident"]' 'todo'
// Note: count = 2 * ('=' + '>') + '+' = 2 * (1 + 1) + 1 = 5
// @count - '//pre[@class="rust macro"]//span[@class="op"]' 5

// @has - '{ ()'
// @has - '//span[@class="op"]' '='
// @has - '//span[@class="op"]' '>'
// @has - '{ ... };'

// @has - '($('
// @has - '//span[@class="macro-nonterminal"]' '$'
// @has - '//span[@class="macro-nonterminal"]' 'arg'
// @has - ':'
// @has - '//span[@class="ident"]' 'tt'
// @has - '),'
// @has - '//span[@class="op"]' '+'
// @has - ')'
pub use std::todo;

mod mod1 {
// @has 'foo/macro.macro1.html'
// @has - 'macro_rules!'
// @has - 'macro1'
// @has - '{ ()'
// @has - '($('
// @has - '//span[@class="macro-nonterminal"]' '$'
// @has - '//span[@class="macro-nonterminal"]' 'arg'
// @has - ':'
// @has - 'expr'
// @has - '),'
// @has - '+'
// @has - ')'
#[macro_export]
macro_rules! macro1 {
() => {};
($($arg:expr),+) => { stringify!($($arg),+) };
}
}
8 changes: 4 additions & 4 deletions src/test/rustdoc/macros.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @has macros/macro.my_macro.html //pre 'macro_rules! my_macro {'
// @has - //pre '() => { ... };'
// @has - //pre '($a:tt) => { ... };'
// @has - //pre '($e:expr) => { ... };'
// @has - //pre '($a : tt) => { ... };'
// @has - //pre '($e : expr) => { ... };'
#[macro_export]
macro_rules! my_macro {
() => [];
Expand All @@ -12,8 +12,8 @@ macro_rules! my_macro {
// Check that exported macro defined in a module are shown at crate root.
// @has macros/macro.my_sub_macro.html //pre 'macro_rules! my_sub_macro {'
// @has - //pre '() => { ... };'
// @has - //pre '($a:tt) => { ... };'
// @has - //pre '($e:expr) => { ... };'
// @has - //pre '($a : tt) => { ... };'
// @has - //pre '($e : expr) => { ... };'
mod sub {
#[macro_export]
macro_rules! my_sub_macro {
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/hygiene/unpretty-debug.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#![feature /* 0#0 */(no_core)]
#![no_core /* 0#0 */]

macro_rules! foo /* 0#0 */ { ($ x : ident) => { y + $ x } }
macro_rules! foo /* 0#0 */ { ($x : ident) => { y + $x } }

fn bar /* 0#0 */() {
let x /* 0#0 */ = 1;
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/proc-macro/meta-macro-hygiene.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ macro_rules! produce_it
*/ {
() =>
{
meta_macro :: print_def_site! ($ crate :: dummy! ()) ;
meta_macro :: print_def_site! ($crate :: dummy! ()) ;
// `print_def_site!` will respan the `$crate` identifier
// with `Span::def_site()`. This should cause it to resolve
// relative to `meta_macro`, *not* `make_macro` (despite
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/proc-macro/nonterminal-token-hygiene.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,9 @@ macro_rules! outer
/*
0#0
*/ {
($ item : item) =>
($item : item) =>
{
macro inner() { print_bang! { $ item } } inner! () ;
macro inner() { print_bang! { $item } } inner! () ;

} ;
}
Expand Down