Metadata collection monster searching for Clippy's configuration options
This PR teaches our lovely metadata collection monster which configurations are available inside Clippy. It then adds a new *Configuration* section to the lint documentation.
---
The implementation uses the `define_Conf!` macro to create a vector of metadata during compilation. This enables easy collection and parsing without the need of searching for the struct during a lint-pass (and it's quite elegant IMO). The information is then parsed into an intermediate struct called `ClippyConfiguration` which will be saved inside the `MetadataCollector` struct itself. It is currently only used to generate the *Configuration* section in the lint documentation, but I'm thinking about adding an overview of available configurations to the website. Saving them in this intermediate state without formatting them right away enables this in the future.
The new parsing will also allow us to have a documentation that spans over multiple lines in the future. For example, this will be valid when the old script has been removed:
```rust
/// Lint: BLACKLISTED_NAME.
/// The list of blacklisted names to lint about. NB: `bar` is not here since it has legitimate uses
(blacklisted_names: Vec<String> = ["foo", "baz", "quux"].iter().map(ToString::to_string).collect())
```
The deprecation reason is also currently being collected but not used any further and that's basically it.
---
See: #7172 for the full metadata collection to-do list or to suggest a new feature in connection to it 🙃
---
changelog: none
r? `@flip1995`
cc `@camsteffen` It would be great if you could also review this PR as you have recently worked on Clippy's `define_Conf!` macro.
`needless_collect` enhancements
fixes#7164
changelog: `needless_collect`: For `BTreeMap` and `HashMap` lint only `is_empty`, as `len` might produce different results than iter's `count`
changelog: `needless_collect`: Lint `LinkedList` and `BinaryHeap` in direct usage case as well
Trigger [`wrong_self_convention`] only if it has implicit self
Lint [`wrong_self_convention`] only if the impl or trait has `self` _per sé_.
Fixes: #7179
changelog: trigger [`wrong_self_convention`] only if it has implicit self
match_single_binding: Fix invalid suggestion when match scrutinee has side effects
fixes#7094
changelog: `match_single_binding`: Fix invalid suggestion when match scrutinee has side effects
---
`Expr::can_have_side_effects` is used to determine the scrutinee has side effects, while this method is a little bit conservative for our use case. But I'd like to use it to avoid reimplementation of the method and too much heuristics. If you think this is problematic, then I'll implement a custom visitor to address it.
* Suggest `&mut iter` when the iterator is used after the loop.
* Suggest `&mut iter` when the iterator is a field in a struct.
* Don't lint when the iterator is a field in a struct, and the struct is
used in the loop.
* Lint when the loop is nested in another loop, but suggest `&mut iter`
unless the iterator is from a local declared inside the loop.
Fix `--remap-path-prefix` not correctly remapping `rust-src` component paths and unify handling of path mapping with virtualized paths
This PR fixes#73167 ("Binaries end up containing path to the rust-src component despite `--remap-path-prefix`") by preventing real local filesystem paths from reaching compilation output if the path is supposed to be remapped.
`RealFileName::Named` introduced in #72767 is now renamed as `LocalPath`, because this variant wraps a (most likely) valid local filesystem path.
`RealFileName::Devirtualized` is renamed as `Remapped` to be used for remapped path from a real path via `--remap-path-prefix` argument, as well as real path inferred from a virtualized (during compiler bootstrapping) `/rustc/...` path. The `local_path` field is now an `Option<PathBuf>`, as it will be set to `None` before serialisation, so it never reaches any build output. Attempting to serialise a non-`None` `local_path` will cause an assertion faliure.
When a path is remapped, a `RealFileName::Remapped` variant is created. The original path is preserved in `local_path` field and the remapped path is saved in `virtual_name` field. Previously, the `local_path` is directly modified which goes against its purpose of "suitable for reading from the file system on the local host".
`rustc_span::SourceFile`'s fields `unmapped_path` (introduced by #44940) and `name_was_remapped` (introduced by #41508 when `--remap-path-prefix` feature originally added) are removed, as these two pieces of information can be inferred from the `name` field: if it's anything other than a `FileName::Real(_)`, or if it is a `FileName::Real(RealFileName::LocalPath(_))`, then clearly `name_was_remapped` would've been false and `unmapped_path` would've been `None`. If it is a `FileName::Real(RealFileName::Remapped{local_path, virtual_name})`, then `name_was_remapped` would've been true and `unmapped_path` would've been `Some(local_path)`.
cc `@eddyb` who implemented `/rustc/...` path devirtualisation
Metadata collection monster eating deprecated lints
This adds the collection of deprecated lints to the metadata collection monster. The JSON output has the same structure with the *new* lint group "DEPRECATED". Here is one of fourteen examples it was able to dig up in Clippy's code:
```JSON
{
"id": "assign_op_pattern",
"id_span": {
"path": "src/assign_ops.rs",
"line": 34
},
"group": "clippy::style",
"docs": " **What it does:** Checks for `a = a op b` or `a = b commutative_op a` patterns.\n\n **Why is this bad?** These can be written as the shorter `a op= b`.\n\n **Known problems:** While forbidden by the spec, `OpAssign` traits may have\n implementations that differ from the regular `Op` impl.\n\n **Example:**\n ```rust\n let mut a = 5;\n let b = 0;\n // ...\n // Bad\n a = a + b;\n\n // Good\n a += b;\n ```\n",
"applicability": {
"is_multi_part_suggestion": false,
"applicability": "MachineApplicable"
}
}
```
And you! Yes you! Sir or Madam can get all of this **for free** in Clippy if this PR gets merged. (Sorry for the silliness ^^)
---
See: #7172 for the full metadata collection to-do list or to suggest a new feature in connection to it 🙃
---
changelog: none
r? `@flip1995`
This PR implements span quoting, allowing proc-macros to produce spans
pointing *into their own crate*. This is used by the unstable
`proc_macro::quote!` macro, allowing us to get error messages like this:
```
error[E0412]: cannot find type `MissingType` in this scope
--> $DIR/auxiliary/span-from-proc-macro.rs:37:20
|
LL | pub fn error_from_attribute(_args: TokenStream, _input: TokenStream) -> TokenStream {
| ----------------------------------------------------------------------------------- in this expansion of procedural macro `#[error_from_attribute]`
...
LL | field: MissingType
| ^^^^^^^^^^^ not found in this scope
|
::: $DIR/span-from-proc-macro.rs:8:1
|
LL | #[error_from_attribute]
| ----------------------- in this macro invocation
```
Here, `MissingType` occurs inside the implementation of the proc-macro
`#[error_from_attribute]`. Previosuly, this would always result in a
span pointing at `#[error_from_attribute]`
This will make many proc-macro-related error message much more useful -
when a proc-macro generates code containing an error, users will get an
error message pointing directly at that code (within the macro
definition), instead of always getting a span pointing at the macro
invocation site.
This is implemented as follows:
* When a proc-macro crate is being *compiled*, it causes the `quote!`
macro to get run. This saves all of the sapns in the input to `quote!`
into the metadata of *the proc-macro-crate* (which we are currently
compiling). The `quote!` macro then expands to a call to
`proc_macro::Span::recover_proc_macro_span(id)`, where `id` is an
opaque identifier for the span in the crate metadata.
* When the same proc-macro crate is *run* (e.g. it is loaded from disk
and invoked by some consumer crate), the call to
`proc_macro::Span::recover_proc_macro_span` causes us to load the span
from the proc-macro crate's metadata. The proc-macro then produces a
`TokenStream` containing a `Span` pointing into the proc-macro crate
itself.
The recursive nature of 'quote!' can be difficult to understand at
first. The file `src/test/ui/proc-macro/quote-debug.stdout` shows
the output of the `quote!` macro, which should make this eaier to
understand.
This PR also supports custom quoting spans in custom quote macros (e.g.
the `quote` crate). All span quoting goes through the
`proc_macro::quote_span` method, which can be called by a custom quote
macro to perform span quoting. An example of this usage is provided in
`src/test/ui/proc-macro/auxiliary/custom-quote.rs`
Custom quoting currently has a few limitations:
In order to quote a span, we need to generate a call to
`proc_macro::Span::recover_proc_macro_span`. However, proc-macros
support renaming the `proc_macro` crate, so we can't simply hardcode
this path. Previously, the `quote_span` method used the path
`crate::Span` - however, this only works when it is called by the
builtin `quote!` macro in the same crate. To support being called from
arbitrary crates, we need access to the name of the `proc_macro` crate
to generate a path. This PR adds an additional argument to `quote_span`
to specify the name of the `proc_macro` crate. Howver, this feels kind
of hacky, and we may want to change this before stabilizing anything
quote-related.
Additionally, using `quote_span` currently requires enabling the
`proc_macro_internals` feature. The builtin `quote!` macro
has an `#[allow_internal_unstable]` attribute, but this won't work for
custom quote implementations. This will likely require some additional
tricks to apply `allow_internal_unstable` to the span of
`proc_macro::Span::recover_proc_macro_span`.
The whole point of named fields is that we don't have to worry about
order. The names, not the position, communicate the information, so
worrying about consistency for consistency's sake is pedantic to a *T*.
Fixes#7192.
wchargin-branch: inconsistent-struct-constructor-pedantic
wchargin-source: 4fe078a21c77ceb625e58fa3b90b613fc4fa6a76
Fix needless_quesiton_mark false positive
changelog: Fix [`needless_question_mark`] false positive where the inner value is implicity dereferenced by the question mark.
Fixes#7107
Handle write!(buf, "\n") case better
Make `write!(buf, "\n")` suggest `writeln!(buf)` by removing
the trailing comma from `writeln!(buf, )`.
changelog: [`write_with_newline`] suggestion on only "\n" improved
Make `write!(buf, "\n")` suggest `writeln!(buf)` by removing
the trailing comma from `writeln!(buf, )`.
changelog: [`write_with_newline`] suggestion on only "\n" improved
It relaxes rules for `to_*` variant, so it doesn't lint in trait definitions
and implementations anymore.
Although, non-`Copy` type implementing trait's `to_*` method taking
`self` feels not good (consumes ownership, so should be rather named `into_`), it would be better if this case was a pedantic lint (allow-by-default) instead.
Refactor: arrange lints in misc_early module
This PR arranges misc_early lints so that they can be accessed more easily.
Basically, I refactored them following the instruction described in #6680.
cc: `@Y-Nak,` `@flip1995,` `@magurotuna`
changelog: Move lints in misc_early module into their own modules.
Fix stack overflow issue in `redundant_pattern_matching`
Fixes#7169
~~cc `@Jarcho` Since tomorrow is release day and we need to get this also fixed in beta, I'll just revert the PR instead of looking into the root issue. Your changes are good, so if you have an idea what could cause this stack overflow and know how to fix it, please open a PR that reverts this revert with a fix.~~
r? `@llogiq`
changelog: none (fixes stack overflow, but this was introduced in this release cycle)
needless_collect: Lint cases with type annotations for indirect usage and recognize `BinaryHeap`
fixes#7110
changelog: needless_collect: Lint cases with type annotations for indirect usage and recognize `BinaryHeap`.
Producing a good suggestion for this lint is already hard when no macros
are involved. With macros the lint message and the suggestion are just
confusing. Since both, producing a good suggestion and figuring out if
this pattern can be re-written inside a macro is nearly impossible, just
bail out.
[single_char_pattern] add strip_prefix and strip_suffix
Title says it all. Adjusted ui tests.
I added the second commit in case you don't like that I moved that table into `single_char_pattern.rs` directly. I don't see any reason why it shouldn't be in that file. It isn't used anywhere else.
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: add strip_prefix and strip_suffix to single_char_pattern lint
while_immutable_cond: check condition for mutation
This fixes#6689 by also checking the bindings mutated in the condition, whereas it was previously only checked in the loop body.
---
changelog: Fix FP in [`while_immutable_cond`] where mutation in the loop variable wasn't picked up.
`implicit_return` improvements
fixes: #6940
changelog: Fix `implicit_return` suggestion for async functions
changelog: Improve `implicit_return` suggestions when returning the result of a macro
changelog: Check for `break` expressions inside a loop which are then implicitly returned
changelog: Allow all diverging functions in `implicit_return`, not just panic functions
Remove leftover plugin conf_file code
changelog: none
Removes dead code that used to support the following syntax:
```rust
#![plugin(clippy(conf_file="path/to/clippy's/configuration"))]
```
RLS (and others?) will need to remove the `&[]` from `clippy_lints::read_conf(&[], sess)`.
r? `@flip1995`
Don't rebuild rustdoc and clippy after checking bootstrap
This works by unconditionally passing -Z unstable-options to the
compiler. This has no affect in practice since bootstrap doesn't use
`deny(rustc::internal)`.
Fixes https://github.com/rust-lang/rust/issues/82461.
r? ```@Mark-Simulacrum```
thread 'rustc' panicked at 'index out of bounds: the len is 0 but the index is 0', src/tools/clippy/clippy_lints/src/matches.rs:1595:53
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
I don't have a minimised testcase because I don't have time to reduce libstd down to a few lines right now.
Fix FN in `iter_cloned_collect` with a large array
fixes#6808
changelog: Fix FN in `iter_cloned_collect` with a large array
I spotted that [is_iterable_array](a362a4d1d0/clippy_lints/src/loops/explicit_iter_loop.rs (L67-L75)) function that `explicit_iter_loop` lint is using only works for array sizes <= 32.
There is this comment:
> IntoIterator is currently only implemented for array sizes <= 32 in rustc
I'm a bit confused, because I read that [IntoIterator for arrays](https://doc.rust-lang.org/src/core/array/mod.rs.html#194-201) with const generic `N` is stable since = "1.0.0". Although Const Generics MVP were stabilized in Rust 1.51.
Should I set MSRV for the current change? I will try to test with older compilers soon.
manual_unwrap_or: fix invalid code suggestion, due to macro expansion
fixes#6965
changelog: fix invalid code suggestion in `manual_unwrap_or` lint, due to macro expansion
extend `single_element_loop` to match `.iter()`
This extends `single_element_loop` to also match `[..].iter()` in the loop argument. Related to #7125, but not completely fixing it due to the lint only firing if the array expression contains a local variable.
---
changelog: none
`single_component_path_imports`: ignore `pub(crate) use some_macro;`
Fixes#7106
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: Ignore exporting a macro within a crate using `pub(crate) use some_macro;` for [`single_component_path_imports`]
Unused io amount detects `.read().ok()?`
fixes#7096
changelog: unused_io_amount now detect expertion like `.read().ok()?`, `.read().or_else(|err| ...)?` and similar expressions.
Better suggestions when returning macro calls.
Suggest changeing all the break expressions in a loop, not just the final statement.
Don't lint divergent functions.
Don't suggest returning the result of any divergent fuction.
Switch transmute_ptr_to_ptr to "pedantic" class.
Per discussion in https://github.com/rust-lang/rust-clippy/issues/6372, this lint has significant false positives.
changelog: transmute_ptr_to_ptr defaults to "allow".
Add lint to check for boolean comparison in assert macro calls
This PR adds a lint to check if an assert macro is using a boolean as "comparison value". For example:
```rust
assert_eq!("a".is_empty(), false);
```
Could be rewritten as:
```rust
assert!(!"a".is_empty());
```
PS: The dev guidelines are amazing. Thanks a lot for writing them!
changelog: Add `bool_assert_comparison` lint
useless use of format! should return function directly
fixes#7066
changelog: [`useless_format`] wraps the content in the braces when it's needed.
r? `@giraffate`
Allow allman style braces in `suspicious_else_formatting`
fixes: #3864
Indentation checks could be added as well, but the lint already doesn't check for it.
changelog: Allow allman style braces in `suspicious_else_formatting`
Fixing FPs for the `branches_sharing_code` lint
Fixes#7053Fixes#7054
And an additional CSS adjustment to support dark mode for every inline code. It currently only works in paragraphs, which was an oversight on my part 😅. [Current Example](https://rust-lang.github.io/rust-clippy/master/index.html#blacklisted_name)
This also includes ~50 lines of doc comments and is therefor not as big as the changes would indicate. 🐧
---
changelog: none
All of these bugs were introduced in this dev version and are therefor not worth a change log entry.
r? `@phansch`
cc: `@camsteffen` since you have a pretty good overview of the `SpanlessEq` implementation 🙃
Add `cloned_instead_of_copied` lint
Don't go cloning all willy-nilly.
Featuring a new `get_iterator_item_ty` util!
changelog: Add cloned_instead_of_copied lint
Closes#3870
Fix: redundant_pattern_matching drop order
Fixes#5746
A note about the change in drop order is added when the scrutinee (or any temporary in the expression) isn't known to be safe to drop in any order (i.e. doesn't implement the `Drop` trait, or contain such a type). There is a whitelist for some `std` types, but it's incomplete. Currently just `Vec<_>`, `Box<_>`, `Rc<_>` and `Arc<_>`, but only if the contained type is also safe to drop in any order.
Another lint for when the drop order changes could be added as allowed by default, but the drop order requirement is pretty subtle in this case. I think the note added to the lint should be enough to make someone think before applying the change.
changelog: Added a note to `redundant_pattern_matching` when the change in drop order might matter
Improve `map_entry` suggestion
fixes: #5176fixes: #4674fixes: #4664fixes: #1450
Still need to handle the value returned by `insert` correctly.
changelog: Improve `map_entry` suggestion. Will now suggest `or_insert`, `insert_with` or `match _.entry(_)` as appopriate.
changelog: Fix `map_entry` false positives where the entry api can't be used. e.g. when the map is used for multiple things.
Don't allow adjustments for `manual_map`
fixes: #7077
The other option here would be to add the return type to the closure. It would be fine for simple types, but longer types can be rather unwieldy. Could also implement the adjustment manually.
changelog: Don't lint `manual_map` when type adjustments are added. e.g. autoderef
Remove `match_path` and `match_qpath`
The only remaining usage is the `author` lint, so the functions are left in for now. The test result for `repl_uninit` changed, the lint was broken before.
The `collapsible_span_lint_calls` and `match_type_on_diag_item` tests have been changed. Both lints were broken when utils was extracted into it's own crate. `match_type_on_diag_item` isn't quite fixed, but it's at least less broken.
changelog: None
Fix false positives where the map is used before inserting into the map.
Fix false positives where two insertions happen.
Suggest using `if let Entry::Vacant(e) = _.entry(_)` when `or_insert` might be a semantic change
tabs_in_doc_comments: Fix ICE due to char indexing
This is a quick-fix for an ICE in `tabs_in_doc_comments`. The problem
was that we we're indexing into possibly multi-byte characters, such as '位'.
More specifically `get_chunks_of_tabs` was returning indices into
multi-byte characters. Those were passed on to a `Span` creation that
then caused the ICE.
This fix makes sure that we don't return indices that point inside a
multi-byte character. *However*, we are still iterating over unicode
codepoints, not grapheme clusters. So a seemingly single character like y̆ ,
which actually consists of two codepoints, will probably still cause
incorrect spans in the output. But I don't think we handle those cases
anywhere in Clippy currently?
Fixes#5835
changelog: Fix ICE in `tabs_in_doc_comments`
Fix a FP in `missing_const_for_fn`
where a function that calls a standard library function whose constness
is unstable is considered as being able to be a const function. Fixes#5995.
The core change is the move from `rustc_mir::const_eval::is_min_const_fn` to `rustc_mir::const_eval::is_const_fn`. I'm not clear about the difference in their purpose between them so I'm not sure if it's acceptable to call `qualify_min_const_fn::is_min_const_fn` this way now.
---
changelog: `missing_const_for_fn`: No longer lints when an unstably const function is called
Fix FP in `wrong_self_convention` lint
Previously, this lint didn't check into impl block when it was implementing a trait.
Recent improvements (#6924) have moved this check and some impl blocks are now checked but they shouldn't, such as in #7032.
Fixes#7032
changelog: Fix FP when not taking `self` in impl block for `wrong_self_convention` lint
Add a note on the issue #5953
Hello,
I thought it would be better to have a note and warning about this issue considering it introduced an UB in the past even with the "Search on Github" feature.
---
changelog: Add a note on the issue #5953 to the known problems section.
Deprecate `filter_map`
Since #6591, `filter_map` does not even lint `filter().map()`. The cases that are still linted make no sense IMO. So this just removes/deprecates it.
changelog: Deprecate `filter_map` lint
Closes#3424Fixes#7050
Fix FP in `single_component_path_imports` lint
Fix FP in `single_component_path_imports` lint when the import is reused with `self`, like in `use self::module`.
Fixes#5210
changelog: none
Invalid null usage v2
This is continuation of #6192 after inactivity.
I plan to move paths into the compiler as diagnostic items after this is merged.
fixes#1703
changelog: none
consider mutability on useless_vec suggestions
fixes#7035
changelog: Now the suggested by `useless_vec` considers mutability to suggest either `&[]`, as before, or `&mut []` if the used reference is mutable.
This is a quick-fix for an ICE in `tabs_in_doc_comments`. The problem
was that we we're indexing into possibly multi-byte characters, such as '位'.
More specifically `get_chunks_of_tabs` was returning indices into
multi-byte characters. Those were passed on to a `Span` creation that
then caused the ICE.
This fix makes sure that we don't return indices that point inside a
multi-byte character. *However*, we are still iterating over unicode
codepoints, not grapheme clusters. So a seemingly single character like y̆ ,
which actually consists of two codepoints, will probably still cause
incorrect spans in the output.
Don't trigger `same_item_push` if the vec is used in the loop body
fixes#6987
changelog: `same_item_push`: Don't trigger if the `vec` is used in the loop body
