Avoid slice indexing in Clippy (down with the ICEs)
While working on #7569 I got about 23 lint reports where we can avoid slice indexing by destructing the slice early. This is a preparation PR to avoid fixing them in the lint PR. (The implementation already takes about 300 lines without tests 😅). Either way, this should hopefully be easy to review 🙃
---
changelog: none
Provide `layout_of` automatically (given tcx + param_env + error handling).
After #88337, there's no longer any uses of `LayoutOf` within `rustc_target` itself, so I realized I could move the trait to `rustc_middle::ty::layout` and redesign it a bit.
This is similar to #88338 (and supersedes it), but at no ergonomic loss, since there's no funky `C: LayoutOf<Ty = Ty>` -> `Ty: TyAbiInterface<C>` generic `impl` chain, and each `LayoutOf` still corresponds to one `impl` (of `LayoutOfHelpers`) for the specific context.
After this PR, this is what's needed to get `trait LayoutOf` (with the `layout_of` method) implemented on some context type:
* `TyCtxt`, via `HasTyCtxt`
* `ParamEnv`, via `HasParamEnv`
* a way to transform `LayoutError`s into the desired error type
* an error type of `!` can be paired with having `cx.layout_of(...)` return `TyAndLayout` *without* `Result<...>` around it, such as used by codegen
* this is done through a new `LayoutOfHelpers` trait (and so is specifying the type of `cx.layout_of(...)`)
When going through this path (and not bypassing it with a manual `impl` of `LayoutOf`), the end result is that only the error case can be customized, the query itself and the success paths are guaranteed to be uniform.
(**EDIT**: just noticed that because of the supertrait relationship, you cannot actually implement `LayoutOf` yourself, the blanket `impl` fully covers all possible context types that could ever implement it)
Part of the motivation for this shape of API is that I've been working on querifying `FnAbi::of_*`, and what I want/need to introduce for that looks a lot like the setup in this PR - in particular, it's harder to express the `FnAbi` methods in `rustc_target`, since they're much more tied to `rustc` concepts.
r? `@nagisa` cc `@oli-obk` `@bjorn3`
Path remapping: Make behavior of diagnostics output dependent on presence of --remap-path-prefix.
This PR fixes a regression (#87745) with `--remap-path-prefix` where the flag stopped causing diagnostic messages to be remapped as well. The regression was introduced in https://github.com/rust-lang/rust/pull/83813 where we erroneously assumed that remapping of diagnostic messages was not desired anymore (because #70642 partially undid that functionality with nobody objecting).
The issue is fixed by making `--remap-path-prefix` remap diagnostic messages again, including for paths that have been remapped in upstream crates (e.g. the standard library). This means that "sysroot-localization" (implemented in #70642) is also disabled if `rustc` is invoked with `--remap-path-prefix`. The assumption is that once someone starts explicitly remapping paths they also don't want paths to their local Rust installation in their build output.
In the future we might want to give more fine-grained control over this behavior via compiler flags (see https://github.com/rust-lang/rfcs/pull/3127 for a related RFC). For now this PR is intended as a regression fix.
This PR is an alternative to https://github.com/rust-lang/rust/pull/88191, which makes diagnostic messages be remapped unconditionally. That approach, however, would effectively revert #70642.
Fixes https://github.com/rust-lang/rust/issues/87745.
cc `@cbeuw`
r? `@ghost`
Fix `option_if_let_else`
fixes: #5822fixes: #6737fixes: #7567
The inference from #6137 still exists so I'm not sure if this should be moved from the nursery. Before doing that though I'd almost want to see this split into two lints. One suggesting `map_or` and the other suggesting `map_or_else`.
`map_or_else` tends to have longer expressions for both branches so it doesn't end up much shorter than a match expression in practice. It also seems most people find it harder to read. `map_or` at least has the terseness benefit of being on one line most of the time, especially when the `None` branch is just a literal or path expression.
changelog: `break` and `continue` statments local to the would-be closure are allowed in `option_if_let_else`
changelog: don't lint in const contexts in `option_if_let_else`
changelog: don't lint when yield expressions are used in `option_if_let_else`
changelog: don't lint when the captures made by the would-be closure conflict with the other branch in `option_if_let_else`
changelog: don't lint when a field of a local is used when the type could be pontentially moved from in `option_if_let_else`
changelog: in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure in `option_if_let_else`
Add module_style lint to style
changelog: Add new [`module_style`] style lint
This is a configurable (no mod file/mod file) lint that determines if `mod.rs` is used consistently or if `mod.rs` is never used (using the new mod layout).
Don't report function calls as unnecessary operation if used in array index
Attempts to fix: #7412
changelog: Don't report function calls used in indexing as unnecessary operation. [`unnecessary_operation`]
Add tests for disallowed_mod in ui-cargo test section
Use correct algorithm to determine if mod.rs is missing
Move to two lints and remove config option
Switch lint names so they read "warn on ..."
Emit the same help info for self_named_mod_file warnings
Bail when both lints are Allow
Reword help message for both module_style lints
Add new lint `negative_feature_names` and `redundant_feature_names`
Add new lint [`negative_feature_names`] to detect feature names with prefixes `no-` or `not-` and new lint [`redundant_feature_names`] to detect feature names with prefixes `use-`, `with-` or suffix `-support`
changelog: Add new lint [`negative_feature_names`] and [`redundant_feature_names`]
New lint `manual_split_once`
This is a WIP because it still needs to recognize more patterns. Currently handles:
```rust
s.splitn(2, ' ').next();
s.splitn(2, ' ').nth(0)
s.splitn(2, ' ').nth(1);
s.splitn(2, ' ').skip(0).next();
s.splitn(2, ' ').skip(1).next();
s.splitn(2, ' ').next_tuple(); // from itertools
// as well as `unwrap()` and `?` forms
```
Still to do:
```rust
let mut iter = s.splitn(2, ' ');
(iter.next().unwrap(), iter.next()?)
let mut iter = s.splitn(2, ' ');
let key = iter.next().unwrap();
let value = iter.next()?;
```
Suggestions on other patterns to check for would be useful. I've done a search on github for uses of `splitn`. Still have yet to actually look through the results.
There's also the question of whether the lint shouold trigger on all uses of `splitn` with two values, or only on recognized usages. The former could have false positives where it couldn't be replaced, but I'm not sure how common that would be.
changelog: Add lint `manual_split_once`
* `break` and `continue` statments local to the would-be closure are allowed
* don't lint in const contexts
* don't lint when yield expressions are used
* don't lint when the captures made by the would-be closure conflict with the other branch
* don't lint when a field of a local is used when the type could be pontentially moved from
* in some cases, don't lint when scrutinee expression conflicts with the captures of the would-be closure
Uplift the invalid_atomic_ordering lint from clippy to rustc
This is mostly just a rebase of https://github.com/rust-lang/rust/pull/79654; I've copy/pasted the text from that PR below.
r? `@lcnr` since you reviewed the last one, but feel free to reassign.
---
This is an implementation of https://github.com/rust-lang/compiler-team/issues/390.
As mentioned, in general this turns an unconditional runtime panic into a (compile time) lint failure. It has no false positives, and the only false negatives I'm aware of are if `Ordering` isn't specified directly and is comes from an argument/constant/whatever.
As a result of it having no false positives, and the alternative always being strictly wrong, it's on as deny by default. This seems right.
In the [zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/233931-t-compiler.2Fmajor-changes/topic/Uplift.20the.20.60invalid_atomic_ordering.60.20lint.20from.20clippy/near/218483957) `@joshtriplett` suggested that lang team should FCP this before landing it. Perhaps libs team cares too?
---
Some notes on the code for reviewers / others below
## Changes from clippy
The code is changed from [the implementation in clippy](68cf94f6a6/clippy_lints/src/atomic_ordering.rs) in the following ways:
1. Uses `Symbols` and `rustc_diagnostic_item`s instead of string literals.
- It's possible I should have just invoked Symbol::intern for some of these instead? Seems better to use symbol, but it did require adding several.
2. The functions are moved to static methods inside the lint struct, as a way to namespace them.
- There's a lot of other code in that file — which I picked as the location for this lint because `@jyn514` told me that seemed reasonable.
3. Supports unstable AtomicU128/AtomicI128.
- I did this because it was almost easier to support them than not — not supporting them would have (ideally) required finding a way not to give them a `rustc_diagnostic_item`, which would have complicated an already big macro.
- These don't have tests since I wasn't sure if/how I should make tests conditional on whether or not the target has the atomic... This is to a certain extent an issue of 64bit atomics too, but 128-bit atomics are much less common. Regardless, the existing tests should be *more* than thorough enough here.
4. Minor changes like:
- grammar tweaks ("loads cannot have `Release` **and** `AcqRel` ordering" => "loads cannot have `Release` **or** `AcqRel` ordering")
- function renames (`match_ordering_def_path` => `matches_ordering_def_path`),
- avoiding clippy-specific helper methods that don't exist in rustc_lint and didn't seem worth adding for this case (for example `cx.struct_span_lint` vs clippy's `span_lint_and_help` helper).
## Potential issues
(This is just about the code in this PR, not conceptual issues with the lint or anything)
1. I'm not sure if I should have used a diagnostic item for `Ordering` and its variants (I couldn't figure out how really, so if I should do this some pointers would be appreciated).
- It seems possible that failing to do this might possibly mean there are more cases this lint would miss, but I don't really know how `match_def_path` works and if it has any pitfalls like that, so maybe not.
2. I *think* I deprecated the lint in clippy (CC `@flip1995` who asked to be notified about clippy changes in the future in [this comment](https://github.com/rust-lang/rust/pull/75671#issuecomment-718731659)) but I'm not sure if I need to do anything else there.
- I'm kind of hoping CI will catch if I missed anything, since `x.py test src/tools/clippy` fails with a lot of errors with and without my changes (and is probably a nonsense command regardless). Running `cargo test` from src/tools/clippy also fails with unrelated errors that seem like refactorings that didnt update clippy? So, honestly no clue.
3. I wasn't sure if the description/example I gave good. Hopefully it is. The example is less thorough than the one from clippy here: https://rust-lang.github.io/rust-clippy/master/index.html#invalid_atomic_ordering. Let me know if/how I should change it if it needs changing.
4. It pulls in the `if_chain` crate. This crate was already used in clippy, and seems like it's used elsewhere in rustc, but I'm willing to rewrite it to not use this if needed (I'd prefer not to, all things being equal).
- Deprecate clippy::invalid_atomic_ordering
- Use rustc_diagnostic_item for the orderings in the invalid_atomic_ordering lint
- Reduce code duplication
- Give up on making enum variants diagnostic items and just look for
`Ordering` instead
I ran into tons of trouble with this because apparently the change to
store HIR attrs in a side table also gave the DefIds of the
constructor instead of the variant itself. So I had to change
`matches_ordering` to also check the grandparent of the defid as well.
- Rename `atomic_ordering_x` symbols to just the name of the variant
- Fix typos in checks - there were a few places that said "may not be
Release" in the diagnostic but actually checked for SeqCst in the lint.
- Make constant items const
- Use fewer diagnostic items
- Only look at arguments after making sure the method matches
This prevents an ICE when there aren't enough arguments.
- Ignore trait methods
- Only check Ctors instead of going through `qpath_res`
The functions take values, so this couldn't ever be anything else.
- Add if_chain to allowed dependencies
- Fix grammar
- Remove unnecessary allow
Manual map 7413
fixes: #7413
This only fixes the specific problem from #7413, not the general case. The full fix requires interacting with the borrow checker to determine the lifetime of all the borrows made in the function. I'll open an issue about it later.
changelog: Don't suggest using `map` when the option is borrowed in the match, and also consumed in the arm.
changelog: Locals declared within the would-be closure will not prevent the closure from being suggested in `manual_map` and `map_entry`
Add `unwrap_or_else_default` lint
---
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: Add a new [`unwrap_or_else_default`] style lint. This will catch `unwrap_or_else(Default::default)` on Result and Option and suggest `unwrap_or_default()` instead.
`never_loop`: suggest using an `if let` instead of a `for` loop
changelog: suggest using an `if let` statement instead of a `for` loop that [`never_loop`]s
Fixes#7537, r? `@camsteffen.`
Cleanup usage of `span_to_snippet` and `LintContext::sess`
- avoid using `SourceMap::span_to_snippet` directly and use `clippy_utils::source::snippet_opt` instead
- don't use `LintContext::sess()` on `EarlyContext`s which have a `sess` field directly available, saving the import of `LintContext`
changelog: none
Don't emit `too_many_lines` for closures
changelog: don't emit the [`too_many_lines`] lint inside closures to avoir duplicated diagnostics.
Fixes#7517.
rustc: Replace `HirId`s with `LocalDefId`s in `AccessLevels` tables
and passes using those tables - primarily privacy checking, stability checking and dead code checking.
All these passes work with definitions rather than with arbitrary HIR nodes.
r? `@cjgillot`
cc `@lambinoo` (#87487)
rfc3052 followup: Remove authors field from Cargo manifests
Since RFC 3052 soft deprecated the authors field, hiding it from
crates.io, docs.rs, and making Cargo not add it by default, and it is
not generally up to date/useful information for contributors, we may as well
remove it from crates in this repo.
Since RFC 3052 soft deprecated the authors field anyway, hiding it from
crates.io, docs.rs, and making Cargo not add it by default, and it is
not generally up to date/useful information, we should remove it from
crates in this repo.
Update lint documentation to use markdown headlines
This PR updates all lint documentation to use markdown headlines. It additionally removed the *Known problems* section for lints without any problems. I've double-checked all automatic replacements, but a second pair of eyes is definitely appreciated!
I wasn't sure when you wanted to switch to the new metadata collection tomorrow, I therefore prepared this PR today. And that's it this is a standalone PR to keep the other related PRs reviewable.
changelog: none
r? `@flip1995`
cc: #7172
Note: This should be merged with the other metadata collection related PRs.
Switch CI to new metadata collection
r? `@xFrednet`
Things we have to keep in mind:
- This removes the template files and the scripts used for deployment from the checkout. This was added in #5517. I don't think we ever needed those there. Not sure though.
- ~~As a result, we can't remove the python scripts yet. We have to wait until this hits a stable Clippy release.~~ I'll just break the next stable deploy and do it by hand once.
- This should be merged together with #7279. Me and `@xFrednet` will coordinate the switch
- ...?
I still have to try out some things:
- [x] Is it worth caching? Yes
- [x] ~~Is it worth to do a release build?~~ Nope
- [x] Does it actually work? With a few changes, yes
- [ ] ...?
changelog: Clippy now uses a lint to generate its documentation 🎉
Changes included:
- Minimum adaption to the new `lints.json` format
- Fixing filtering for the new `lints.json` format; hardcoding the
lint groups in the index
- Recreating the original doc styling for the new format
- Fixed sytax highlighting for rust,ignore code blocks
- Fixed markdown table extraction in the metadata collector and
fixed lint level output
- Adding the additional information row for lints
- Changed the website title to Clippy's lint list
- Flexing the website for mobile users
- Added (?) references for lint levels and groups
- Making deprecated lints look dead
- Removed JS code block language extraction in favor of a rust
implementation `rust-clippy#7352`
- Added the suspicious lint group to the lint list
- Remove trailing whitespaces from index.html
- Fix code highlighting
- Use default value if the docVersion is empty
Co-authored-by: Philipp Krones <hello@philkrones.com>
This updates all the deploy scripts and the deploy workflow.
The deploy workflow now runs the metadata collector to collect the lint
documentation. It also changes the files that are checked out in the
deploy workflow from master and adds an explanation why we have to do
this.
Fix docs of disallowed_type
Add ability to name primitive types without import path
Move primitive resolution to clippy_utils path_to_res fn
Refactor Res matching, fix naming and docs from review
Use tcx.def_path_str when emitting the lint
Use diagnostic items where possible
Clippy still uses a bunch of paths in places that could easily use already defined diagnostic items. This PR updates all references to such paths and also removes a bunch of them that are no longer needed after this cleanup.
Some paths are also used to construct new paths and can therefore not be removed that easily. I've added a doc comment to those instances that recommends the use of the diagnostic item where possible.
And that's it, cleaning crew signing off 🧹🗑️
---
changelog: none
(only internal improvements)
cc: #5393
Prefer a code snipped over formatting the self type (`new_without_default`)
Fixes: rust-lang/rust-clippy#7220
changelog: [`new_without_default`]: The `Default` impl block type doesn't use the full type path qualification
Have a nice day to everyone reading this 🙃
Some `clippy::author` improvements
changelog: none
* Use `Debug` instead of re-implementing it for some things
* Fix block trailing expression handing
* Don't double print on stmt/expr with `#[clippy::author]` attribute
similar_names: No longer suggest inserting or appending an underscore
changelog: [`similar_names`] lint no longer suggests to insert or add an underscore to "fix" too similar names
New lint: [`self_named_constructor`]
Adds the `self_named_constructor` lint for detecting when an implemented method has the same name as the type it is implemented for.
changelog: [`self_named_constructor`]
closes: #7142
Remove refs from Pat slices
Changes `PatKind::Or(&'hir [&'hir Pat<'hir>])` to `PatKind::Or(&'hir [Pat<'hir>])` and others. This is more consistent with `ExprKind`, saves a little memory, and is a little easier to use.
FP fix and documentation for `branches_sharing_code` lint
Closesrust-lang/rust-clippy#7369
Related rust-lang/rust-clippy#7452 I'm still thinking about the best way to fix this. I could simply add another visitor to ensure that the moved expressions don't modify values being used in the condition, but I'm not totally happy with this due to the complexity. I therefore only documented it for now
changelog: [`branches_sharing_code`] fixed false positive where block expressions would sometimes be ignored.
Fix internal `default_hash_types` lint to use resolved path
I run into false positives now and then (mostly in Clippy) when I want to name some util after HashMap.
Don't suggest doc(hidden) or unstable variants in wildcard lint
Clippy's wildcard lint would suggest doc(hidden) and unstable variants for non_exhaustive enums, even though those aren't part of the public interface (yet) and should only be matched on using a `_`, just like potential future additions to the enum. There was already some logic to exclude a *single* doc(hidden) variant. This extends that to all hidden variants, and also hides `#[unstable]` variants.
See https://github.com/rust-lang/rust/pull/85746#issuecomment-868886893
This PR includes https://github.com/rust-lang/rust-clippy/pull/7406 as the first commit.
Here's the diff that this PR adds on top of that PR: https://github.com/m-ou-se/rust-clippy/compare/std-errorkind...m-ou-se:doc-hidden-variants
---
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: No longer suggest unstable and doc(hidden) variants in wildcard lint. wildcard_enum_match_arm, match_wildcard_for_single_variants
This fixes a bug where match_wildcard_for_single_variants produced a
bad suggestion where besides the missing variant, one or more hidden
variants were left.
This also adds tests to the ui-tests match_wildcard_for_single_variants
and wildcard_enum_match_arm to make sure that the correct suggestion is
produced.
New lint: `disallowed_script_idents`
This PR implements a new lint to restrict locales that can be used in the code,
as proposed in #7376.
Current concerns / unresolved questions:
- ~~Mixed usage of `script` (as a Unicode term) and `locale` (as something that is easier to understand for the broad audience). I'm not sure whether these terms are fully interchangeable and whether in the current form it is more confusing than helpful.~~ `script` is now used everywhere.
- ~~Having to mostly copy-paste `AllowedScript`. Probably it's not a big problem, as the list of scripts is standardized and is unlikely to change, and even if we'd stick to the `unicode_script::Script`, we'll still have to implement custom deserialization, and I don't think that it will be shorter in terms of the amount of LoC.~~ `unicode::Script` is used together with a filtering deserialize function.
- Should we stick to the list of "recommended scripts" from [UAX #31](http://www.unicode.org/reports/tr31/#Table_Recommended_Scripts) in the configuration?
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: ``[`disallowed_script_idents`]``
r? `@Manishearth`
Improve lint message for match-same-arms lint
fixes#7331
Follow-up to #7377
This PR improves the lint message for `match-same-arms` lint and adds `todo!(..)` example to the lint docs.
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: None
Do not spawn blacklisted_name lint in test context
---
fixed#7305
*Please write a short comment explaining your change (or "none" for internal only changes)*
changelog: `blacklisted_name` lint is not spawned in the test context anymore.
Fix detecting of the 'test' attribute
Update UI test to actually check that warning is not triggered in the test code
Fix approach for detecting the test module
Add nested test case
Remove code duplication by extracting 'is_test_module_or_function' into 'clippy_utils'
Cleanup the code
Add import_rename lint, this adds a field on the Conf struct
fixes#7276
changelog: Add ``[`import_rename`]`` a lint that enforces import renaming defined in the config file.
Fixed broken deploy script due to multiline configuration docs
The deploy script on master currently runs into an error (See [log](https://github.com/rust-lang/rust-clippy/runs/2865828873)) due to the new configuration documentation added in #7299. The current documentation collection for the configuration macro sadly doesn't support multiline doc comments. This will be changes in the future with the new metadata collector tracked in #7172 For now we have to use `<br>` inside doc comments to add paragraphs.
This PR restricts `define_Conf!` macro to single lines and adds a comment explaining the reasoning behind it. It also adjusted the actual document parsing to fix a bug. (The parsing was automatically stopping on the first curly bracket, even if it was part of a doc comment).
changelog: none
Problem:
for code like
````
fn main() {
println!("Hello, world!");
}
````
clippy will issue a warning to use a clippy.toml option instead:
````
warning: lint `clippy::wrong_pub_self_convention` has been removed: set the `avoid_breaking_exported_api` config option to `false` to enable the `wrong_self_convention` lint for public items
--> src/main.rs:2:9
|
2 | #![warn(clippy::wrong_pub_self_convention)]
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[warn(renamed_and_removed_lints)]` on by default
````
But using the lint name as seen in the warning message
echo "avoid_breaking_exported_api = true\n" > clippy.toml
Will cause an error:
````
error: error reading Clippy's configuration file `/tmp/clippytest/clippy.toml`: unknown field `avoid_breaking_exported_api`, expected one of `avoid-breaking-exported-api`, ...
````
Replace the underscores with dashes in the deprecation message.
changelog: avoid_breaking_exported_api: suggest correct clippy config toml option in the deprecation message
Add macro_braces lint to check for irregular brace use in certain macros
The name is a bit long but this sounds good as `#[allow(unconventional_macro_braces)]` and it seems more clear that we are talking about the macro call not macro definitions, any feedback let me know. Thanks!
fixes#7278
changelog: Add ``[`unconventional_macro_braces`]`` lint that checks for uncommon brace usage with macros.
Remove some last remants of {push,pop}_unsafe!
These macros have already been removed, but there was still some code handling these macros. That code is now removed.
Rename unconventional -> nonstandard, add config field
Add standard_macro_braces fields so users can specify macro names and
brace combinations to lint for in the clippy.toml file.
Fix errors caused by nonstandard_macro_braces in other lint tests
Fix users ability to override the default nonstandard macro braces
Add type position macros impl `check_ty`
Remove requirement of fully qualified path for disallowed_method/type
changelog: Remove the need for fully qualified paths in disallowed_method and disallowed_type
After fixing this issue in [import_rename](https://github.com/rust-lang/rust-clippy/pull/7300#discussion_r650127720) I figured I'd fix it for these two.
If this does in fact fix the **Known problems:** section I was planning to remove them from both lints after confirmation.
Vec extend to append
This PR adds a check to suggest changes of vector from
```
vec.extend(other_vec.drain(..))
```
could be written as
```
vec![].append(&mut vec![]);
```
changelog: Add vec_extend_to_append lint
issue: #7209
Don't trigger `field_reassign_with_default` in macros
Fixes#7155
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.
changelog: [`field_reassign_with_default`] No longer triggers in macros
---
No that our reviewing queue is under control, I want to start hacking on Clippy myself again. Starting with an easy issue to get back in :)
