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

Reimplement routes! and catchers! as proc_macros #680

Closed
wants to merge 5 commits into from

Conversation

jebrosen
Copy link
Collaborator

What the title says. Also adds compiletest to the codegen_next crate and updates all examples and tests to properly import the macros.

Unfortunately, at present #![feature(proc_macro_non_items)] is required on downstream crates. They are also imported with use like items are instead of with #[macro_use], which makes for a noisy commit.

This branch does not currently move any of the associated documentation.

link_flag("-L", "crate", &[]),
link_flag("-L", "dependency", &["deps"]),
extern_dep("rocket_codegen", Kind::Dynamic).expect("find codegen dep"),
extern_dep("rocket", Kind::Static).expect("find core dep")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should rocket_codegen_next go in this list as well?

let mut paths = parser.parse_sep(Seperator::Comma, |p| {
p.parse::<Path>()
})?;
parser.eof()?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Testing for eof() is necessary, otherwise something like routes![a, b c] will be accepted and parse as only a,b. The compiler plugin macro says it expected ,, ::, or <eof>. This proc_macro currently just says expected eof.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try replacing the error message with a better one, like "expected paths separated by commas".

@jebrosen
Copy link
Collaborator Author

jebrosen commented Jul 27, 2018

As discussed in IRC, it would be nice if users didn't need to import these macros just like today.

Current situation:
routes!, catchers!, uri!, and rocket_internal_uri! appear to be "magic" globally available macros because they're globally registered via the compiler plugin. They don't need to be (and probably can't) be imported via any means other than the plugin, that I know of.

proc_macro situation:
routes! and catchers! (so far) are proc_macros. These are only available via use_extern_macros, whether by a use statement or a full path at the call site. I thought that a macro_rules macro could get around that, but I always seem to end up needing use_extern_macros in the consumer crate anyway. It does look like it will be possible to use a macro_rules macro in conjunction with some pub use and $crate in order to allow #[macro_use] instead of use rocket::routes;, but the feature gate may have to stay.

2018 crates:
Crates targeting 2018 can drop 'extern crate' declarations, and importing individual macros via use is posed as an alternative for #[macro_use]. The question is, what path should Rocket's macros be available at for 2018 crates. I will do some more testing to figure out how modules, paths, and imports interact with this.

Proposed design, assuming this all works:

  • proc_macros (routes, catchers, etc.) are defined in a proc_macro crate
  • The rocket crate will import the proc_macros into <MODULE A>
  • The rocket crate will define macro_rules wrapper macros in <MODULE B> that call $crate::<MODULE A>::macro!
  • 2015 crates will now add #[macro_use] to extern crate rocket; and have access to the macro_rules macros
  • 2018 crates do not need #[macro_use]. They should be able to use macros from either <MODULE A> or <MODULE B> (I will verify this) but we should recommend one or the other, probably the one with the direct proc_macro reexports.

One of <MODULE A> or <MODULE B> could be the crate root, depending on how we want the paths to look to the 2018 crates that import macros.

Note that attribute proc_macros and proc_macro derives are not covered above. They are in a similar situation as far as imports go, but a macro_rules workaround can't be used in this situation. Derives appear to be covered by #[macro_use], I'm not sure about attributes though.

@jebrosen
Copy link
Collaborator Author

jebrosen commented Aug 4, 2018

Now implements the following variant of the above design:

  • routes and catchers are reimplemented as proc_macros: rocket_routes_internal and rocket_catchers_internal, in rocket_codegen_next
  • rocket already glob-reexports the macros from rocket_codegen_next
  • rocket now defines a macro_rules! macro called routes! that simply forwards its arguments to $crate::rocket_routes_internal, and the same with catchers!
    • this requires use_extern_macros, which will hopefully be stable soon. It is already active in Rocket's examples and tests via the decl_macro feature.
  • All consumer crates need to add #[feature(proc_macro_non_items)], and import the macros:
    • Import Option A, idiomatic for Rust 2015: add a #[macro_use] extern crate rocket;
    • Import Option B, idiomatic for Rust 2018: add a use rocket::routes; or use rocket::catchers; and omit extern crate rocket; completely.

My plan is to investigate the use of proc_macro_hack next. Hopefully it will make #[feature(proc_macro_path_invoc)] and maybe even the macro_rules wrappers unnecessary. mashup is another possibility, but the error messages can be very confusing when routes! is used incorrectly.

EDIT: proc_macro_hack looks undesirable because it goes through strings, discarding all useful span information.

@jebrosen jebrosen force-pushed the proc_macros_a2 branch 3 times, most recently from 433feb3 to 0902910 Compare August 7, 2018 07:31
@jebrosen jebrosen force-pushed the proc_macros_a2 branch 2 times, most recently from a29c47a to ff69434 Compare August 24, 2018 21:32
@SergioBenitez
Copy link
Member

Merged in 8e77961. Woo!

@SergioBenitez SergioBenitez added the pr: merged This pull request was merged manually. label Sep 17, 2018
@jebrosen jebrosen deleted the proc_macros_a2 branch September 17, 2018 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: merged This pull request was merged manually.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants