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

Continue refactoring resolve and hygiene #63535

Merged
merged 16 commits into from Aug 16, 2019

Conversation

@petrochenkov
Copy link
Contributor

commented Aug 13, 2019

The general goal is addressing FIXMEs from the previous PRs.

Merging similar data structures (+ prerequisites for such merging), accounting for the fact that all ExpnIds have associated data in HygieneData now (less Options).

Also, some renaming.
This should be the last renaming session in this area, I think.

r? @matthewjasper

@@ -130,6 +130,17 @@ pub struct ParentScope<'a> {
derives: Vec<ast::Path>,
}

impl<'a> ParentScope<'a> {
pub fn default(module: Module<'a>) -> ParentScope<'a> {

This comment has been minimized.

Copy link
@matklad

matklad Aug 13, 2019

Member

Defeault with parameter looks odd. Perhaps module_scope?

This comment has been minimized.

Copy link
@matthewjasper

matthewjasper Aug 15, 2019

Contributor

I agree that default isn't great here, but I'm not sure what to call it. Either way, I feel like a comment for this function would be helpful.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Aug 15, 2019

Author Contributor

Renamed into ParentScope::module and some comments are added.

pub struct BuildReducedGraphVisitor<'a, 'b> {
pub r: &'b mut Resolver<'a>,
pub parent_scope: ParentScope<'a>,
struct BuildReducedGraphVisitor<'a, 'b> {

This comment has been minimized.

Copy link
@matklad

matklad Aug 13, 2019

Member

👍 I find privacy super useful when reading new parts of the compiler

@matthewjasper

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

r=me with conflicts resolved, with or without the comment being addressed.

petrochenkov added some commits Aug 10, 2019

syntax_pos: `NO_EXPANSION`/`SyntaxContext::empty()` -> `SyntaxContext…
…::root()`

For consistency with `ExpnId::root`.

Also introduce a helper `Span::with_root_ctxt` for creating spans with `SyntaxContext::root()` context
`Ident::with_empty_ctxt` -> `Ident::with_dummy_span`
`Ident` has had a full span rather than just a `SyntaxContext` for a long time now.
hygiene: Remove `Option`s from functions returning `ExpnInfo`
The expansion info is not optional and should always exist
resolve: Move macro resolution traces from `Module`s to `Resolver`
Traces already contain module info without that.
It's easy to forget to call `finalize_*` on a module.
In particular, macros enum and trait modules weren't finalized.
By happy accident macros weren't placed into those modules until now.
resolve: Do not "normalize away" trait/enum modules prematurely
The previous approach was brittle - what would happen if `ParentScope` wasn't created by `invoc_parent_scope`?
That's exactly the case for various uses of `ParentScope` in diagnostics and in built-in attribute validation.
resolve: Add `ParentScope::default`, eliminate `dummy_parent_scope`
Remove some unnecessary parameters from functions
resolve: Eliminate `InvocationData`
It was very similar to `ParentScope` and mostly could be replaced by it.
resolve: Make `ParentScope` `Copy`
By allocating its derive paths on the resolver arena.
syntax_pos: Remove the duplicate global edition
It was introduced to avoid going through `hygiene_data`, but now it's read only once, when `ParseSess` is created, so going through a lock is ok.
hygiene: `ExpnInfo` -> `ExpnData`
For naming consistency with everything else in this area

@petrochenkov petrochenkov force-pushed the petrochenkov:expndata branch from f1ef7a9 to c762773 Aug 15, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

@bors r=matthewjasper

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

📌 Commit c762773 has been approved by matthewjasper

Centril added a commit to Centril/rust that referenced this pull request Aug 15, 2019

Rollup merge of rust-lang#63535 - petrochenkov:expndata, r=matthewjasper
Continue refactoring resolve and hygiene

The general goal is addressing FIXMEs from the previous PRs.

Merging similar data structures (+ prerequisites for such merging), accounting for the fact that all `ExpnId`s have associated data in `HygieneData` now (less `Option`s).

Also, some renaming.
This should be the last renaming session in this area, I think.

r? @matthewjasper

bors added a commit that referenced this pull request Aug 15, 2019

Auto merge of #63611 - Centril:rollup-rpuz20q, r=Centril
Rollup of 3 pull requests

Successful merges:

 - #63535 (Continue refactoring resolve and hygiene)
 - #63539 (Suggest Rust 2018 on `<expr>.await` with no such field)
 - #63584 (libcore: more cleanups using `#![feature(associated_type_bounds)]`)

Failed merges:

 - #63545 (Feature gate 'yield $expr?' pre-expansion)

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Aug 15, 2019

Rollup merge of rust-lang#63535 - petrochenkov:expndata, r=matthewjasper
Continue refactoring resolve and hygiene

The general goal is addressing FIXMEs from the previous PRs.

Merging similar data structures (+ prerequisites for such merging), accounting for the fact that all `ExpnId`s have associated data in `HygieneData` now (less `Option`s).

Also, some renaming.
This should be the last renaming session in this area, I think.

r? @matthewjasper

bors added a commit that referenced this pull request Aug 15, 2019

Auto merge of #63617 - Centril:rollup-zqsuhyy, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #62593 (Group all ABI tests.)
 - #63487 (Remove meaningless comments in src/test)
 - #63535 (Continue refactoring resolve and hygiene)
 - #63539 (Suggest Rust 2018 on `<expr>.await` with no such field)
 - #63584 (libcore: more cleanups using `#![feature(associated_type_bounds)]`)
 - #63612 (Do not suggest `try_into` for base types inside of macro expansions)
 - #63615 (Fix typo in DoubleEndedIterator::nth_back doc)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019

Rollup merge of rust-lang#63535 - petrochenkov:expndata, r=matthewjasper
Continue refactoring resolve and hygiene

The general goal is addressing FIXMEs from the previous PRs.

Merging similar data structures (+ prerequisites for such merging), accounting for the fact that all `ExpnId`s have associated data in `HygieneData` now (less `Option`s).

Also, some renaming.
This should be the last renaming session in this area, I think.

r? @matthewjasper

bors added a commit that referenced this pull request Aug 16, 2019

Auto merge of #63627 - Centril:rollup-v8i7x5i, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #62593 (Group all ABI tests.)
 - #63173 (Use libunwind from llvm-project submodule for musl targets)
 - #63535 (Continue refactoring resolve and hygiene)
 - #63539 (Suggest Rust 2018 on `<expr>.await` with no such field)
 - #63584 (libcore: more cleanups using `#![feature(associated_type_bounds)]`)
 - #63612 (Do not suggest `try_into` for base types inside of macro expansions)
 - #63615 (Fix typo in DoubleEndedIterator::nth_back doc)

Failed merges:

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

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

@bors bors merged commit c762773 into rust-lang:master Aug 16, 2019

4 checks passed

pr Build #20190815.42 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

lzutao added a commit to lzutao/rust-clippy that referenced this pull request Aug 16, 2019

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#63667 - petrochenkov:deriveholders, r=matth…
…ewjasper

resolve: Properly integrate derives and `macro_rules` scopes

So,
```rust
#[derive(A, B)]
struct S;

m!();
```
turns into something like
```rust
struct S;

A_placeholder!( struct S; );

B_placeholder!( struct S; );

m!();
```
during expansion.

And for `m!()` its "`macro_rules` scope" (aka "legacy scope") should point to the `B_placeholder` call rather than to the derive container `#[derive(A, B)]`.

`fn build_reduced_graph` now makes sure the legacy scope points to the right thing.
(It's still a mystery for me why this worked before rust-lang#63535.)

Unfortunately, placeholders from derives are currently treated separately from placeholders from other macros and need to be passed as `extra_placeholders` rather than a part of the AST fragment.
That's fixable, but I wanted to keep this PR more minimal to close the regression faster.

Fixes rust-lang#63651
r? @matthewjasper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.