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

Make proc-macros hygienic #27

Closed
wants to merge 4 commits into from
Closed

Make proc-macros hygienic #27

wants to merge 4 commits into from

Conversation

prokopyl
Copy link
Member

This is an issue that I stumbled upon when working on #21, which made me create and auto-implement new traits (URIDCacheMapping), I noticed that they currently rely on the appropriate traits/types to be imported into the current scope, hence "leaking" a bit everywhere to the end-user, who has to import items they don't use.

This is because the proc-macro currently generates things along the lines of:

impl FeatureCollection for #struct {}

…which fails if FeatureCollection is not into scope.

The usual solution would be to do something along the lines of:

impl lv2_core::FeatureCollection for #struct {}

However, the LV2 Core crate can be imported through either lv2 or lv2_core directly, and we can't easily know which one in advance.

Other proc-macro-heavy crates like serde tackles the multiple-name problem by assuming you simply use serde by default, and exposes an extra attribute to allow the user to change the imported path like this:

#[derive(Serialize)]
#[serde(crate = "my_serde::path")]
struct MyStruct { /* ... */ }

This works for them because people renaming the crate/importing it through another is a very rare occurence.

However in our case, we recommend using lv2 for prototyping (or if you really need the whole shebang), and using the lv2-xxx crates for more optimized builds. Therefore the expected number of users for each is around 50/50 ant there really isn't a sensible default.

My solution here is to use the proc-macro-crate crate, that checks in Cargo.toml which crate is used and uses the right one (or fallbacks to lv2_core), while also supporting renaming.

I'm not too keen on introducing an additional dependency (although it's only a build-time dependency), but it's the only robust solution I have found to work that allows to make the proc-macros hygienic. (I have tried introducing additional extern crate or use statement, but it always break some test or another). 😕

@prokopyl prokopyl added the 🐞 Bug Something isn't working label Oct 23, 2019
@prokopyl prokopyl self-assigned this Oct 23, 2019
@JOOpdenhoevel
Copy link
Contributor

Looks good so far. However, I've got some points:

When you write a test in the lv2_core crate that uses one of the procedural macros, neither lv2::core nor lv2_core exist. Instead, you have to use crate. Maybe that path retrieval function should account for that too.

Also, you've written that method twice: Once for core and once for urid. Maybe you should find a way to make that code reusable, since only the path names are different.

I also don't like that you have to pass that path to every function that uses it. It would be best if it could be stored in a constant or if you would refactor the functions to be methods of a struct that stores the path.

Copy link
Contributor

@JOOpdenhoevel JOOpdenhoevel left a comment

Choose a reason for hiding this comment

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

Formally requesting changes

@JOOpdenhoevel
Copy link
Contributor

I've tested the changes with an external crate and it looks like proc_macro_crate is broken: It never finds any crates, even when I'm using lv2_core directly, unrenamed. Therefore, this effort can not be continued with this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants