Track whether module declarations are inline (fixes #12590)

[tinco]

To track whether module declarations are inline I added a field inline: bool to ast::Mod. The main use case is for pretty to know whether it should render the items associated with the module, but perhaps there are use cases for this information to not be forgotten in the AST.

[rust-highfive]

r? @pnkfelix

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

[rust-highfive]

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

[00:04:25] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:25] tidy error: /checkout/src/libsyntax/parse/parser.rs:6225: line longer than 100 chars
[00:04:26] some tidy checks failed
[00:04:26] 
[00:04:26] 
[00:04:26] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:26] 
[00:04:26] 
[00:04:26] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:26] Build completed unsuccessfully in 0:00:46
[00:04:26] Build completed unsuccessfully in 0:00:46
[00:04:26] Makefile:79: recipe for target 'tidy' failed
[00:04:26] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:29f22201
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:0a179cd8:start=1531419239276042629,finish=1531419239284730691,duration=8688062
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1987f593
$ head -30 ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
head: cannot open ‘./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers’ for reading: No such file or directory
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:08720b83
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[tinco]

Woops, forgot to run tidy

[pnkfelix]

This is a relatively minor point, but: You might as well move this line into the body of fn eval_src_mod, right? We might as well have it fully encapsulate setting up all the state related to reading a file as a module, rather than leaving this one step for its caller...

[tinco]

Yes, I was afraid maybe someone would call eval_src_mod from somewhere else, but later I discovered this is the only place it is called.

[pnkfelix]

A more important issue: I actually am in the camp that regards the current behavior as a feature, not a bug. I like getting a single file with the whole module tree in it...

Having said that, I guess I'd be okay if the semantics of "one file with whole module tree" were restricted to --pretty=expanded and everything beyond, while the behavior described on this PR were restricted to --pretty=normal.

Can you tell me off hand how your PR handles --pretty=expanded? My current guess is that the change in the semantics here is being applied here to all variants of --pretty, which I personally would not be a big fan of...

• (see comment linked above for at least one good reason why that change to --pretty=expanded would be undesirable)

[tinco]

Good point, I'll add a test for expanded to verify that it works for expanded.

[tinco]

@pnkfelix I made the behavior as you described, but for some reason the test suite doesn't pick it up. I tried to debug by adding printlines to the parser, but that breaks compilation of stage0, and for some reason --keep-stage 0 is broken so I can't skip that compilation. I couldn't print to stderr either because the rust test harness eats stderr. I'll see if I can figure it out from the source code tomorrow.

It's hard to keep a nice development pace when it's interspersed by these 15 minute+ compilation bouts, I spent 4 hours on finding out that my println!'s were breaking the stage0 compilation, just because I get distracted between invocations and go do other tasks. Do you have some trick to be productive while working on rustc?

[pnkfelix]

@tinco I'll check out your repo and take a look.

Out of curiosity, in your attempt to instrument the code, why did you use println! rather than debug!?

[pnkfelix]

@tinco I think I see what you mean: when I run rustc -Z unpretty=expanded, I see output with the module contents inlined. Even so, the compiletest run is for some reason claiming that it sees no content emitted, e.g. just mod issue_12590_b { } for the case of pretty/issue_12590_c.rs

I'll keep digging for a bit. Not sure whether this is some compiletest oddity.

[pnkfelix]

OH!

The compiletest, at least for the pretty printer: It runs by piping the source file into a run of rustc!

That's why, for example, if you add --verbose to the command invocation of compiletest, you'll see this output for its description of how it is invoking rustc for this test:

  executing "/home/pnkfelix/Dev/Mozilla/rust-pp/objdir-dbgopt/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "-" "-Z" "unpretty=expanded" "--target" "x86_64-unknown-linux-gnu" "-L" "/home/pnkfelix/Dev/Mozilla/rust-pp/objdir-dbgopt/build/x86_64-unknown-linux-gnu/test/pretty/issue_12590_c/auxiliary.pretty" "-Crpath" "-Zunstable-options" "-Lnative=/home/pnkfelix/Dev/Mozilla/rust-pp/objdir-dbgopt/build/x86_64-unknown-linux-gnu/native/rust-test-helpers"
                                                                                                               ~~~
                                                                                                               This is where the input source file would normally be written

I don't even know if we specify how mod f; behaves when rustc is taking its input from ; my instinct would be that it should look for the mod in the current working directory, but the "feature" becomes quite questionable/risky at that point.

So, what now?!

We can either:

1. Figure out how to make compiletest use actual paths rather than for the pretty print output, (which might require also learning why it was written to use in the first place), or
2. Encode your unit test in some other fashion, like a run-make/ test, for example, where we have more control over the exact way that rustc is invoked.

[pnkfelix]

I bet the reason that compiletest tests the pretty printing via piping the source is it uses a iterative loop where it takes the printed output and feeds that back into the compiler again.

[pnkfelix]

If you want to try to tackle changing compiletest so that it feeds in the original filename on the first iteration (and only that iteration), the place to start looking is here, the entry point for where we run a given pretty-printing test:

rust/src/tools/compiletest/src/runtest.rs

Line 393 in 025e04e

fn run_pretty_test(&self) {

The iterative loop I described is starts here:

rust/src/tools/compiletest/src/runtest.rs

Line 416 in 025e04e

while round < rounds {

The actual invocations of the compiler are driven by the calls to print_source, here:

rust/src/tools/compiletest/src/runtest.rs

Line 424 in 025e04e

let proc_res = self.print_source(srcs[round].to_owned(), &self.props.pretty_mode);

... whose source code is right below, here:

rust/src/tools/compiletest/src/runtest.rs

Line 498 in 025e04e

fn print_source(&self, src: String, pretty_type: &str) -> ProcRes {

and you can see the spot where we are feeding in "-" as the input to read source from on this line:

rust/src/tools/compiletest/src/runtest.rs

Line 503 in 025e04e

.arg("-")

So I might suggest changing fn print_source to take a flag indicating whether to read from stdin or not, and then have the iterative loop tell it to read from stdin whenever round > 0.

[pnkfelix]

In fact, after writing up those instructions above, I figured it would be easiest if I just went ahead and made the necessary change to your PR myself.

[pnkfelix]

@bors r+

[bors]

📌 Commit fcbb3dd has been approved by pnkfelix

[tinco]

hi @pnkfelix thanks a lot for chiming in on this PR! I would've probably taken a couple days to figure out how compiletest worked :)

I used println! instead of debug! because I didn't know/forgot it existed :P I had a 2 year break from rust, and although I'm still ok with the general idea and semantics, I'm a bit rusty (hehe) with many workflow details. Working on these compiler bugs is fun because I read through loads of rust and get back into it :)

I found this bug just by filtering a little bit and sorting on oldest first, and looking at the first one that didn't have many people theorizing on what to do. If you know of any other bug that's ready for someone to dive in to that has slightly more relevance to the real world than this one please let me know!

[bors]

⌛ Testing commit fcbb3dd with merge bc234f177bb2202a1a56d62c09dffb3f4737e0a0...

[bors]

💔 Test failed - status-appveyor

[tinco]

Alright, so there's two bugs at play here. The first is that for some reason for this stmt_expr_attributes.rs test on Windows has a result with one extra line ending at the end. I'm not sure why it's only this test, and why it's only on Windows.

The second is that the diff thingy hangs on windows when the two files that are compared are too large it seems. I have narrowed the test case down to three test functions, and removing either of them leads to a test that fails, instead of hanging. It might be that this is actually also the case on linux, but that we simply haven't seen it fail this way yet on linux.

[tinco]

Hmm no, the line ending thing was a red herring. I added a newline at the end of the file I'm testing with, and now if I remove any of the three tests I have left the suite passes. So there's something fishy going on with the comparison, regardless of whether it's going to fail or not.

This is the file I've made that hangs on Windows, and if you remove any of the three test functions it passes:

// Copyright 2015 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.

// pp-exact

#![feature(custom_attribute)]
#![feature(box_syntax)]
#![feature(stmt_expr_attributes)]

fn main() { }

struct Foo {
    data: (),
}

struct Bar(());

fn _9() {
    macro_rules! stmt_mac((  ) => { let _ = (  ) ; });

    #[attr]
    stmt_mac!();

    /*
    // pre existing pp bug: delimiter styles gets lost:

    #[attr]
    stmt_mac!{ };

    #[attr]
    stmt_mac![];

    #[attr]
    stmt_mac!{ } // pre-existing pp bug: compiler ICEs with a None unwrap
    */

    let _ = ();
}

macro_rules! expr_mac((  ) => { (  ) });

fn _10() {

    let _ = #[attr] expr_mac!();

    /*
    // pre existing pp bug: delimiter styles gets lost:
    let _ = #[attr] expr_mac![];
    let _ = #[attr] expr_mac!{};
    */
}

fn _11() {
    let _ = #[attr] box 0;
    let _: [(); 0] = #[attr] [#![attr] ];
    let _ = #[attr] [#![attr] 0, 0];
    let _ = #[attr] [#![attr] 0; 0];
    let _ = #[attr] foo();
    let _ = #[attr] 1i32.clone();
    let _ = #[attr] (#![attr] );
    let _ = #[attr] (#![attr] 0);
    let _ = #[attr] (#![attr] 0,);
    let _ = #[attr] (#![attr] 0, 0);
    let _ = #[attr] 0 + #[attr] 0;
    let _ = #[attr] !0;
    let _ = #[attr] -0i32;
    let _ = #[attr] false;
    let _ = #[attr] 'c';
    let _ = #[attr] 0;
    let _ = #[attr] 0 as usize;
    let _ =
        #[attr] while false {
                    #![attr]
                };
    let _ =
        #[attr] while let None = Some(()) {
                    #![attr]
                };
    let _ =
        #[attr] for _ in 0..0 {
                    #![attr]
                };
    // FIXME: pp bug, two spaces after the loop
    let _ =
        #[attr] loop  {
                    #![attr]
                };
    let _ =
        #[attr] match false {
                    #![attr]
                    _ => (),
                };
    let _ = #[attr] || #[attr] ();
    let _ = #[attr] move || #[attr] ();
    let _ =
        #[attr] ||
                    {
                        #![attr]
                        #[attr]
                        ()
                    };
    let _ =
        #[attr] move ||
                    {
                        #![attr]
                        #[attr]
                        ()
                    };
    let _ =
        #[attr] {
                    #![attr]
                };
    let _ =
        #[attr] {
                    #![attr]
                    let _ = ();
                };
    let _ =
        #[attr] {
                    #![attr]
                    let _ = ();
                    ()
                };
    let mut x = 0;
    let _ = #[attr] x = 15;
    let _ = #[attr] x += 15;
    let s = Foo{data: (),};
    let _ = #[attr] s.data;
    let _ = (#[attr] s).data;
    let t = Bar(());
    let _ = #[attr] t.0;
    let _ = (#[attr] t).0;
    let v = vec!(0);
    let _ = #[attr] v[0];
    let _ = (#[attr] v)[0];
    let _ = #[attr] 0..#[attr] 0;
    let _ = #[attr] 0..;
    let _ = #[attr] (0..0);
    let _ = #[attr] (0..);
    let _ = #[attr] (..0);
    let _ = #[attr] (..);
    let _: fn(&u32) -> u32 = #[attr] std::clone::Clone::clone;
    let _ = #[attr] &0;
    let _ = #[attr] &mut 0;
    let _ = #[attr] &#[attr] 0;
    let _ = #[attr] &mut #[attr] 0;
    // FIXME: pp bug, extra space after keyword?
    while false { let _ = #[attr] continue ; }
    while true { let _ = #[attr] break ; }
    || #[attr] return;
    let _ = #[attr] expr_mac!();
    /* FIXME: pp bug, losing delimiter styles
    let _ = #[attr] expr_mac![];
    let _ = #[attr] expr_mac!{};
    */
    let _ = #[attr] Foo{#![attr] data: (),};
    let _ = #[attr] Foo{#![attr] ..s};
    let _ = #[attr] Foo{#![attr] data: (), ..s};
    let _ = #[attr] (#![attr] 0);
}

fn foo() { }
fn foo3(_: i32, _: (), _: ()) { }
fn qux(_: i32) { }

[tinco]

I narrowed it down to where just removing 1 character makes it pass. The character can be inside a comment.

[tinco]

BAM! I got it :D so the thing was, @pnkfelix added this option to read source from a file instead of reading it from standard in. This works, but for some reason the variable that's supposed to hold the string to be written to standard in is still filled. This is no problem on Linux and OSX, because when you write to the stdin of a command that's not reading any stdin, it just gets infinitely buffered or thrown away or something. On Windows however there is apparently only a very small buffer for stdin, so when you write a large file to the stdin of a command that's not reading from stdin (even though that command has exited) it blocks indefinitely! So the fix is to not write to stdin, if we told it to read from a file.

It reveals a different bug though, why do we pass in a populated src, if we're trying to read from a file? I'll see what's going on, maybe that's a bug as well.

[TimNN]

Ping from triage @pnkfelix: It looks like this PR is ready for your review.

[pnkfelix]

@bors r+

[bors]

📌 Commit b985e91 has been approved by pnkfelix

[bors]

⌛ Testing commit b985e91 with merge c4501a0...

[tinco]

yessss.... 🥁

[bors]

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

[topecongiro]

Nice, this PR will make rustfmt's life a bit easier 🙌