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.

Sign up for GitHub

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

Attributes for tools, 2.0 #2103

Merged
merged 2 commits into from
Sep 19, 2017
Merged

Attributes for tools, 2.0 #2103

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3104ea0
Attributes for tools, 2.0
nrc Aug 11, 2017
e7ef3cc
Extend scoped attributes to lints
nrc Aug 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 61 additions & 17 deletions text/0000-tool-attributes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
- Feature Name: tool_attributes
- Feature Name: tool_attributes, tool_lints
- Start Date: 2016-09-22
- RFC PR:
- Rust Issue:
Expand Down Expand Up @@ -33,6 +33,9 @@ crate, it will read the attibute and skip formatting `foo` (note that we make no
provision for reading the attribute or doing anything with it, that is all up to
the tool).

This RFC proposes a second mechanism for scoping lints for tools. Similar to
attributes, we propose a subset of a hypothetical long-term solution.

This RFC supersedes #1755.

# Motivation
Expand Down Expand Up @@ -70,10 +73,16 @@ Rustfmt is mostly ready to move towards stabilisation, but requires some kind of
reasonable long-term solution and addresses the needs of some important tools
today.

Similarly, tools (e.g., Clippy) may want to use their own lints without the
compiler warning about unused lints. E.g., we want a user to be able to write
`#![allow(clippy::some_lint)]` in their crate without warning.


# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

### Attributes

This section assumes that attributes (e.g., `#[test]`) have already been taught.

You can use attibutes in your crate to pass information to tools. For now, this
Expand All @@ -99,6 +108,21 @@ mod baz {
}
```

### Lints

This section assumes lints have already been taught.

Lints can be defined hierarchically as a path, as well as just a single name.
For example, `nonstandard_style::non_snake_case_functions` and
`nonstandard_style::uppercase_variables`. Note this RFC is not proposing
changing any existing lints, just extending the current lint naming system. Lint
names cannot be imported using `use`.

Lints can be enforced by tools other than the compiler. For example, Clippy
provides a large suite of lints to catch common mistakes and improve your Rust
code. Lints for tools are prefixed with the tool name, e.g., `clippy::box_vec`.


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

Expand Down Expand Up @@ -127,6 +151,8 @@ tries to find a macro using the [macro name resolution rules](https://github.com
If this fails, then it reports a macro not found error. The compiler *may*
Copy link
Contributor

@petrochenkov petrochenkov Sep 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good change, thanks!

suggest mis-typed attributes (declared or built-in).

A similar opt-in mechanism will exist for lints.


## Proposed for immediate implementation

Expand All @@ -138,12 +164,12 @@ feature gate.
E.g., `#[rustdoc::foo]` will be permitted in stable Rust code; `#[rustdoc]` will
still be treated as a custom attribute.

The initial list of allowed prefixes is `rustc`, `rustdoc`, and `rls`. As tools
are added to the distribution, they will be allowed as path prefixes in
attributes. We expect to add `rustfmt` and `clippy` in the near future. Note
that whether one of these names can be used does not depend on whether the
relevant component is installed on the user's system; this is a simple,
universal white list.
The initial list of allowed prefixes is `rustc`, `rustdoc`, and `rls` (but see
note below on activation). As tools are added to the distribution, they will be
allowed as path prefixes in attributes. We expect to add `rustfmt` and `clippy`
in the near future. Note that whether one of these names can be used does not
depend on whether the relevant component is installed on the user's system; this
is a simple, universal white list.

Given the earlier rules on name resolution, these attributes would shadow any
attribute macro with the same name. This is not problematic because a macro
Expand All @@ -155,6 +181,29 @@ Tool-scoped attributes should be preserved by the compiler for as long as
possible through compilation. This allows tools which plug into the compiler
(like Clippy) to observe these attributes on items during type checking, etc.

Likewise, white-listed tools may be used a prefix for lints. So for example,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: may be used as a

`rustfmt::foo` and `clippy::bar` are both valid lint names, from the compiler's
perspective.


### Activation and unused attibutes/lints

For each name on the whitelist, it is indicated if the name is active for
attributes or lints. A name is only activated if required. So for example,
`rustdoc` will not be activated at all until it takes advantage of this feature.
I expect `clippy` will be activated only for lints, and `rustfmt` only for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

attributes.

A tool that has an active name *must* check for unused lints/attibutes. For
example, if `rustfmt` becomes active for attributes, and only recognises
`rustfmt::skip`, it must produce a warning if a user uses `rustfmt::foo` in
their code.

These two requirements together mean that we do not lose checking of unused
attributes/lints in any circumstance and we can move to having the compiler
check for unused attributes/lints as part of a possible long-term solution
without introducing new warnings or errors.


### Forward and backward compatability

Expand All @@ -174,13 +223,6 @@ term solution? One could imagine either leaving them implicit (similar to the
libraries prelude) or using warning cycles or an epoch to move them to explicit
opt-in.

Another hazard is that if in the long-term solution, the compiler checks
attribute paths for validity rather than only the prefix. For example, if
Rustfmt only defines `skip`, then `#[rustfmt::foo]` would not give an error
under this proposal, but would in the long term. Given that `#[rustfmt::foo]` is
almost certainly a mistake today, it seems easy to migrate to a stricter system
with a warning cycle.


# Drawbacks
[drawbacks]: #drawbacks
Expand All @@ -190,9 +232,9 @@ attributes (I consider this a feature, not a bug, but others may differ).

Some tools are clearly given special treatment.

We permit some useless attributes without warning (e.g., `#[rustfmt::foo]`,
assuming Rustfmt does nothing with `foo`). Tools could (and probably should)
warn or error on such attributes.
We permit some useless attributes without warning from the compiler (e.g.,
`#[rustfmt::foo]`, assuming Rustfmt does nothing with `foo`). However, tools
should warn or error on such attributes.

We are not planning any infrastructure to help tools use these attributes. That
seems fine for now, I imagine a long-term solution should include some library
Expand All @@ -216,3 +258,5 @@ Are there other tools that should be included on the whitelist (`#[test]` perhap

Should we try and move some top-level attributes that are compiler-specific
(rather than language-specific) to use `#[rustc::]`? (E.g., `crate_type`).

How should the compiler expose path lints to lint plugins/lint tools?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.