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

Support registering tool attributes from command line and un-support Registry::register_attribute #57921

Closed
wants to merge 2 commits into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jan 26, 2019

New unstable command line option -Z attr-tool=tool_name allows to use tool attributes from that tool: #[tool_name::anything].

This is supposed to

  1. Move the tool attributes story further, with an eye on eventual stabilization.
  2. Replace Registry::register_attribute from the legacy plugin interface which isn't going to be stabilized, but which is still used by Servo. Servo will be able to add -Z attr-tool=servo to command line and use #[servo::must_root] instead of using #[must_root] registered via plugin.

r? @nrc

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2019
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 26, 2019

The primary alternative to a command line option is registering the tools through a crate-level attribute:

#![register_tools(servo, another_tool)]

#[servo::must_root]
#[another_tool::another_attr]
fn foo() {}

and perhaps attributes themselves as well, as a more direct replacement for Registry::register_attribute and also replacement for feature(custom_attribute):

#![register_attributes(must_root, another_attr)]

#[must_root]
#[another_attr]
fn foo() {}

I don't know what option is better and need feedback.
cc @SimonSapin @rust-lang/dev-tools

Note however, that crate-level attributes can be supplied through command line via -Z crate-attr=ATTR, so the attributes kind of subsume command line, but that option is currently unstable and intended for internal use.

@petrochenkov petrochenkov added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. labels Jan 26, 2019
@Manishearth
Copy link
Member

I prefer the crate level attribute form, this does not make sense within a -Z flag since this forces consumers of the crate to build it in a particular manner. If anything, I find -Z to be more unstable here.

@Centril
Copy link
Contributor

Centril commented Jan 26, 2019

So if I understand this right, whether you have -Z attr-tool=servo or #![register_tools(servo)], the semantics are achieved by having an outside and separate tool read the file and then do stuff (e.g. like clippy). No procedural macro or plugin is run inside the compiler to give the attributes semantics (e.g. like in servo).

In the case of servo, as I noted in the tracking issue, I'm unsure how that brings #[must_root] closer to stable when it afaics depends on accessing the HIR (which we will never expose in a stable fashion).

@Manishearth
Copy link
Member

It makes it easier to compile servo on stable, the must_root lint is an important compile-time check that is not necessary to produce a servo binary. Currently a blocker to making this work is that we have custom attributes peppered throughout the codebase.

@Centril
Copy link
Contributor

Centril commented Jan 26, 2019

That makes sense to me. It seems like an improvement over the status quo. However, it seems like we're possibly inviting more reliance on the internal structures of the compiler on nightly. That said, overall it seems a net benefit and I think we should do one of the two versions.

@petrochenkov petrochenkov changed the title Support registering tool attributes from command line an un-support Registry::register_attribute Support registering tool attributes from command line and un-support Registry::register_attribute Jan 26, 2019
@Manishearth
Copy link
Member

However, it seems like we're possibly inviting more reliance on the internal structures of the compiler on nightly

I'm not sure how this is the case, servo has been doing this for forever, and this doesn't really change anything.

Less nightly-ish use cases for this are basically where other tools may wish to consume this. The -Z option helps but having an attribute api is nice too.

@Centril
Copy link
Contributor

Centril commented Jan 26, 2019

I'm not sure how this is the case, servo has been doing this for forever, and this doesn't really change anything.

I was thinking it would invite more applications to do what servo does; but that's speculative and might not pan out... we'll see I guess; I support finding out. :)

@Manishearth
Copy link
Member

I was thinking it would invite more applications to do what servo does

well yeah, but the registry api has been around forever, and if you actually want to do what servo does you need the registry api anyway, so this isn't removing any barriers or anything

@SimonSapin
Copy link
Contributor

Servo will be able to add -Z attr-tool=servo to command line

I assume this is the rustc command line which Servo’s build system doesn’t drive directly, Cargo does. Increased reliance on RUSTFLAGS is problematic, because of rust-lang/cargo#5376.

@bors
Copy link
Contributor

bors commented Feb 7, 2019

☔ The latest upstream changes (presumably #58254) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster
Copy link
Member

In a related world, I'd like to be able to write

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

Discussing with @petrochenkov, it was suggested that something like this could be acceptable:

#[proc_macro_derive(Example, tools(example))]

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2019
@SimonSapin
Copy link
Contributor

Increased reliance on RUSTFLAGS is problematic

This sounded a bit euphemistic when I read it again. To be clear, I think that requiring a rustc command-line argument is not viable for Servo. A crate-level attribute would work though.

What is the motivation for removing Registry::register_attribute while #[plugin_registrar] and Registry still exist?

@petrochenkov
Copy link
Contributor Author

I'll close this for now and reopen later with implementation based on crate-level attributes.

@petrochenkov petrochenkov deleted the tool branch June 5, 2019 16:33
Centril added a commit to Centril/rust that referenced this pull request Jun 8, 2019
Minimize use of `#![feature(custom_attribute)]`

Some preparations before resurrecting rust-lang#57921.
bors added a commit that referenced this pull request Nov 10, 2019
Support registering inert attributes and attribute tools using crate-level attributes

And remove `#[feature(custom_attribute)]`.
(`rustc_plugin::Registry::register_attribute` is not removed yet, I'll do it in a follow up PR.)

```rust
#![register_attr(my_attr)]
#![register_tool(my_tool)]

#[my_attr] // OK
#[my_tool::anything] // OK
fn main() {}
```

---
Some tools (`rustfmt` and `clippy`) used in tool attributes are hardcoded in the compiler.
We need some way to introduce them without hardcoding as well.

This PR introduces a way to do it with a crate level attribute.
The previous attempt to introduce them through command line (#57921) met some resistance.

This probably needs to go through an RFC before stabilization.
However, I'd prefer to land *this* PR without an RFC to able to remove `#[feature(custom_attribute)]` and `Registry::register_attribute` while also providing a replacement.

---
`register_attr` is a direct replacement for `#![feature(custom_attribute)]` (#29642), except it doesn't rely on implicit fallback from unresolved attributes to custom attributes (which was always hacky and is the primary reason for the removal of `custom_attribute`) and requires registering the attribute explicitly.
It's not clear whether it should go through stabilization or not.
It's quite possible that all the uses should migrate to `#![register_tool]` (#66079) instead.

---

Details:
- The naming is `register_attr`/`register_tool` rather than some `register_attributes` (plural, no abbreviation) for consistency with already existing attributes like `cfg_attr`, or `feature`, etc.
---
Previous attempt: #57921
cc #44690
Tracking issues: #66079 (`register_tool`), #66080 (`register_attr`)
Closes #29642
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants