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

Tracking issue for custom_attribute features #29642

Closed
aturon opened this issue Nov 5, 2015 · 22 comments · Fixed by #66070
Closed

Tracking issue for custom_attribute features #29642

aturon opened this issue Nov 5, 2015 · 22 comments · Fixed by #66070
Assignees
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Nov 5, 2015

Part of RFC 572. This issue tracks stabilization/deprecation.

@aturon aturon added T-lang Relevant to the language team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Nov 5, 2015
@aturon aturon changed the title Tracking issue for custom_attribute feature Tracking issue for custom_attribute, rustc_attrs features Nov 5, 2015
@brson
Copy link
Contributor

brson commented Jan 11, 2017

Is this feature ever going to be stabilized?

It is currently used by Rocket: rwf2/Rocket#19 (comment). Does Rocket have alternatives?

@steveklabnik
Copy link
Member

Isn't this part of macros 2.0? that is #[proc_macro_attribute] https://github.com/rust-lang/rfcs/blob/master/text/1566-proc-macros.md

@beamspease
Copy link

Just found this ticket as serde's main documentation says to use it...
https://serde.rs/enum-representations.html

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@thijsc
Copy link

thijsc commented May 17, 2018

It seems like serde relies on this feature a lot, see: https://serde.rs/attributes.html

Is the plan to stabilize this?

@Centril Centril changed the title Tracking issue for custom_attribute, rustc_attrs features Tracking issue for custom_attribute features Jan 17, 2019
@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 17, 2019
@scottmcm
Copy link
Member

If this is no longer for rustc_attrs, I think there should be a PR to the gate list and unstable book to stop pointing here, and say why.

@nikomatsakis nikomatsakis removed the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 17, 2019
@nikomatsakis
Copy link
Contributor

Discussed in the @rust-lang/lang meeting:

We believe we should remove the custom_attribute feature gate from the compiler. If I'm not mistaken, it allows the older behavior where you could use any attribute #[foo] anywhere. We don't — afaik — intend to go back to that world. Moreover, removing this feature would probably let us give more definitive and clear error messages that guide people towards using procedural macros.

I think this is the main chunk of code that would change:

} else if !features.custom_attribute {
let msg = format!("The attribute `{}` is currently unknown to the \
compiler and may have meaning added to it in the \
future", path);
feature_err(&self.session.parse_sess, "custom_attribute", path.span,
GateIssue::Language, &msg).emit();
}

Nominating for @rust-lang/compiler mainly to see if anybody wants to follow-up or mentor this.

@nikomatsakis
Copy link
Contributor

There are some applicable mentoring instructions here:

https://forge.rust-lang.org/stabilization-guide.html

However, these instructions are oriented towards stabilizing a feature. We want to remove the feature.

The main question mark for me is what sort of error we want to give. @cramertj had some ideas here.

Also: cc @dtolnay in case you can think of any problems with removing this. =)

@SimonSapin
Copy link
Contributor

This does not include rustc_plugin::registry::Registry::register_attribute, right? (Servo still uses that, for a custom lint.)

@petrochenkov
Copy link
Contributor

The way custom_attribute is implemented now (fallback from unresolved attributes) is certainly not appropriate for stabilization and even for keeping in unstable form.

If you search the rust codebase for custom_attribute though you'll see a lot of uses, most of them in the test suite.
Since it's useful for internal testing and similar things, I had an alternative idea in mind:

  • Support explicitly registering custom inert attributes with a crate-level attribute #![custom_attributes(attr1, attr2, ...)], still under the custom_attribute feature.
  • Remove fallback-based custom attributes in its favor.
  • Remove rustc_plugin::registry::Registry::register_attribute in its favor.
  • Never stabilize it (possibly rename to rustc_custom_attributes), or, alternatively, stabilize it if it's still necessary in a couple of years or so.

The main question mark for me is what sort of error we want to give

A usual error about unresolved name?

(I'll happily do the necessary work, it's already in my work queue.)

@petrochenkov petrochenkov self-assigned this Jan 17, 2019
@Centril
Copy link
Contributor

Centril commented Jan 17, 2019

@petrochenkov

  • Support explicitly registering custom inert attributes with a crate-level attribute #![custom_attributes(attr1, attr2, ...)], still under the custom_attribute feature.

Interesting; could you elaborate a bit on the semantics of this?

@petrochenkov
Copy link
Contributor

@Centril
Same semantics as Registry::register_attribute - you get a name in prelude that refers to an inert attribute.

@dtolnay
Copy link
Member

dtolnay commented Jan 17, 2019

Thanks for the ping @nikomatsakis. I don't know of any code using custom_attribute and I don't have any reason to keep it.

@nikomatsakis
Copy link
Contributor

@petrochenkov

That sounds like a reasonable step forward. I did do a quick ripgrep through the rust sources and I was surprised by how many uses there are -- did you look more deeply at how e.g. libsyntax_pos uses this attribute and so forth?

Regarding the rustc internal tests, I suspect we could also get by by adding a few fixed attributes (e.g., rustc_dummy) that are gated on rustc_attrs.

But that wouldn't accommodate the register_attribute use case. OTOH, I think the way we want that to work is for the lint API to somehow declare the attributes that it may use, right? (Similar to custom derive)

@Centril
Copy link
Contributor

Centril commented Jan 22, 2019

@nikomatsakis

But that wouldn't accommodate the register_attribute use case. OTOH, I think the way we want that to work is for the lint API to somehow declare the attributes that it may use, right? (Similar to custom derive)

Is this "lint API" for user defined lints? Can you elaborate on that (examples would be nice...)?

@nikomatsakis
Copy link
Contributor

It is. It is also nowhere near stable, of course. I don't really remember the details -- basically there is some hook and you can invoke some methods to inject lints that the compiler will later run. Here is the example that @SimonSapin gave:

https://github.com/servo/servo/blob/866fd55ded6ded98b49862a29951d746a7846f64/components/script_plugins/lib.rs#L38-L46

(I presume that Clippy doesn't use any custom attributes? cc @oli-obk @Manishearth)

@Manishearth
Copy link
Member

Clippy does, but it declares them beforehand, and they're all scoped under clippy::. I think.

@Manishearth
Copy link
Member

Oh, it doesn't, my bad. But they do have a use case there.

Either way, we could scope them.

@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 22, 2019

Clippy can use "tool attributes" clippy::foo right now, because clippy and rustfmt are hard-coded in rustc in accordance with the tool attributes RFC.

In the future such attributes are supposed to be registered through command line like --tool clippy (again, in accordance with the tool attributes RFC).
Non-clippy lints could use the same mechanism, like --tool servo + #[servo::must_root].

@Centril
Copy link
Contributor

Centril commented Jan 22, 2019

@nikomatsakis

https://github.com/servo/servo/blob/866fd55ded6ded98b49862a29951d746a7846f64/components/script_plugins/lib.rs#L38-L46

Hmm... even if we had an API for defining lints, would that actually be of use since this depends on the HIR (and the type system... from what I can see, or at least it needs to operate on more than syntax?) ?

(also I was hoping for a more "here is what I think using the API would look like for an end user on stable..." =P )

@petrochenkov
Copy link
Contributor

I submitted #57921 to move this story further.

@pnkfelix
Copy link
Member

If this is no longer for rustc_attrs, I think there should be a PR to the gate list and unstable book to stop pointing here, and say why.

We probably should address the rustc_attrs question separately from how we resolve the custom_attributes matter in general.

(Unless our plan is to somehow use the same new machinery for both cases...?)

@petrochenkov
Copy link
Contributor

Second attempt: #66070.

SimonSapin added a commit to servo/servo that referenced this issue Nov 8, 2019
It errors in today’s Nightly:

```rust
error[E0557]: feature has been removed
 --> components/script/lib.rs:9:12
  |
9 | #![feature(on_unimplemented)]
  |            ^^^^^^^^^^^^^^^^ feature has been removed

error[E0658]: this is an internal attribute that will never be stable
  --> components/script/dom/bindings/conversions.rs:77:1
   |
77 | #[rustc_on_unimplemented(message = "The IDL interface `{Self}` is not derived from `{T}`.")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: for more information, see rust-lang/rust#29642
   = help: add `#![feature(rustc_attrs)]` to the crate attributes to enable

error: aborting due to 2 previous errors
```
bors-servo pushed a commit to servo/servo that referenced this issue Nov 9, 2019
Remove use of on_unimplemented

It errors in today’s Nightly:

```rust
error[E0557]: feature has been removed
 --> components/script/lib.rs:9:12
  |
9 | #![feature(on_unimplemented)]
  |            ^^^^^^^^^^^^^^^^ feature has been removed

error[E0658]: this is an internal attribute that will never be stable
  --> components/script/dom/bindings/conversions.rs:77:1
   |
77 | #[rustc_on_unimplemented(message = "The IDL interface `{Self}` is not derived from `{T}`.")]
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: for more information, see rust-lang/rust#29642
   = help: add `#![feature(rustc_attrs)]` to the crate attributes to enable

error: aborting due to 2 previous errors
```
@bors bors closed this as completed in 3fc30d8 Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet