pprust: fix parenthesization of exprs #43742

spernsteiner · 2017-08-08T14:32:38Z

The pretty printer in syntax::print::pprust currently relies on the presence of ExprKind::Paren hints in order to correctly parenthesize expressions in its output. If Paren nodes are missing, it sometimes produces wrong output, such as printing 1 - (2 - 3) as 1 - 2 - 3. This PR fixes pprust to correctly print expressions regardless of the presence or absence of Paren nodes. This should make pprust easier to use with programmatically constructed ASTs.

A few notes:

I added a function for assigning precedence values to exprs in syntax::util::parser, since there is already code there for assigning precedence values to binops. Let me know if I should move this somewhere more pprust-specific.
I also moved the contains_exterior_struct_lit function from rustc_lint::unused::UnusedParens into syntax::util::parser, since it's needed for determining the correct parenthesization of if/while conditional expressions.
I couldn't find a good way to compare two exprs for equivalence while ignoring semantically-irrelevant details like spans. So the test for the new behavior relies on a slight hack: it adds Paren nodes everywhere, so that the pretty-printed version exactly reflects the structure of the AST, and then compares the printed strings. This works, but let me know if there's a better way.

rust-highfive · 2017-08-08T14:32:49Z

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

aidanhs · 2017-08-10T01:57:06Z

Thanks for the PR @epdtry! We'll check in now and again to make sure @pnkfelix or another reviewer gets to this soon.

petrochenkov · 2017-08-12T20:41:17Z

Looks good to me.
I didn't figure out how exactly the test works though.
I assume this was tested on some real autogenerated code as well? (There are some quite non-obvious cases that are treated correctly.)

There's also HIR pretty-printing in librustc/hir/print.rs which is an almost exact copy of AST pretty-printing in libsyntax/print/pprust.rs.
Could you reapply the changes to that file as well?
HIR doesn't have any information about parens, so HIR pretty-printing would benefit from this patch even more.

arielb1 · 2017-08-22T22:27:28Z

Hi @epdtry! I think @petrochenkov has some concerns about your PR. Could you try to address them? Just a friendly ping to make sure this isn't getting lost

spernsteiner · 2017-08-23T17:50:21Z

Hi, sorry about the delay. I've written some new commits to address @petrochenkov's comments, but I did the work for this PR "on the clock", so there's a review process the changes need to go through before I can publish them anywhere. This will hopefully be done soon, but it could be up to another week or two. I'll update this PR with the new commits as soon as I get the approval.

spernsteiner · 2017-09-05T21:31:30Z

@petrochenkov I've added similar parenthesization fixes to the HIR printer. This part doesn't have a test case, because I'm not sure how to write one. (The tricks I used for testing the AST printer don't work easily because there's no HIR parser and no HIR Paren nodes.) I also added a comment to the pprust test case, to better explain how the test is meant to work.

spernsteiner · 2017-09-06T17:08:36Z

Rebased (adding support for ExprYield) to fix the previous travis build error

spernsteiner · 2017-09-06T21:28:11Z

Github says there was an error, but I don't see any errors on the travis build page, only some xcode builds that were cancelled midway through cloning the repo. Can I ignore this, or is something actually broken?

petrochenkov · 2017-09-06T21:47:36Z

The Travis error looks spurious.

@bors r+
Thanks! This is a great patch.

bors · 2017-09-06T21:47:37Z

📌 Commit 347db06 has been approved by petrochenkov

bors · 2017-09-06T23:26:20Z

⌛ Testing commit 347db06 with merge 6de8f2b23b0f876de69fbc48dac8ff776ab5e4fe...

bors · 2017-09-07T01:04:29Z

💔 Test failed - status-appveyor

kennytm · 2017-09-07T07:16:40Z

check-aux x86_64-pc-windows-msvc failed, pretty\issue-4264.rs formatted differently. Legit.

Error message

failures:
---- [pretty] pretty\issue-4264.rs stdout ----
	
error: pretty-printed source does not match expected source
expected:
------------------------------------------
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// pretty-compare-only
// pretty-mode:hir,typed
// pp-exact:issue-4264.pp
// #4264 fixed-length vector types
pub fn foo(_: [i32; (3 as usize)]) ({ } as ())
pub fn bar() ({
                  const FOO: usize = ((5 as usize) - (4 as usize) as usize);
                  let _: [(); (FOO as usize)] = ([(() as ())] as [(); 1]);
                  let _: [(); (1 as usize)] = ([(() as ())] as [(); 1]);
                  let _ =
                      (((&([(1 as i32), (2 as i32), (3 as i32)] as [i32; 3])
                            as &[i32; 3]) as *const _ as *const [i32; 3]) as
                          *const [i32; (3 as usize)] as *const [i32; 3]);
                  ((::fmt::format as
                       fn(std::fmt::Arguments<'_>) -> std::string::String {std::fmt::format})(((<::std::fmt::Arguments>::new_v1
                                                                                                   as
                                                                                                   fn(&[&str], &[std::fmt::ArgumentV1<'_>]) -> std::fmt::Arguments<'_> {std::fmt::Arguments<'_>::new_v1})(({
                                                                                                                                                                                                               static __STATIC_FMTSTR:
                                                                                                                                                                                                                      &'static [&'static str]
                                                                                                                                                                                                                      =
                                                                                                                                                                                                                   (&([("test"
                                                                                                                                                                                                                           as
                                                                                                                                                                                                                           &'static str)]
                                                                                                                                                                                                                         as
                                                                                                                                                                                                                         [&'static str; 1])
                                                                                                                                                                                                                       as
                                                                                                                                                                                                                       &'static [&'static str; 1]);
                                                                                                                                                                                                               (__STATIC_FMTSTR
                                                                                                                                                                                                                   as
                                                                                                                                                                                                                   &'static [&'static str])
                                                                                                                                                                                                           }
                                                                                                                                                                                                              as
                                                                                                                                                                                                              &[&str]),
                                                                                                                                                                                                          (&(match (()
                                                                                                                                                                                                                       as
                                                                                                                                                                                                                       ())
                                                                                                                                                                                                                 {
                                                                                                                                                                                                                 ()
                                                                                                                                                                                                                 =>
                                                                                                                                                                                                                 ([]
                                                                                                                                                                                                                     as
                                                                                                                                                                                                                     [std::fmt::ArgumentV1<'_>; 0]),
                                                                                                                                                                                                             }
                                                                                                                                                                                                                as
                                                                                                                                                                                                                [std::fmt::ArgumentV1<'_>; 0])
                                                                                                                                                                                                              as
                                                                                                                                                                                                              &[std::fmt::ArgumentV1<'_>; 0]))
                                                                                                  as
                                                                                                  std::fmt::Arguments<'_>))
                      as std::string::String);
              } as ())
pub type Foo = [i32; (3 as usize)];
pub struct Bar {
    pub x: [i32; (3 as usize)],
}
pub struct TupleBar([i32; (4 as usize)]);
pub enum Baz { BazVariant([i32; (5 as usize)]), }
pub fn id<T>(x: T) -> T ({ (x as T) } as T)
pub fn use_id() ({
                     let _ =
                         ((id::<[i32; (3 as usize)]> as
                              fn([i32; 3]) -> [i32; 3] {id::<[i32; 3]>})(([(1
                                                                               as
                                                                               i32),
                                                                           (2
                                                                               as
                                                                               i32),
                                                                           (3
                                                                               as
                                                                               i32)]
                                                                             as
                                                                             [i32; 3]))
                             as [i32; 3]);
                 } as ())
fn main() ({ } as ())
------------------------------------------
actual:
------------------------------------------
#[prelude_import]
use std::prelude::v1::*;
#[macro_use]
extern crate std as std;
// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
// pretty-compare-only
// pretty-mode:hir,typed
// pp-exact:issue-4264.pp
// #4264 fixed-length vector types
pub fn foo(_: [i32; (3 as usize)]) ({ } as ())
pub fn bar() ({
                  const FOO: usize = ((5 as usize) - (4 as usize) as usize);
                  let _: [(); (FOO as usize)] = ([(() as ())] as [(); 1]);
                  let _: [(); (1 as usize)] = ([(() as ())] as [(); 1]);
                  let _ =
                      (((&([(1 as i32), (2 as i32), (3 as i32)] as [i32; 3])
                            as &[i32; 3]) as *const _ as *const [i32; 3]) as
                          *const [i32; (3 as usize)] as *const [i32; 3]);
                  ((::fmt::format as
                       fn(std::fmt::Arguments<'_>) -> std::string::String {std::fmt::format})(((<::std::fmt::Arguments>::new_v1
                                                                                                   as
                                                                                                   fn(&[&str], &[std::fmt::ArgumentV1<'_>]) -> std::fmt::Arguments<'_> {std::fmt::Arguments<'_>::new_v1})(({
                                                                                                                                                                                                               static __STATIC_FMTSTR:
                                                                                                                                                                                                                      &'static [&'static str]
                                                                                                                                                                                                                      =
                                                                                                                                                                                                                   (&([("test"
                                                                                                                                                                                                                           as
                                                                                                                                                                                                                           &'static str)]
                                                                                                                                                                                                                         as
                                                                                                                                                                                                                         [&'static str; 1])
                                                                                                                                                                                                                       as
                                                                                                                                                                                                                       &'static [&'static str; 1]);
                                                                                                                                                                                                               (__STATIC_FMTSTR
                                                                                                                                                                                                                   as
                                                                                                                                                                                                                   &'static [&'static str])
                                                                                                                                                                                                           }
                                                                                                                                                                                                              as
                                                                                                                                                                                                              &[&str]),
                                                                                                                                                                                                          (&((match (()
                                                                                                                                                                                                                        as
                                                                                                                                                                                                                        ())
                                                                                                                                                                                                                  {
                                                                                                                                                                                                                  ()
                                                                                                                                                                                                                  =>
                                                                                                                                                                                                                  ([]
                                                                                                                                                                                                                      as
                                                                                                                                                                                                                      [std::fmt::ArgumentV1<'_>; 0]),
                                                                                                                                                                                                              }
                                                                                                                                                                                                                 as
                                                                                                                                                                                                                 [std::fmt::ArgumentV1<'_>; 0]))
                                                                                                                                                                                                              as
                                                                                                                                                                                                              &[std::fmt::ArgumentV1<'_>; 0]))
                                                                                                  as
                                                                                                  std::fmt::Arguments<'_>))
                      as std::string::String);
              } as ())
pub type Foo = [i32; (3 as usize)];
pub struct Bar {
    pub x: [i32; (3 as usize)],
}
pub struct TupleBar([i32; (4 as usize)]);
pub enum Baz { BazVariant([i32; (5 as usize)]), }
pub fn id<T>(x: T) -> T ({ (x as T) } as T)
pub fn use_id() ({
                     let _ =
                         ((id::<[i32; (3 as usize)]> as
                              fn([i32; 3]) -> [i32; 3] {id::<[i32; 3]>})(([(1
                                                                               as
                                                                               i32),
                                                                           (2
                                                                               as
                                                                               i32),
                                                                           (3
                                                                               as
                                                                               i32)]
                                                                             as
                                                                             [i32; 3]))
                             as [i32; 3]);
                 } as ())
fn main() ({ } as ())
------------------------------------------
thread '[pretty] pretty\issue-4264.rs' panicked at 'explicit panic', src\tools\compiletest\src\runtest.rs:376:12
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    [pretty] pretty\issue-4264.rs

Diff (ignoring whitespace)

--- 1.rs	2017-09-07 15:14:53.000000000 +0800
+++ 2.rs	2017-09-07 15:15:06.000000000 +0800
@@ -44,7 +44,7 @@
                                                                                                                                                                                                            }
                                                                                                                                                                                                               as
                                                                                                                                                                                                               &[&str]),
-                                                                                                                                                                                                          (&(match (()
+                                                                                                                                                                                                          (&((match (()
                                                                                                                                                                                                                        as
                                                                                                                                                                                                                        ())
                                                                                                                                                                                                                  {
@@ -55,7 +55,7 @@
                                                                                                                                                                                                                      [std::fmt::ArgumentV1<'_>; 0]),
                                                                                                                                                                                                              }
                                                                                                                                                                                                                 as
-                                                                                                                                                                                                                [std::fmt::ArgumentV1<'_>; 0])
+                                                                                                                                                                                                                 [std::fmt::ArgumentV1<'_>; 0]))
                                                                                                                                                                                                               as
                                                                                                                                                                                                               &[std::fmt::ArgumentV1<'_>; 0]))
                                                                                                   as

spernsteiner · 2017-09-07T14:36:27Z

Should be fixed in the latest commit. I turned up the precedence of the block-like exprs ({}, if, while, etc) so that &match ... no longer gets needlessly parenthesized as &(match ...).

petrochenkov · 2017-09-07T14:41:33Z

@bors r+

bors · 2017-09-07T14:41:34Z

📌 Commit 3454d99 has been approved by petrochenkov

bors · 2017-09-07T15:55:53Z

⌛ Testing commit 3454d99 with merge fd20f603855e10430b2cf9b06793f5c854c603c3...

bors · 2017-09-07T16:40:15Z

💔 Test failed - status-travis

kennytm · 2017-09-07T16:43:39Z

@bors retry

Travis's Macs are not feeling well this week. https://www.traviscistatus.com/incidents/4qylrqvy50gy

bors · 2017-09-07T17:38:21Z

⌛ Testing commit 3454d99 with merge 7eefbffdd79d1f1b18395bb578a607a2647c6b82...

bors · 2017-09-07T18:46:09Z

💔 Test failed - status-travis

spernsteiner · 2017-09-07T19:05:03Z

I see failures in run-make/sanitizer-address and run-make/sanitizer-dylib-link. I can't imagine how those tests could be affected by this PR, so is this another travis bug of some kind? If not, how should I start debugging? I've run those tests locally and they both pass.

petrochenkov · 2017-09-07T20:19:26Z

@bors retry

bors · 2017-09-07T22:35:50Z

⌛ Testing commit 3454d99 with merge cd398828b8d7e44c534eabcfd198dbe82007debc...

petrochenkov · 2017-09-07T22:40:02Z

Travis is probably going to fail again due to the issue addressed by #44398 and #44399

alexcrichton · 2017-09-07T22:42:07Z

@bors: retry

prioritizing travis: Downgrade to previous images temporarily #44399

bors · 2017-09-08T01:57:16Z

⌛ Testing commit 3454d99 with merge 57fe99b7a42ae25b3ea22f1bf37786c531cbbb56...

bors · 2017-09-08T04:58:07Z

💔 Test failed - status-appveyor

kennytm · 2017-09-08T05:27:39Z

@bors retry check i686-pc-windows-msvc 3 hour timed out.

bors · 2017-09-08T06:56:16Z

⌛ Testing commit 3454d99 with merge d6ad402...

pprust: fix parenthesization of exprs The pretty printer in `syntax::print::pprust` currently relies on the presence of `ExprKind::Paren` hints in order to correctly parenthesize expressions in its output. If `Paren` nodes are missing, it sometimes produces wrong output, such as printing `1 - (2 - 3)` as `1 - 2 - 3`. This PR fixes `pprust` to correctly print expressions regardless of the presence or absence of `Paren` nodes. This should make `pprust` easier to use with programmatically constructed ASTs. A few notes: * I added a function for assigning precedence values to exprs in `syntax::util::parser`, since there is already code there for assigning precedence values to binops. Let me know if I should move this somewhere more `pprust`-specific. * I also moved the `contains_exterior_struct_lit` function from `rustc_lint::unused::UnusedParens` into `syntax::util::parser`, since it's needed for determining the correct parenthesization of `if`/`while` conditional expressions. * I couldn't find a good way to compare two exprs for equivalence while ignoring semantically-irrelevant details like spans. So the test for the new behavior relies on a slight hack: it adds `Paren` nodes everywhere, so that the pretty-printed version exactly reflects the structure of the AST, and then compares the printed strings. This works, but let me know if there's a better way.

bors · 2017-09-08T09:47:55Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing d6ad402 to master...

Detect invalid exprs in parser used by pretty-printer tests This PR fixes a bug in rust-lang#133730 inherited from rust-lang#43742. Before this fix, the test might silently only operate on a prefix of some of the test cases in this table: https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/tests/ui-fulldeps/pprust-parenthesis-insertion.rs#L57 For example, adding the test case `1 .. 2 .. 3` (a syntactically invalid expression) into the table would unexpectedly succeed the test instead of crashing at this unwrap: https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/tests/ui-fulldeps/pprust-parenthesis-insertion.rs#L199-L200 because `parse_expr` would successfully parse just `1 .. 2` and disregard the last `.. 3`. This PR adds a check that `parse_expr` reaches `Eof`, ensuring all the test cases actually test the whole expression they look like they are supposed to.

Rollup merge of rust-lang#134599 - dtolnay:fulldepsparser, r=fmease Detect invalid exprs in parser used by pretty-printer tests This PR fixes a bug in rust-lang#133730 inherited from rust-lang#43742. Before this fix, the test might silently only operate on a prefix of some of the test cases in this table: https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/tests/ui-fulldeps/pprust-parenthesis-insertion.rs#L57 For example, adding the test case `1 .. 2 .. 3` (a syntactically invalid expression) into the table would unexpectedly succeed the test instead of crashing at this unwrap: https://github.com/rust-lang/rust/blob/13170cd787cb733ed24842ee825bcbd98dc01476/tests/ui-fulldeps/pprust-parenthesis-insertion.rs#L199-L200 because `parse_expr` would successfully parse just `1 .. 2` and disregard the last `.. 3`. This PR adds a check that `parse_expr` reaches `Eof`, ensuring all the test cases actually test the whole expression they look like they are supposed to.

rust-highfive assigned pnkfelix Aug 8, 2017

alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2017

petrochenkov self-assigned this Aug 12, 2017

nrc added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2017

aidanhs added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 31, 2017

spernsteiner added 3 commits September 6, 2017 10:26

pprust: fix parenthesization of exprs

b79dada

better explanatory comment for the pprust-expr-roundtrip test

5b2151e

hir::print: fix parenthesization of exprs

347db06

pprust: increase precedence of block-like exprs

3454d99

bors mentioned this pull request Sep 7, 2017

Fix extended bootstrap issues with OpenSSL on NetBSD build hosts #44320

Closed

kennytm mentioned this pull request Sep 7, 2017

Migrate a slew of metadata methods to queries #44142

Merged

bors merged commit 3454d99 into rust-lang:master Sep 8, 2017

bors mentioned this pull request Sep 8, 2017

Only consider yields coming after the expressions when computing generator interiors #44392

Merged

petrochenkov mentioned this pull request May 24, 2019

[let_chains, 2/6] Introduce Let(..) in AST, remove IfLet + WhileLet and parse let chains #60861

Merged

Aaron1011 mentioned this pull request Aug 20, 2020

Pretty-printing adds additional parentheses, breaking the proc-macro reparse check #75734

Closed

dtolnay mentioned this pull request Dec 21, 2024

Detect invalid exprs in parser used by pretty-printer tests #134599

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pprust: fix parenthesization of exprs #43742

pprust: fix parenthesization of exprs #43742

spernsteiner commented Aug 8, 2017

rust-highfive commented Aug 8, 2017

aidanhs commented Aug 10, 2017

petrochenkov commented Aug 12, 2017

arielb1 commented Aug 22, 2017

spernsteiner commented Aug 23, 2017

spernsteiner commented Sep 5, 2017

spernsteiner commented Sep 6, 2017

spernsteiner commented Sep 6, 2017

petrochenkov commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 7, 2017

kennytm commented Sep 7, 2017 •

edited

Loading

spernsteiner commented Sep 7, 2017

petrochenkov commented Sep 7, 2017

bors commented Sep 7, 2017

bors commented Sep 7, 2017

bors commented Sep 7, 2017

kennytm commented Sep 7, 2017

bors commented Sep 7, 2017

bors commented Sep 7, 2017

spernsteiner commented Sep 7, 2017

petrochenkov commented Sep 7, 2017

bors commented Sep 7, 2017

petrochenkov commented Sep 7, 2017

alexcrichton commented Sep 7, 2017

bors commented Sep 8, 2017

bors commented Sep 8, 2017

kennytm commented Sep 8, 2017

bors commented Sep 8, 2017

bors commented Sep 8, 2017

pprust: fix parenthesization of exprs #43742

pprust: fix parenthesization of exprs #43742

Conversation

spernsteiner commented Aug 8, 2017

rust-highfive commented Aug 8, 2017

aidanhs commented Aug 10, 2017

petrochenkov commented Aug 12, 2017

arielb1 commented Aug 22, 2017

spernsteiner commented Aug 23, 2017

spernsteiner commented Sep 5, 2017

spernsteiner commented Sep 6, 2017

spernsteiner commented Sep 6, 2017

petrochenkov commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 6, 2017

bors commented Sep 7, 2017

kennytm commented Sep 7, 2017 • edited Loading

spernsteiner commented Sep 7, 2017

petrochenkov commented Sep 7, 2017

bors commented Sep 7, 2017

bors commented Sep 7, 2017

bors commented Sep 7, 2017

kennytm commented Sep 7, 2017

bors commented Sep 7, 2017

bors commented Sep 7, 2017

spernsteiner commented Sep 7, 2017

petrochenkov commented Sep 7, 2017

bors commented Sep 7, 2017

petrochenkov commented Sep 7, 2017

alexcrichton commented Sep 7, 2017

bors commented Sep 8, 2017

bors commented Sep 8, 2017

kennytm commented Sep 8, 2017

bors commented Sep 8, 2017

bors commented Sep 8, 2017

kennytm commented Sep 7, 2017 •

edited

Loading