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

camelid · 2021-06-14T00:05:52Z

Fixes #86208.

rust-highfive · 2021-06-14T00:05:55Z

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

camelid · 2021-06-14T00:06:24Z

r? @jyn514 (feel free to reassign)

camelid · 2021-06-14T00:07:13Z

Beta-nominating because this fixes a P-high stable-to-beta regression.

camelid · 2021-06-14T00:10:05Z

Before/after:

stable (pre-regression)

beta (with regression)

this PR (with fix)

camelid · 2021-06-14T00:11:15Z

I left some comments in the commit messages too, so they may be helpful to read.

camelid · 2021-06-14T00:18:30Z

src/test/rustdoc/macro_rules-matchers.rs

+// @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="ident"]' 'arg'
+// @has - ':'
+// @has - '//span[@class="ident"]' 'tt'
+// @has - '//span[@class="op"]' '+'
+// @has - ')'


Do you know of a way to do a @has check that matches on the innerText of an element (in this case, <pre class="rust macro">) to avoid having so many individual checks?

I don't, @GuillaumeGomez may.

Actually my understanding is that is always used innerText, are you sure doing it the obvious way doesn't work?

Just like @jyn514 said. The problem might be that the text doesn't "look like" what you might expect but that's it. :)

I had tried something like @has - 'macro_rules! todo {' and it failed; maybe I need to explicitly specify //pre? I also had this issue in #83501 where @has - 'Size: 1 byte' didn't work because Size: was wrapped with <b>.

Ah! Maybe try with //text() at the end.

Hmm, it doesn't seem to be working. //text() by itself gave this error:

27: Non-absolute XPath is not supported due to implementation issues // @has - '//text()' 'macro_rules! macro1 {' 28: Non-absolute XPath is not supported due to implementation issues // @has - '//text()' '() => { ... };' 29: Non-absolute XPath is not supported due to implementation issues // @has - '//text()' '$ ($ arg : expr), +) => { ... };' 30: Non-absolute XPath is not supported due to implementation issues // @has - '//text()' '}' Encountered 4 errors

and //pre//text() gave this:

27: @has check failed `XPATH PATTERN` did not match // @has - '//pre//text()' 'macro_rules! macro1 {' 28: @has check failed `XPATH PATTERN` did not match // @has - '//pre//text()' '() => { ... };' 29: @has check failed `XPATH PATTERN` did not match // @has - '//pre//text()' '$ ($ arg : expr), +) => { ... };' 30: @has check failed `XPATH PATTERN` did not match // @has - '//pre//text()' '}' Encountered 4 errors

I also tried //pre///text() (extra slash), but that didn't work either.

camelid · 2021-06-14T00:37:36Z

I figured out a way to print the matchers in a prettier way, but it may have edge cases that I'm not aware of. Here's the new output:

And here's what the code looks like:

pub(super) fn render_macro_matcher(matcher: &TokenTree) -> String {
    match matcher {
        TokenTree::Token(tok) => rustc_ast_pretty::pprust::token_to_string(tok),
        TokenTree::Delimited(_span, delim_tok, stream) => {
            let open_tok = Token::new(token::OpenDelim(*delim_tok), DUMMY_SP);
            let close_tok = Token::new(token::CloseDelim(*delim_tok), DUMMY_SP);
            format!(
                "{}{}{}",
                rustc_ast_pretty::pprust::token_to_string(&open_tok),
                stream.trees().map(|tt| render_macro_matcher(&tt)).collect::<String>(),
                rustc_ast_pretty::pprust::token_to_string(&close_tok)
            )
        }
    }
}

jyn514

I figured out a way to print the matchers in a prettier way, but it may have edge cases that I'm not aware of.

I don't think rustdoc's pretty print should behave differently from rustc's. Can you change this in rustc_ast_pretty instead? It should be easier there because these already have to be handled.

jyn514 · 2021-06-14T01:02:36Z

src/librustdoc/clean/utils.rs

+    matchers
+        .iter()
+        .map(|matcher| format!("    {} => {{ ... }};\n", render_macro_matcher(matcher)))
+        .collect::<String>()


Rather than allocating a new string for each arm, can you use a for loop and push_str?

I changed it to use writeln!, which I think should generate code fairly similar to push_str, but is more readable.

jyn514 · 2021-06-14T01:03:10Z

src/librustdoc/clean/utils.rs

+/// as part of an item declaration.
+pub(super) fn render_macro_matcher(matcher: &TokenTree) -> String {
+    rustc_ast_pretty::pprust::tt_to_string(matcher)
+}


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

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.

Please remove it until and unless we add more logic.

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.

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.

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.

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

jyn514 · 2021-06-14T01:03:49Z

src/test/rustdoc/macro_rules-matchers.rs

+// @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="ident"]' 'arg'
+// @has - ':'
+// @has - '//span[@class="ident"]' 'tt'
+// @has - '//span[@class="op"]' '+'
+// @has - ')'


I don't, @GuillaumeGomez may.

Actually my understanding is that is always used innerText, are you sure doing it the obvious way doesn't work?

src/librustdoc/clean/inline.rs

camelid · 2021-06-14T20:48:38Z

Can you change this in rustc_ast_pretty instead? It should be easier there because these already have to be handled.

The problem is that they aren't already handled. For example, this is what print_tt()'s docs say (tt_to_string() is just a wrapper around this function):

This doesn’t deserve to be called “pretty” printing, but it should be meaning-preserving. A quick hack that might help would be to look at the spans embedded in the TTs to decide where to put spaces and newlines. But it’d be better to parse these according to the grammar of the appropriate macro, transcribe back into the grammar we just parsed from, and then pretty-print the resulting AST nodes (so, e.g., we print expression arguments as expressions). It can be done! I think.

From what I can tell, it just puts a space after every token, which is why the output is not so great:

        match tt {
            TokenTree::Token(token) => {
                let token_str = self.token_to_string_ext(&token, convert_dollar_crate);
                self.word(token_str);
                if let token::DocComment(..) = token.kind {
                    self.hardbreak()
                }
            }
            // ...

So I don't think it would be easier to change in rustc_ast_pretty. It might even be harder because we'd have to deal with the existing pretty-printing logic.

Mark-Simulacrum · 2021-06-14T22:02:00Z

Dropping beta nomination; this is not ready for beta backport based on the extensive discussion on PR and #86208 does not seem like a critical bug. #86208 will shortly be a stable/stable regression and I don't think this warrants a backport in that case.

camelid · 2021-06-14T22:08:43Z

Dropping beta nomination; this is not ready for beta backport based on the extensive discussion on PR and #86208 does not seem like a critical bug. #86208 will shortly be a stable/stable regression and I don't think this warrants a backport in that case.

Yeah, I realized that a backport into 1.53 wouldn't make sense, but I left the label on since this will likely land in 1.55 and need to be backported to 1.54. Should I just wait until 1.53 is released before adding the label back?

Mark-Simulacrum · 2021-06-14T22:11:00Z

I do not think it should be backported to 1.54 either; it is merely fixing a (seemingly to me) minor bug, and not patching a beta regression.

jyn514 · 2021-06-14T23:02:48Z

@Mark-Simulacrum it is a beta regression (stable now; it broke in 1.53), but for something that has been broken and fixed several times in the past.

camelid · 2021-07-04T22:04:01Z

I guess we should. But AFAIK rustc_ast_pretty is mostly only used for -Zunpretty.

I think the problem is that stringify! relies on rustc_ast_pretty.

For example, this code:

fn main() {
    println!("{}", stringify!($foo));
}

produces the following on nightly:

$ foo

but produces this instead after this PR:

$foo

petrochenkov · 2021-07-04T22:18:15Z

It seems fine to omit the space after $, although eventually we should fully preserve token jointness information produced by lexer and pretty-print precisely instead of guessing with fn tt_prepend_space.

I think the problem is that stringify! relies on rustc_ast_pretty.

It's not a problem, similar changes were done to the pretty-printer quite often in the past, stringify! docs even have a disclaimer about this.

r? @jyn514

jyn514 · 2021-07-04T22:21:20Z

@bors r+

bors · 2021-07-04T22:21:22Z

📌 Commit 7ffec70 has been approved by jyn514

bors · 2021-07-05T05:09:41Z

⌛ Testing commit 7ffec70 with merge 09d9b60...

bors · 2021-07-05T07:27:46Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 09d9b60 to master...

…uillaumeGomez rustdoc: Preserve rendering of macro_rules matchers when possible Fixes rust-lang#92331. This approach restores the behavior prior to rust-lang#86282 **if** the matcher token held by the compiler **and** the matcher token found in the source code are identical TokenTrees. Thus rust-lang#86208 remains fixed, but without regressing formatting for the vast majority of macros which are not macro-generated.

camelid added A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) T-rustdoc labels Jun 14, 2021

rust-highfive assigned ollie27 Jun 14, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 14, 2021

rust-highfive assigned jyn514 and unassigned ollie27 Jun 14, 2021

camelid added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 14, 2021

camelid force-pushed the macro_rules-matchers branch 2 times, most recently from 656da0c to 87e0452 Compare June 14, 2021 00:17

camelid commented Jun 14, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

camelid force-pushed the macro_rules-matchers branch from 87e0452 to 391233e Compare June 14, 2021 00:35

This comment has been minimized.

Sign in to view

jyn514 reviewed Jun 14, 2021

View reviewed changes

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 14, 2021

jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed T-rustdoc-temp labels Jun 22, 2021

This comment has been minimized.

Sign in to view

jyn514 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2021

rust-highfive assigned jyn514 and unassigned petrochenkov Jul 4, 2021

petrochenkov approved these changes Jul 4, 2021

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 4, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 5, 2021

bors merged commit 09d9b60 into rust-lang:master Jul 5, 2021

rustbot added this to the 1.55.0 milestone Jul 5, 2021

bors mentioned this pull request Jul 5, 2021

rustdoc: Replace FakeDefId with new ItemId type #86644

Merged

camelid deleted the macro_rules-matchers branch July 5, 2021 21:23

This was referenced Dec 26, 2021

Documentation of macro-generated macro shows unexpanded matchers #45939

Closed

Rustdoc can generate wrong macro_rules! syntax #60543

Closed

camelid mentioned this pull request Dec 27, 2021

rustdoc: macros expanded from macros don't get their rules documented correctly #89367

Closed

This was referenced Dec 27, 2021

rustdoc: Regression in whitespacing inside macro_rules matchers #92331

Closed

rustdoc: Preserve rendering of macro_rules matchers when possible #92334

Merged

camelid mentioned this pull request May 22, 2024

doc does not correctly display macro match arms if they are created inside of a proc macro #43759

Closed

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

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

Conversation

camelid commented Jun 14, 2021

rust-highfive commented Jun 14, 2021

camelid commented Jun 14, 2021

camelid commented Jun 14, 2021

camelid commented Jun 14, 2021

stable (pre-regression)

beta (with regression)

this PR (with fix)

camelid commented Jun 14, 2021

camelid Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

camelid commented Jun 14, 2021

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camelid commented Jun 14, 2021

Mark-Simulacrum commented Jun 14, 2021

camelid commented Jun 14, 2021

Mark-Simulacrum commented Jun 14, 2021

jyn514 commented Jun 14, 2021

This comment has been minimized.

camelid commented Jul 4, 2021

petrochenkov commented Jul 4, 2021

jyn514 commented Jul 4, 2021

bors commented Jul 4, 2021

bors commented Jul 5, 2021

bors commented Jul 5, 2021

camelid Jun 14, 2021 •

edited

Loading