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

ast: Keep expansion status for out-of-line module items #82238

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

petrochenkov
Copy link
Contributor

I.e. whether a module mod foo; is already loaded from a file or not.
This is a pre-requisite to correctly treating inner attributes on such modules (#81661).

With this change AST structures for mod items diverge even more for AST structure for the crate root, which previously used ast::Mod.
Therefore this PR removes ast::Mod from ast::Crate in the first commit, these two things are sufficiently different from each other, at least at syntactic level.
Customization points for visiting a "mod item or crate root" were also removed from AST visitors (fn visit_mod).
ast::Mod itself was refactored away in the second commit in favor of ItemKind::Mod(Unsafe, ModKind).

@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 17, 2021
@petrochenkov
Copy link
Contributor Author

r? @Aaron1011

@rust-highfive rust-highfive assigned Aaron1011 and unassigned estebank Feb 17, 2021
@jyn514 jyn514 added A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 17, 2021
@@ -1014,19 +1010,18 @@ pub fn noop_visit_crate<T: MutVisitor>(krate: &mut Crate, vis: &mut T) {
id: DUMMY_NODE_ID,
vis: item_vis,
span,
kind: ItemKind::Mod(module),
kind: ItemKind::Mod(Mod { inner: span, unsafety: Unsafe::No, items, inline: true }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constructing this synthetic Mod seems kind of unfortunate, but it looks like it's necessary for the flat_map_item handling below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this either and left a FIXME above.
I'll try to investigate removing this some time later, together with similar code in fn expand_crate.

fn visit_mod(&mut self, m: &'a Mod, _s: Span, _attrs: &[Attribute], n: NodeId) {
let def_id = self.lctx.lower_node_id(n).expect_owner();

self.lctx.modules.insert(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be moved to the ItemKind::Mod(..) => match arm below? That would allow you to avoid using lctx.modules.entry in some of the other code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think entry is preferable, why pre-inserting empty values explicitly if entry can do everything for you automatically.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that the get_mut().unwrap() might have been a check to ensure that we had already processed the module in some way. If that's not the case, then I think your change is fine.

Crate root is sufficiently different from `mod` items, at least at syntactic level.

Also remove customization point for "`mod` item or crate root" from AST visitors.
Also remove `ast::Mod` which is mostly redundant now
self.end(); // end inner head-block
self.end(); // end outer head-block
match mod_kind {
ModKind::Loaded(items, ..) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this will always pretty-print the contents of outline modules - is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For --pretty=normal and --pretty=expanded this is a replacement of the removed self.is_expanded flag.

For synthesizing tokens for inner macro attributes on modules (which are broken until #81661 is fixed) I think it also makes sense to tokenize the whole module rather than just the mod foo; header.

@Aaron1011
Copy link
Member

r=me assuming that the pretty-printer change is intentional.

@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 18, 2021

📌 Commit 4a88165 has been approved by Aaron1011

@bors 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 Feb 18, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 19, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#79747 (Add explanations and suggestions to `irrefutable_let_patterns` lint)
 - rust-lang#81496 (name async generators something more human friendly in type error diagnostic)
 - rust-lang#81873 (Add Mutex::unlock)
 - rust-lang#82093 (Add tests for Atomic*::fetch_{min,max})
 - rust-lang#82238 (ast: Keep expansion status for out-of-line module items)
 - rust-lang#82245 (Do not ICE when evaluating locals' types of invalid `yield`)
 - rust-lang#82259 (Fix popping singleton paths in when generating E0433)
 - rust-lang#82261 (rustdoc: Support argument files)
 - rust-lang#82274 (libtest: Fix unwrap panic on duplicate TestDesc)
 - rust-lang#82275 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 30f39fe into rust-lang:master Feb 19, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 19, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 25, 2021
ast: Keep expansion status for out-of-line module items

I.e. whether a module `mod foo;` is already loaded from a file or not.
This is a pre-requisite to correctly treating inner attributes on such modules (rust-lang#81661).

With this change AST structures for `mod` items diverge even more for AST structure for the crate root, which previously used `ast::Mod`.
Therefore this PR removes `ast::Mod` from `ast::Crate` in the first commit, these two things are sufficiently different from each other, at least at syntactic level.
Customization points for visiting a "`mod` item or crate root" were also removed from AST visitors (`fn visit_mod`).
`ast::Mod` itself was refactored away in the second commit in favor of `ItemKind::Mod(Unsafe, ModKind)`.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 1, 2021
expand: Turn `ast::Crate` into a first class expansion target

And stop creating a fake `mod` item for the crate root when expanding a crate, thus addressing FIXMEs left in rust-lang#82238, and making a step towards a proper support for crate-level macro attributes (cc rust-lang#54726).

I haven't added token collection support for the whole crate in this PR, maybe later.
r? `@Aaron1011`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parser Area: The parsing of Rust source code to an AST S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants