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

proc_macro_derive(attributes(path::to)) both does and doesn't work #55168

Closed
CAD97 opened this issue Oct 18, 2018 · 7 comments · Fixed by #58899
Closed

proc_macro_derive(attributes(path::to)) both does and doesn't work #55168

CAD97 opened this issue Oct 18, 2018 · 7 comments · Fixed by #58899
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another.

Comments

@CAD97
Copy link
Contributor

CAD97 commented Oct 18, 2018

// crate `derive`
// lib.proc-macro = true
#[proc_macro_derive(WellThen, attributes(well::then))]
pub fn derive(i: ::proc_macro::TokenStream) -> ::proc_macro::TokenStream {
    i
}

// crate `user`
use derive::WellThen;
#[derive(WellThen)]
#[well::then]
struct A;
error[E0433]: failed to resolve. Use of undeclared type or module `well`
  --> user\main.rs:3:3
   |
 3 | #[well::then]
   |   ^^^^ Use of undeclared type or module `well`

I would expect either for this to work or for #[proc_macro_derive(attributes(..))] to reject a path and only accept an ident, as specifying a path here doesn't work.

@petrochenkov
Copy link
Contributor

This may be an accidental regression from #50030, similarly to #53489.

This cannot work without significant work and introducing an entirely new entity into name resolution ("derive helper modules" or something), so #[proc_macro_derive(attributes(..))] needs to validate its arguments harder and reject non-identifier paths.

@Havvy Havvy added A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. labels Oct 18, 2018
@shepmaster
Copy link
Member

reject non-identifier paths

I don't think you can do this, as these work for attributes within the type on stable 1.32:

use repro_derive::Example;

#[derive(Example)]
#[example::attr]     // Does not work
struct Demo {
    #[example::attr] // Works
    field: i32,
}

fn main() {}
extern crate proc_macro;

use proc_macro::TokenStream;

#[proc_macro_derive(Example, attributes(example::attr))]
pub fn example_derive(_input: TokenStream) -> TokenStream {
    TokenStream::new()
}

@petrochenkov
Copy link
Contributor

Oh god.
If not reject, then issue a deprecation warning immediately.

Self-assigning, I should be able to fix it this weekend.

@petrochenkov petrochenkov self-assigned this Feb 27, 2019
@petrochenkov petrochenkov added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Feb 27, 2019
@shepmaster
Copy link
Member

Is there any suggested way to have my desired syntax of

#[derive(Example)]
#[example::attr]
struct Demo {
    #[example::attr]
    field: i32,
}

@petrochenkov
Copy link
Contributor

@shepmaster
Not right now, but something like #57921 may help.

Actually, the derive macro itself could be able to introduce "tool modules" into scope (like rustfmt in rustfmt::skip, but not global to the whole crate).
#[proc_macro_derive(Example, tools(example))] or something.
Right now this is almost trivial to add for "macro expanded" attribute targets, like the item itself, but some additional work is needed to support it for other targets, e.g. fields (as you can see #[rustfmt::skip] doesn't work on fields yet).

@Centril
Copy link
Contributor

Centril commented Feb 28, 2019

@petrochenkov Is there a reason we cannot let #[proc_macro_derive(Example, attributes(example::attr))] mean #[proc_macro_derive(Example, tools(example))] plus some validation for the ::attr part? handwave

From a user facing perspective at least, @shepmaster's #55168 (comment) example seems reasonable and "expected".

@petrochenkov
Copy link
Contributor

Fixed in #58899

Is there a reason we cannot let #[proc_macro_derive(Example, attributes(example::attr))] ...

I'd prefer to see crater results first.

@petrochenkov petrochenkov removed their assignment Mar 9, 2019
Centril added a commit to Centril/rust that referenced this issue Mar 10, 2019
Do not accidentally treat multi-segment meta-items as single-segment

Fixes rust-lang#55168 and many other regressions from rust-lang#50030

Basically, attributes like `#[any::prefix::foo]` were commonly interpreted as `#[foo]` due to `name()` successfully returning the last segment (this applies to nested things as well `#[attr(any::prefix::foo)]`).
Centril added a commit to Centril/rust that referenced this issue Mar 13, 2019
Do not accidentally treat multi-segment meta-items as single-segment

Fixes rust-lang#55168 and many other regressions from rust-lang#50030

Basically, attributes like `#[any::prefix::foo]` were commonly interpreted as `#[foo]` due to `name()` successfully returning the last segment (this applies to nested things as well `#[attr(any::prefix::foo)]`).
bors added a commit that referenced this issue Mar 16, 2019
Do not accidentally treat multi-segment meta-items as single-segment

Fixes #55168 and many other regressions from #50030

Basically, attributes like `#[any::prefix::foo]` were commonly interpreted as `#[foo]` due to `name()` successfully returning the last segment (this applies to nested things as well `#[attr(any::prefix::foo)]`).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-bug Category: This is a bug. regression-from-stable-to-stable Performance or correctness regression from one stable version to another.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants