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: RFC 2103 - attributes for tools #44690

Open
nrc opened this Issue Sep 19, 2017 · 61 comments

Comments

@nrc
Member

nrc commented Sep 19, 2017

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

RFC
discussion thread

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

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Sep 19, 2017

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

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 20, 2017

@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?

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Sep 20, 2017

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

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Sep 25, 2017

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

This comment has been minimized.

Contributor

nikomatsakis commented Oct 16, 2017

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

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Oct 16, 2017

@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

This comment has been minimized.

Contributor

nikomatsakis commented Oct 16, 2017

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

rep-nop added a commit to rep-nop/rust that referenced this issue Oct 17, 2017

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Oct 19, 2017

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"
@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Oct 26, 2017

@nikomatsakis @nrc any input? :)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 30, 2017

@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.

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Nov 9, 2017

@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

This comment has been minimized.

Contributor

petrochenkov commented Nov 10, 2017

@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

This comment has been minimized.

Member

nrc commented Nov 14, 2017

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.

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Nov 16, 2017

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

This comment has been minimized.

Contributor

topecongiro commented Jan 15, 2018

@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.

@rep-nop

This comment has been minimized.

Contributor

rep-nop commented Jan 19, 2018

@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! :)

bors added a commit that referenced this issue Feb 4, 2018

Auto merge of #47773 - topecongiro:rfc2103, r=petrochenkov
Implement tool_attributes feature (RFC 2103)

This PR implements a `tool_attributes` feature. If this implementaion is acceptable, I am going to create a similar PR that implements `tool_lints`.

This PR only adds `rustfmt` and `clippy` to known tool name list. The list resides in libsyntax/attr.rs.

cc #44690.

r? @nrc
@flip1995

This comment has been minimized.

Contributor

flip1995 commented Mar 20, 2018

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

This comment has been minimized.

Contributor

topecongiro commented Mar 21, 2018

@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

This comment has been minimized.

Contributor

petrochenkov commented Aug 6, 2018

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

This comment has been minimized.

Contributor

oli-obk commented Aug 6, 2018

makes sense.

@nrc

This comment has been minimized.

Member

nrc commented Aug 8, 2018

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.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Aug 22, 2018

Rollup merge of rust-lang#53459 - petrochenkov:stabmore, r=nrc
Stabilize a few secondary macro features

