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

Custom attibutes #1755

Closed
wants to merge 2 commits into from
Closed

Custom attibutes #1755

wants to merge 2 commits into from

Conversation

nrc
Copy link
Member

@nrc nrc commented Sep 23, 2016

This RFC specifies custom attributes, which can be used by both external tools
and by compiler plugins of various kinds. It also specifies the interaction of
custom attributes, built-in attributes, and attribute-like macros.

Rendered

This RFC specifies custom attributes, which can be used by both external tools
and by compiler plugins of various kinds. It also specifies the interaction of
custom attributes, built-in attributes, and attribute-like macros.
@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Sep 23, 2016
@steveklabnik
Copy link
Member

I understand why it's being done, but #[attribute] feels extremely redundant to me, given that #[] is already an attribute.

@alexcrichton
Copy link
Member

To clarify, does this means that all crates which would like to use #[rustfmt::skip], for example, have to have #![attribute(rustfmt)] at the top?

@nrc
Copy link
Member Author

nrc commented Sep 27, 2016

@alexcrichton yes

@alexcrichton
Copy link
Member

Thanks for the clarification! Do you know if it'd be possible to work around that restriction? It seems odd that attributes like #[rustfmt::skip] would require a crate annotation where attributes like #[serde(rename = "...")] wouldn't? (as the serde one is stripped during #[derive])

@nrc
Copy link
Member Author

nrc commented Sep 27, 2016

@alexcrichton in a way the serde one is declared when you import the Serde macro, so they are both declared somehow (although I agree they are somewhat different for the user). I don't think it is possible to work around - the key thing is that you need a list of attributes that the compiler knows are not macros, because otherwise you get inappropriate missing macro errors (or no missing macro errors).

@sfackler
Copy link
Member

We could add a pass that explicitly checks all attributes against #![attribute(..)]/built in macros to enforce some consistency here, right?

@nrc
Copy link
Member Author

nrc commented Sep 28, 2016

We could add a pass that explicitly checks all attributes against #![attribute(..)]/built in macros to enforce some consistency here, right?

Do you mean even those inside macro-attributed items before those macros are expanded?

@sfackler
Copy link
Member

That'd be the idea, yeah.

@bjorn3
Copy link
Member

bjorn3 commented Sep 28, 2016

Maybeuse something like

//crate rustfmt
#![export_attribute(skip)]

//crate example
extern crate rustfmt as abc_rustfmt;

#[abc_rustfmt::skip]
fn main(){
     // ...
}

This prevents you from having to define the attributes in every crate and allows the attributes to be renamed (in the future?).

@nrc
Copy link
Member Author

nrc commented Sep 28, 2016

Maybeuse something like ...

In general though, the tool supplying the attribute will not be linked as an upstream crate, in fact there will be no indication that the tool is used in the code that it is used upon. That is why we need some additional, explicit opt-in rather than relying on extern crates, etc.

@nrc
Copy link
Member Author

nrc commented Sep 28, 2016

That'd be the idea, yeah.

This seems a reasonable alternative, my reasoning is that we shouldn't require the boilerplate where it is not necessary and that macros should be able to do anything they want to their 'source' code. However, there is benefit in uniformity, so it is not a clear trade-off.


Currently, custom attributes (i.e., those not known to the compiler, e.g.,
`rustfmt_skip`) are unstable. There is a future compatibility hazard with custom
attributes: if we add `#[foo]` to the language, than any users using a `foo`
Copy link
Member

Choose a reason for hiding this comment

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

then*

@nrc nrc self-assigned this Sep 29, 2016
@glaebhoerl
Copy link
Contributor

With apologies for the not-at-all substantive comment, I think the RFC could benefit from some more examples for illustration. At the moment the wall of text is slightly intimidating. (Also the pull request and commit title contain a typo.)

@tanriol
Copy link

tanriol commented Oct 10, 2016

In general though, the tool supplying the attribute will not be linked as an upstream crate,in fact there will be no indication that the tool is used in the code that it is used upon. That is why we need some additional, explicit opt-in rather than relying on extern crates, etc.

As an idea, what about

#[attribute_namespace]
extern crate rustfmt;

which means "crate rustfmt is used for nop attributes only, declare the path without doing anything else"?

@Ericson2314
Copy link
Contributor

@nrc

In general though, the tool supplying the attribute will not be linked as an upstream crate, in fact there will be no indication that the tool is used in the code that it is used upon. That is why we need some additional, explicit opt-in rather than relying on extern crates, etc.

Can you elaborate a bit? I think this might be relevant to my concern rust-lang/rust#35900 (comment) from the 1.1 tracking issue.

@nrc
Copy link
Member Author

nrc commented Oct 26, 2016

A couple of alternatives from irc (apologies, I forget who I was discussing this with):

  • extern attribute foo; or extern tool foo; as an alternative to #![attribute(foo)] (c.f., this suggestion: Custom attibutes #1755 (comment))
  • tools could provide manifests describing attributes they expect. These are hosted on crates.io and handled by Cargo similarly to crates. The compiler checks attribute uses against these manifests. This gives much stricter checking, but requires much more far-reaching changes and more effort from tools.

It has also been brought up that macros 1.1 custom derives want to use attributes and for the compiler to accept them without trying to do a macro lookup (currently, macros must remove these when processing items, but that is undesirable). This requires some formalisation of the note at https://github.com/nrc/rfcs/blob/fe63acb87f97b8205cad8adb20effb6490d42503/text/0000-attributes.md#a-note-on-macro-attributes

Ideally, I think we'd like to use macro name as a prefix, e.g.,

#[derive(Foo)]
struct Bar {
    #[Foo::baz]
    field: T,
}

Here Foo::baz is permitted because Foo is a macro. However, in the general case the macro name may be looked up using imports and so we should accept different versions of the prefix (e.g., absolute path, and relative path using an import). This is complicated since macro names might be a prefix of each other, e.g., consider two macros foo::bar (a macro bar defined in foo) and foo::bar::baz ( macro baz defined in the module foo::bar). It's possible hygiene would complicate things further.

An alternative is to permit attributes prefixed with the crate name, e.g., if we have #[macro_use] extern crate foo; (which allows #[derive(Foo)]) then the compiler will allow #[foo::P] for any path P without looking for a macro so named. This clearly does not extend to macro 2.0 smoothly.

@liigo
Copy link
Contributor

liigo commented Oct 28, 2016

#![use_attr(rustfmt)] makes sense to me, it imports a macro namespace to current scope, similar to use std::io statement.

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

@steveklabnik wrote:

I understand why it's being done, but #[attribute] feels extremely redundant to me, given that #[] is already an attribute.

But #[attribute(...)] is an attribute about the attributes listed in the ... ?

So the attribute key seems natural to me. Or at least as natural as the feature key is in

#![feature(asm)]

Still, maybe this is a sign that @liigo has a point about whether the name of the key should change?

@pnkfelix
Copy link
Member

pnkfelix commented Jan 3, 2017

@nrc are you planning to incorporate the alternatives listed here #1755 (comment) into the RFC text in some way?

Any other next steps? From skimming the discussion it seems like there is a general request for more examples in the RFC text?

@aturon
Copy link
Member

aturon commented Apr 29, 2017

Ping @nrc, is this something we still want to make progress on in the near future, or should we punt it?

@nrc
Copy link
Member Author

nrc commented Aug 11, 2017

Closed in favour of #2103

@nrc nrc closed this Aug 11, 2017
@nrc nrc mentioned this pull request Aug 31, 2017
@petrochenkov petrochenkov added the T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. label Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-dev-tools Relevant to the development tools team, which will review and decide on the RFC. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.