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.

Sign up for GitHub

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

Jump to bottom

Compiler panic on a macro invocation combined with invalid pub declaration #63223

Closed
xl483 opened this issue Aug 2, 2019 · 3 comments · Fixed by #63400
Closed

Compiler panic on a macro invocation combined with invalid pub declaration #63223

xl483 opened this issue Aug 2, 2019 · 3 comments · Fixed by #63400
Assignees
@petrochenkov
Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@xl483
Copy link

xl483 commented Aug 2, 2019

The compiler panics when compiling the following code. I had been expecting to see syntax errors instead. Deleting either line causes the compiler not to panic and give expected error messages.

const FOO: &str = b!("foo");
 
pub struct Bar { pub(in ::a) x: u64 }

(Playground)
@jonas-schievink jonas-schievink added A-resolve Area: Name resolution C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Aug 2, 2019
@Centril
Copy link
Contributor

Centril commented Aug 2, 2019

Reduced:

foo!();

pub(in ::bar) struct Baz {}

cc @petrochenkov

@petrochenkov
Copy link
Contributor

Nice.
This is not even a regression, it requires --edition 2018 and happens since 1.30 when the edition was stabilized.

@petrochenkov petrochenkov self-assigned this Aug 2, 2019
@nagisa nagisa removed the I-nominated label Aug 8, 2019
@Centril Centril added the A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) label Aug 8, 2019
@petrochenkov
Copy link
Contributor

Fixed in #63400 (as a side effect of making resolve_visibility to use early resolution methods).

@petrochenkov petrochenkov mentioned this issue Aug 10, 2019
Merged
Centril added a commit to Centril/rust that referenced this issue Aug 10, 2019
@Centril
Rollup merge of rust-lang#63400 - petrochenkov:resplit, r=eddyb
9d76a93 
Try to break resolve into more isolated parts

Some small step towards resolve librarification.

"Late resolution" is the pass that resolves most of names in a crate beside imports and macros.
It runs when the crate is fully expanded and its module structure is fully built.
So we just walk through the crate and resolve all the expressions, types, etc.

This pass is pretty self-contained, but it was previously done by implementing `Visitor` on the whole `Resolver` (which is used for many other tasks), and fields specific to this pass were indiscernible from the global `Resolver` state.

This PR moves the late resolution pass into a separate visitor and a separate file, fields specific to this visitor are moved from `Resolver` as well.

I'm especially happy about `current_module` being removed from `Resolver`.
It was used even for operations not related to visiting and changing the `current_module` position in process.
It was also used as an implicit argument for some functions used in this style
```rust
let orig_current_module = mem::replace(&mut self.current_module, module);
self.resolve_ident_somewhere();
self.current_module = orig_current_module;
```
and having effects on e.g. privacy checking somewhere deeply inside `resolve_ident_somewhere`.
Now we explicitly pass a `ParentScope` to those functions instead, which includes the module and some other data describing our position in the crate relatively to which we resolve names.

Rustdoc was one of the users of `current_module`, it set it for resolving intra-doc links.
Now it passes it explicitly as an argument as well (I also supported resolving paths from rustdoc in unnamed blocks as a drive-by fix).

Visibility resolution is also changed to use early resolution (which is correct because it's used during the work of `BuildReducedGraphVisitor`, i.e. integration of a new AST fragment into the existing partially built module structures.) instead of untimely late resolution (which worked only due to restrictions on paths in visibilities like inability to refer to anything except ancestor modules).
This slightly regresses its diagnostics because late resolution has a more systematic error detection and recovery currently.
Due to changes in `current_module` and visibilities `BuildReducedGraphVisitor` ended up almost as heavily affected by this refactoring as late resolution.

Fixes rust-lang#63223 (due to visibility resolution changes).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@petrochenkov petrochenkov

Labels
A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) A-resolve Area: Name resolution C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Try to break resolve into more isolated parts petrochenkov/rust
5 participants
@nagisa @Centril @jonas-schievink @petrochenkov @xl483