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

Attribute macro helper attributes #65823

Open
dhardy opened this issue Oct 25, 2019 · 22 comments
Open

Attribute macro helper attributes #65823

dhardy opened this issue Oct 25, 2019 · 22 comments
Labels
A-attributes Area: #[attributes(..)] A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@dhardy
Copy link
Contributor

dhardy commented Oct 25, 2019

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

@jonas-schievink jonas-schievink added A-attributes Area: #[attributes(..)] A-macros Area: All kinds of macros (custom derive, macro_rules!, proc macros, ..) C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 25, 2019
@Centril
Copy link
Contributor

Centril commented Oct 26, 2019

cc @petrochenkov

@petrochenkov
Copy link
Contributor

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.

@Centril
Copy link
Contributor

Centril commented Oct 26, 2019

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
Copy link
Contributor

nikomatsakis commented Oct 31, 2019

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
Copy link
Member

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
Copy link
Contributor

petrochenkov commented Nov 3, 2019

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

@calebzulawski
Copy link
Member

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
Copy link

extrawurst commented Nov 13, 2019

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.

@macpp
Copy link

macpp commented Nov 17, 2019

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
Copy link

Any updates on this?

@liubog2008
Copy link

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
Copy link

@petrochenkov

@ModProg
Copy link
Contributor

ModProg commented Nov 15, 2021

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
Copy link
Contributor Author

dhardy commented Nov 15, 2021

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).

@dhardy dhardy closed this as completed Nov 15, 2021
@daxpedda
Copy link
Contributor

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
Copy link
Contributor Author

dhardy commented Nov 17, 2021

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.

@dhardy dhardy reopened this Nov 17, 2021
@ModProg
Copy link
Contributor

ModProg commented Nov 18, 2021

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
Copy link
Contributor

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
Copy link
Contributor

daxpedda commented Nov 18, 2021

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
Copy link
Contributor Author

dhardy commented Nov 18, 2021

@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
Copy link
Contributor

daxpedda commented Nov 19, 2021

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
Copy link

Mulling commented May 27, 2024

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.

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-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests