Attributes for tools, 2.0

[nrc]

This RFC proposes a temporary solution to the problem of letting tools use
attributes. We outline a (partial) long-term solution and propose a step towards
that solution for tools which are part of the Rust distribution.

Rendered

[theduke]

Rendered

[repax]

Could a special prefix be used for tools? I'm thinking of an analogue to the different proposals for crates:

• [crate]::foo::bar
• @crate::foo::bar
• extern crate::foo::bar
  ...

So, #[<prefix> tool::foo].

[oli-obk]

Note that clippy uses very few attributes directly. Instead almost all of the #[cfg_attr...] that we see in code using clippy is #![cfg_attr(clippy, warn(clippy))] or #![cfg_attr(clippy, allow(some_overzealous_lint))].

[mcarton]

@oli-obk I have wanted many times to add attributes for clippy but the uglyness of #![cfg_attr(clippy, …)] stopped me 😄

[dhardy]

#![cfg_attr(clippy, allow(some_lint))]

Should these be migrated from "optional language lints" to "clippy lints", and use syntax like the following?

#[clippy::allow(some_lint)]

Alternatively one can imagine scoping the lints themselves, but this may be harder to introduce now, and doesn't make it quite so obvious that this is a clippy-specific attribute:

#[allow(clippy::some_lint)]

[mcarton]

#[allow(clippy::some_lint)] fits better in the current model. Clippy does not manage allow/warn/deny by itself, and just relies on rustc to do that. But if rustc knew that some lints are from clippy, it could just skip the unknow lint message.

[Ericson2314]

This sounds good. Per the unresolved question, I think we should definitely triage all existing attributes and give them better names---I wouldn't a lang prefix for builtin non-rustc-specific stuff either.

Also, I'd like #[test] and friends to eventually be procedural macros instead of baked into the compiler---it would be wonderful if we can somhow pave the way for that with this.

[jan-hudec]

A used macro should be possible to use without prefix, no? Tools not, but I guess if somebody really wanted, they could create a macro to hide it from the compiler.

[nrc]

Yes, this RFC does not affect imported macros.

[nrc]

Regarding lints, it sounds like a potential extension is allowing tool prefixes on lints, so rather than lints being just a single identifier (some_lint) they can be a path (foo::some_lint). Furthermore, that using a known tool's name as the root of the path (e.g., clippy::foo::some_lint) would prevent the compiler giving an 'unknown lint' error.

I think this is a good idea!

It seems a bit orthogonal to this RFC, so I'm not sure whether we should add it. Ideally, it should be a separate RFC, but it would be quite a small addition to this RFC, and I would like to avoid clippy moving from cfg(attr) to clippy::allow(some_lint) to allow(clippy::some_lint), rather than doing that in a single step.

Clippy people (@Manishearth, @oli-obk, @iLogiq) thoughts? Specifically, does anyone want to write the scoped lints proposal up as an RFC in the near future? (I'd be happy to help out). Does anyone think it is essential or terrible to include here? And do you have an idea for how hard the implementation would be?

[Manishearth]

I think it's fine to include here, I would phrase it as #[allow(clippy::foo)]. The basic concern is that there should be an easy mechanism to say "ignore clippy::* when clippy isn't on instead of unknown-lint warning it"

(We could alternatively special case the clippy string, which may happen anyway)

[nrc]

I've added tool-scoped lints to the RFC. I expect these to be implemented separately and not depend on one another.

Ping @rust-lang/lang this is a fairly significant extension to the RFC, but logical, I think.

[oli-obk]

typo: may be used as a

[oli-obk]

clippy will be activated for both, since we have some attributes (and want to extend the set of) in order to make some lints configurable.

[oli-obk]

I'd assume as an initial step, We can extend LintPass with an associated constant &str, which produces the tool name. While this means that clippy could accidentally produce a tool name clippy and clippz if someone makes a typo (we have like 50 LintPasses), this seems like a minor danger. Alternatively, we can pass the tool name via the methods on the Registry, but that has the same typo issue.

As a third option, we can hide the lint registering methods behind a wrapper, which at creation time gets its tool name once, and then forwards it to the rustc data structures. That requires some more implementation effort, and can be done after the second option if it is deemed necessary.

[nrc]

@rfcbot fcp merge

There's not been much discussion, but it is a fairly simple RFC and a stepping stone to a general solution that leaves nearly every sensible possibility open. It hasn't been open long, but the previous RFC (#1755) has been around for much longer. Therefore I think it is OK to FCP this now.

[rfcbot]

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

• @aturon
• @eddyb
• @japaric
• @jonathandturner
• @killercup
• @michaelwoerister
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

No concerns currently listed.

Once these reviewers reach consensus, 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.

[petrochenkov]

This looks like an inconsistent prioritization to me.

It would be good if resolution for attribute paths worked consistently with the rest of resolution,
as if attribute names were just names defined in macro namespace with usual scoping, shadowing, etc.

In particular, I see parallels with built-in types, std prelude types and user defined types.

• User defined macro attributes -> user defined types, found first by name resolution.
• Standard attributes potentially expressible as standard library macros (e.g. potentially #[test]) -> std prelude (e.g. Box), found as a fallback if no user-defined names are found.
• Built-in attributes (e.g. #[repr]) -> built-in types (e.g. u8), language prelude, found as a fallback if no std prelude names are found.

This way we can always introduce new built-in attributes without breaking user-defined macros, like it was done with u128/i128 for types.

Now attributes for tools need to somehow be fitted into this scheme.
This test implies that they should shadow everything when enabled (e.g. through --extern-attr) and break macros. I'd actually expect the opposite priority and placing them into the language prelude for example.
(There shouldn't be conflicts there due to different number of path segments - 1 for built-in, >1 for extern).

[nrc]

This seems like a good change, thanks!

[sophiajt]

@rfcbot reviewed

[michaelwoerister]

@rfcbot reviewed

[killercup]

@rfcbot reviewed

[japaric]

@rfcbot reviewed

[rfcbot]

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

[vitiral]

my only concern is to make sure that this uses the same syntax as the #2126. There is some discussion (although it isn't gaining much ground) of requiring use extern::foo::bar for external crates. If that were the case, this feature should also use #[extern::clippy::the_lint]

[rfcbot]

The final comment period is now complete.