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

macros: support invocation paths (e.g. foo::bar!()) behind #![feature(use_extern_macros)] #38082

Merged
merged 3 commits into from Dec 4, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Nov 30, 2016

r? @nrc

@jseyfried
Copy link
Contributor Author

cc #35896

@bors
Copy link
Contributor

bors commented Nov 30, 2016

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

@nrc
Copy link
Member

nrc commented Nov 30, 2016

Could you add an r-pass test for a successful macro use via a path please?

I don't see any checks for the feature gate - is it checked implicitly somewhere?

r = me with me the extra test and the feature gate checked if necessary

let kind =
if segments.last().unwrap().parameters.is_empty() { "module" } else { "macro" };
let msg = format!("type parameters are not allowed on {}s", kind);
self.session.span_err(path.span, &msg);
return Err(Determinacy::Determined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: error recovery here would be trivial - just proceed further instead of returning Err.

@jseyfried jseyfried force-pushed the macro_invocation_paths branch 2 times, most recently from 0e60b92 to ed96c0b Compare November 30, 2016 21:21
@jseyfried
Copy link
Contributor Author

I cleaned up the feature gating code, made macro invocation paths without use_extern_macros a gated feature error (instead of leaving the error unchanged), and added more tests.

@KalitaAlexey
Copy link
Contributor

Hi @jseyfried,
foo may be any path or it must be crate name?

@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 1, 2016

@KalitaAlexey It can be any path.

However, paths cannot resolve to local macro_rules! macros -- they must resolve to macros from external crates or imports/reexports of macros from external crates (this test has an example of both).

@jseyfried
Copy link
Contributor Author

@bors r=nrc

@bors
Copy link
Contributor

bors commented Dec 2, 2016

📌 Commit ff621ec has been approved by nrc

@bors
Copy link
Contributor

bors commented Dec 4, 2016

⌛ Testing commit ff621ec with merge b462e8f...

bors added a commit that referenced this pull request Dec 4, 2016
macros: support invocation paths (e.g. `foo::bar!()`) behind `#![feature(use_extern_macros)]`

r? @nrc
@bors bors merged commit ff621ec into rust-lang:master Dec 4, 2016
@jseyfried jseyfried deleted the macro_invocation_paths branch December 4, 2016 20:11
@bluss
Copy link
Member

bluss commented Dec 6, 2016

This brings up a question with self-recursive macros, should they always call themselves using the full path, using $crate? Maybe there should be a new variable representing the macro itself. At the same time, with better macro modularity, we'll have less self recursive macros and more that call each other.

@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 6, 2016

@bluss
A self-recursive macro that is used in the crate in which it is defined cannot call itself using the full path because a non-trivial macro path must resolve to an exported macro from an extern crate (whether macro_rules!, custom derive, or eventually macros 2.0) or a use import thereof.

In other words, a non-trivial macro invocation path must resolve to an item. Exported macros from extern crates are items and use import/reexports of these macros are items, but crate-local macro_rules! are not items -- their scope is difficult/impossible to coherently shoehorn into an item scope and they don't support pub anyway (I can go into more detail here if you'd like).

Maybe there should be a new variable representing the macro itself

I think it's best to wait for fully hygienic macros by example 2.0 (macro), which won't need this or $crate.

@bluss
Copy link
Member

bluss commented Dec 7, 2016

The limitation that they can't be called inside their own crate sounds fair and not like a too big problem.

@jseyfried
Copy link
Contributor Author

jseyfried commented Dec 8, 2016

@bluss (cc @nrc)
My main motivation for #![feature(use_extern_macros)] is to let crates that migrate to macros 2.0 use macros from upstream dependencies that haven't migrated yet. This way, crate authors won't need use and #[macro_use] macro imports in the same crate (they interact badly w.r.t. shadowing), and deprecating #[macro_use] would be less painful.

It turned out that we can land this (and even stabilize if we want to) before macros by example 2.0 (RFC 1584), expediting these benefits. However, since MBE 2.0 is so close, I think it would be a bad idea to land any further improvements that won't be needed with MBE 2.0.

That being said, I think we should land further improvements that will be useful with MBE 2.0. For example, if an upstream crate defines a recursive macro_rules! foo, it will currently not expand correctly if downstream crates use upstream::foo as bar; (this also applies to mutually recursive 1.0 macros from upstream crates).

I believe we can solve this backwards compatibly by resolving a macro name from an external macro_rules! macro definition first in the external crate's exported macros. If this changes the resolution (seems unlikely), we could just use the old resolution for back-compat, do a warning cycle, etc.

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.

None yet

6 participants