clippy_lints: Do not warn against Box parameter in C FFI
changelog: [`boxed_local`]: don't lint in `extern fn` arguments
Fixes#5542.
When using C FFI, to handle pointers in parameters it is needed to
declare them as `Box` in its Rust-side signature. However, the current
linter warns against the usage of Box stating that "local variable
doesn't need to be boxed here".
This commit fixes it by ignoring functions whose Abi is C.
Downgrade string_lit_as_bytes to nursery
Between #1402 (regarding `to_owned`) and #4494 (regarding `impl Read`), as well as other confusion I've seen hit in my work codebase involving string_lit_as_bytes (`"...".as_bytes().into()`), I don't think this lint is at a quality to be enabled by default.
I would consider re-enabling this lint after it is updated to understand when the surrounding type information is sufficient to unsize `b"..."` to &\[u8\] without causing a type error.
As currently implemented, this lint is pushing people to write `&b"_"[..]` which is not an improvement over `"_".as_bytes()` as far as I am concerned.
---
changelog: Remove string_lit_as_bytes from default set of enabled lints
Fix unicode regexen with bytes::Regex
fixes#6005
The rationale for this is that since we wrote that lint, `bytes::Regex` was extended to be able to use unicode character classes.
---
changelog: [`invalid_regex`]: allow unicode character classes in bytes regex.
Fixes#5542.
When using C FFI, to handle pointers in parameters it is needed to
declare them as `Box` in its Rust-side signature. However, the current
linter warns against the usage of Box stating that "local variable
doesn't need to be boxed here".
This commit fixes it by ignoring functions whose Abi is Cdecl.
Downgrade rc_buffer to restriction
I think Arc\<Vec\<T\>\> and Arc\<String\> and similar are a totally reasonable data structure, as observed by others in the comments on [#6044](https://github.com/rust-lang/rust-clippy/pull/6044#event-3799579830) as well. Doing `Arc::make_mut(&mut self.vec).push(...)` or `Arc::make_mut(&mut self.string).push_str("...")` is a terrific and well performing copy-on-write pattern. Linting this with an enabled-by-default <kbd>performance</kbd> lint strikes me as an unacceptable false positive balance.
As of #6090 the documentation of this lint now contains:
> **Known problems:** This pattern can be desirable ...
which should indicate that we shouldn't be linting against correct, reasonable, well-performing patterns with an enabled-by-default lint.
Mentioning #6044, #6090.
r? `@yaahc,` who reviewed the lint.
---
changelog: Remove rc_buffer from default set of enabled lints
Fix LitKind's byte buffer to use refcounted slice
While working on adding a new lint for clippy (see https://github.com/rust-lang/rust-clippy/pull/6044) for avoiding shared ownership of "mutable buffer" types (such as using `Rc<Vec<T>>` instead of `Rc<[T]>`), I noticed a type exported from rustc_ast and used by clippy gets caught by the lint. This PR fixes the exported type.
This PR includes the actual change to clippy too, but I will open a PR directly against clippy for that part (although it will currently fail to build there).
unnecessary sort by: avoid dereferencing the suggested closure parameter
This change tries to simplify the solution for problematic cases but is less restrictive than #6006.
* We can't dereference shared references to non-Copy types, so the new suggestion does not do that. Note that this implies that the suggested closure parameter will be a reference.
* We can't take a reference to the closure parameter in the returned key, so we don't lint in those cases. This can happen either because the key borrows from the parameter (e.g. `|a| a.borrows()`), or because we suggest `|a| Reverse(a)`. If we did we would hit this error:
```
error: lifetime may not live long enough
--> /home/ebroto/src/ebroto-clippy/tests/ui/unnecessary_sort_by.fixed:19:25
|
19 | vec.sort_by_key(|b| Reverse(b));
| -- ^^^^^^^^^^ returning this value requires that `'1` must outlive `'2`
| ||
| |return type of closure is Reverse<&'2 isize>
| has type `&'1 isize`
error: aborting due to previous error
```
Note that Clippy does not currently have the (MIR-based) machinery necessary to check that what is borrowed is actually the closure parameter.
changelog: [`unnecessary_sort_by`]: avoid dereferencing the suggested closure parameter
Fixes#6001
Do not lint float fractions in `mistyped_literal_suffixes`
As suggested in https://github.com/rust-lang/rust-clippy/issues/4706#issuecomment-544797928, the fractional part is now ignored (the integer part is checked instead).
Fixes: #4706
changelog: `mistyped_literal_suffixes` no longer warns on the fractional part of a float (e.g. 713.23_64)
fix a false positive in two `interior_mutable_const` lints where a constant with enums gets linted
even if it uses a clearly unfrozen variant. Note that the code uses the MIR interpreter, which
the author of #3962 thought unlikely to be a solution. This might be over-engineering;
but, I think it's important to be able to work with the 'http' crate (#3825).
Lint for invisible Unicode characters other than ZWSP
This PR extends the existing `zero_width_space` lint to look for other invisible characters as well (in this case, `\\u{ad}` soft hyphen.
I feel like this lint is the logical place to add the check, but I also realize the lint name is not particularly flexible, but I also understand that it shouldn't be renamed for compatibility reasons.
Open questions:
- What other characters should trigger the lint?
- What should be done with the lint name?
- How to indicate the change in functionality?
Motivation behind this PR: https://github.com/rust-lang/rust/issues/77417 - I managed to shoot myself in the foot by an invisible character pasted into my test case.
changelog: rename [`zero_width_space`] to [`invisible_characters`] and add SHY and WJ to the list.
Don't emit a lint for the suggestion leading to errors in `needless_range_loop`
Fix#5945
changelog: Don't emit a lint for the suggestion leading to errors in `needless_range_loop`
New Lint: disallowed_method
Fixes https://github.com/rust-lang/rust-clippy/issues/6073
I added a `disallowed_method` lint that works similar to `blacklisted_name`, e.g allows config-defined disallowed method calls.
This lint allows advanced users to patch in undesired method calls for specific projects. It basically uses the DefId of method calls and matches it to parsed strings. Is there an alternative approach that could use more easily digestible configuration strings?
All tests pass as expected except for a linting error on `ui/redundant_pattern_matching_option`, which I am not sure how to resolve since I don't think I changed anything affecting it.
changelog: Add disallowed_method lint to warn on user-defined method calls
Fix FP in `print_stdout`
Fix#6041
This lint shouldn't be emitted in `build.rs` as `println!` and `print!` are used for the build script.
changelog: none
Allow a unique name to be assigned to dataflow graphviz output
Previously, if the same analysis were invoked multiple times in a single compilation session, the graphviz output for later runs would overwrite that of previous runs. Allow callers to add a unique identifier to each run so this can be avoided.
Revert: or_fun_call should lint calls to `const fn`s with no args
The changes in #5889 and #5984 were done under the incorrect assumption that a `const fn` with no args was guaranteed to be evaluated at compile time. A `const fn` is only guaranteed to be evaluated at compile time if it's inside a const context (the initializer of a `const` or a `static`).
See this [zulip conversation](https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Common.20misconception.3A.20.60const.20fn.60.20and.20its.20effect.20on.20codegen/near/208059113) for more details on this common misconception.
Given that none of the linted methods by `or_fun_call` can be called in const contexts, the lint should make no exceptions.
changelog: [`or_fun_call`] lints again calls to `const fn` with no args
Fix a FP in `explicit_counter_loop`
Fixes#4677 and #6074
Fix a false positive in `explicit_counter_loop` where the loop counter is used after incremented, adjust the test so that counters are incremented at the end of the loop and add the test for this false positive.
---
changelog: Fix a false positive in `explicit_counter_loop` where the loop counter is used after incremented
Add `rc_buffer` lint for checking Rc<String> and friends
Fixes#2623
This is a bit different from the original PR attempting to implement this type of lint. Rather than linting against converting into the unwanted types, this PR lints against declaring the unwanted type in a struct or function definition.
I'm reasonably happy with what I have here, although I used the fully qualified type names for the Path and OsString suggestions, and I'm not sure if I should have just used the short versions instead, even if they might not have been declared via use.
Also, I don't know if "buffer type" is the best way to put it or not. Alternatively I could call it a "growable type" or "growable buffer type", but I was thinking of PathBuf when I started making the lint.
changelog: Add `rc_buffer` lint
Fix a false positive in `explicit_counter_loop` where the loop counter is used after incremented,
adjust the test so that counters are incremented at the end of the loop
and add the test for this false positive.
Stabilize some Result methods as const
Stabilize the following methods of Result as const:
- `is_ok`
- `is_err`
- `as_ref`
A test is also included, analogous to the test for `const_option`.
These methods are currently const under the unstable feature `const_result` (tracking issue: #67520).
I believe these methods to be eligible for stabilization because of the stabilization of #49146 (Allow if and match in constants) and the trivial implementations, see also: [PR#75463](https://github.com/rust-lang/rust/pull/75463) and [PR#76135](https://github.com/rust-lang/rust/pull/76135).
Note: these methods are the only methods currently under the `const_result` feature, thus this PR results in the removal of the feature.
Related: #76225
* stop linting associated types and generic type parameters
* start linting ones in trait impls
whose corresponding definitions in the traits are generic
* remove the `is_copy` check
as presumably the only purpose of it is to allow
generics with `Copy` bounds as `Freeze` is internal
and generics are no longer linted
* remove the term 'copy' from the tests
as being `Copy` no longer have meaning
option_if_let_else - distinguish pure from impure else expressions
Addresses partially #5821.
changelog: improve the lint `option_if_let_else`. Suggest `map_or` or `map_or_else` based on the else expression purity.
Extend invalid_atomic_ordering for compare_exchange{,_weak} and fetch_update
changelog: The invalid_atomic_ordering lint can now detect misuse of `compare_exchange`, `compare_exchange_weak`, and `fetch_update`.
---
I was surprised not to find an issue or existing support here, since these are the functions which are always hardest to get the ordering right on for me (as the allowed orderings for `fail` depend on the `success` parameter).
I believe this lint now covers all atomic methods which care about their ordering now, but I could be wrong.
Hopefully I didn't forget to do anything for the PR!
{print,write}-with-newline: do not suggest empty format string
changelog: do not suggest empty format strings in `print-with-newline` and `write-with-newline`
clarify margin of error in wording of float comparison operator lint messages
fixes#6040
changelog: change wording of float comparison operator to make margin of error less ambiguous
Add map_err_ignore lint
In a large code base a lot of times errors are ignored by using something like:
```rust
foo.map_err(|_| Some::Enum)?;
```
This drops the original error in favor of a enum that will not have the original error's context. This lint helps catch throwing away the original error in favor of an enum without its context.
---
*Please keep the line below*
changelog: Added map_err_ignore lint
Add a new lint, `manual-strip`, that suggests using the `str::strip_prefix`
and `str::strip_suffix` methods introduced in Rust 1.45 when the same
functionality is performed 'manually'.
Closes#5734
Attach tokens to all AST types used in `Nonterminal`
We perform token capturing when we have outer attributes (for nonterminals that support attributes - e.g. `Stmt`), or when we parse a `Nonterminal` for a `macro_rules!` argument. The full list of `Nonterminals` affected by this PR is:
* `NtBlock`
* `NtStmt`
* `NtTy`
* `NtMeta`
* `NtPath`
* `NtVis`
* `NtLiteral`
Of these nonterminals, only `NtStmt` and `NtLiteral` (which is actually just an `Expr`), support outer attributes - the rest only ever have token capturing perform when they match a `macro_rules!` argument.
This makes progress towards solving https://github.com/rust-lang/rust/issues/43081 - we now collect tokens for everything that might need them. However, we still need to handle `#[cfg]`, inner attributes, and misc pretty-printing issues (e.g. #75734)
I've separated the changes into (mostly) independent commits, which could be split into individual PRs for each `Nonterminal` variant. The purpose of having them all in one PR is to do a single Crater run for all of them.
Most of the changes in this PR are trivial (adding `tokens: None` everywhere we construct the various AST structs). The significant changes are:
* `ast::Visibility` is changed from `type Visibility = Spanned<VisibilityKind>` to a `struct Visibility { kind, span, tokens }`.
* `maybe_collect_tokens` is made generic, and used for both `ast::Expr` and `ast::Stmt`.
* Some of the statement-parsing functions are refactored so that we can capture the trailing semicolon.
* `Nonterminal` and `Expr` both grew by 8 bytes, as some of the structs which are stored inline (rather than behind a `P`) now have an `Option<TokenStream>` field. Hopefully the performance impact of doing this is negligible.
Add lint panic in result
### Change
Adding a new "restriction" lint that will emit a warning when using "panic", "unimplemented" or "unreachable" in a function of type option/result.
### Motivation
Some codebases must avoid crashes at all costs, and hence functions of type option/result must return an error instead of crashing.
### Test plan
Running:
TESTNAME=panic_in_result cargo uitest ---
changelog: none
Improve the "known problems" section of `interior_mutable_key`
* Remove the mention to `Rc` and `Arc` as these are `Freeze` (despite my intuition) so the lint correctly handles already.
* Instead, explain what could cause a false positive, and mention `bytes` as an example.
---
changelog: Improved the "known problems" section of `interior_mutable_key`
improve the suggestion of the lint `unit-arg`
Fixes#5823Fixes#6015
Changes
```
help: move the expression in front of the call...
|
3 | g();
|
help: ...and use a unit literal instead
|
3 | o.map_or((), |i| f(i))
|
```
into
```
help: move the expression in front of the call and replace it with the unit literal `()`
|
3 | g();
| o.map_or((), |i| f(i))
|
```
changelog: improve the suggestion of the lint `unit-arg`
Add CONST_ITEM_MUTATION lint
Fixes#74053Fixes#55721
This PR adds a new lint `CONST_ITEM_MUTATION`.
Given an item `const FOO: SomeType = ..`, this lint fires on:
* Attempting to write directly to a field (`FOO.field = some_val`) or
array entry (`FOO.array_field[0] = val`)
* Taking a mutable reference to the `const` item (`&mut FOO`), including
through an autoderef `FOO.some_mut_self_method()`
The lint message explains that since each use of a constant creates a
new temporary, the original `const` item will not be modified.
* Remove the mention to `Rc` and `Arc` as these are `Freeze`
so the lint correctly handles already.
* Instead, explain what could cause a false positive,
and mention `bytes` as an example.
We no longer lint assignments to const item fields in the
`temporary_assignment` lint, since this is now covered by the
`CONST_ITEM_MUTATION` lint.
Additionally, we `#![allow(const_item_mutation)]` in the
`borrow_interior_mutable_const.rs` test. Clippy UI tests are run with
`-D warnings`, which seems to cause builtin lints to prevent Clippy
lints from running.
Support dataflow problems on arbitrary lattices
This PR implements last of the proposed extensions I mentioned in the design meeting for the original dataflow refactor. It extends the current dataflow framework to work with arbitrary lattices, not just `BitSet`s. This is a prerequisite for dataflow-enabled MIR const-propagation. Personally, I am skeptical of the usefulness of doing const-propagation pre-monomorphization, since many useful constants only become known after monomorphization (e.g. `size_of::<T>()`) and users have a natural tendency to hand-optimize the rest. It's probably worth exprimenting with, however, and others have shown interest cc `@rust-lang/wg-mir-opt.`
The `Idx` associated type is moved from `AnalysisDomain` to `GenKillAnalysis` and replaced with an associated `Domain` type that must implement `JoinSemiLattice`. Like before, each `Analysis` defines the "bottom value" for its domain, but can no longer override the dataflow join operator. Analyses that want to use set intersection must now use the `lattice::Dual` newtype. `GenKillAnalysis` impls have an additional requirement that `Self::Domain: BorrowMut<BitSet<Self::Idx>>`, which effectively means that they must use `BitSet<Self::Idx>` or `lattice::Dual<BitSet<Self::Idx>>` as their domain.
Most of these changes were mechanical. However, because a `Domain` is no longer always a powerset of some index type, we can no longer use an `IndexVec<BasicBlock, GenKillSet<A::Idx>>>` to store cached block transfer functions. Instead, we use a boxed `dyn Fn` trait object. I discuss a few alternatives to the current approach in a commit message.
The majority of new lines of code are to preserve existing Graphviz diagrams for those unlucky enough to have to debug dataflow analyses. I find these diagrams incredibly useful when things are going wrong and considered regressing them unacceptable, especially the pretty-printing of `MovePathIndex`s, which are used in many dataflow analyses. This required a parallel `fmt` trait used only for printing dataflow domains, as well as a refactoring of the `graphviz` module now that we cannot expect the domain to be a `BitSet`. Some features did have to be removed, such as the gen/kill display mode (which I didn't use but existed to mirror the output of the old dataflow framework) and line wrapping. Since I had to rewrite much of it anyway, I took the opportunity to switch to a `Visitor` for printing dataflow state diffs instead of using cursors, which are error prone for code that must be generic over both forward and backward analyses. As a side-effect of this change, we no longer have quadratic behavior when writing graphviz diagrams for backward dataflow analyses.
r? `@pnkfelix`
Fix FP in `same_item_push`
Don't emit a lint when the pushed item doesn't have Clone trait
Fix#5979
changelog: Fix FP in `same_item_push` not to emit a lint when the pushed item doesn't have Clone trait
useless_attribute: Permit wildcard_imports and enum_glob_use
Fixes#5918
changelog: `useless_attribute`: Permit `wildcard_imports` and `enum_glob_use` on `use` items
default_trait_access: Fix wrong suggestion
https://github.com/rust-lang/rust-clippy/issues/5975#issuecomment-683751131
> I think the underlying problem is clippy suggests code with complete parameters, not clippy triggers this lint even for complex types. AFAIK, If code compiles with `Default::default`, it doesn't need to specify any parameters, as type inference is working. (So, in this case, `default_trait_access` should suggest `RefCell::default`.)
Fixes#5975Fixes#5990
changelog: `default_trait_access`: fixed wrong suggestion
Add a lint for an async block/closure that yields a type that is itself awaitable.
This catches bugs of the form
tokio::spawn(async move {
let f = some_async_thing();
f // Oh no I forgot to await f so that work will never complete.
});
See the two XXXkhuey comments and the unfixed `_l` structure for things that need more thought.
*Please keep the line below*
changelog: none
or_fn_call: ignore nullary associated const fns
The fix in #5889 was missing associated functions.
changelog: Ignore also `const fn` methods in [`or_fun_call`]
Fixes#5693
This catches bugs of the form
tokio::spawn(async move {
let f = some_async_thing();
f // Oh no I forgot to await f so that work will never complete.
});
Fix FP in `to_string_in_display`
Don't emit a lint when `.to_string()` on anything that is not `self`
Fix#5967
changelog: Fix FP in `to_string_in_display` when calling `.to_string()` on anything that is not `self`
