rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
95.04k stars 12.26k forks source link

Attribute macro helper attributes #65823

Open dhardy opened 4 years ago

dhardy commented 4 years ago

The reference states that custom derive macros support defined helper attributes:

#[proc_macro_derive(HelperAttr, attributes(helper))]
pub fn derive_helper_attr(_item: TokenStream) -> TokenStream {
    TokenStream::new()
}

However, the same does not appear to apply to proc_macro_attribute. Please extend it to do so.

#[proc_macro_attribute(attributes(helper))]
pub fn my_attribute(attr: TokenStream, item: TokenStream) -> TokenStream { .. }

Motivation is limited, but this could provide simpler specification here.

Related: #53012

Centril commented 4 years ago

cc @petrochenkov

petrochenkov commented 4 years ago

This is not technically necessary because proc macro attributes can remove these attributes from their input unlike derives, but should be supportable pretty easily as a convenience feature once https://github.com/rust-lang/rust/pull/64694 lands.

Centril commented 4 years ago

Good point; I do think it would be useful for the language to do this automatically as an ergonomics thing from the POV of a macro author myself. Nominating based on @petrochenkov's comment to see what the preliminary interest is in the language team. We may or may not ask for an RFC based on the discussion.

nikomatsakis commented 4 years ago

Can we clarify what the expected behavior is here?

If you include attributes(helper), does it just strip out that attribute from wherever it appears in the final output?

I'm generally positive on the idea but would like to see a more complete write-up, at minimum. I mildly prefer a small RFC but I think a PR that links to a good write-up would probably suffice.

(Presumably we would then create a tracking issue.)

calebzulawski commented 4 years ago

This is not technically necessary because proc macro attributes can remove these attributes from their input unlike derives, but should be supportable pretty easily as a convenience feature once #64694 lands.

Is this always true? I just tried this with a proc macro attribute on a function, and the compiler attempted to expand the "helper" attribute and failed before the macro got a chance to strip it. In my scenario the helper attribute was placed on a statement in the function, if that makes any difference.

petrochenkov commented 4 years ago

@calebzulawski Could you give a minimized reproduction? This shouldn't happen in theory.

calebzulawski commented 4 years ago

I just tried a minimal example and it worked as you describe, I imagine it's likely I wasn't properly stripping out the helper in my larger example. So is this safe behavior to rely on right now? Similar questions were asked in rust-lang/reference#578 and rust-lang/reference#692 but it doesn't look like there's a clear answer. Perhaps helper attribute syntax would be nice but I think documenting the behavior might go a long way.

extrawurst commented 4 years ago

Having gone through and stumbled over this myself just recently (I actually thought adding a remark to the docs would make sense: https://github.com/rust-lang/reference/pull/716) I must say I really think this needs no compiler support. Here are just a few simple lines (using the syn crate) that completely obliterate all attributes from the AST given to it: https://github.com/Extrawurst/alpaca-rust/commit/46dbba66cf73f6d05f7fdda2d6e1b9d6e30384c2#diff-8f097454b072a95a96c470f9c3f84c5fR136-R151 but that could easily be extended to just the attributes that you want to remove.

macpp commented 4 years ago

Motivation is limited

This will make things easier when multiple attribute macros use the same helper attributes. With proc_macro_derive you can write

#[derive(Serialize,Deserialize)]
struct Foo {
   #[serde(rename = "bar")]
   x: i32
}

And none of the derives has to worry that serde attribute will be stolen by some other macro. In attribute macros this is probably trickier - you could for example check other attributes that are applied to struct, and skip cleaning helper attributes if you know that this macro will use them. The downside is that this can break easily if user renames attribute or imports it from unrelated liblary.

saskenuba commented 3 years ago

Any updates on this?

liubog2008 commented 3 years ago

I find stabilized rfc2565 which seems related to this issue.

But I find no way to define #[path_param] and #[query_param] in rfc

 fn get_person(
        &self,
        #[path_param = "name"] name: String, // <-- formal function parameter.
        #[query_param = "limit"] limit: Option<u32>, // <-- here too.
    ) {
        ...
    }
liubog2008 commented 3 years ago

@petrochenkov

ModProg commented 2 years ago

Having gone through and stumbled over this myself just recently (I actually thought adding a remark to the docs would make sense: rust-lang/reference#716) I must say I really think this needs no compiler support. Here are just a few simple lines (using the syn crate) that completely obliterate all attributes from the AST given to it: extrawurst/alpaca-rust@46dbba6#diff-8f097454b072a95a96c470f9c3f84c5fR136-R151 but that could easily be extended to just the attributes that you want to remove.

While technically true, this would mean, you can only have one proc_macro call, because you have no way of telling, if any of the other procmacro calls are the same macro as aliasing is a thing.

dhardy commented 2 years ago

Since 1.52 and #79078 I think this is no longer an issue: the primary attribute can strip out all helper attributes, with the sole restriction that helper attributes cannot appear before the primary attribute (this restriction also applies to #[derive] now).

This goes back to @nikomatsakis's comment:

If you include attributes(helper), does it just strip out that attribute from wherever it appears in the final output?

This potential feature would now be the sole point of "helper attributes", but in my opinion it's just not necessary (to be useful, custom parsing code must match them anyway, so it might as well remove them from the output too).

daxpedda commented 2 years ago

The issue arises when you need support for multiple calls to a proc_macro_attribute.

Imagine a proc_macro_attribute called macro_x with an attribute attr_y:

#[macro_x(some_options_here)]
#[macro_x(some_other_options_here)]
struct Test {
   a: u8,
   #[attr_y]
   b: String,
}

You can't just start stripping the field attributes, because the second call to macro_x might require them. A solution would be to parse all calls to macro_x the first time macro_x is called, this is problematic because we might be unable to determine macro_xs path:

#[macro_x(...)]
#[macro_x_crate::macro_x(...)]
#[re_export::macro_x(...)]
#[import_as_macro_x(...)]

This ofc can be solved by just switching to proc_macro_derive because it does support helper attributes:

#[derive(macro_x)]
#[macro_x(some_options_here)]
#[macro_x(some_other_options_here)]
struct Test {
   a: u8,
   #[attr_y]
   b: String,
}

Now both problems are fixed, helper attributes don't need to be stripped and #[macro_x(...)] isn't a path anymore but has to be written a certain way to be valid.

This is just an example that can work with a derive, others might not, in any case, it would be appreciated if you could re-open the issue @dhardy.

dhardy commented 2 years ago

Fair argument, though personally I don't agree.

Your example uses derive(macro_x) to do the parsing and macro_x as a parameter.

But you can do the same with any type of proc macro preceeding the repeated attribute, e.g.

#[my_proc_attribute_macro]
#[my_extra_attr(foo)]
#[my_extra_attr(bar)]
struct S;

The only difference from proc_macro_derive is that your macro is responsible for removing the attribute from output.

ModProg commented 2 years ago

Fair argument, though personally I don't agree.

Your example uses derive(macro_x) to do the parsing and macro_x as a parameter.

But you can do the same with any type of proc macro preceeding the repeated attribute, e.g.

#[my_proc_attribute_macro]
#[my_extra_attr(foo)]
#[my_extra_attr(bar)]
struct S;

The only difference from proc_macro_derive is that your macro is responsible for removing the attribute from output.

The idea was to have multiple calls to a proc macro being able to read the same attributes without having to delete them.

petrochenkov commented 2 years ago

The idea was to have multiple calls to a proc macro being able to read the same attributes without having to delete them.

That's a pretty good motivation, IMO.

daxpedda commented 2 years ago

But you can do the same with any type of proc macro preceeding the repeated attribute, e.g.

#[my_proc_attribute_macro]
#[my_extra_attr(foo)]
#[my_extra_attr(bar)]
struct S;

@dhardy you are right that this is exactly the same as the derive example I gave above. The issue here is that it's necessary.

The idea was to have multiple calls to a proc macro being able to read the same attributes without having to delete them.

Basically what I would like is to have the ability to do this:

#[my_proc_attribute_macro(foo)]
#[my_proc_attribute_macro(bar)]
struct S;

Without a necessary line on top. I hope my explanation helps.

dhardy commented 2 years ago

@daxpedda actually, what you mean is the ability to have multiple uses of a single attribute which can observe other uses of that same attribute, and without the first instance doing the work of implementing all instances (which is also possible).

Note that there was also an afore-mentioned point about paths and renaming of the macro import, which is still a potential issue here.

daxpedda commented 2 years ago

The goal is

... to have multiple uses of a single attribute which can observe other uses of that same attribute ...

#[my_proc_attribute_macro(foo)]
#[my_proc_attribute_macro(bar)]
struct S {
  #[helper(option)]
  a: u8,
}

The way to implement this, is to have

... the first instance doing the work of implementing all instances (which is also possible).

The reason this isn't possible, is because of

... paths and renaming of the macro import ...

The solution to that problem is to replace multiple uses of a proc_macro_attribute with a single use of a proc_macro_attribute. The only need to have multiple uses of a proc_macro_attribute is because it would contain different options. So to implement this solution, all options have to be moved to a helper attribute:

#[my_proc_attribute_macro]
#[my_proc_attribute_macro_helper(foo)]
#[my_proc_attribute_macro_helper(bar)]
struct S {
  #[helper(option)]
  a: u8,
}

This avoids any pathing issues, because attribute helpers don't use paths. This still requires cleaning all helper attributes and is, from the point of view of syntactic effort, equivalent to:

#[derive(my_proc_attribute_macro)]
#[my_proc_attribute_macro_helper(foo)]
#[my_proc_attribute_macro_helper(bar)]
struct S {
  #[helper(option)]
  a: u8,
}

With the additional benefit of not requiring to clean up helper attributes.

Of course this whole argument only makes sense when talking about using proc_macro_attribute on structs, enums or unions, derive isn't usable anywhere else, but proc_macro_attribute is.

Not trying to argue against myself here. I would still like to see support for attribute helpers in proc_macro_attribute, as it would not require this "workaround". Just want to make clear what the issues are and point out all workarounds so the Rust team can make the best decisions ❤️.

Mulling commented 1 month ago

What's stopping this from going forward? I think @daxpedda outlined it very well, it's useful in several situations and not that hard to implement.

Tangentially, we shouldn't offload the problem upwards, i.e., expect syn to solve everything. The ergonomics of macro development are important.