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 printer inlines modules #12590

Closed
chris-morgan opened this Issue Feb 27, 2014 · 6 comments

Comments

Projects
None yet
6 participants
@chris-morgan
Copy link
Member

chris-morgan commented Feb 27, 2014

a.rs:

// a.rs
mod b;

b.rs:

// b.rs
fn b() { }

Expected output of rustc --pretty normal a.rs:

// a.rs
mod b;

Actual output of rustc --pretty normal a.rs:

// a.rs
mod b {
    fn b() { }
}

Note the two problems here: (a) the module is inlined, and (b) the comment in the module is lost. But by fixing the first, the second becomes irrelevant.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Apr 16, 2015

Triage: the output here has gotten weirder:

// a.rs
mod b {

    fn b() { }
}

I'm guessing the blank line comes from dropping the comment when b gets inlined, but who knows.

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jun 27, 2016

Triage: it is now un-weird, and back to the original way it was reported in the ticket.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented May 6, 2017

I personally thought that --pretty expanding modules was a feature. Is that incorrect? It seems like it's a good idea, not a bad one--unexpanded modules would make --pretty expanded less useful on multi-file codebases, since it wouldn't show expansion of anything but the crate root, which is non-ideal.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented May 6, 2017

But the original report is about --pretty=normal, not --pretty=expanded... @Mark-Simulacrum

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented May 6, 2017

Oh. Yeah, that's true. I'm not even really sure what the intended purpose of --pretty=normal is, I can't really think of any cases where it'd currently be the right solution. rustfmt is better if you just want pretty code...

@tinco

This comment has been minimized.

Copy link
Contributor

tinco commented Jul 12, 2018

I made a PR that fixes this behavior, turned out the parser simply forgot where module items come from, so it didn't know how to not expand modules, the fix was to track this information in the Ast.

bors added a commit that referenced this issue Jul 17, 2018

Auto merge of #52319 - tinco:issue_12590, r=pnkfelix
Track whether module declarations are inline (fixes #12590)

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.

bors added a commit that referenced this issue Jul 19, 2018

Auto merge of #52319 - tinco:issue_12590, r=pnkfelix
Track whether module declarations are inline (fixes #12590)

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.

bors added a commit that referenced this issue Sep 27, 2018

Auto merge of #52319 - tinco:issue_12590, r=pnkfelix
Track whether module declarations are inline (fixes #12590)

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.

@bors bors closed this in c3afb16 Sep 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.