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

Move name resolution into phase 2 #33997

Merged
merged 3 commits into from
Jun 3, 2016
Merged

Conversation

jseyfried
Copy link
Contributor

r? @nrc

@jseyfried
Copy link
Contributor Author

This "unfixes" #33231, i.e. the following will error with rustc -Z no-analysis:

extern crate doesnt_exist; //~ ERROR can't find crate
fn main() {}

Currently, the following errors with rustc -Z no-analysis:

#[macro_use] extern crate doesnt_exist; //~ ERROR can't find crate
fn main() {}

I could keep #33231 fixed in this PR by checking for no-analysis in phase_2_configure_and_expand, but it would cause problems later on when expansion depends on import resolution, for example:

extern crate doesnt_exist; //< We need to know if this crate has a top-level module `bar` ...
mod bar {
    macro_rules! m { () => {} }
}
fn main() {
    use doesnt_exist::*; //< ... (which would be imported here) ...
    bar::m!(); //< ... before we can expand this.
}


let krate = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any particular reason for this change besides style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No.

@nrc
Copy link
Member

nrc commented Jun 1, 2016

r=me with the renamed struct.

@bors
Copy link
Contributor

bors commented Jun 1, 2016

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

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Jun 1, 2016

📌 Commit 3fc0407 has been approved by nrc

@bors
Copy link
Contributor

bors commented Jun 3, 2016

⌛ Testing commit 3fc0407 with merge 1ceaa86...

bors added a commit that referenced this pull request Jun 3, 2016
Move name resolution into phase 2

r? @nrc
@bors bors merged commit 3fc0407 into rust-lang:master Jun 3, 2016
@SiegeLord
Copy link
Contributor

I really wish I was pinged about this. I spent a lot of effort fixing this issue, and now my projects don't compile because my fix was reverted.

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 12, 2016

@SiegeLord sorry you weren't pinged.

We will need to load all extern crates (not just #[macro_use] crates) during macro expansion to implement RFC 1561. rustc -Z no-analysis does expansion, so it will need to load all extern crates.

If you want to avoid loading extern crates, you will have to use rustc -Z parse-only, which does not do expansion. However, rustc -Z parse-only --emit dep-info does not emit anything because macro expansion can introduce new non-inline modules (e.g. mod foo;), which affect the dep-info.

That being said, I believe it would be possible to have rustc -Z parse-only --emit dep-info emit the dep-info on a best effort basis. Would that work for your use case?

@SiegeLord
Copy link
Contributor

I'm sure there's a solution, but that's not really the issue here. In my fix I specifically added a test so that this wouldn't get re-broken, but if there's an option to remove the test then there's literally 0 motivation to fix this again, because then somebody else will just remove the test next time it's convenient for them. I'll just stop using this broken feature.

@jseyfried
Copy link
Contributor Author

jseyfried commented Jun 19, 2016

@SiegeLord
Your test wasn't removed just because it was convenient, it was removed because it is not feasible to support the feature that it was testing due to the upcoming improvements to macros.
As I described here, we will need to load all extern crates, not just #[macro_use] extern crates, to be able to perform macro expansion.

That being said, as I stated earlier, you should have been pinged so we could discuss ways to still support your use case.

I am offering to implement rustc -Z parse-only --emit dep-info emitting the dep info on a best effort basis. This would be feasible to support indefinitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants