Rust: Tracking issue: RFC 2103 - attributes for tools

[nrc]

Support scoped attributes for white-listed tools, e.g., #[rustfmt::skip]

RFC
discussion thread

• [x] implement for attributes (mentoring instructions here)
• [ ] implement for lints (https://github.com/rust-lang/rust/pull/52018; see https://github.com/rust-lang/rust/issues/44690#issuecomment-405749334)
• [ ] Adjust documentation (see instructions on forge)
• [ ] Stabilization PR (see instructions on forge)

Current status: stabilised for attributes, some work still to do on lints.

[repnop]

As mentioned on the Gitter chat, I'm willing to take a try at this! Should just need pointed in the right direction :)

[nikomatsakis]

@petrochenkov I suspect you'd be the best one to write up some mentoring instructions here. Any chance you can put some thought into it?

[repnop]

I'll definitely be getting myself familiar with the parser and AST in the mean time, so no big rush!

[nikomatsakis]

OK, since @petrochenkov has been busy, let me take a shot at some notes. Perhaps he can correct me if he thinks things could be done a better way.

To start, the structure that represents attributes in the AST is called Attribute. From what I can tell, the parser already parses attributes and permits them to contain general paths.

Basically, the model is this: within the AST, an attribute is just a token tree. However, sometimes, the compiler tries to parse and impose structure on this tree by parsing it as something like #[derive(Foo)]. This parsed form is represented as a MetaItem struct.

This parsing that converts an Attribute into a MetaItem is when you wind up with an error due to something like #[foo::bar(...)]. In particular, such a path is flagged as an error by the helper parse_meta. One part of this RFC then is just to tweak this check to permit some cases of non-identifier paths.

However, we must also decide how to expand MetaItem -- right now, it's name field has type Name -- i.e., a single name -- but we now want to represent compound names like a::b. The hackiest thing to do would be to intern the combined string as a big string. A less hacky thing would probably be to change Name into Vec<Name> -- or perhaps an enum that chooses between a single name and multiple names. I'm not sure which is the best way forward there.

[nikomatsakis]

@rep-nop hey, I'm just checking in -- are you still hacking on this? How's it going?

[repnop]

@nikomatsakis sorry, school has been keeping me fairly busy lately but things look like they're starting to lighten up for now! I spent ~2ish hours about 2 weeks ago on it and was getting somewhere I think, but I'm going to be hopefully trying a new approach that I hope will work better within the week (if not today even!)

[nikomatsakis]

@rep-nop great =) Let me know if I can be of any assistance.

[repnop]

Okay so while doing a more thorough read through the RFC I came up with a quick requirements document to track progress on it (below, it wont let me attach it for some reason? Let me know if I missed anything major or important!). The only part of the RFC I don't think I understand very well is the "Activation and unused attributes/lints" section. It talks about rustdoc, for example, not being "activated" until needed, what exactly does that imply?

```
RFC 2103 - Attributes for Tools Requirements

┌───────────┐
│ Long-term │
└───────────┘
[x] Some unspecified opt-in mechanism
│
├> #[cfg_tool(mytool)] ?
│
└> #[allow_tool(mytool)] ?

┌──────────────────┐
│ RFC Requirements │
└──────────────────┘
[x] Allow any tool that ships with Rust to be included
│ by default
│
└> { rustc, rustdoc, rls } (should I add clippy and rustfmt? the RFC talks about adding them "soon")

[x] Attributes must be long-lived in compilation

[x] Compiler doesn't warn on unused/unknown attributes,
│ however tools must
│
│ Allowed by compiler
├> #[rustfmt::foo]
│ Tool produced warning/error
└> ex. "rustfmt: warn: foo is an unknown attribute"

[repnop]

@nikomatsakis @nrc any input? :)

[nikomatsakis]

@rep-nop seems reasonable. =)

Activation and unused attributes/lints

I think what this means it that we won't accept rustdoc:foo attributes until rustdoc starts having a use for them.

[repnop]

@nikomatsakis so is that something that we're going to have to keep track of or..? Also, any idea where the whitelist will be located inside of the compiler? Currently, I just have it inside of the function as an array and the function checks against just the tool name (so should I make an attribute list for each one of them also?) and error if its not one of the whitelisted tools. The RFC talks about adding tools to that list from inside the source, however it looks like it wasn't decided on what that'd look like. @petrochenkov any suggestions/tips for a proper implementation?

[petrochenkov]

@rep-nop
I don't know how the proper implementation should look exactly.
Name resolution for attributes is kinda mess because it works with all kinds of attributes - built in, custom, derives, legacy derives, procedural macros, legacy procedural macros. I never looked at that code carefully.
Someone needs to figure out how it works exactly and how attributes for tools fit into it.
(I can figure it out myself, but then it would be much faster and easier to proceed and implement this myself as well, mentoring is harder and more time consuming.)

[nrc]

so is that something that we're going to have to keep track of or..?

Yes. There are essentially two lists - one of lints and one of attributes. The compiler only cares about the active tools (initially both lists will be empty). So if the active attributes whitelist is rustfmt, clippy and the active lints whitelist is clippy, then a rustfmt:: lint would still be an error (as would an rls:: attribute).

should I add clippy and rustfmt? the RFC talks about adding them "soon"

Yes, they are both on their way.

The requirements looks good.

Also, any idea where the whitelist will be located inside of the compiler

Good question! The lists could be provided by the build system, probably as an env var (or two). It should live somewhere that deals with tools. They could be hard-wired into the compiler, but I'm not really sure where I would put it. I don't think we need to worry about adding tools to the whitelist, we can do that manually.

In terms of implementation, I think the checking of the whitelist names in attributes should happen either during macro name resolution or just afterwards. The key to when to do the check is about the behaviour we want with precedence (tool vs macro). If you have questions, @jseyfried is probably the best person to ask.

[repnop]

Thanks for the answers! :) @nrc right now I have it checking the names at name resolution when it detects a path inside the attribute, which wasn't too difficult to get a quick and crude implementation done (which I believe is linked above in a commit I did). I'll look into using an env var to specify tool names!

[topecongiro]

@rep-nop Do you have any update? This is (potentially) blocking rustfmt from reaching 1.0, so I would like to push this forward. If you do not have enough time to work on this, I am willing to take a shot at this.

[repnop]

@topecongiro ah if this is going to be blocking rustfmt I'll release it as I'm going to be fairly busy this upcoming semester. I was unfortunately not able to progress as much as I'd liked to have for various reasons, so have at it! :)

[flip1995]

I would like to continue working on this, except @topecongiro wants to pick this up again?

Since #47773 already implemented the tool_attributes part, the next step would be doing the same for tool_lints?

[topecongiro]

@flip1995 I do not think that I can update the initial PR any time soon. So it will be great if you are going to work on this!

[flip1995]

I'm currently looking into tool_lints and I'm wondering if #![deny(clippy)] should still trigger the unknown lint warning. The RFC specifies that external attributes always have to be scoped. Should this also apply to external lints? In a more general way: should we allow #![deny(LINT_TOOL)] or should we require from the LINT_TOOL to implement something like #![deny(LINT_TOOL::all)]?

[xd009642]

So just wondering if there's a process currently for tool developers to request to be added to the whitelist? I understand there's a potential issue if a tool gets abandoned by it's owner. So if the whitelist is frozen for now have their been discussions on a crate level macro to define a tool name?

[nrc]

Basically I think the tools which are supported here are those that are distributed by Rustup using the Rust infra. In the future, I hope there will be a generic mechanism for this where tools do not have to be 'blessed'.

[xd009642]

That's fair enough, I was expecting that to be the case. I'll just stick with cfg_attr for now then.

[nrc]

I propose we stabilise the attributes (not lints) part of this. We need to adjust the docs, but we can do that as part of the stabilisation PR.

This is a fairly quick stabilisation, but it is a small feature and is blocking stable versions of Rustfmt and Clippy.

@rfcbot fcp merge

[rfcbot]

Team member @nrc has proposed to merge this. The next step is review by the rest of the tagged teams:

• [x] @Manishearth
• [x] @fitzgen
• [x] @japaric
• [x] @killercup
• [x] @nrc
• [x] @steveklabnik
• [ ] @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Manishearth]

AIUI the lint stuff isn't yet implemented, but I don't see that blocking stabilization, we can add that part later.

@rfcbot reviewed

[petrochenkov]

The feature is not implemented properly.
We'll have to make breaking changes if it's stabilized now.

I'm talking about any interactions with other kinds of attributes/macros, like

#![feature(tool_attributes, decl_macro)]

mod rustfmt {
    pub macro zzz() {}
}

#[rustfmt::zzz] // Should be: ERROR, expected attribute macro, found fn-like macro
fn f() {}

fn main() {}

[flip1995]

Also #51277 needs to be fixed, before stabilization.

[petrochenkov]

@flip1995
Did you mean https://github.com/rust-lang/rust/issues/51277? (That's the same problem as https://github.com/rust-lang/rust/issues/44690#issuecomment-398307401 basically.)

I'll try to fix resolution for multi-segment attributes (at least they are legacy-free) in the next 6 weeks once use_extern_macros is stable.

[flip1995]

Oh yeah, sorry for the typo!

[Manishearth]

@flip1995 Any progress on the lint side implementation of this? 😄

[flip1995]

I have some semi working code lying around somewhere. I'll try to make a PR out of it by the end of the week.
I'm still not sure how to handle this https://github.com/rust-lang/rust/issues/44690#issuecomment-392800598

[Manishearth]

_personally_, I'd like it to work as:

• #![deny(clippy::lintname)] for lint names
• #[deny(clippy::lintgroup)] for groups like pedantic
• #![deny(clippy::all)] doesn't have to exist but if it did it could either turn on every lint in clippy, _or_ be like #![deny(warnings)] and turn already-on lints to deny. We may add this idk.
• #![deny(clippy)] currently doesn't exist and doesn't need to exist, but if it did it would probably be an alias for clippy::all

Ideally we'd have an initial phase where when using clippy both namespaced and non-namespaced lint/group names work, and we start linting to recommend they switch.

[flip1995]

I mostly agree with you and would also suggest to recommend a division into lintname, lintgroup and all in the documentation of tool_lints.

#![deny(clippy)] currently exists though. It's in the Clippy Readme and behaves like #![deny(warnings)] for Clippy lints. Following code gives an error, when running cargo clippy on it:

#![allow(unknown_lints, unused_assignments)]
#![deny(clippy)]

fn main() {
    let mut a = 0;
    a = a + 1; // error: assign_op_pattern lint (warn-by-default)
    a += 1;    // no-error: assign_ops lint (allow-by-default)
}

Playground

I would suggest to stay consistent with the tool_attributes and only allow scoped tool_lints (and lintgroups/all) and not just #![deny(LINT_TOOL)] (maybe we could extend the unknown_lints lint in the future to suggest to use the all variant for known lint tools). Relevant RFC Paragraph.

This would mean to introduce #![deny(clippy::all)] and remove #![deny(clippy)] from the Clippy documentation, but this is on the Clippy side of things anyway.

[Manishearth]

Oh, I was under the impression deny(clippy) was no longer a thing. My bad.

It seems to be more of a "deny all by-default Warn lints" rather than "error on all warnings" which is what deny(warnings) does -- if you have a default-warn lint that you mark as Allow deny(warnings) will not affect that even in a lower scope.

Your proposal seems fine. Perhaps a clippy::all and clippy::warn would be nice, where all applies to all non-restriction/internal lints. I feel like it's no big deal to have clippy alias to clippy::warn though, but we don't really _need_ that 😄

[nrc]

@Manishearth @oli-obk do you think we are complete on the implementation of the lints part of this? (https://github.com/rust-lang/rust/pull/52018) Or is there still work to do? Could we stabilise the lints part or does it need more time?

[Manishearth]

Clippy doesn't use it yet. cc @flip1995 do you know what the status is here?

[killercup]

@rfcbot reviewed

[fitzgen]

@rfcbot reviewed

[flip1995]

The lints part isn't completed yet. Currently the tool_lints cannot be used by lint tools, like it is documented. I already talked with @oli-obk about how to do this and I'm working on it. I'm currently studying for an exam though. I try to make a PR by the end of next week. Sorry about the delay...

This won't be a breaking change though, because tool_lints can't be used right now :smile:

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[nrc]

\o/

Note that stabilisation applies to attributes only, not lints, and is still blocked on https://github.com/rust-lang/rust/issues/51277

[nrc]

There's a fix ready for #51277, so this is ready for a stabilisation PR.

[mominul]

I'd be happy to handle this!

[nrc]

@mominul great, thank you! Are you good to go with the links in the OP or would you like some more pointers?

[mominul]

@nrc Welcome! I will go with the links first, for now. If I get stuck, I will ask here or in the discord channel, Ok? :blush:

Thanks!

[petrochenkov]

@nrc
To confirm, something like

#[rustfmt::arbitrary::number::of::segments(then * arbitrary !!! token ~~~ tree ^_^)]

is considered a tool attribute too, without restrictions?
I.e. everything after the first segment tool:: is supposed to be a mini-DSL interpreted entirely by the tool, and the tool is permitted but not required to reject nonsensical constructions by itself.

[petrochenkov]

(Also, feature(tool_attributes) can't be stabilized right away because it depends on macro modularization feature(use_extern_macros), but it's going to be completed for 1.30 anyway.)

[mominul]

I might be getting the wrong idea, as @petrochenkov saying that feature(tool_attributes) won't be stabilized right away. Then should I only work on the tool_lints feature?

[nrc]

@mominul I think use_extern_macros will be ready for stabilisation very soon, so it is worth working on the stabilisation PR for tool_attibrutes. tool_lints probably won't be ready for stabilisation for another few months.

@petrochenkov yes, that's all correct

[petrochenkov]

@nrc
Question: should tool attributes be marked as "used" unconditionally (i.e. not report the "unused attribute" warning)?
I assume yes, but custom attributes (including rustfmt_skip) weren't marked as used previously.
Were conditional attributes like #[cfg_attr(rustfmt, rustfmt_skip)] a workaround for the "unused attribute" warning as well?

EDIT: Oh, the RFC text actually had a clause about this.

[petrochenkov]

@nrc
I'm also slightly concerned about hardcoding the knowledge about rustfmt and clippy into the compiler.

Can we perhaps simultaneously add some command line option for introducing tools?
It should be as simple as --tool=rustfmt --tool=clippy or something like this.
Cargo will add this automatically to compiler invocations, so most of users won't notice the difference.

[oli-obk]

@petrochenkov what's the difference between cargo having this hardcoded list and rustc having it?

[petrochenkov]

Well, cargo is more aware about the environment and conventions in general.

Also, as I understand it, the end goal is to register arbitrary tools explicitly in Cargo.toml, so cargo will eventually need to issue warnings about tools passed to rustc implicitly and suggest the fix.
rustc can't issue such warnings, so if they are hardcoded in rustc there's a chance they will need to be hardcoded in both places in the future.

[oli-obk]

makes sense.

[nrc]

I'm also slightly concerned about hardcoding the knowledge about rustfmt and clippy into the compiler.

Can we perhaps simultaneously add some command line option for introducing tools?
It should be as simple as --tool=rustfmt --tool=clippy or something like this.

I worry that when we have a long-term solution, we'll have to keep this for back compat and it will be used to circumvent any rules we put in place. Since the whitelist is only meant to be temporary, I think it is fine to have in the compiler, it's a bit hacky, but that seems to be the only downside.

Cargo will add this automatically to compiler invocations, so most of users won't notice the difference.

It might be a problem for the tools we want to support, e.g., the RLS would have to pass those arguments too.

[nrc]

This is only partially stable.

[aturon]

@nrc Is this stable "enough" for the edition? Can it be removed from the milestone?

[nrc]

@nrc Is this stable "enough" for the edition? Can it be removed from the milestone?

Yes!

[sanmai-NL]

@nrc: kind reminder to update the opening post with the current progress.

[Nemo157]

This appears to be stablilized, should the OP checkbox be ticked?

[petrochenkov]

https://github.com/rust-lang/rust/pull/66070 introduces a way to introduce attribute tools without hardcoding them into the compiler, https://github.com/rust-lang/rust/issues/66079 is the tracking issue for that feature.