Better tokentree pretty printer #39079

bjorn3 · 2017-01-15T16:37:21Z

The example at the homepage now print like this when parsed as tokentrees:

fn main()
{
    let program = "+ + * - /";
    let mut accumulator = 0;
    . .. ...;
    a.b;
    .. .a;
    a. ..;
    for token in program.chars()
    {
        match token {
                        '+' => accumulator += 1 , '-' => accumulator -= 1 , '*' => accumulator *= 2 , '/' => accumulator /= 2 , _ => {}

                    }

    }

    println !("The program \"{}\" calculates the value {}" , program , accumulator );
}

rust-highfive · 2017-01-15T16:37:35Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bjorn3 · 2017-01-15T18:35:29Z

src/libsyntax/print/pprust.rs

-                match *tk {
-                    parse::token::DocComment(..) => {
-                        hardbreak(&mut self.s)
+        self.print_tts(unsafe{::std::slice::from_raw_parts(tt as *const _, 1)}); // Unsafe block to convert ref to single item slice ref without cloning


Is this allowed and is there a other way to do this?

bjorn3 · 2017-01-16T17:28:04Z

src/libsyntax/print/pprust.rs

-                        hardbreak(&mut self.s)
+    /// Forwards to print_tts
+    pub fn print_tt(&mut self, tt: &ast::TokenTree) -> io::Result<()> {
+        self.print_tts(unsafe{::std::slice::from_raw_parts(tt as *const _, 1)}); // Unsafe block to convert ref to single item slice ref without cloning


Is this allowed and is there a other way to do this?

bjorn3 · 2017-01-17T15:11:04Z

What is going on here while compiling stage1?

   Compiling fmt_macros v0.0.0 (file:///checkout/src/libfmt_macros)
error: no rules expected the token `static`
   --> /checkout/src/liblog/lib.rs:215:5
    |
215 | static LOCAL_LOGGER: RefCell<Option<Box<Logger + Send>>> = {
    | ^^^^^^

steveklabnik · 2017-01-17T15:29:02Z

/cc @nrc @rust-lang/compiler

pnkfelix · 2017-01-17T16:35:29Z

src/libsyntax/print/pprust.rs

+                        word(&mut self.s, &token_to_string(token))?;
+                        match (token, tts_iter.peek()){
+                            (&parse::token::DocComment(..), _) => hardbreak(&mut self.s)?, // ///abc
+                                                                            // ^^^---


I'm pretty sure this comment is not what you wanted...

Forgot to fix indentation :(

pnkfelix

I'm ... pretty sure we don't approve PR's with merge commits in them. So you probably need to clean up your revision history; it may suffice to just rebase.

pnkfelix · 2017-01-17T16:38:48Z

Also, I'm not yet sure what this is doing, because I don't remember what the token-tree printer was, what it used to look like, whether it was good/bad, et cetera.

I say this mostly as a way of saying "I'm giving feedback on relatively minor points, but you should not interpret that as giving any indication about my feelings on the PR as a whole."

bjorn3 · 2017-01-17T16:46:45Z

It is used in error mesages when for example a proc macro outputs fn a+b(){} which doesnt parse as far as i known. Will rebase soon.

nikomatsakis · 2017-01-17T16:52:06Z

Pre-existing, but it seems like it'd be good to have some tests of this, no?

bjorn3 · 2017-01-17T17:02:58Z

Will add them when i rebase.

bjorn3 · 2017-01-17T18:37:34Z

Messed up a bit with rebasing. will have to do the fix build cycle from before again :(

bjorn3

Few questions

bjorn3 · 2017-01-17T19:36:09Z

src/libsyntax/print/pprust.rs

+    for token in program.chars()
+    {
+        match token {
+                        '+' => accumulator += 1 , '-' => accumulator -= 1 , '*' => accumulator *= 2 , '/' => accumulator /= 2 , _ => {}


What should I do here to fix the tidy error? Can't put a newline because I would have another string then.

bjorn3 · 2017-01-17T19:37:14Z

src/libsyntax/print/pprust.rs

-            }
-        }
+        // Unsafe block to convert ref to single item slice ref without cloning
+        self.print_tts(unsafe{::std::slice::from_raw_parts(tt as *const _, 1)})


Is this allowed and is there a other way to do this?

I don't think this is worth using unsafe.
I would just clone the token tree (i.e. self.print_tts(&[tt.clone()])) -- it isn't a deep clone, just a copy plus an Rc ref-count update. You could also refactor print_tts to take an Iterator<Item=&TokenTree>.

bjorn3 · 2017-01-17T20:48:29Z

Build fails due to bug in this tt pretty printer.

bjorn3 · 2017-01-19T19:13:38Z

I had a break which I meant to break out of the match not the loop 🤦. This meant skipping a great part of the thread_local macro.

bjorn3 · 2017-01-19T19:47:39Z

Accidentally added a nul byte.

bjorn3 · 2017-01-19T20:51:27Z

Why do the tests fail?

bjorn3 · 2017-01-20T17:19:37Z

Rebased

eddyb · 2017-01-20T22:23:59Z

src/libsyntax/print/pprust.rs

+                    space(&mut self.s)?;
+                    word(&mut self.s, ")")?;
+                    if let Some(ref tk) = seq.separator {
+                        word(&mut self.s, &token_to_string(tk))?;


Is there no way to print a single token? I guess the same is a problem above.

eddyb · 2017-01-20T22:24:54Z

src/libsyntax/print/pprust.rs

+                        word(&mut self.s, ";")?;
+                        hardbreak(&mut self.s)?;
+                    } else {
+                        word(&mut self.s, &token_to_string(token))?;


Sad. I guess it's pre-existing, though.

eddyb · 2017-01-20T22:31:02Z

I have no suggestions about the bug other than "comment out the special cases and see if that still fails".
If this is not a very weird bug, it might reproduce with ./x.py test --stage 1 src/test/codegen.

bjorn3 · 2017-01-22T08:50:39Z

@eddyb I can't reproduce the travis error on local mac osx. Only run-fail/assert-macro-explicit.rs fails due to a added space.

bjorn3 · 2017-01-22T09:09:03Z

Are the location of stringify! spaces and newlines (semi)guaranteed. If so this is a breaking change.

eddyb · 2017-01-22T13:00:59Z

@bjorn3 Alright, you can still sort of abuse Travis to test it for you. I'd suggest pushing a single commit that comments out all the special cases but leaves your own code for the default cases.
If that works, you can then bisect which special case breaks. If it doesn't... You can bisect to the original.

bors · 2017-01-28T00:59:03Z

📌 Commit cc4eb99 has been approved by pnkfelix

bors · 2017-01-28T02:49:26Z

⌛ Testing commit cc4eb99 with merge ded091d...

alexcrichton · 2017-01-28T02:50:47Z

@bors: retry

prioritizing rollup

bors · 2017-01-28T05:18:59Z

⌛ Testing commit cc4eb99 with merge cbf842f...

bors · 2017-01-28T05:59:20Z

💔 Test failed - status-appveyor

bjorn3 · 2017-01-29T09:34:01Z

@pnkfelix test passes.

bjorn3 · 2017-01-29T11:03:29Z

Squashed

+ 3c7226b...0409508 pp-tokentree -> pp-tokentree (forced update) (just in case)

pnkfelix · 2017-01-30T12:19:58Z

@bors r+

bors · 2017-01-30T12:19:59Z

📌 Commit 0409508 has been approved by pnkfelix

bors · 2017-01-30T12:20:08Z

⌛ Testing commit 0409508 with merge a62accf...

bors · 2017-01-30T13:04:06Z

💔 Test failed - status-travis

bjorn3 · 2017-01-30T15:36:18Z

I think i will make a script to update the failing pretty print tests:

test result: FAILED. 2462 passed; 122 failed; 27 ignored; 0 measured

pnkfelix · 2017-02-01T13:56:48Z

@bjorn3 travis says tidy checks are failling.

eddyb · 2017-02-04T19:50:33Z

src/libsyntax/print/pprust.rs

-                        '+' => accumulator += 1 , '-' => accumulator -= 1 , '*' => accumulator *= 2 , '/' => accumulator /= 2 , _ => {}
+        match token 
+        {
+            \'+\' => accumulator += 1 , \'-\' => accumulator -= 1 , \'*\' => accumulator *= 2 , \'/\' => accumulator /= 2 , _ => {}


Two interesting things here:

{ should never start a new line before it, always after

it gets trickier sometimes thought (} and ; come to be mind)

the whole reason there's a pp.rs is for automating breaks

but I'm not sure the algorithm is appropriate for Rust or bug-free

, should never have a space before it, always after

I dont really understand print::pp to be honest. Documentation would be really appreciated.

Who does? shrug

Found docs at the Printer struct. Opened #39557 with a few doc improvements.

bors · 2017-03-01T08:01:08Z

☔ The latest upstream changes (presumably #39419) made this pull request unmergeable. Please resolve the merge conflicts.

pnkfelix · 2017-03-20T10:08:37Z

@bjorn3 are you going to be able to address the problems on this PR? (From what I can tell, Travis was never satisfied with it...?) Or should I close it to clean up the queue?

bjorn3 · 2017-03-20T15:09:49Z

If I want to retry this in the future I will make a new pr.

rust-highfive assigned pnkfelix Jan 15, 2017

bjorn3 commented Jan 15, 2017

View reviewed changes

bjorn3 commented Jan 16, 2017

View reviewed changes

pnkfelix reviewed Jan 17, 2017

View reviewed changes

bjorn3 force-pushed the pp-tokentree branch 2 times, most recently from 5daf971 to 757ee29 Compare January 17, 2017 18:12

bjorn3 commented Jan 17, 2017

View reviewed changes

bjorn3 force-pushed the pp-tokentree branch 2 times, most recently from ad4eb97 to f60ed6f Compare January 20, 2017 17:19

eddyb reviewed Jan 20, 2017

View reviewed changes

bjorn3 force-pushed the pp-tokentree branch from f591fd9 to 18c0757 Compare January 22, 2017 09:41

bjorn3 force-pushed the pp-tokentree branch from cc4eb99 to ef4954f Compare January 28, 2017 13:05

bjorn3 force-pushed the pp-tokentree branch from 3c7226b to 0409508 Compare January 29, 2017 11:03

bjorn3 force-pushed the pp-tokentree branch from 1b835de to 8acc8f3 Compare February 1, 2017 19:19

eddyb reviewed Feb 4, 2017

View reviewed changes

Better tokentree pretty printer

9d6b7c7

bjorn3 force-pushed the pp-tokentree branch from 67aa983 to 9d6b7c7 Compare February 5, 2017 09:01

bjorn3 added 3 commits February 5, 2017 15:58

Even better tokentree pretty printer

74c639b

Prettier

0380fc3

Space after comma

c010b1e

bjorn3 closed this Mar 20, 2017

bjorn3 deleted the pp-tokentree branch December 17, 2017 11:25

Better tokentree pretty printer #39079

Better tokentree pretty printer #39079

Conversation

bjorn3 commented Jan 15, 2017

rust-highfive commented Jan 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn3 commented Jan 17, 2017 • edited Loading

steveklabnik commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnkfelix left a comment

Choose a reason for hiding this comment

pnkfelix commented Jan 17, 2017

bjorn3 commented Jan 17, 2017

nikomatsakis commented Jan 17, 2017

bjorn3 commented Jan 17, 2017

bjorn3 commented Jan 17, 2017

bjorn3 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

bjorn3 commented Jan 17, 2017

bjorn3 commented Jan 19, 2017

bjorn3 commented Jan 19, 2017

bjorn3 commented Jan 19, 2017 • edited Loading

bjorn3 commented Jan 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Jan 20, 2017

bjorn3 commented Jan 22, 2017

bjorn3 commented Jan 22, 2017

eddyb commented Jan 22, 2017 • edited Loading

bors commented Jan 28, 2017

bors commented Jan 28, 2017

alexcrichton commented Jan 28, 2017

bors commented Jan 28, 2017

bors commented Jan 28, 2017

bjorn3 commented Jan 29, 2017

bjorn3 commented Jan 29, 2017 • edited Loading

pnkfelix commented Jan 30, 2017

bors commented Jan 30, 2017

bors commented Jan 30, 2017

bors commented Jan 30, 2017

bjorn3 commented Jan 30, 2017

pnkfelix commented Feb 1, 2017

eddyb Feb 4, 2017 • 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

bors commented Mar 1, 2017

pnkfelix commented Mar 20, 2017

bjorn3 commented Mar 20, 2017

bjorn3 commented Jan 17, 2017 •

edited

Loading

bjorn3 commented Jan 19, 2017 •

edited

Loading

eddyb commented Jan 22, 2017 •

edited

Loading

bjorn3 commented Jan 29, 2017 •

edited

Loading

eddyb Feb 4, 2017 •

edited

Loading