Tracking issue: RFC 2103 - attributes for tools

[nrc]

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

RFC
discussion thread

• implement for attributes (mentoring instructions here)
• implement for lints (Implementation of tool lints. #52018; see Tracking issue: RFC 2103 - attributes for tools #44690 (comment))
• 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!

[petrochenkov]

#66070 introduces a way to introduce attribute tools without hardcoding them into the compiler, #66079 is the tracking issue for that feature.

[jyn514]

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

I opened #83216 which makes register_tool apply to tool lints too.

[inquisitivecrystal]

Is this done after #83216, or is there more left to do?

[jyn514]

Well, register_tool is still unstable. I think it's tracked elsewhere, I forget which issue.

[runiq]

Well, register_tool is still unstable. I think it's tracked elsewhere, I forget which issue.

#66079

[str4d]

I cannot find any existing issue covering this particular case:

I currently use #[cfg_attr(rustfmt, rustfmt_skip)] on some macros, to prevent rustfmt from applying unreadable formatting to them. Rust tells me that "cfg_attr is deprecated for rustfmt and got replaced by tool attributes". Adding #![allow(clippy::deprecated_cfg_attr)] to my crate does not silence the cfg_attr warnings, which prevents my CI from passing with -D warnings.

When I replace all these with #[rustfmt::skip] I get the error "attributes on expressions are experimental" (#15701). I'm somewhat confused that the cfg_attr approach was not causing an error, but in any case, how would I go about resolving this? (Other than removing the macros, which might end up happening soon.)

[petrochenkov]

I'm somewhat confused that the cfg_attr approach was not causing an error

This is one of several stability checking holes in expression/statement attributes :)
Without bugs it would be unstable, but this specific case is unlikely to change before stabilization, so we are not fixing the bugs to avoid breaking people's code, and are waiting for stabilization to make it supported de jure instead.

[petrochenkov]

but in any case, how would I go about resolving this?

The workaround is to place #[rustfmt::skip] on some outer block or function, and not directly on the expression.

[pnkfelix]

visiting for backlog bonanza.

It seems like as of end of 2019, there was still some remaining work to be done here. It is not super clear what remains to be done.

For now, marking with implementation unfinished.

@rustbot label: S-tracking-impl-incomplete

[wesleywiser]

Visited during compiler team tracking issue triage. After skimming through the discussion, it's not clear at this time what the current state of this feature is.

For anyone who wants to help move this forward, I think the most valuable thing at this time would be a summary of what work has been completed and what (if any) known issues are with the current implementation as well as any salient points from the prior discussion.