Continue refactoring resolve and hygiene #63535
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matklad
reviewed
Aug 13, 2019
src/librustc_resolve/lib.rs
Outdated
| @@ -130,6 +130,17 @@ pub struct ParentScope<'a> { | |||
| derives: Vec<ast::Path>, | |||
| } | |||
|
|
|||
| impl<'a> ParentScope<'a> { | |||
| pub fn default(module: Module<'a>) -> ParentScope<'a> { | |||
There was a problem hiding this comment.
Defeault with parameter looks odd. Perhaps module_scope?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
👍 I find privacy super useful when reading new parts of the compiler
petrochenkov
added
the
S-waiting-on-review
|
r=me with conflicts resolved, with or without the comment being addressed. |
…::root()` For consistency with `ExpnId::root`. Also introduce a helper `Span::with_root_ctxt` for creating spans with `SyntaxContext::root()` context
`Ident` has had a full span rather than just a `SyntaxContext` for a long time now.
The expansion info is not optional and should always exist
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.
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.
Remove some unnecessary parameters from functions
It was very similar to `ParentScope` and mostly could be replaced by it.
By allocating its derive paths on the resolver arena.
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.
For naming consistency with everything else in this area
|
@bors r=matthewjasper |
|
📌 Commit c762773 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 15, 2019
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
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 15, 2019
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
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 16, 2019
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
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
|
☔ The latest upstream changes (presumably #63627) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
tesuji
added a commit
to tesuji/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
…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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 inHygieneDatanow (lessOptions).Also, some renaming.
This should be the last renaming session in this area, I think.
r? @matthewjasper