Remove use of decl_macro by using a different hack.

[jebrosen]

This removes the use of decl_macro in generated routes, removing it from the list of unstable features rocket depends on. This is done according to the strategy outlined in #19 (comment), which became possible when uniform_paths was implemented and/or stabilized.

However, it requires that crates that use #[route] are compiled with the 2018 edition - see https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=17ec58c07d13f2309458dcf9060e642a, a smaller reproduction that works in the 2018 edition but fails on 2015 for the same reason as this approach.

Unresolved:

• Is reexporting macro_rules macros like this really intended to work? It almost seems like an accident, and it doesn't seem to be communicated/documented very well. Yes, it is.
• Is it at all possible to accomplish this in the 2015 edition? Whatever solution is used must also work for crates using the 2018 edition. Note that async/await support will almost certainly require all application crates to update to the 2018 edition anyway, so 2015 compatibility for this issue might not be worth the effort.
  • Yes, as long as rocket_codegen itself is 2018 edition and we use the right spans the "hack" will work for both 2015 and 2018 application crates.
• The random suffix generation is necessary with #[macro_export] + pub use, because all macros globally exported with macro_export must be unambiguous. It looks like we can skip #[macro_export] and do a pub(crate) use instead, making the random generation unnecessary. However, I believe that would restrict the uri! macro to being called from the same crate where the route was defined.
  • Instead of a random suffix, this could be a hash of file+line+column or something similar.
• blah blah 2015 2018 worrying Will wait for Rust 2018 #1013 to be merged, and rocket_codegen is 2018 edition and this PR will be much easier.
• The "real" macro should be #[doc(hidden)] if possible.
• This bumps the minimum rustc version, probably around 2019-01-13 (not as recent as try_from, but this is codegen and try_from is in core)
  • Something else already bumped this version up

[macpp]

Is reexporting macro_rules macros like this really intended to work?

I think that this is intended, pr that implemented this #rust-lang/rust#56759 was reviewed by many rust team members.

[jebrosen]

As an alternative to random generation, we could hash the source file path + line + column of a function available via Span to generate the "random" suffix. It should the added benefit of being relatively stable and we don't need to depend on rand that way.

[petrochenkov]

Is reexporting macro_rules macros like this really intended to work?

Yes!
"Uniform paths" means that import paths are indeed uniform with non-import paths and anything can be reexported.

Is it at all possible to accomplish this in the 2015 edition?

If the pub use #inner_generated_macro_name as #generated_macro_name; code is generated by a 2018 proc macro crate (which seems to be the case), then it will "just work" even if used from a 2015 crate due to edition hygiene.

[jebrosen]

Is reexporting macro_rules macros like this really intended to work?

Yes!
"Uniform paths" means that import paths are indeed uniform with non-import paths and anything can be reexported.

Awesome, thanks for the confirmation!

If the pub use #inner_generated_macro_name as #generated_macro_name; code is generated by a 2018 proc macro crate (which seems to be the case), then it will "just work" even if used from a 2015 crate due to edition hygiene.

The proc macro crate is actually still 2015 edition. I will try all the combinations with a minimal test case to double check, and I'll come back to this PR after #1013 is taken care of.

[jebrosen]

I think this is ready for a review. I'm not happy with the way the documentation looks - I wanted the real, ugly-named macro to be doc(hidden) but inline its docs onto the nicely named re-export. But I could only seem to get both or neither to be documented.

[SergioBenitez]

This is phenomenal! I'm so excited to get rid of decl_macro!

My biggest concern is that we've increased our use of unstable APIs from proc_macro_span in the generation of the pseudo-random hash. In particular, through the use of SourceFile, start(), and source_file(). Why isn't hashing the function name sufficient? Two functions in the same scope must necessary have different function names. Does doing this result in poor error messages?

More procedurally, I'd like to see this PR split into two commits: one to implement the hack, and the other to remove the decl_macro feature flag everywhere.

[SergioBenitez]

Do we actually need this? This is more of a curiosity level question than a "change this" comment.

[jebrosen]

No, it's not needed anymore. This might have been because rocket_codegen was itself a 2015 edition crate when I started working on this, and edition hygiene is a bit wonky sometimes. I'll make sure we have appropriate test coverage.

[SergioBenitez]

I think this is ready for a review. I'm not happy with the way the documentation looks - I wanted the real, ugly-named macro to be doc(hidden) but inline its docs onto the nicely named re-export. But I could only seem to get both or neither to be documented.

Which macros are you referring to? I don't see any issues with the docs, but I may be missing or, or I may be expecting to see neither macro.

[jebrosen]

Why isn't hashing the function name sufficient?

The same function name might be used in two different modules, and two macros with #[macro_export] must be unambiguous across the entire crate - see https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9f139eb8cba9c54e49c380c7afb13736 for an example.

I only saw three ways out: 1) get rid of the #[macro_export], 2) generate a random disambiguator, or 3) generate a disambiguator based on more than just the name.

Unfortunately these all have downsides: 1) means we have to reexport with a pub(crate) use instead of a pub use, which would then restrict uri! from working across crates, 2) is not deterministic, and 3) uses more unstable features -- which I hadn't realized.

Which macros are you referring to? I don't see any issues with the docs, but I may be missing or, or I may be expecting to see neither macro.

In the past a macro was visible in the docs for the crate the routes were defined in (maybe only if the route was pub). I would just as soon make them all hidden on purpose, come to think of it.

[SergioBenitez]

Why isn't hashing the function name sufficient?

The same function name might be used in two different modules, and two macros with #[macro_export] must be unambiguous across the entire crate - see https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9f139eb8cba9c54e49c380c7afb13736 for an example.

Got it. Thanks for explaining this.

I only saw three ways out: 1) get rid of the #[macro_export], 2) generate a random disambiguator, or 3) generate a disambiguator based on more than just the name.

Unfortunately these all have downsides: 1) means we have to reexport with a pub(crate) use instead of a pub use, which would then restrict uri! from working across crates, 2) is not deterministic, and 3) uses more unstable features -- which I hadn't realized.

Why does the random part need to be deterministic? Don't we simply need it to be unique? If so, a static AtomicUsize would suffice, calling fetch_add every time a new macro name is needed.

[jebrosen]

Why does the random part need to be deterministic?

It's more of an inconvenience - it makes reproducible builds impossible, assuming a reproducible build is possible already.

If so, a static AtomicUsize would suffice, calling fetch_add every time a new macro name is needed.

I am really nervous about doing that because of incremental or parallel compilation: I'm afraid we would get name collisions because of a reused "_1" from a previous build and a new "_1" from a current build. If we can be guaranteed not to run into that kind of problem now (or in the future), I would be on board with this.

[SergioBenitez]

It's more of an inconvenience - it makes reproducible builds impossible, assuming a reproducible build is possible already.

I don't think this has any effect on reproducibility. Macro names should/would not be part of the compiled binary.

I am really nervous about doing that because of incremental or parallel compilation: I'm afraid we would get name collisions because of a reused "_1" from a previous build and a new "_1" from a current build. If we can be guaranteed not to run into that kind of problem now (or in the future), I would be on board with this.

I would agree, though I've inquired about this in the past and learned that a single proc macro is "guaranteed" not to be run in parallel with itself. That is, the static would work as desired. This is, of course, simply an ad-hoc conversation I had (with eddyb), though I do believe existing proc-macros depend on this behavior for correctness. Nevertheless, it isn't documented or specified, so I understand your concern. We could either ask for this to be specified or do something else, though I feel like trading off one bit of unstableness for another isn't gaining us much.

[jebrosen]

It sounds like our best options are to either get a guarantee that any time a given proc_macro or macro crate is run it is being re-run on the entire crate, or to use rand. I'm happy to pursue either, and I can forget the hashing approach for now. After seeing the discussions around line and column information I don't want to depend on that API unnecessarily.

[SergioBenitez]

Did you take a look at what causes the error messages to be reversed? I'm concerned that this will result in error messages being more confusing, due to ordering, for users.

I think we can do this using the file/line/column based approach for now. We'll need something that will definitely be stabilized eventually; perhaps that's this interface, or something entirely different (say, gensym, though that seems unlikely).

[SergioBenitez]

I'm going to merge this now, but I would like to know if we can get error messages back to the original ordering.

[SergioBenitez]

Merged in 2f458b5 and 3e4f845. This is an incredible step towards compiling on stable. Great find, @jebrosen!

[jebrosen]

Awesome. I will try to reproduce the error message reversal with a reduced test case. And I believe the credit for pointing out this was possible goes to @macpp!

[jebrosen]

I've found some kind of a pattern here. uri! invocations can produce at least 3 kinds of errors, all with ui test coverage: 1) "invalid-style", such as mixing named and unnamed parameters, 2) "bad-params", where the name and/or number of parameters are checked against what the route actually has, and 3) "bad-type", where you see errors like the trait `FromUriParam<Path, &str>` is not implemented for `i32`.

The errors always appear in this sequence: all of category 1, all of category 2 in reverse order, and finally all of category 3.

This makes some sense: category 1 errors are emitted in the invocation of uri! itself, category 2 are emitted by rocket_internal_uri! after being invoked by the generated macro_rules macro, and category 3 are emitted after all macro expansion is done.

Switching the macro generation to macro_rules! #generated_macro_name makes category 2 show up in source order again, so I think it's something specific to macro re-exporting. macro (decl_macro) vs macro_rules! doesn't make a difference.

[jebrosen]

Unfortunately this changes name resolution. If a route wants to accept a PathBuf, either the module that invokes uri! now needs to have PathBuf in scope or the route has to accept it by the fully-qualified name std::path::PathBuf. I assume this is because macro has path/item hygiene unlike macro_rules! which only has identifier hygiene.