- `tool_attributes` - closes rust-lang#44690
- `proc_macro_path_invoc` - this feature was created due to issues with tool attributes (rust-lang#51277), those issues are now fixed (rust-lang#52841)
- partially `proc_macro_gen` - this feature was created due to issue rust-lang#50504, the issue is now fixed (rust-lang#51952), so proc macros can generate modules. They still can't generate `macro_rules` items though due to unclear hygiene interactions.

bors added a commit that referenced this issue Aug 23, 2018

Auto merge of #53459 - petrochenkov:stabmore, r=nrc
Stabilize a few secondary macro features

- `tool_attributes` - closes #44690
- `proc_macro_path_invoc` - this feature was created due to issues with tool attributes (#51277), those issues are now fixed (#52841)
- partially `proc_macro_gen` - this feature was created due to issue #50504, the issue is now fixed (#51952), so proc macros can generate modules. They still can't generate `macro_rules` items though due to unclear hygiene interactions.

bors added a commit that referenced this issue Aug 23, 2018

Auto merge of #53459 - petrochenkov:stabmore, r=nrc
Stabilize a few secondary macro features

- `tool_attributes` - closes #44690
- `proc_macro_path_invoc` - this feature was created due to issues with tool attributes (#51277), those issues are now fixed (#52841)
- partially `proc_macro_gen` - this feature was created due to issue #50504, the issue is now fixed (#51952), so proc macros can generate modules. They still can't generate `macro_rules` items though due to unclear hygiene interactions.

@bors bors closed this in #53459 Aug 23, 2018

@nrc nrc reopened this Aug 23, 2018

@nrc

This comment has been minimized.

Member

nrc commented Aug 23, 2018

This is only partially stable.

bors added a commit that referenced this issue Aug 30, 2018

Auto merge of #53762 - flip1995:tool_lints, r=Manishearth
Backwards compatibility for tool/clippy lints

cc #44690
cc rust-lang-nursery/rust-clippy#2977 (comment)

This is the next step towards `tool_lints`.

This makes Clippy lints still work without scoping, but will warn and suggest the new scoped name. This warning will only appear if the code is checked with Clippy itself.

There is still an issue with using the old lint name in inner attributes. For inner attributes the warning gets emitted twice. I'm currently not really sure why this happens, but will try to fix this ASAP.

r? @Manishearth

bors added a commit that referenced this issue Sep 1, 2018

Auto merge of #53762 - flip1995:tool_lints, r=Manishearth
Backwards compatibility for tool/clippy lints

cc #44690
cc rust-lang-nursery/rust-clippy#2977 (comment)

This is the next step towards `tool_lints`.

This makes Clippy lints still work without scoping, but will warn and suggest the new scoped name. This warning will only appear if the code is checked with Clippy itself.

There is still an issue with using the old lint name in inner attributes. For inner attributes the warning gets emitted twice. I'm currently not really sure why this happens, but will try to fix this ASAP.

r? @Manishearth
@aturon

This comment has been minimized.

Member

aturon commented Sep 5, 2018

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

@nrc

This comment has been minimized.

Member

nrc commented Sep 5, 2018

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

Yes!

@nrc nrc removed this from the Rust 2018 RC milestone Sep 5, 2018

@sanmai-NL

This comment has been minimized.

sanmai-NL commented Sep 7, 2018

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

ltratt added a commit to softdevteam/grmtools that referenced this issue Sep 21, 2018

Stop rustfmt formatting some things that it can't do a good job of.
Some of the tests in this repository are formatted to make reading them (and
spotting problems) much easier (generally by following vertical information).
rustfmt doesn't (and can't) know about it. Although this commit makes all of
grmtools nightly only (at least until tool_attributes stabilises
rust-lang/rust#44690), it gets us much closer to "run
rustfmt all the time."

ltratt added a commit to softdevteam/grmtools that referenced this issue Sep 21, 2018

Stop rustfmt formatting some things that it can't do a good job of.
Some of the tests in this repository are formatted to make reading them (and
spotting problems) much easier (generally by following vertical information).
rustfmt doesn't (and can't) know about it. Although this commit makes all of
grmtools nightly only (at least until tool_attributes stabilises
rust-lang/rust#44690), it gets us much closer to "run
rustfmt all the time."

ltratt added a commit to softdevteam/grmtools that referenced this issue Sep 21, 2018

Stop rustfmt formatting some things that it can't do a good job of.
Some of the tests in this repository are formatted to make reading them (and
spotting problems) much easier (generally by following vertical information).
rustfmt doesn't (and can't) know about it. Although this commit makes all of
grmtools nightly only (at least until tool_attributes stabilises
rust-lang/rust#44690), it gets us much closer to "run
rustfmt all the time."

pietroalbini added a commit to pietroalbini/rust that referenced this issue Oct 10, 2018

Rollup merge of rust-lang#54870 - flip1995:stabilize_tool_lints, r=Ma…
…nishearth

Stabilize tool lints

cc rust-lang#44690

This stabilizes the `tool_lints` feature.

This is my first stabilization PR, let me know if I missed something. I followed the guide on https://forge.rust-lang.org. Should the documentation of the `tool_lints` be moved to `rust-lang-nursery/reference`?

r? @Manishearth @nrc

Manishearth added a commit to Manishearth/rust that referenced this issue Oct 10, 2018

Rollup merge of rust-lang#54870 - flip1995:stabilize_tool_lints, r=Ma…
…nishearth

Stabilize tool lints

cc rust-lang#44690

This stabilizes the `tool_lints` feature.

This is my first stabilization PR, let me know if I missed something. I followed the guide on https://forge.rust-lang.org. Should the documentation of the `tool_lints` be moved to `rust-lang-nursery/reference`?

r? @Manishearth @nrc

Manishearth added a commit to Manishearth/rust that referenced this issue Oct 10, 2018

Rollup merge of rust-lang#54870 - flip1995:stabilize_tool_lints, r=Ma…
…nishearth

Stabilize tool lints

cc rust-lang#44690

This stabilizes the `tool_lints` feature.

This is my first stabilization PR, let me know if I missed something. I followed the guide on https://forge.rust-lang.org. Should the documentation of the `tool_lints` be moved to `rust-lang-nursery/reference`?

r? @Manishearth @nrc

@petrochenkov petrochenkov removed their assignment Oct 13, 2018

@Nemo157

This comment has been minimized.

Contributor

Nemo157 commented Oct 15, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment