diff options
62 files changed, 2599 insertions, 387 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 851a5aa8789..fd7232e280f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5132,6 +5132,7 @@ Released 2018-09-13 [`misnamed_getters`]: https://rust-lang.github.io/rust-clippy/master/index.html#misnamed_getters [`misrefactored_assign_op`]: https://rust-lang.github.io/rust-clippy/master/index.html#misrefactored_assign_op [`missing_assert_message`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_assert_message +[`missing_asserts_for_indexing`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_asserts_for_indexing [`missing_const_for_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn [`missing_docs_in_private_items`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_docs_in_private_items [`missing_enforced_import_renames`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_enforced_import_renames diff --git a/Cargo.toml b/Cargo.toml index cd04c78ef9e..2d8b590dbe3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,7 @@ tempfile = { version = "3.2", optional = true } termize = "0.1" [dev-dependencies] -ui_test = "0.17.0" +ui_test = "0.20" tester = "0.9" regex = "1.5" toml = "0.7.3" diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index daaefd06a97..b02457307d7 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -14,8 +14,11 @@ - [Basics](development/basics.md) - [Adding Lints](development/adding_lints.md) - [Defining Lints](development/defining_lints.md) + - [Writing tests](development/writing_tests.md) - [Lint Passes](development/lint_passes.md) + - [Emitting lints](development/emitting_lints.md) - [Type Checking](development/type_checking.md) + - [Trait Checking](development/trait_checking.md) - [Method Checking](development/method_checking.md) - [Macro Expansions](development/macro_expansions.md) - [Common Tools](development/common_tools_writing_lints.md) diff --git a/book/src/development/emitting_lints.md b/book/src/development/emitting_lints.md new file mode 100644 index 00000000000..a12f6aa91b3 --- /dev/null +++ b/book/src/development/emitting_lints.md @@ -0,0 +1,217 @@ +# Emitting a lint + +Once we have [defined a lint](defining_lints.md), written [UI +tests](writing_tests.md) and chosen [the lint pass](lint_passes.md) for the lint, +we can begin the implementation of the lint logic so that we can emit it and +gradually work towards a lint that behaves as expected. + +Note that we will not go into concrete implementation of a lint logic in this +chapter. We will go into details in later chapters as well as in two examples of +real Clippy lints. + +To emit a lint, we must implement a pass (see [Lint Passes](lint_passes.md)) for +the lint that we have declared. In this example we'll implement a "late" lint, +so take a look at the [LateLintPass][late_lint_pass] documentation, which +provides an abundance of methods that we can implement for our lint. + +```rust +pub trait LateLintPass<'tcx>: LintPass { + // Trait methods +} +``` + +By far the most common method used for Clippy lints is [`check_expr` +method][late_check_expr], this is because Rust is an expression language and, +more often than not, the lint we want to work on must examine expressions. + +> _Note:_ If you don't fully understand what expressions are in Rust, take a +> look at the official documentation on [expressions][rust_expressions] + +Other common ones include the [`check_fn` method][late_check_fn] and the +[`check_item` method][late_check_item]. + +### Emitting a lint + +Inside the trait method that we implement, we can write down the lint logic and +emit the lint with suggestions. + +Clippy's [diagnostics] provides quite a few diagnostic functions that we can use +to emit lints. Take a look at the documentation to pick one that suits your +lint's needs the best. Some common ones you will encounter in the Clippy +repository includes: + +- [`span_lint`]: Emits a lint without providing any other information +- [`span_lint_and_note`]: Emits a lint and adds a note +- [`span_lint_and_help`]: Emits a lint and provides a helpful message +- [`span_lint_and_sugg`]: Emits a lint and provides a suggestion to fix the code +- [`span_lint_and_then`]: Like `span_lint`, but allows for a lot of output + customization. + +```rust +impl<'tcx> LateLintPass<'tcx> for LintName { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + // Imagine that `some_lint_expr_logic` checks for requirements for emitting the lint + if some_lint_expr_logic(expr) { + span_lint_and_help( + cx, // < The context + LINT_NAME, // < The name of the lint in ALL CAPS + expr.span, // < The span to lint + "message on why the lint is emitted", + None, // < An optional help span (to highlight something in the lint) + "message that provides a helpful suggestion", + ); + } + } +} +``` + +> Note: The message should be matter of fact and avoid capitalization and +> punctuation. If multiple sentences are needed, the messages should probably be +> split up into an error + a help / note / suggestion message. + +## Suggestions: Automatic fixes + +Some lints know what to change in order to fix the code. For example, the lint +[`range_plus_one`][range_plus_one] warns for ranges where the user wrote `x..y + +1` instead of using an [inclusive range][inclusive_range] (`x..=y`). The fix to +this code would be changing the `x..y + 1` expression to `x..=y`. **This is +where suggestions come in**. + +A suggestion is a change that the lint provides to fix the issue it is linting. +The output looks something like this (from the example earlier): + +```text +error: an inclusive range would be more readable + --> $DIR/range_plus_minus_one.rs:37:14 + | +LL | for _ in 1..1 + 1 {} + | ^^^^^^^^ help: use: `1..=1` +``` + +**Not all suggestions are always right**, some of them require human +supervision, that's why we have [Applicability][applicability]. + +Applicability indicates confidence in the correctness of the suggestion, some +are always right (`Applicability::MachineApplicable`), but we use +`Applicability::MaybeIncorrect` and others when talking about a suggestion that +may be incorrect. + +### Example + +The same lint `LINT_NAME` but that emits a suggestion would look something like this: + +```rust +impl<'tcx> LateLintPass<'tcx> for LintName { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + // Imagine that `some_lint_expr_logic` checks for requirements for emitting the lint + if some_lint_expr_logic(expr) { + span_lint_and_sugg( // < Note this change + cx, + LINT_NAME, + span, + "message on why the lint is emitted", + "use", + format!("foo + {} * bar", snippet(cx, expr.span, "<default>")), // < Suggestion + Applicability::MachineApplicable, + ); + } + } +} +``` + +Suggestions generally use the [`format!`][format_macro] macro to interpolate the +old values with the new ones. To get code snippets, use one of the `snippet*` +functions from `clippy_utils::source`. + +## How to choose between notes, help messages and suggestions + +Notes are presented separately from the main lint message, they provide useful +information that the user needs to understand why the lint was activated. They +are the most helpful when attached to a span. + +Examples: + +### Notes + +```text +error: calls to `std::mem::forget` with a reference instead of an owned value. Forgetting a reference does nothing. + --> $DIR/drop_forget_ref.rs:10:5 + | +10 | forget(&SomeStruct); + | ^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::forget-ref` implied by `-D warnings` +note: argument has type &SomeStruct + --> $DIR/drop_forget_ref.rs:10:12 + | +10 | forget(&SomeStruct); + | ^^^^^^^^^^^ +``` + +### Help Messages + +Help messages are specifically to help the user. These are used in situation +where you can't provide a specific machine applicable suggestion. They can also +be attached to a span. + +Example: + +```text +error: constant division of 0.0 with 0.0 will always result in NaN + --> $DIR/zero_div_zero.rs:6:25 + | +6 | let other_f64_nan = 0.0f64 / 0.0; + | ^^^^^^^^^^^^ + | + = help: consider using `f64::NAN` if you would like a constant representing NaN +``` + +### Suggestions + +Suggestions are the most helpful, they are changes to the source code to fix the +error. The magic in suggestions is that tools like `rustfix` can detect them and +automatically fix your code. + +Example: + +```text +error: This `.fold` can be more succinctly expressed as `.any` +--> $DIR/methods.rs:390:13 + | +390 | let _ = (0..3).fold(false, |acc, x| acc || x > 2); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.any(|x| x > 2)` + | +``` + +### Snippets + +Snippets are pieces of the source code (as a string), they are extracted +generally using the [`snippet`][snippet_fn] function. + +For example, if you want to know how an item looks (and you know the item's +span), you could use `snippet(cx, span, "..")`. + +## Final: Run UI Tests to Emit the Lint + +Now, if we run our [UI test](writing_tests.md), we should see that Clippy now +produces output that contains the lint message we designed. + +The next step is to implement the logic properly, which is a detail that we will +cover in the next chapters. + +[diagnostics]: https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/diagnostics/index.html +[late_check_expr]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_expr +[late_check_fn]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_fn +[late_check_item]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html#method.check_item +[late_lint_pass]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint/trait.LateLintPass.html +[rust_expressions]: https://doc.rust-lang.org/reference/expressions.html +[`span_lint`]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint.html +[`span_lint_and_note`]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_note.html +[`span_lint_and_help`]: https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_help.html +[`span_lint_and_sugg`]: https://doc.rust-lang.org/nightly/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html +[`span_lint_and_then`]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_then.html +[range_plus_one]: https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one +[inclusive_range]: https://doc.rust-lang.org/std/ops/struct.RangeInclusive.html +[applicability]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_errors/enum.Applicability.html +[snippet_fn]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/source/fn.snippet.html +[format_macro]: https://doc.rust-lang.org/std/macro.format.html diff --git a/book/src/development/trait_checking.md b/book/src/development/trait_checking.md new file mode 100644 index 00000000000..fb263922cf8 --- /dev/null +++ b/book/src/development/trait_checking.md @@ -0,0 +1,105 @@ +# Trait Checking + +Besides [type checking](type_checking.md), we might want to examine if +a specific type `Ty` implements certain trait when implementing a lint. +There are three approaches to achieve this, depending on if the target trait +that we want to examine has a [diagnostic item][diagnostic_items], +[lang item][lang_items], or neither. + +## Using Diagnostic Items + +As explained in the [Rust Compiler Development Guide][rustc_dev_guide], diagnostic items +are introduced for identifying types via [Symbols][symbol]. + +For instance, if we want to examine whether an expression implements +the `Iterator` trait, we could simply write the following code, +providing the `LateContext` (`cx`), our expression at hand, and +the symbol of the trait in question: + +```rust +use clippy_utils::is_trait_method; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_span::symbol::sym; + +impl LateLintPass<'_> for CheckIteratorTraitLint { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + let implements_iterator = cx.tcx.get_diagnostic_item(sym::Iterator).map_or(false, |id| { + implements_trait(cx, cx.typeck_results().expr_ty(arg), id, &[]) + }); + if implements_iterator { + // [...] + } + + } +} +``` + +> **Note**: Refer to [this index][symbol_index] for all the defined `Symbol`s. + +## Using Lang Items + +Besides diagnostic items, we can also use [`lang_items`][lang_items]. +Take a look at the documentation to find that `LanguageItems` contains +all language items defined in the compiler. + +Using one of its `*_trait` method, we could obtain the [DefId] of any +specific item, such as `Clone`, `Copy`, `Drop`, `Eq`, which are familiar +to many Rustaceans. + +For instance, if we want to examine whether an expression `expr` implements +`Drop` trait, we could access `LanguageItems` via our `LateContext`'s +[TyCtxt], which provides a `lang_items` method that will return the id of +`Drop` trait to us. Then, by calling Clippy utils function `implements_trait` +we can check that the `Ty` of the `expr` implements the trait: + +```rust +use clippy_utils::implements_trait; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; + +impl LateLintPass<'_> for CheckDropTraitLint { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + let ty = cx.typeck_results().expr_ty(expr); + if cx.tcx.lang_items() + .drop_trait() + .map_or(false, |id| implements_trait(cx, ty, id, &[])) { + println!("`expr` implements `Drop` trait!"); + } + } +} +``` + +## Using Type Path + +If neither diagnostic item nor a language item is available, we can use +[`clippy_utils::paths`][paths] with the `match_trait_method` to determine trait +implementation. + +> **Note**: This approach should be avoided if possible, the best thing to do would be to make a PR to [`rust-lang/rust`][rust] adding a diagnostic item. + +Below, we check if the given `expr` implements the `Iterator`'s trait method `cloned` : + +```rust +use clippy_utils::{match_trait_method, paths}; +use rustc_hir::Expr; +use rustc_lint::{LateContext, LateLintPass}; + +impl LateLintPass<'_> for CheckTokioAsyncReadExtTrait { + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + if match_trait_method(cx, expr, &paths::CORE_ITER_CLONED) { + println!("`expr` implements `CORE_ITER_CLONED` trait!"); + } + } +} +``` + +[DefId]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def_id/struct.DefId.html +[diagnostic_items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html +[lang_items]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/lang_items/struct.LanguageItems.html +[paths]: https://github.com/rust-lang/rust-clippy/blob/master/clippy_utils/src/paths.rs +[rustc_dev_guide]: https://rustc-dev-guide.rust-lang.org/ +[symbol]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/symbol/struct.Symbol.html +[symbol_index]: https://doc.rust-lang.org/beta/nightly-rustc/rustc_span/symbol/sym/index.html +[TyCtxt]: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html +[rust]: https://github.com/rust-lang/rust diff --git a/book/src/development/writing_tests.md b/book/src/development/writing_tests.md new file mode 100644 index 00000000000..8937e0d8e94 --- /dev/null +++ b/book/src/development/writing_tests.md @@ -0,0 +1,218 @@ +# Testing + +Developing lints for Clippy is a Test-Driven Development (TDD) process because +our first task before implementing any logic for a new lint is to write some test cases. + +## Develop Lints with Tests + +When we develop Clippy, we enter a complex and chaotic realm full of +programmatic issues, stylistic errors, illogical code and non-adherence to convention. +Tests are the first layer of order we can leverage to define when and where +we want a new lint to trigger or not. + +Moreover, writing tests first help Clippy developers to find a balance for +the first iteration of and further enhancements for a lint. +With test cases on our side, we will not have to worry about over-engineering +a lint on its first version nor missing out some obvious edge cases of the lint. +This approach empowers us to iteratively enhance each lint. + +## Clippy UI Tests + +We use **UI tests** for testing in Clippy. These UI tests check that the output +of Clippy is exactly as we expect it to be. Each test is just a plain Rust file +that contains the code we want to check. + +The output of Clippy is compared against a `.stderr` file. Note that you don't +have to create this file yourself. We'll get to generating the `.stderr` files +with the command [`cargo bless`](#cargo-bless) (seen later on). + +### Write Test Cases + +Let us now think about some tests for our imaginary `foo_functions` lint. We +start by opening the test file `tests/ui/foo_functions.rs` that was created by +`cargo dev new_lint`. + +Update the file with some examples to get started: + +```rust +#![warn(clippy::foo_functions)] // < Add this, so the lint is guaranteed to be enabled in this file + +// Impl methods +struct A; +impl A { + pub fn fo(&self) {} + pub fn foo(&self) {} //~ ERROR: function called "foo" + pub fn food(&self) {} +} + +// Default trait methods +trait B { + fn fo(&self) {} + fn foo(&self) {} //~ ERROR: function called "foo" + fn food(&self) {} +} + +// Plain functions +fn fo() {} +fn foo() {} //~ ERROR: function called "foo" +fn food() {} + +fn main() { + // We also don't want to lint method calls + foo(); + let a = A; + a.foo(); +} +``` + +Without actual lint logic to emit the lint when we see a `foo` function name, +this test will just pass, because no lint will be emitted. However, we can now +run the test with the following command: + +```sh +$ TESTNAME=foo_functions cargo uitest +``` + +Clippy will compile and it will conclude with an `ok` for the tests: + +``` +...Clippy warnings and test outputs... +test compile_test ... ok +test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.48s +``` + +This is normal. After all, we wrote a bunch of Rust code but we haven't really +implemented any logic for Clippy to detect `foo` functions and emit a lint. + +As we gradually implement our lint logic, we will keep running this UI test command. +Clippy will begin outputting information that allows us to check if the output is +turning into what we want it to be. + +### Example output + +As our `foo_functions` lint is tested, the output would look something like this: + +``` +failures: +---- compile_test stdout ---- +normalized stderr: +error: function called "foo" + --> $DIR/foo_functions.rs:6:12 + | +LL | pub fn foo(&self) {} + | ^^^ + | + = note: `-D clippy::foo-functions` implied by `-D warnings` +error: function called "foo" + --> $DIR/foo_functions.rs:13:8 + | +LL | fn foo(&self) {} + | ^^^ +error: function called "foo" + --> $DIR/foo_functions.rs:19:4 + | +LL | fn foo() {} + | ^^^ +error: aborting due to 3 previous errors +``` + +Note the *failures* label at the top of the fragment, we'll get rid of it +(saving this output) in the next section. + +> _Note:_ You can run multiple test files by specifying a comma separated list: +> `TESTNAME=foo_functions,bar_methods,baz_structs`. + +### `cargo bless` + +Once we are satisfied with the output, we need to run this command to +generate or update the `.stderr` file for our lint: + +```sh +$ TESTNAME=foo_functions cargo uibless +``` + +This writes the emitted lint suggestions and fixes to the `.stderr` file, with +the reason for the lint, suggested fixes, and line numbers, etc. + +Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit +our lint, we need to commit the generated `.stderr` files, too. + +In general, you should only commit files changed by `cargo bless` for the +specific lint you are creating/editing. + +> _Note:_ If the generated `.stderr`, and `.fixed` files are empty, +> they should be removed. + +## `toml` Tests + +Some lints can be configured through a `clippy.toml` file. Those configuration +values are tested in `tests/ui-toml`. + +To add a new test there, create a new directory and add the files: + +- `clippy.toml`: Put here the configuration value you want to test. +- `lint_name.rs`: A test file where you put the testing code, that should see a + different lint behavior according to the configuration set in the + `clippy.toml` file. + +The potential `.stderr` and `.fixed` files can again be generated with `cargo +bless`. + +## Cargo Lints + +The process of testing is different for Cargo lints in that now we are +interested in the `Cargo.toml` manifest file. In this case, we also need a +minimal crate associated with that manifest. Those tests are generated in +`tests/ui-cargo`. + +Imagine we have a new example lint that is named `foo_categories`, we can run: + +```sh +$ cargo dev new_lint --name=foo_categories --pass=late --category=cargo +``` + +After running `cargo dev new_lint` we will find by default two new crates, +each with its manifest file: + +* `tests/ui-cargo/foo_categories/fail/Cargo.toml`: this file should cause the + new lint to raise an error. +* `tests/ui-cargo/foo_categories/pass/Cargo.toml`: this file should not trigger + the lint. + +If you need more cases, you can copy one of those crates (under +`foo_categories`) and rename it. + +The process of generating the `.stderr` file is the same as for other lints +and prepending the `TESTNAME` variable to `cargo uitest` works for Cargo lints too. + +## Rustfix Tests + +If the lint you are working on is making use of structured suggestions, +[`rustfix`] will apply the suggestions from the lint to the test file code and +compare that to the contents of a `.fixed` file. + +Structured suggestions tell a user how to fix or re-write certain code that has +been linted with [`span_lint_and_sugg`]. + +Should `span_lint_and_sugg` be used to generate a suggestion, but not all +suggestions lead to valid code, you can use the `//@no-rustfix` comment on top +of the test file, to not run `rustfix` on that file. + +We'll talk about suggestions more in depth in a [later chapter](emitting_lints.md). + +Use `cargo bless` to automatically generate the `.fixed` file after running +the tests. + +[`rustfix`]: https://github.com/rust-lang/rustfix +[`span_lint_and_sugg`]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html + +## Testing Manually + +Manually testing against an example file can be useful if you have added some +`println!`s and the test suite output becomes unreadable. + +To try Clippy with your local modifications, run from the working copy root. + +```sh +$ cargo dev lint input.rs +``` diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 70dcda6a557..150d883985c 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -457,6 +457,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::misc_early::ZERO_PREFIXED_LITERAL_INFO, crate::mismatching_type_param_order::MISMATCHING_TYPE_PARAM_ORDER_INFO, crate::missing_assert_message::MISSING_ASSERT_MESSAGE_INFO, + crate::missing_asserts_for_indexing::MISSING_ASSERTS_FOR_INDEXING_INFO, crate::missing_const_for_fn::MISSING_CONST_FOR_FN_INFO, crate::missing_doc::MISSING_DOCS_IN_PRIVATE_ITEMS_INFO, crate::missing_enforced_import_rename::MISSING_ENFORCED_IMPORT_RENAMES_INFO, diff --git a/clippy_lints/src/default_union_representation.rs b/clippy_lints/src/default_union_representation.rs index 03b5a2d6d08..bbce6e1dd8f 100644 --- a/clippy_lints/src/default_union_representation.rs +++ b/clippy_lints/src/default_union_representation.rs @@ -69,6 +69,9 @@ impl<'tcx> LateLintPass<'tcx> for DefaultUnionRepresentation { } /// Returns true if the given item is a union with at least two non-ZST fields. +/// (ZST fields having an arbitrary offset is completely inconsequential, and +/// if there is only one field left after ignoring ZST fields then the offset +/// of that field does not matter either.) fn is_union_with_two_non_zst_fields(cx: &LateContext<'_>, item: &Item<'_>) -> bool { if let ItemKind::Union(data, _) = &item.kind { data.fields().iter().filter(|f| !is_zst(cx, f.ty)).count() >= 2 diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 58c27855000..1b23f01913f 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -3,7 +3,7 @@ use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exact use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::has_enclosing_paren; -use clippy_utils::ty::{is_copy, peel_mid_ty_refs}; +use clippy_utils::ty::{implements_trait, is_copy, peel_mid_ty_refs}; use clippy_utils::{ expr_use_ctxt, get_parent_expr, get_parent_node, is_lint_allowed, path_to_local, DefinedTy, ExprUseNode, }; @@ -33,7 +33,6 @@ use rustc_middle::ty::{ use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::sym; use rustc_span::{Span, Symbol}; -use rustc_trait_selection::infer::InferCtxtExt as _; use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _; use rustc_trait_selection::traits::{Obligation, ObligationCause}; use std::collections::VecDeque; @@ -452,13 +451,12 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { // Trait methods taking `self` arg_ty } && impl_ty.is_ref() - && cx.tcx.infer_ctxt().build() - .type_implements_trait( - trait_id, - [impl_ty.into()].into_iter().chain(args.iter().copied()), - cx.param_env, - ) - .must_apply_modulo_regions() + && implements_trait( + cx, + impl_ty, + trait_id, + &args[..cx.tcx.generics_of(trait_id).params.len() - 1], + ) { false } else { @@ -609,12 +607,14 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { adjusted_ty, }, )); - } else if stability.is_deref_stable() { + } else if stability.is_deref_stable() + && let Some(parent) = get_parent_expr(cx, expr) + { self.state = Some(( State::ExplicitDeref { mutability: None }, StateData { - span: expr.span, - hir_id: expr.hir_id, + span: parent.span, + hir_id: parent.hir_id, adjusted_ty, }, )); diff --git a/clippy_lints/src/ignored_unit_patterns.rs b/clippy_lints/src/ignored_unit_patterns.rs index c635120b882..d8ead1c9d9f 100644 --- a/clippy_lints/src/ignored_unit_patterns.rs +++ b/clippy_lints/src/ignored_unit_patterns.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use hir::PatKind; +use hir::{Node, PatKind}; use rustc_errors::Applicability; use rustc_hir as hir; use rustc_lint::{LateContext, LateLintPass}; @@ -37,6 +37,17 @@ declare_lint_pass!(IgnoredUnitPatterns => [IGNORED_UNIT_PATTERNS]); impl<'tcx> LateLintPass<'tcx> for IgnoredUnitPatterns { fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx hir::Pat<'tcx>) { + match cx.tcx.hir().get_parent(pat.hir_id) { + Node::Param(param) if matches!(cx.tcx.hir().get_parent(param.hir_id), Node::Item(_)) => { + // Ignore function parameters + return; + }, + Node::Local(local) if local.ty.is_some() => { + // Ignore let bindings with explicit type + return; + }, + _ => {}, + } if matches!(pat.kind, PatKind::Wild) && cx.typeck_results().pat_ty(pat).is_unit() { span_lint_and_sugg( cx, diff --git a/clippy_lints/src/implied_bounds_in_impls.rs b/clippy_lints/src/implied_bounds_in_impls.rs index c6d1acad3a0..64bdb8ba84e 100644 --- a/clippy_lints/src/implied_bounds_in_impls.rs +++ b/clippy_lints/src/implied_bounds_in_impls.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use rustc_errors::{Applicability, SuggestionStyle}; -use rustc_hir::def_id::LocalDefId; +use rustc_hir::def_id::{DefId, LocalDefId}; use rustc_hir::intravisit::FnKind; use rustc_hir::{ Body, FnDecl, FnRetTy, GenericArg, GenericBound, ImplItem, ImplItemKind, ItemKind, TraitBoundModifier, TraitItem, @@ -9,7 +9,7 @@ use rustc_hir::{ }; use rustc_hir_analysis::hir_ty_to_ty; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, ClauseKind, TyCtxt}; +use rustc_middle::ty::{self, ClauseKind, Generics, Ty, TyCtxt}; use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; @@ -45,52 +45,80 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.73.0"] pub IMPLIED_BOUNDS_IN_IMPLS, - complexity, + nursery, "specifying bounds that are implied by other bounds in `impl Trait` type" } declare_lint_pass!(ImpliedBoundsInImpls => [IMPLIED_BOUNDS_IN_IMPLS]); -/// This function tries to, for all type parameters in a supertype predicate `GenericTrait<U>`, -/// check if the substituted type in the implied-by bound matches with what's subtituted in the -/// implied type. +/// Tries to "resolve" a type. +/// The index passed to this function must start with `Self=0`, i.e. it must be a valid +/// type parameter index. +/// If the index is out of bounds, it means that the generic parameter has a default type. +fn try_resolve_type<'tcx>( + tcx: TyCtxt<'tcx>, + args: &'tcx [GenericArg<'tcx>], + generics: &'tcx Generics, + index: usize, +) -> Option<Ty<'tcx>> { + match args.get(index - 1) { + Some(GenericArg::Type(ty)) => Some(hir_ty_to_ty(tcx, ty)), + Some(_) => None, + None => Some(tcx.type_of(generics.params[index].def_id).skip_binder()), + } +} + +/// This function tries to, for all generic type parameters in a supertrait predicate `trait ...<U>: +/// GenericTrait<U>`, check if the substituted type in the implied-by bound matches with what's +/// subtituted in the implied bound. /// /// Consider this example. /// ```rust,ignore /// trait GenericTrait<T> {} /// trait GenericSubTrait<T, U, V>: GenericTrait<U> {} -/// ^ trait_predicate_args: [Self#0, U#2] +/// ^^^^^^^^^^^^^^^ trait_predicate_args: [Self#0, U#2] +/// (the Self#0 is implicit: `<Self as GenericTrait<U>>`) /// impl GenericTrait<i32> for () {} /// impl GenericSubTrait<(), i32, ()> for () {} -/// impl GenericSubTrait<(), [u8; 8], ()> for () {} +/// impl GenericSubTrait<(), i64, ()> for () {} /// -/// fn f() -> impl GenericTrait<i32> + GenericSubTrait<(), [u8; 8], ()> { -/// ^^^ implied_args ^^^^^^^^^^^^^^^ implied_by_args -/// (we are interested in `[u8; 8]` specifically, as that -/// is what `U` in `GenericTrait<U>` is substituted with) -/// () +/// fn f() -> impl GenericTrait<i32> + GenericSubTrait<(), i64, ()> { +/// ^^^ implied_args ^^^^^^^^^^^ implied_by_args +/// (we are interested in `i64` specifically, as that +/// is what `U` in `GenericTrait<U>` is substituted with) /// } /// ``` -/// Here i32 != [u8; 8], so this will return false. -fn is_same_generics( - tcx: TyCtxt<'_>, - trait_predicate_args: &[ty::GenericArg<'_>], - implied_by_args: &[GenericArg<'_>], - implied_args: &[GenericArg<'_>], +/// Here i32 != i64, so this will return false. +fn is_same_generics<'tcx>( + tcx: TyCtxt<'tcx>, + trait_predicate_args: &'tcx [ty::GenericArg<'tcx>], + implied_by_args: &'tcx [GenericArg<'tcx>], + implied_args: &'tcx [GenericArg<'tcx>], + implied_by_def_id: DefId, + implied_def_id: DefId, ) -> bool { + // Get the generics of the two traits to be able to get default generic parameter. + let implied_by_generics = tcx.generics_of(implied_by_def_id); + let implied_generics = tcx.generics_of(implied_def_id); + trait_predicate_args .iter() .enumerate() .skip(1) // skip `Self` implicit arg .all(|(arg_index, arg)| { - if let Some(ty) = arg.as_type() - && let &ty::Param(ty::ParamTy{ index, .. }) = ty.kind() - // Since `trait_predicate_args` and type params in traits start with `Self=0` - // and generic argument lists `GenericTrait<i32>` don't have `Self`, - // we need to subtract 1 from the index. - && let GenericArg::Type(ty_a) = implied_by_args[index as usize - 1] - && let GenericArg::Type(ty_b) = implied_args[arg_index - 1] - { - hir_ty_to_ty(tcx, ty_a) == hir_ty_to_ty(tcx, ty_b) + if let Some(ty) = arg.as_type() { + if let &ty::Param(ty::ParamTy { index, .. }) = ty.kind() + // `index == 0` means that it's referring to `Self`, + // in which case we don't try to substitute it + && index != 0 + && let Some(ty_a) = try_resolve_type(tcx, implied_by_args, implied_by_generics, index as usize) + && let Some(ty_b) = try_resolve_type(tcx, implied_args, implied_generics, arg_index) + { + ty_a == ty_b + } else if let Some(ty_b) = try_resolve_type(tcx, implied_args, implied_generics, arg_index) { + ty == ty_b + } else { + false + } } else { false } @@ -121,7 +149,7 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { && let predicates = cx.tcx.super_predicates_of(trait_def_id).predicates && !predicates.is_empty() // If the trait has no supertrait, there is nothing to add. { - Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates)) + Some((bound.span(), path.args.map_or([].as_slice(), |a| a.args), predicates, trait_def_id)) } else { None } @@ -135,18 +163,27 @@ fn check(cx: &LateContext<'_>, decl: &FnDecl<'_>) { && let [.., path] = poly_trait.trait_ref.path.segments && let implied_args = path.args.map_or([].as_slice(), |a| a.args) && let Some(def_id) = poly_trait.trait_ref.path.res.opt_def_id() - && let Some(implied_by_span) = implied_bounds.iter().find_map(|&(span, implied_by_args, preds)| { - preds.iter().find_map(|(clause, _)| { - if let ClauseKind::Trait(tr) = clause.kind().skip_binder() - && tr.def_id() == def_id - && is_same_generics(cx.tcx, tr.trait_ref.args, implied_by_args, implied_args) - { - Some(span) - } else { - None - } + && let Some(implied_by_span) = implied_bounds + .iter() + .find_map(|&(span, implied_by_args, preds, implied_by_def_id)| { + preds.iter().find_map(|(clause, _)| { + if let ClauseKind::Trait(tr) = clause.kind().skip_binder() + && tr.def_id() == def_id + && is_same_generics( + cx.tcx, + tr.trait_ref.args, + implied_by_args, + implied_args, + implied_by_def_id, + def_id, + ) + { + Some(span) + } else { + None + } + }) }) - }) { let implied_by = snippet(cx, implied_by_span, ".."); span_lint_and_then( diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f3559771a57..4ff97e03a14 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -210,6 +210,7 @@ mod misc; mod misc_early; mod mismatching_type_param_order; mod missing_assert_message; +mod missing_asserts_for_indexing; mod missing_const_for_fn; mod missing_doc; mod missing_enforced_import_rename; @@ -1100,6 +1101,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(ignored_unit_patterns::IgnoredUnitPatterns)); store.register_late_pass(|_| Box::<reserve_after_initialization::ReserveAfterInitialization>::default()); store.register_late_pass(|_| Box::new(implied_bounds_in_impls::ImpliedBoundsInImpls)); + store.register_late_pass(|_| Box::new(missing_asserts_for_indexing::MissingAssertsForIndexing)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/loops/never_loop.rs b/clippy_lints/src/loops/never_loop.rs index cc19ac55e5e..3d8a4cd948a 100644 --- a/clippy_lints/src/loops/never_loop.rs +++ b/clippy_lints/src/loops/never_loop.rs @@ -1,13 +1,13 @@ use super::utils::make_iterator_snippet; use super::NEVER_LOOP; -use clippy_utils::consts::{constant, Constant}; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::ForLoop; +use clippy_utils::macros::root_macro_call_first_node; use clippy_utils::source::snippet; use rustc_errors::Applicability; use rustc_hir::{Block, Destination, Expr, ExprKind, HirId, InlineAsmOperand, Pat, Stmt, StmtKind}; use rustc_lint::LateContext; -use rustc_span::Span; +use rustc_span::{sym, Span}; use std::iter::{once, Iterator}; pub(super) fn check<'tcx>( @@ -18,7 +18,7 @@ pub(super) fn check<'tcx>( for_loop: Option<&ForLoop<'_>>, ) { match never_loop_block(cx, block, &mut Vec::new(), loop_id) { - NeverLoopResult::AlwaysBreak => { + NeverLoopResult::Diverging => { span_lint_and_then(cx, NEVER_LOOP, span, "this loop never actually loops", |diag| { if let Some(ForLoop { arg: iterator, @@ -39,67 +39,76 @@ pub(super) fn check<'tcx>( } }); }, - NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Otherwise => (), - NeverLoopResult::IgnoreUntilEnd(_) => unreachable!(), + NeverLoopResult::MayContinueMainLoop | NeverLoopResult::Normal => (), } } +/// The `never_loop` analysis keeps track of three things: +/// +/// * Has any (reachable) code path hit a `continue` of the main loop? +/// * Is the current code path diverging (that is, the next expression is not reachable) +/// * For each block label `'a` inside the main loop, has any (reachable) code path encountered a +/// `break 'a`? +/// +/// The first two bits of information are in this enum, and the last part is in the +/// `local_labels` variable, which contains a list of `(block_id, reachable)` pairs ordered by +/// scope. #[derive(Copy, Clone)] enum NeverLoopResult { - // A break/return always get triggered but not necessarily for the main loop. - AlwaysBreak, - // A continue may occur for the main loop. + /// A continue may occur for the main loop. MayContinueMainLoop, - // Ignore everything until the end of the block with this id - IgnoreUntilEnd(HirId), - Otherwise, + /// We have not encountered any main loop continue, + /// but we are diverging (subsequent control flow is not reachable) + Diverging, + /// We have not encountered any main loop continue, + /// and subsequent control flow is (possibly) reachable + Normal, } #[must_use] fn absorb_break(arg: NeverLoopResult) -> NeverLoopResult { match arg { - NeverLoopResult::AlwaysBreak | NeverLoopResult::Otherwise => NeverLoopResult::Otherwise, + NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal, NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop, - NeverLoopResult::IgnoreUntilEnd(id) => NeverLoopResult::IgnoreUntilEnd(id), } } // Combine two results for parts that are called in order. #[must_use] -fn combine_seq(first: NeverLoopResult, second: NeverLoopResult) -> NeverLoopResult { +fn combine_seq(first: NeverLoopResult, second: impl FnOnce() -> NeverLoopResult) -> NeverLoopResult { match first { - NeverLoopResult::AlwaysBreak | NeverLoopResult::MayContinueMainLoop | NeverLoopResult::IgnoreUntilEnd(_) => { - first - }, - NeverLoopResult::Otherwise => second, + NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop => first, + NeverLoopResult::Normal => second(), + } +} + +// Combine an iterator of results for parts that are called in order. +#[must_use] +fn combine_seq_many(iter: impl IntoIterator<Item = NeverLoopResult>) -> NeverLoopResult { + for e in iter { + if let NeverLoopResult::Diverging | NeverLoopResult::MayContinueMainLoop = e { + return e; + } } + NeverLoopResult::Normal } // Combine two results where only one of the part may have been executed. #[must_use] -fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult, ignore_ids: &[HirId]) -> NeverLoopResult { +fn combine_branches(b1: NeverLoopResult, b2: NeverLoopResult) -> NeverLoopResult { match (b1, b2) { - (NeverLoopResult::IgnoreUntilEnd(a), NeverLoopResult::IgnoreUntilEnd(b)) => { - if ignore_ids.iter().find(|&e| e == &a || e == &b).unwrap() == &a { - NeverLoopResult::IgnoreUntilEnd(b) - } else { - NeverLoopResult::IgnoreUntilEnd(a) - } - }, - (i @ NeverLoopResult::IgnoreUntilEnd(_), NeverLoopResult::AlwaysBreak) - | (NeverLoopResult::AlwaysBreak, i @ NeverLoopResult::IgnoreUntilEnd(_)) => i, - (NeverLoopResult::AlwaysBreak, NeverLoopResult::AlwaysBreak) => NeverLoopResult::AlwaysBreak, (NeverLoopResult::MayContinueMainLoop, _) | (_, NeverLoopResult::MayContinueMainLoop) => { NeverLoopResult::MayContinueMainLoop }, - (NeverLoopResult::Otherwise, _) | (_, NeverLoopResult::Otherwise) => NeverLoopResult::Otherwise, + (NeverLoopResult::Normal, _) | (_, NeverLoopResult::Normal) => NeverLoopResult::Normal, + (NeverLoopResult::Diverging, NeverLoopResult::Diverging) => NeverLoopResult::Diverging, } } fn never_loop_block<'tcx>( cx: &LateContext<'tcx>, block: &Block<'tcx>, - ignore_ids: &mut Vec<HirId>, + local_labels: &mut Vec<(HirId, bool)>, main_loop_id: HirId, ) -> NeverLoopResult { let iter = block @@ -107,15 +116,21 @@ fn never_loop_block<'tcx>( .iter() .filter_map(stmt_to_expr) .chain(block.expr.map(|expr| (expr, None))); - - iter.map(|(e, els)| { - let e = never_loop_expr(cx, e, ignore_ids, main_loop_id); + combine_seq_many(iter.map(|(e, els)| { + let e = never_loop_expr(cx, e, local_labels, main_loop_id); // els is an else block in a let...else binding els.map_or(e, |els| { - combine_branches(e, never_loop_block(cx, els, ignore_ids, main_loop_id), ignore_ids) + combine_seq(e, || match never_loop_block(cx, els, local_labels, main_loop_id) { + // Returning MayContinueMainLoop here means that + // we will not evaluate the rest of the body + NeverLoopResult::MayContinueMainLoop => NeverLoopResult::MayContinueMainLoop, + // An else block always diverges, so the Normal case should not happen, + // but the analysis is approximate so it might return Normal anyway. + // Returning Normal here says that nothing more happens on the main path + NeverLoopResult::Diverging | NeverLoopResult::Normal => NeverLoopResult::Normal, + }) }) - }) - .fold(NeverLoopResult::Otherwise, combine_seq) + })) } fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'tcx Block<'tcx>>)> { @@ -131,76 +146,69 @@ fn stmt_to_expr<'tcx>(stmt: &Stmt<'tcx>) -> Option<(&'tcx Expr<'tcx>, Option<&'t fn never_loop_expr<'tcx>( cx: &LateContext<'tcx>, expr: &Expr<'tcx>, - ignore_ids: &mut Vec<HirId>, + local_labels: &mut Vec<(HirId, bool)>, main_loop_id: HirId, ) -> NeverLoopResult { - match expr.kind { + let result = match expr.kind { ExprKind::Unary(_, e) | ExprKind::Cast(e, _) | ExprKind::Type(e, _) | ExprKind::Field(e, _) | ExprKind::AddrOf(_, _, e) | ExprKind::Repeat(e, _) - | ExprKind::DropTemps(e) => never_loop_expr(cx, e, ignore_ids, main_loop_id), - ExprKind::Let(let_expr) => never_loop_expr(cx, let_expr.init, ignore_ids, main_loop_id), - ExprKind::Array(es) | ExprKind::Tup(es) => never_loop_expr_all(cx, &mut es.iter(), ignore_ids, main_loop_id), + | ExprKind::DropTemps(e) => never_loop_expr(cx, e, local_labels, main_loop_id), + ExprKind::Let(let_expr) => never_loop_expr(cx, let_expr.init, local_labels, main_loop_id), + ExprKind::Array(es) | ExprKind::Tup(es) => never_loop_expr_all(cx, es.iter(), local_labels, main_loop_id), ExprKind::MethodCall(_, receiver, es, _) => never_loop_expr_all( cx, - &mut std::iter::once(receiver).chain(es.iter()), - ignore_ids, + std::iter::once(receiver).chain(es.iter()), + local_labels, main_loop_id, ), ExprKind::Struct(_, fields, base) => { - let fields = never_loop_expr_all(cx, &mut fields.iter().map(|f| f.expr), ignore_ids, main_loop_id); + let fields = never_loop_expr_all(cx, fields.iter().map(|f| f.expr), local_labels, main_loop_id); if let Some(base) = base { - combine_seq(fields, never_loop_expr(cx, base, ignore_ids, main_loop_id)) + combine_seq(fields, || never_loop_expr(cx, base, local_labels, main_loop_id)) } else { fields } }, - ExprKind::Call(e, es) => never_loop_expr_all(cx, &mut once(e).chain(es.iter()), ignore_ids, main_loop_id), + ExprKind::Call(e, es) => never_loop_expr_all(cx, once(e).chain(es.iter()), local_labels, main_loop_id), ExprKind::Binary(_, e1, e2) | ExprKind::Assign(e1, e2, _) | ExprKind::AssignOp(_, e1, e2) - | ExprKind::Index(e1, e2, _) => { - never_loop_expr_all(cx, &mut [e1, e2].iter().copied(), ignore_ids, main_loop_id) - }, + | ExprKind::Index(e1, e2, _) => never_loop_expr_all(cx, [e1, e2].iter().copied(), local_labels, main_loop_id), ExprKind::Loop(b, _, _, _) => { - // Break can come from the inner loop so remove them. - absorb_break(never_loop_block(cx, b, ignore_ids, main_loop_id)) + // We don't attempt to track reachability after a loop, + // just assume there may have been a break somewhere + absorb_break(never_loop_block(cx, b, local_labels, main_loop_id)) }, ExprKind::If(e, e2, e3) => { - let e1 = never_loop_expr(cx, e, ignore_ids, main_loop_id); - let e2 = never_loop_expr(cx, e2, ignore_ids, main_loop_id); - // If we know the `if` condition evaluates to `true`, don't check everything past it; it - // should just return whatever's evaluated for `e1` and `e2` since `e3` is unreachable - if let Some(Constant::Bool(true)) = constant(cx, cx.typeck_results(), e) { - return combine_seq(e1, e2); - } - let e3 = e3.as_ref().map_or(NeverLoopResult::Otherwise, |e| { - never_loop_expr(cx, e, ignore_ids, main_loop_id) - }); - combine_seq(e1, combine_branches(e2, e3, ignore_ids)) + let e1 = never_loop_expr(cx, e, local_labels, main_loop_id); + combine_seq(e1, || { + let e2 = never_loop_expr(cx, e2, local_labels, main_loop_id); + let e3 = e3.as_ref().map_or(NeverLoopResult::Normal, |e| { + never_loop_expr(cx, e, local_labels, main_loop_id) + }); + combine_branches(e2, e3) + }) }, ExprKind::Match(e, arms, _) => { - let e = never_loop_expr(cx, e, ignore_ids, main_loop_id); - if arms.is_empty() { - e - } else { - let arms = never_loop_expr_branch(cx, &mut arms.iter().map(|a| a.body), ignore_ids, main_loop_id); - combine_seq(e, arms) - } + let e = never_loop_expr(cx, e, local_labels, main_loop_id); + combine_seq(e, || { + arms.iter().fold(NeverLoopResult::Diverging, |a, b| { + combine_branches(a, never_loop_expr(cx, b.body, local_labels, main_loop_id)) + }) + }) }, ExprKind::Block(b, l) => { if l.is_some() { - ignore_ids.push(b.hir_id); - } - let ret = never_loop_block(cx, b, ignore_ids, main_loop_id); - if l.is_some() { - ignore_ids.pop(); + local_labels.push((b.hir_id, false)); } + let ret = never_loop_block(cx, b, local_labels, main_loop_id); + let jumped_to = l.is_some() && local_labels.pop().unwrap().1; match ret { - NeverLoopResult::IgnoreUntilEnd(a) if a == b.hir_id => NeverLoopResult::Otherwise, + NeverLoopResult::Diverging if jumped_to => NeverLoopResult::Normal, _ => ret, } }, @@ -211,74 +219,78 @@ fn never_loop_expr<'tcx>( if id == main_loop_id { NeverLoopResult::MayContinueMainLoop } else { - NeverLoopResult::AlwaysBreak + NeverLoopResult::Diverging } }, - // checks if break targets a block instead of a loop - ExprKind::Break(Destination { target_id: Ok(t), .. }, e) if ignore_ids.contains(&t) => e - .map_or(NeverLoopResult::IgnoreUntilEnd(t), |e| { - never_loop_expr(cx, e, ignore_ids, main_loop_id) - }), - ExprKind::Break(_, e) | ExprKind::Ret(e) => e.as_ref().map_or(NeverLoopResult::AlwaysBreak, |e| { - combine_seq( - never_loop_expr(cx, e, ignore_ids, main_loop_id), - NeverLoopResult::AlwaysBreak, - ) - }), - ExprKind::Become(e) => combine_seq( - never_loop_expr(cx, e, ignore_ids, main_loop_id), - NeverLoopResult::AlwaysBreak, - ), - ExprKind::InlineAsm(asm) => asm - .operands - .iter() - .map(|(o, _)| match o { - InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => { - never_loop_expr(cx, expr, ignore_ids, main_loop_id) - }, - InlineAsmOperand::Out { expr, .. } => { - never_loop_expr_all(cx, &mut expr.iter().copied(), ignore_ids, main_loop_id) - }, - InlineAsmOperand::SplitInOut { in_expr, out_expr, .. } => never_loop_expr_all( - cx, - &mut once(*in_expr).chain(out_expr.iter().copied()), - ignore_ids, - main_loop_id, - ), - InlineAsmOperand::Const { .. } - | InlineAsmOperand::SymFn { .. } - | InlineAsmOperand::SymStatic { .. } => NeverLoopResult::Otherwise, + ExprKind::Break(_, e) | ExprKind::Ret(e) => { + let first = e.as_ref().map_or(NeverLoopResult::Normal, |e| { + never_loop_expr(cx, e, local_labels, main_loop_id) + }); + combine_seq(first, || { + // checks if break targets a block instead of a loop + if let ExprKind::Break(Destination { target_id: Ok(t), .. }, _) = expr.kind { + if let Some((_, reachable)) = local_labels.iter_mut().find(|(label, _)| *label == t) { + *reachable = true; + } + } + NeverLoopResult::Diverging }) - .fold(NeverLoopResult::Otherwise, combine_seq), + }, + ExprKind::Become(e) => combine_seq(never_loop_expr(cx, e, local_labels, main_loop_id), || { + NeverLoopResult::Diverging + }), + ExprKind::InlineAsm(asm) => combine_seq_many(asm.operands.iter().map(|(o, _)| match o { + InlineAsmOperand::In { expr, .. } | InlineAsmOperand::InOut { expr, .. } => { + never_loop_expr(cx, expr, local_labels, main_loop_id) + }, + InlineAsmOperand::Out { expr, .. } => { + never_loop_expr_all(cx, expr.iter().copied(), local_labels, main_loop_id) + }, + InlineAsmOperand::SplitInOut { in_expr, out_expr, .. } => never_loop_expr_all( + cx, + once(*in_expr).chain(out_expr.iter().copied()), + local_labels, + main_loop_id, + ), + InlineAsmOperand::Const { .. } | InlineAsmOperand::SymFn { .. } | InlineAsmOperand::SymStatic { .. } => { + NeverLoopResult::Normal + }, + })), ExprKind::OffsetOf(_, _) | ExprKind::Yield(_, _) | ExprKind::Closure { .. } | ExprKind::Path(_) | ExprKind::ConstBlock(_) | ExprKind::Lit(_) - | ExprKind::Err(_) => NeverLoopResult::Otherwise, + | ExprKind::Err(_) => NeverLoopResult::Normal, + }; + let result = combine_seq(result, || { + if cx.typeck_results().expr_ty(expr).is_never() { + NeverLoopResult::Diverging + } else { + NeverLoopResult::Normal + } + }); + if let NeverLoopResult::Diverging = result && + let Some(macro_call) = root_macro_call_first_node(cx, expr) && + let Some(sym::todo_macro) = cx.tcx.get_diagnostic_name(macro_call.def_id) + { + // We return MayContinueMainLoop here because we treat `todo!()` + // as potentially containing any code, including a continue of the main loop. + // This effectively silences the lint whenever a loop contains this macro anywhere. + NeverLoopResult::MayContinueMainLoop + } else { + result } } fn never_loop_expr_all<'tcx, T: Iterator<Item = &'tcx Expr<'tcx>>>( cx: &LateContext<'tcx>, - es: &mut T, - ignore_ids: &mut Vec<HirId>, - main_loop_id: HirId, -) -> NeverLoopResult { - es.map(|e| never_loop_expr(cx, e, ignore_ids, main_loop_id)) - .fold(NeverLoopResult::Otherwise, combine_seq) -} - -fn never_loop_expr_branch<'tcx, T: Iterator<Item = &'tcx Expr<'tcx>>>( - cx: &LateContext<'tcx>, - e: &mut T, - ignore_ids: &mut Vec<HirId>, + es: T, + local_labels: &mut Vec<(HirId, bool)>, main_loop_id: HirId, ) -> NeverLoopResult { - e.fold(NeverLoopResult::AlwaysBreak, |a, b| { - combine_branches(a, never_loop_expr(cx, b, ignore_ids, main_loop_id), ignore_ids) - }) + combine_seq_many(es.map(|e| never_loop_expr(cx, e, local_labels, main_loop_id))) } fn for_to_if_let_sugg(cx: &LateContext<'_>, iterator: &Expr<'_>, pat: &Pat<'_>) -> String { diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 8ac04015290..81223fa8d95 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -3055,12 +3055,12 @@ declare_clippy_lint! { /// /// ### Example /// ```rust - /// vec!(1, 2, 3, 4, 5).resize(0, 5) + /// vec![1, 2, 3, 4, 5].resize(0, 5) /// ``` /// /// Use instead: /// ```rust - /// vec!(1, 2, 3, 4, 5).clear() + /// vec![1, 2, 3, 4, 5].clear() /// ``` #[clippy::version = "1.46.0"] pub VEC_RESIZE_TO_ZERO, diff --git a/clippy_lints/src/missing_asserts_for_indexing.rs b/clippy_lints/src/missing_asserts_for_indexing.rs new file mode 100644 index 00000000000..08fec2b8ec8 --- /dev/null +++ b/clippy_lints/src/missing_asserts_for_indexing.rs @@ -0,0 +1,391 @@ +use std::mem; +use std::ops::ControlFlow; + +use clippy_utils::comparisons::{normalize_comparison, Rel}; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet; +use clippy_utils::visitors::for_each_expr; +use clippy_utils::{eq_expr_value, hash_expr, higher}; +use rustc_ast::{LitKind, RangeLimits}; +use rustc_data_structures::unhash::UnhashMap; +use rustc_errors::{Applicability, Diagnostic}; +use rustc_hir::{BinOp, Block, Expr, ExprKind, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::source_map::Spanned; +use rustc_span::{sym, Span}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for repeated slice indexing without asserting beforehand that the length + /// is greater than the largest index used to index into the slice. + /// + /// ### Why is this bad? + /// In the general case where the compiler does not have a lot of information + /// about the length of a slice, indexing it repeatedly will generate a bounds check + /// for every single index. + /// + /// Asserting that the length of the slice is at least as large as the largest value + /// to index beforehand gives the compiler enough information to elide the bounds checks, + /// effectively reducing the number of bounds checks from however many times + /// the slice was indexed to just one (the assert). + /// + /// ### Drawbacks + /// False positives. It is, in general, very difficult to predict how well + /// the optimizer will be able to elide bounds checks and it very much depends on + /// the surrounding code. For example, indexing into the slice yielded by the + /// [`slice::chunks_exact`](https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunks_exact) + /// iterator will likely have all of the bounds checks elided even without an assert + /// if the `chunk_size` is a constant. + /// + /// Asserts are not tracked across function calls. Asserting the length of a slice + /// in a different function likely gives the optimizer enough information + /// about the length of a slice, but this lint will not detect that. + /// + /// ### Example + /// ```rust + /// fn sum(v: &[u8]) -> u8 { + /// // 4 bounds checks + /// v[0] + v[1] + v[2] + v[3] + /// } + /// ``` + /// Use instead: + /// ```rust + /// fn sum(v: &[u8]) -> u8 { + /// assert!(v.len() > 4); + /// // no bounds checks + /// v[0] + v[1] + v[2] + v[3] + /// } + /// ``` + #[clippy::version = "1.70.0"] + pub MISSING_ASSERTS_FOR_INDEXING, + restriction, + "indexing into a slice multiple times without an `assert`" +} +declare_lint_pass!(MissingAssertsForIndexing => [MISSING_ASSERTS_FOR_INDEXING]); + +fn report_lint<F>(cx: &LateContext<'_>, full_span: Span, msg: &str, indexes: &[Span], f: F) +where + F: FnOnce(&mut Diagnostic), +{ + span_lint_and_then(cx, MISSING_ASSERTS_FOR_INDEXING, full_span, msg, |diag| { + f(diag); + for span in indexes { + diag.span_note(*span, "slice indexed here"); + } + diag.note("asserting the length before indexing will elide bounds checks"); + }); +} + +#[derive(Copy, Clone, Debug)] +enum LengthComparison { + /// `v.len() < 5` + LengthLessThanInt, + /// `5 < v.len()` + IntLessThanLength, + /// `v.len() <= 5` + LengthLessThanOrEqualInt, + /// `5 <= v.len()` + IntLessThanOrEqualLength, +} + +/// Extracts parts out of a length comparison expression. +/// +/// E.g. for `v.len() > 5` this returns `Some((LengthComparison::IntLessThanLength, 5, `v.len()`))` +fn len_comparison<'hir>( + bin_op: BinOp, + left: &'hir Expr<'hir>, + right: &'hir Expr<'hir>, +) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> { + macro_rules! int_lit_pat { + ($id:ident) => { + ExprKind::Lit(Spanned { + node: LitKind::Int($id, _), + .. + }) + }; + } + + // normalize comparison, `v.len() > 4` becomes `4 < v.len()` + // this simplifies the logic a bit + let (op, left, right) = normalize_comparison(bin_op.node, left, right)?; + match (op, &left.kind, &right.kind) { + (Rel::Lt, int_lit_pat!(left), _) => Some((LengthComparison::IntLessThanLength, *left as usize, right)), + (Rel::Lt, _, int_lit_pat!(right)) => Some((LengthComparison::LengthLessThanInt, *right as usize, left)), + (Rel::Le, int_lit_pat!(left), _) => Some((LengthComparison::IntLessThanOrEqualLength, *left as usize, right)), + (Rel::Le, _, int_lit_pat!(right)) => Some((LengthComparison::LengthLessThanOrEqualInt, *right as usize, left)), + _ => None, + } +} + +/// Attempts to extract parts out of an `assert!`-like expression +/// in the form `assert!(some_slice.len() > 5)`. +/// +/// `assert!` has expanded to an if expression at the HIR, so this +/// actually works not just with `assert!` specifically, but anything +/// that has a never type expression in the `then` block (e.g. `panic!`). +fn assert_len_expr<'hir>( + cx: &LateContext<'_>, + expr: &'hir Expr<'hir>, +) -> Option<(LengthComparison, usize, &'hir Expr<'hir>)> { + if let Some(higher::If { cond, then, .. }) = higher::If::hir(expr) + && let ExprKind::Unary(UnOp::Not, condition) = &cond.kind + && let ExprKind::Binary(bin_op, left, right) = &condition.kind + + && let Some((cmp, asserted_len, slice_len)) = len_comparison(*bin_op, left, right) + && let ExprKind::MethodCall(method, recv, ..) = &slice_len.kind + && cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice() + && method.ident.name == sym::len + + // check if `then` block has a never type expression + && let ExprKind::Block(Block { expr: Some(then_expr), .. }, _) = then.kind + && cx.typeck_results().expr_ty(then_expr).is_never() + { + Some((cmp, asserted_len, recv)) + } else { + None + } +} + +#[derive(Debug)] +enum IndexEntry<'hir> { + /// `assert!` without any indexing (so far) + StrayAssert { + asserted_len: usize, + comparison: LengthComparison, + assert_span: Span, + slice: &'hir Expr<'hir>, + }, + /// `assert!` with indexing + /// + /// We also store the highest index to be able to check + /// if the `assert!` asserts the right length. + AssertWithIndex { + highest_index: usize, + asserted_len: usize, + assert_span: Span, + slice: &'hir Expr<'hir>, + indexes: Vec<Span>, + comparison: LengthComparison, + }, + /// Indexing without an `assert!` + IndexWithoutAssert { + highest_index: usize, + indexes: Vec<Span>, + slice: &'hir Expr<'hir>, + }, +} + +impl<'hir> IndexEntry<'hir> { + pub fn slice(&self) -> &'hir Expr<'hir> { + match self { + IndexEntry::StrayAssert { slice, .. } + | IndexEntry::AssertWithIndex { slice, .. } + | IndexEntry::IndexWithoutAssert { slice, .. } => slice, + } + } + + pub fn index_spans(&self) -> Option<&[Span]> { + match self { + IndexEntry::StrayAssert { .. } => None, + IndexEntry::AssertWithIndex { indexes, .. } | IndexEntry::IndexWithoutAssert { indexes, .. } => { + Some(indexes) + }, + } + } +} + +/// Extracts the upper index of a slice indexing expression. +/// +/// E.g. for `5` this returns `Some(5)`, for `..5` this returns `Some(4)`, +/// for `..=5` this returns `Some(5)` +fn upper_index_expr(expr: &Expr<'_>) -> Option<usize> { + if let ExprKind::Lit(lit) = &expr.kind && let LitKind::Int(index, _) = lit.node { + Some(index as usize) + } else if let Some(higher::Range { end: Some(end), limits, .. }) = higher::Range::hir(expr) + && let ExprKind::Lit(lit) = &end.kind + && let LitKind::Int(index @ 1.., _) = lit.node + { + match limits { + RangeLimits::HalfOpen => Some(index as usize - 1), + RangeLimits::Closed => Some(index as usize), + } + } else { + None + } +} + +/// Checks if the expression is an index into a slice and adds it to `indexes` +fn check_index<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut UnhashMap<u64, Vec<IndexEntry<'hir>>>) { + if let ExprKind::Index(slice, index_lit, _) = expr.kind + && cx.typeck_results().expr_ty_adjusted(slice).peel_refs().is_slice() + && let Some(index) = upper_index_expr(index_lit) + { + let hash = hash_expr(cx, slice); + + let indexes = map.entry(hash).or_default(); + let entry = indexes.iter_mut().find(|entry| eq_expr_value(cx, entry.slice(), slice)); + + if let Some(entry) = entry { + match entry { + IndexEntry::StrayAssert { asserted_len, comparison, assert_span, slice } => { + *entry = IndexEntry::AssertWithIndex { + highest_index: index, + asserted_len: *asserted_len, + assert_span: *assert_span, + slice, + indexes: vec![expr.span], + comparison: *comparison, + }; + }, + IndexEntry::IndexWithoutAssert { highest_index, indexes, .. } + | IndexEntry::AssertWithIndex { highest_index, indexes, .. } => { + indexes.push(expr.span); + *highest_index = (*highest_index).max(index); + }, + } + } else { + indexes.push(IndexEntry::IndexWithoutAssert { + highest_index: index, + indexes: vec![expr.span], + slice, + }); + } + } +} + +/// Checks if the expression is an `assert!` expression and adds it to `asserts` +fn check_assert<'hir>(cx: &LateContext<'_>, expr: &'hir Expr<'hir>, map: &mut UnhashMap<u64, Vec<IndexEntry<'hir>>>) { + if let Some((comparison, asserted_len, slice)) = assert_len_expr(cx, expr) { + let hash = hash_expr(cx, slice); + let indexes = map.entry(hash).or_default(); + + let entry = indexes.iter_mut().find(|entry| eq_expr_value(cx, entry.slice(), slice)); + + if let Some(entry) = entry { + if let IndexEntry::IndexWithoutAssert { + highest_index, + indexes, + slice, + } = entry + { + *entry = IndexEntry::AssertWithIndex { + highest_index: *highest_index, + indexes: mem::take(indexes), + slice, + assert_span: expr.span, + comparison, + asserted_len, + }; + } + } else { + indexes.push(IndexEntry::StrayAssert { + asserted_len, + comparison, + assert_span: expr.span, + slice, + }); + } + } +} + +/// Inspects indexes and reports lints. +/// +/// Called at the end of this lint after all indexing and `assert!` expressions have been collected. +fn report_indexes(cx: &LateContext<'_>, map: &UnhashMap<u64, Vec<IndexEntry<'_>>>) { + for bucket in map.values() { + for entry in bucket { + let Some(full_span) = entry + .index_spans() + .and_then(|spans| spans.first().zip(spans.last())) + .map(|(low, &high)| low.to(high)) + else { + continue; + }; + + match entry { + IndexEntry::AssertWithIndex { + highest_index, + asserted_len, + indexes, + comparison, + assert_span, + slice, + } if indexes.len() > 1 => { + // if we have found an `assert!`, let's also check that it's actually right + // and if it convers the highest index and if not, suggest the correct length + let sugg = match comparison { + // `v.len() < 5` and `v.len() <= 5` does nothing in terms of bounds checks. + // The user probably meant `v.len() > 5` + LengthComparison::LengthLessThanInt | LengthComparison::LengthLessThanOrEqualInt => Some( + format!("assert!({}.len() > {highest_index})", snippet(cx, slice.span, "..")), + ), + // `5 < v.len()` == `v.len() > 5` + LengthComparison::IntLessThanLength if asserted_len < highest_index => Some(format!( + "assert!({}.len() > {highest_index})", + snippet(cx, slice.span, "..") + )), + // `5 <= v.len() == `v.len() >= 5` + LengthComparison::IntLessThanOrEqualLength if asserted_len <= highest_index => Some(format!( + "assert!({}.len() > {highest_index})", + snippet(cx, slice.span, "..") + )), + _ => None, + }; + + if let Some(sugg) = sugg { + report_lint( + cx, + full_span, + "indexing into a slice multiple times with an `assert` that does not cover the highest index", + indexes, + |diag| { + diag.span_suggestion( + *assert_span, + "provide the highest index that is indexed with", + sugg, + Applicability::MachineApplicable, + ); + }, + ); + } + }, + IndexEntry::IndexWithoutAssert { + indexes, + highest_index, + slice, + } if indexes.len() > 1 => { + // if there was no `assert!` but more than one index, suggest + // adding an `assert!` that covers the highest index + report_lint( + cx, + full_span, + "indexing into a slice multiple times without an `assert`", + indexes, + |diag| { + diag.help(format!( + "consider asserting the length before indexing: `assert!({}.len() > {highest_index});`", + snippet(cx, slice.span, "..") + )); + }, + ); + }, + _ => {}, + } + } + } +} + +impl LateLintPass<'_> for MissingAssertsForIndexing { + fn check_block(&mut self, cx: &LateContext<'_>, block: &Block<'_>) { + let mut map = UnhashMap::default(); + + for_each_expr(block, |expr| { + check_index(cx, expr, &mut map); + check_assert(cx, expr, &mut map); + ControlFlow::<!, ()>::Continue(()) + }); + + report_indexes(cx, &map); + } +} diff --git a/clippy_lints/src/raw_strings.rs b/clippy_lints/src/raw_strings.rs index ccabb577cb7..a4fe501f79c 100644 --- a/clippy_lints/src/raw_strings.rs +++ b/clippy_lints/src/raw_strings.rs @@ -1,7 +1,7 @@ use std::iter::once; use std::ops::ControlFlow; -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::snippet; use rustc_ast::ast::{Expr, ExprKind}; use rustc_ast::token::LitKind; @@ -9,6 +9,7 @@ use rustc_errors::Applicability; use rustc_lint::{EarlyContext, EarlyLintPass, LintContext}; use rustc_middle::lint::in_external_macro; use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{BytePos, Pos, Span}; declare_clippy_lint! { /// ### What it does @@ -76,14 +77,24 @@ impl EarlyLintPass for RawStrings { } if !str.contains(['\\', '"']) { - span_lint_and_sugg( + span_lint_and_then( cx, NEEDLESS_RAW_STRINGS, expr.span, "unnecessary raw string literal", - "try", - format!("{}\"{}\"", prefix.replace('r', ""), lit.symbol), - Applicability::MachineApplicable, + |diag| { + let (start, end) = hash_spans(expr.span, prefix, 0, max); + + // BytePos: skip over the `b` in `br`, we checked the prefix appears in the source text + let r_pos = expr.span.lo() + BytePos::from_usize(prefix.len() - 1); + let start = start.with_lo(r_pos); + + diag.multipart_suggestion( + "try", + vec![(start, String::new()), (end, String::new())], + Applicability::MachineApplicable, + ); + }, ); return; @@ -96,13 +107,6 @@ impl EarlyLintPass for RawStrings { let num = str.as_bytes().iter().chain(once(&0)).try_fold(0u8, |acc, &b| { match b { b'"' if !following_quote => (following_quote, req) = (true, 1), - // I'm a bit surprised the compiler didn't optimize this out, there's no - // branch but it still ends up doing an unnecessary comparison, it's: - // - cmp r9b,1h - // - sbb cl,-1h - // which will add 1 if it's true. With this change, it becomes: - // - add cl,r9b - // isn't that so much nicer? b'#' => req += u8::from(following_quote), _ => { if following_quote { @@ -126,18 +130,58 @@ impl EarlyLintPass for RawStrings { }; if req < max { - let hashes = "#".repeat(req as usize); - - span_lint_and_sugg( + span_lint_and_then( cx, NEEDLESS_RAW_STRING_HASHES, expr.span, "unnecessary hashes around raw string literal", - "try", - format!(r#"{prefix}{hashes}"{}"{hashes}"#, lit.symbol), - Applicability::MachineApplicable, + |diag| { + let (start, end) = hash_spans(expr.span, prefix, req, max); + + let message = match max - req { + _ if req == 0 => "remove all the hashes around the literal".to_string(), + 1 => "remove one hash from both sides of the literal".to_string(), + n => format!("remove {n} hashes from both sides of the literal"), + }; + + diag.multipart_suggestion( + message, + vec![(start, String::new()), (end, String::new())], + Applicability::MachineApplicable, + ); + }, ); } } } } + +/// Returns spans pointing at the unneeded hashes, e.g. for a `req` of `1` and `max` of `3`: +/// +/// ```ignore +/// r###".."### +/// ^^ ^^ +/// ``` +fn hash_spans(literal_span: Span, prefix: &str, req: u8, max: u8) -> (Span, Span) { + let literal_span = literal_span.data(); + + // BytePos: we checked prefix appears literally in the source text + let hash_start = literal_span.lo + BytePos::from_usize(prefix.len()); + let hash_end = literal_span.hi; + + // BytePos: req/max are counts of the ASCII character # + let start = Span::new( + hash_start + BytePos(req.into()), + hash_start + BytePos(max.into()), + literal_span.ctxt, + None, + ); + let end = Span::new( + hash_end - BytePos(req.into()), + hash_end - BytePos(max.into()), + literal_span.ctxt, + None, + ); + + (start, end) +} diff --git a/clippy_lints/src/undocumented_unsafe_blocks.rs b/clippy_lints/src/undocumented_unsafe_blocks.rs index f2ef602012f..7649ce7fafb 100644 --- a/clippy_lints/src/undocumented_unsafe_blocks.rs +++ b/clippy_lints/src/undocumented_unsafe_blocks.rs @@ -341,44 +341,21 @@ fn block_parents_have_safety_comment( id: hir::HirId, ) -> bool { if let Some(node) = get_parent_node(cx.tcx, id) { - return match node { - Node::Expr(expr) => { - if let Some( - Node::Local(hir::Local { span, .. }) - | Node::Item(hir::Item { - kind: hir::ItemKind::Const(..) | ItemKind::Static(..), - span, - .. - }), - ) = get_parent_node(cx.tcx, expr.hir_id) - { - let hir_id = match get_parent_node(cx.tcx, expr.hir_id) { - Some(Node::Local(hir::Local { hir_id, .. })) => *hir_id, - Some(Node::Item(hir::Item { owner_id, .. })) => { - cx.tcx.hir().local_def_id_to_hir_id(owner_id.def_id) - }, - _ => unreachable!(), - }; - - // if unsafe block is part of a let/const/static statement, - // and accept_comment_above_statement is set to true - // we accept the safety comment in the line the precedes this statement. - accept_comment_above_statement - && span_with_attrs_in_body_has_safety_comment( - cx, - *span, - hir_id, - accept_comment_above_attributes, - ) - } else { - !is_branchy(expr) - && span_with_attrs_in_body_has_safety_comment( - cx, - expr.span, - expr.hir_id, - accept_comment_above_attributes, - ) - } + let (span, hir_id) = match node { + Node::Expr(expr) => match get_parent_node(cx.tcx, expr.hir_id) { + Some(Node::Local(hir::Local { span, hir_id, .. })) => (*span, *hir_id), + Some(Node::Item(hir::Item { + kind: hir::ItemKind::Const(..) | ItemKind::Static(..), + span, + owner_id, + .. + })) => (*span, cx.tcx.hir().local_def_id_to_hir_id(owner_id.def_id)), + _ => { + if is_branchy(expr) { + return false; + } + (expr.span, expr.hir_id) + }, }, Node::Stmt(hir::Stmt { kind: @@ -387,28 +364,27 @@ fn block_parents_have_safety_comment( | hir::StmtKind::Semi(hir::Expr { span, hir_id, .. }), .. }) - | Node::Local(hir::Local { span, hir_id, .. }) => { - span_with_attrs_in_body_has_safety_comment(cx, *span, *hir_id, accept_comment_above_attributes) - }, + | Node::Local(hir::Local { span, hir_id, .. }) => (*span, *hir_id), Node::Item(hir::Item { kind: hir::ItemKind::Const(..) | ItemKind::Static(..), span, owner_id, .. - }) => span_with_attrs_in_body_has_safety_comment( - cx, - *span, - cx.tcx.hir().local_def_id_to_hir_id(owner_id.def_id), - accept_comment_above_attributes, - ), - _ => false, + }) => (*span, cx.tcx.hir().local_def_id_to_hir_id(owner_id.def_id)), + _ => return false, }; + // if unsafe block is part of a let/const/static statement, + // and accept_comment_above_statement is set to true + // we accept the safety comment in the line the precedes this statement. + accept_comment_above_statement + && span_with_attrs_has_safety_comment(cx, span, hir_id, accept_comment_above_attributes) + } else { + false } - false } /// Extends `span` to also include its attributes, then checks if that span has a safety comment. -fn span_with_attrs_in_body_has_safety_comment( +fn span_with_attrs_has_safety_comment( cx: &LateContext<'_>, span: Span, hir_id: HirId, @@ -420,7 +396,7 @@ fn span_with_attrs_in_body_has_safety_comment( span }; - span_in_body_has_safety_comment(cx, span) + span_has_safety_comment(cx, span) } /// Checks if an expression is "branchy", e.g. loop, match/if/etc. @@ -444,7 +420,7 @@ fn block_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool { matches!( span_from_macro_expansion_has_safety_comment(cx, span), HasSafetyComment::Yes(_) - ) || span_in_body_has_safety_comment(cx, span) + ) || span_has_safety_comment(cx, span) } fn include_attrs_in_span(cx: &LateContext<'_>, hir_id: HirId, span: Span) -> Span { @@ -639,29 +615,36 @@ fn get_body_search_span(cx: &LateContext<'_>) -> Option<Span> { let body = cx.enclosing_body?; let map = cx.tcx.hir(); let mut span = map.body(body).value.span; + let mut maybe_global_var = false; for (_, node) in map.parent_iter(body.hir_id) { match node { Node::Expr(e) => span = e.span, - Node::Block(_) - | Node::Arm(_) - | Node::Stmt(_) - | Node::Local(_) - | Node::Item(hir::Item { + Node::Block(_) | Node::Arm(_) | Node::Stmt(_) | Node::Local(_) => (), + Node::Item(hir::Item { kind: hir::ItemKind::Const(..) | ItemKind::Static(..), .. - }) => (), + }) => maybe_global_var = true, + Node::Item(hir::Item { + kind: hir::ItemKind::Mod(_), + span: item_span, + .. + }) => { + span = *item_span; + break; + }, + Node::Crate(mod_) if maybe_global_var => { + span = mod_.spans.inner_span; + }, _ => break, } } Some(span) } -fn span_in_body_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool { +fn span_has_safety_comment(cx: &LateContext<'_>, span: Span) -> bool { let source_map = cx.sess().source_map(); let ctxt = span.ctxt(); - if ctxt == SyntaxContext::root() - && let Some(search_span) = get_body_search_span(cx) - { + if ctxt.is_root() && let Some(search_span) = get_body_search_span(cx) { if let Ok(unsafe_line) = source_map.lookup_line(span.lo()) && let Some(body_span) = walk_span_to_context(search_span, SyntaxContext::root()) && let Ok(body_line) = source_map.lookup_line(body_span.lo()) diff --git a/clippy_lints/src/unit_return_expecting_ord.rs b/clippy_lints/src/unit_return_expecting_ord.rs index dd829ded0d0..de4b8738e35 100644 --- a/clippy_lints/src/unit_return_expecting_ord.rs +++ b/clippy_lints/src/unit_return_expecting_ord.rs @@ -26,7 +26,7 @@ declare_clippy_lint! { /// /// ### Example /// ```rust - /// let mut twins = vec!((1, 1), (2, 2)); + /// let mut twins = vec![(1, 1), (2, 2)]; /// twins.sort_by_key(|x| { x.1; }); /// ``` #[clippy::version = "1.47.0"] diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index dc4ee725681..0b2d1e2b089 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -362,7 +362,7 @@ pub fn snippet_block_with_context<'a>( } /// Same as `snippet_with_applicability`, but first walks the span up to the given context. This -/// will result in the macro call, rather then the expansion, if the span is from a child context. +/// will result in the macro call, rather than the expansion, if the span is from a child context. /// If the span is not from a child context, it will be used directly instead. /// /// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR node diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index ee5a49a2073..ae8ee371ffa 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -88,7 +88,7 @@ impl<'a> Sugg<'a> { } /// Same as `hir`, but first walks the span up to the given context. This will result in the - /// macro call, rather then the expansion, if the span is from a child context. If the span is + /// macro call, rather than the expansion, if the span is from a child context. If the span is /// not from a child context, it will be used directly instead. /// /// e.g. Given the expression `&vec![]`, getting a snippet from the span for `vec![]` as a HIR diff --git a/clippy_utils/src/ty/type_certainty/mod.rs b/clippy_utils/src/ty/type_certainty/mod.rs index 06fd9529044..dc7756533ac 100644 --- a/clippy_utils/src/ty/type_certainty/mod.rs +++ b/clippy_utils/src/ty/type_certainty/mod.rs @@ -150,7 +150,7 @@ fn generic_args_certainty(cx: &LateContext<'_>, args: &GenericArgs<'_>) -> Certa } /// Tries to tell whether a `QPath` resolves to something certain, e.g., whether all of its path -/// segments generic arguments are are instantiated. +/// segments generic arguments are instantiated. /// /// `qpath` could refer to either a type or a value. The heuristic never needs the `DefId` of a /// value. So `DefId`s are retained only when `resolves_to_type` is true. diff --git a/tests/compile-test.rs b/tests/compile-test.rs index 844e66728f2..171dea49d19 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -114,34 +114,26 @@ fn canonicalize(path: impl AsRef<Path>) -> PathBuf { } fn base_config(test_dir: &str) -> (Config, Args) { - let bless = var_os("RUSTC_BLESS").is_some_and(|v| v != "0") || env::args().any(|arg| arg == "--bless"); - - let args = Args { - filters: env::var("TESTNAME") - .map(|filters| filters.split(',').map(str::to_string).collect()) - .unwrap_or_default(), - quiet: false, - check: !bless, - threads: match std::env::var_os("RUST_TEST_THREADS") { - Some(n) => n.to_str().unwrap().parse().unwrap(), - None => std::thread::available_parallelism().unwrap(), - }, - skip: Vec::new(), - }; + let mut args = Args::test().unwrap(); + args.bless |= var_os("RUSTC_BLESS").is_some_and(|v| v != "0"); let mut config = Config { - mode: Mode::Yolo { rustfix: true }, + mode: Mode::Yolo { + rustfix: ui_test::RustfixMode::Everything, + }, stderr_filters: vec![(Match::PathBackslash, b"/")], stdout_filters: vec![], - output_conflict_handling: if bless { - OutputConflictHandling::Bless - } else { - OutputConflictHandling::Error("cargo uibless".into()) - }, + filter_files: env::var("TESTNAME") + .map(|filters| filters.split(',').map(str::to_string).collect()) + .unwrap_or_default(), target: None, out_dir: canonicalize(var_os("CARGO_TARGET_DIR").unwrap_or_else(|| "target".into())).join("ui_test"), ..Config::rustc(Path::new("tests").join(test_dir)) }; + config.with_args(&args, /* bless by default */ false); + if let OutputConflictHandling::Error(err) = &mut config.output_conflict_handling { + *err = "cargo uibless".into(); + } let current_exe_path = env::current_exe().unwrap(); let deps_path = current_exe_path.parent().unwrap(); let profile_path = deps_path.parent().unwrap(); @@ -184,7 +176,6 @@ fn run_ui() { ui_test::run_tests_generic( vec![config], - args, ui_test::default_file_filter, ui_test::default_per_file_config, if quiet { @@ -209,7 +200,6 @@ fn run_internal_tests() { ui_test::run_tests_generic( vec![config], - args, ui_test::default_file_filter, ui_test::default_per_file_config, if quiet { @@ -243,7 +233,6 @@ fn run_ui_toml() { ui_test::run_tests_generic( vec![config], - args, ui_test::default_file_filter, |config, path, _file_contents| { config @@ -302,8 +291,7 @@ fn run_ui_cargo() { ui_test::run_tests_generic( vec![config], - args, - |path, args, _config| path.ends_with("Cargo.toml") && ui_test::default_filter_by_arg(path, args), + |path, config| path.ends_with("Cargo.toml") && ui_test::default_any_file_filter(path, config), |config, path, _file_contents| { config.out_dir = canonicalize( std::env::current_dir() diff --git a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs index b907b43be05..b28e1b7d180 100644 --- a/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs +++ b/tests/ui-toml/undocumented_unsafe_blocks/undocumented_unsafe_blocks.rs @@ -564,4 +564,18 @@ fn issue_8679<T: Copy>() { unsafe {} } +mod issue_11246 { + // Safety: foo + const _: () = unsafe {}; + + // Safety: A safety comment + const FOO: () = unsafe {}; + + // Safety: bar + static BAR: u8 = unsafe { 0 }; +} + +// Safety: Another safety comment +const FOO: () = unsafe {}; + fn main() {} diff --git a/tests/ui/crashes/ice-11422.fixed b/tests/ui/crashes/ice-11422.fixed new file mode 100644 index 00000000000..ca5721cbb2b --- /dev/null +++ b/tests/ui/crashes/ice-11422.fixed @@ -0,0 +1,25 @@ +#![warn(clippy::implied_bounds_in_impls)] + +use std::fmt::Debug; +use std::ops::*; + +fn gen() -> impl PartialOrd + Debug {} + +struct Bar {} +trait Foo<T = Self> {} +trait FooNested<T = Option<Self>> {} +impl Foo for Bar {} +impl FooNested for Bar {} + +fn foo() -> impl Foo + FooNested { + Bar {} +} + +fn test_impl_ops() -> impl Add + Sub + Mul + Div { + 1 +} +fn test_impl_assign_ops() -> impl AddAssign + SubAssign + MulAssign + DivAssign { + 1 +} + +fn main() {} diff --git a/tests/ui/crashes/ice-11422.rs b/tests/ui/crashes/ice-11422.rs new file mode 100644 index 00000000000..355ec2480bb --- /dev/null +++ b/tests/ui/crashes/ice-11422.rs @@ -0,0 +1,25 @@ +#![warn(clippy::implied_bounds_in_impls)] + +use std::fmt::Debug; +use std::ops::*; + +fn gen() -> impl PartialOrd + PartialEq + Debug {} + +struct Bar {} +trait Foo<T = Self> {} +trait FooNested<T = Option<Self>> {} +impl Foo for Bar {} +impl FooNested for Bar {} + +fn foo() -> impl Foo + FooNested { + Bar {} +} + +fn test_impl_ops() -> impl Add + Sub + Mul + Div { + 1 +} +fn test_impl_assign_ops() -> impl AddAssign + SubAssign + MulAssign + DivAssign { + 1 +} + +fn main() {} diff --git a/tests/ui/crashes/ice-11422.stderr b/tests/ui/crashes/ice-11422.stderr new file mode 100644 index 00000000000..1930a517397 --- /dev/null +++ b/tests/ui/crashes/ice-11422.stderr @@ -0,0 +1,15 @@ +error: this bound is already specified as the supertrait of `PartialOrd` + --> $DIR/ice-11422.rs:6:31 + | +LL | fn gen() -> impl PartialOrd + PartialEq + Debug {} + | ^^^^^^^^^ + | + = note: `-D clippy::implied-bounds-in-impls` implied by `-D warnings` +help: try removing this bound + | +LL - fn gen() -> impl PartialOrd + PartialEq + Debug {} +LL + fn gen() -> impl PartialOrd + Debug {} + | + +error: aborting due to previous error + diff --git a/tests/ui/crashes/ice-360.rs b/tests/ui/crashes/ice-360.rs index 28589e1efed..0d10932b098 100644 --- a/tests/ui/crashes/ice-360.rs +++ b/tests/ui/crashes/ice-360.rs @@ -3,7 +3,8 @@ fn main() {} fn no_panic<T>(slice: &[T]) { let mut iter = slice.iter(); loop { - //~^ ERROR: this loop could be written as a `while let` loop + //~^ ERROR: this loop never actually loops + //~| ERROR: this loop could be written as a `while let` loop //~| NOTE: `-D clippy::while-let-loop` implied by `-D warnings` let _ = match iter.next() { Some(ele) => ele, diff --git a/tests/ui/crashes/ice-360.stderr b/tests/ui/crashes/ice-360.stderr index 292b27dd934..73287d231de 100644 --- a/tests/ui/crashes/ice-360.stderr +++ b/tests/ui/crashes/ice-360.stderr @@ -1,10 +1,24 @@ +error: this loop never actually loops + --> $DIR/ice-360.rs:5:5 + | +LL | / loop { +LL | | +LL | | +LL | | +... | +LL | | +LL | | } + | |_____^ + | + = note: `#[deny(clippy::never_loop)]` on by default + error: this loop could be written as a `while let` loop --> $DIR/ice-360.rs:5:5 | LL | / loop { LL | | LL | | -LL | | let _ = match iter.next() { +LL | | ... | LL | | LL | | } @@ -13,7 +27,7 @@ LL | | } = note: `-D clippy::while-let-loop` implied by `-D warnings` error: empty `loop {}` wastes CPU cycles - --> $DIR/ice-360.rs:12:9 + --> $DIR/ice-360.rs:13:9 | LL | loop {} | ^^^^^^^ @@ -21,5 +35,5 @@ LL | loop {} = help: you should either use `panic!()` or add `std::thread::sleep(..);` to the loop body = note: `-D clippy::empty-loop` implied by `-D warnings` -error: aborting due to 2 previous errors +error: aborting due to 3 previous errors diff --git a/tests/ui/empty_loop.rs b/tests/ui/empty_loop.rs index 54e8fb4907c..be347563135 100644 --- a/tests/ui/empty_loop.rs +++ b/tests/ui/empty_loop.rs @@ -7,10 +7,12 @@ use proc_macros::{external, inline_macros}; fn should_trigger() { loop {} + #[allow(clippy::never_loop)] loop { loop {} } + #[allow(clippy::never_loop)] 'outer: loop { 'inner: loop {} } @@ -18,6 +20,7 @@ fn should_trigger() { #[inline_macros] fn should_not_trigger() { + #[allow(clippy::never_loop)] loop { panic!("This is fine") } diff --git a/tests/ui/empty_loop.stderr b/tests/ui/empty_loop.stderr index 7602412334b..8594f26036e 100644 --- a/tests/ui/empty_loop.stderr +++ b/tests/ui/empty_loop.stderr @@ -8,7 +8,7 @@ LL | loop {} = note: `-D clippy::empty-loop` implied by `-D warnings` error: empty `loop {}` wastes CPU cycles - --> $DIR/empty_loop.rs:11:9 + --> $DIR/empty_loop.rs:12:9 | LL | loop {} | ^^^^^^^ @@ -16,7 +16,7 @@ LL | loop {} = help: you should either use `panic!()` or add `std::thread::sleep(..);` to the loop body error: empty `loop {}` wastes CPU cycles - --> $DIR/empty_loop.rs:15:9 + --> $DIR/empty_loop.rs:17:9 | LL | 'inner: loop {} | ^^^^^^^^^^^^^^^ diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 86bee11eb87..d046a926935 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -205,7 +205,7 @@ fn main() { } } - f_str(&&ref_str); // `needless_borrow` will suggest removing both references + f_str(&ref_str); // `needless_borrow` will suggest removing both references f_str(&ref_str); // `needless_borrow` will suggest removing only one reference let x = &&40; @@ -293,4 +293,10 @@ fn main() { fn return_dyn_assoc<'a>(x: &'a &'a u32) -> &'a <&'a u32 as WithAssoc>::Assoc { *x } + + // Issue #11366 + let _: &mut u32 = match &mut Some(&mut 0u32) { + Some(x) => x, + None => panic!(), + }; } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 7a505bdf558..3dd08e487f8 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -293,4 +293,10 @@ fn main() { fn return_dyn_assoc<'a>(x: &'a &'a u32) -> &'a <&'a u32 as WithAssoc>::Assoc { *x } + + // Issue #11366 + let _: &mut u32 = match &mut Some(&mut 0u32) { + Some(x) => &mut *x, + None => panic!(), + }; } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 34ce188530c..e98f1c40b27 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -193,10 +193,10 @@ LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try: `ref_ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:208:13 + --> $DIR/explicit_auto_deref.rs:208:12 | LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references - | ^^^^^^^^ help: try: `ref_str` + | ^^^^^^^^^ help: try: `ref_str` error: deref which would be done by auto-deref --> $DIR/explicit_auto_deref.rs:209:12 @@ -234,5 +234,11 @@ error: deref which would be done by auto-deref LL | *x | ^^ help: try: `x` -error: aborting due to 39 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:299:20 + | +LL | Some(x) => &mut *x, + | ^^^^^^^ help: try: `x` + +error: aborting due to 40 previous errors diff --git a/tests/ui/ignored_unit_patterns.fixed b/tests/ui/ignored_unit_patterns.fixed index 2912eede4d9..6c6f21fee16 100644 --- a/tests/ui/ignored_unit_patterns.fixed +++ b/tests/ui/ignored_unit_patterns.fixed @@ -1,5 +1,5 @@ #![warn(clippy::ignored_unit_patterns)] -#![allow(clippy::redundant_pattern_matching, clippy::single_match)] +#![allow(clippy::let_unit_value, clippy::redundant_pattern_matching, clippy::single_match)] fn foo() -> Result<(), ()> { unimplemented!() @@ -7,9 +7,19 @@ fn foo() -> Result<(), ()> { fn main() { match foo() { - Ok(()) => {}, - Err(()) => {}, + Ok(()) => {}, //~ ERROR: matching over `()` is more explicit + Err(()) => {}, //~ ERROR: matching over `()` is more explicit } if let Ok(()) = foo() {} + //~^ ERROR: matching over `()` is more explicit let _ = foo().map_err(|()| todo!()); + //~^ ERROR: matching over `()` is more explicit +} + +#[allow(unused)] +pub fn moo(_: ()) { + let () = foo().unwrap(); + //~^ ERROR: matching over `()` is more explicit + let _: () = foo().unwrap(); + let _: () = (); } diff --git a/tests/ui/ignored_unit_patterns.rs b/tests/ui/ignored_unit_patterns.rs index d180cd8d2fd..5e8c2e03ba2 100644 --- a/tests/ui/ignored_unit_patterns.rs +++ b/tests/ui/ignored_unit_patterns.rs @@ -1,5 +1,5 @@ #![warn(clippy::ignored_unit_patterns)] -#![allow(clippy::redundant_pattern_matching, clippy::single_match)] +#![allow(clippy::let_unit_value, clippy::redundant_pattern_matching, clippy::single_match)] fn foo() -> Result<(), ()> { unimplemented!() @@ -7,9 +7,19 @@ fn foo() -> Result<(), ()> { fn main() { match foo() { - Ok(_) => {}, - Err(_) => {}, + Ok(_) => {}, //~ ERROR: matching over `()` is more explicit + Err(_) => {}, //~ ERROR: matching over `()` is more explicit } if let Ok(_) = foo() {} + //~^ ERROR: matching over `()` is more explicit let _ = foo().map_err(|_| todo!()); + //~^ ERROR: matching over `()` is more explicit +} + +#[allow(unused)] +pub fn moo(_: ()) { + let _ = foo().unwrap(); + //~^ ERROR: matching over `()` is more explicit + let _: () = foo().unwrap(); + let _: () = (); } diff --git a/tests/ui/ignored_unit_patterns.stderr b/tests/ui/ignored_unit_patterns.stderr index 6cb8b60ac0c..81aa067daef 100644 --- a/tests/ui/ignored_unit_patterns.stderr +++ b/tests/ui/ignored_unit_patterns.stderr @@ -19,10 +19,16 @@ LL | if let Ok(_) = foo() {} | ^ help: use `()` instead of `_`: `()` error: matching over `()` is more explicit - --> $DIR/ignored_unit_patterns.rs:14:28 + --> $DIR/ignored_unit_patterns.rs:15:28 | LL | let _ = foo().map_err(|_| todo!()); | ^ help: use `()` instead of `_`: `()` -error: aborting due to 4 previous errors +error: matching over `()` is more explicit + --> $DIR/ignored_unit_patterns.rs:21:9 + | +LL | let _ = foo().unwrap(); + | ^ help: use `()` instead of `_`: `()` + +error: aborting due to 5 previous errors diff --git a/tests/ui/implied_bounds_in_impls.fixed b/tests/ui/implied_bounds_in_impls.fixed index 952c2b70619..69fcc8921d6 100644 --- a/tests/ui/implied_bounds_in_impls.fixed +++ b/tests/ui/implied_bounds_in_impls.fixed @@ -65,4 +65,22 @@ impl SomeTrait for SomeStruct { } } +mod issue11422 { + use core::fmt::Debug; + // Some additional tests that would cause ICEs: + + // `PartialOrd` has a default generic parameter and does not need to be explicitly specified. + // This needs special handling. + fn default_generic_param1() -> impl PartialOrd + Debug {} + fn default_generic_param2() -> impl PartialOrd + Debug {} + + // Referring to `Self` in the supertrait clause needs special handling. + trait Trait1<X: ?Sized> {} + trait Trait2: Trait1<Self> {} + impl Trait1<()> for () {} + impl Trait2 for () {} + + fn f() -> impl Trait1<()> + Trait2 {} +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.rs b/tests/ui/implied_bounds_in_impls.rs index 475f2621bbf..5504feae613 100644 --- a/tests/ui/implied_bounds_in_impls.rs +++ b/tests/ui/implied_bounds_in_impls.rs @@ -65,4 +65,22 @@ impl SomeTrait for SomeStruct { } } +mod issue11422 { + use core::fmt::Debug; + // Some additional tests that would cause ICEs: + + // `PartialOrd` has a default generic parameter and does not need to be explicitly specified. + // This needs special handling. + fn default_generic_param1() -> impl PartialEq + PartialOrd + Debug {} + fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {} + + // Referring to `Self` in the supertrait clause needs special handling. + trait Trait1<X: ?Sized> {} + trait Trait2: Trait1<Self> {} + impl Trait1<()> for () {} + impl Trait2 for () {} + + fn f() -> impl Trait1<()> + Trait2 {} +} + fn main() {} diff --git a/tests/ui/implied_bounds_in_impls.stderr b/tests/ui/implied_bounds_in_impls.stderr index 8dffc674444..d6a4ae889fe 100644 --- a/tests/ui/implied_bounds_in_impls.stderr +++ b/tests/ui/implied_bounds_in_impls.stderr @@ -119,5 +119,29 @@ LL - fn f() -> impl Deref + DerefMut<Target = u8> { LL + fn f() -> impl DerefMut<Target = u8> { | -error: aborting due to 10 previous errors +error: this bound is already specified as the supertrait of `PartialOrd` + --> $DIR/implied_bounds_in_impls.rs:74:41 + | +LL | fn default_generic_param1() -> impl PartialEq + PartialOrd + Debug {} + | ^^^^^^^^^ + | +help: try removing this bound + | +LL - fn default_generic_param1() -> impl PartialEq + PartialOrd + Debug {} +LL + fn default_generic_param1() -> impl PartialOrd + Debug {} + | + +error: this bound is already specified as the supertrait of `PartialOrd` + --> $DIR/implied_bounds_in_impls.rs:75:54 + | +LL | fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {} + | ^^^^^^^^^ + | +help: try removing this bound + | +LL - fn default_generic_param2() -> impl PartialOrd + PartialEq + Debug {} +LL + fn default_generic_param2() -> impl PartialOrd + Debug {} + | + +error: aborting due to 12 previous errors diff --git a/tests/ui/iter_out_of_bounds.rs b/tests/ui/iter_out_of_bounds.rs index c34eb1be0c5..3cfe6e82fc1 100644 --- a/tests/ui/iter_out_of_bounds.rs +++ b/tests/ui/iter_out_of_bounds.rs @@ -8,6 +8,7 @@ fn opaque_empty_iter() -> impl Iterator<Item = ()> { } fn main() { + #[allow(clippy::never_loop)] for _ in [1, 2, 3].iter().skip(4) { //~^ ERROR: this `.skip()` call skips more items than the iterator will produce unreachable!(); diff --git a/tests/ui/iter_out_of_bounds.stderr b/tests/ui/iter_out_of_bounds.stderr index 98ee28fcaf6..f235faec8e5 100644 --- a/tests/ui/iter_out_of_bounds.stderr +++ b/tests/ui/iter_out_of_bounds.stderr @@ -1,5 +1,5 @@ error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:11:14 + --> $DIR/iter_out_of_bounds.rs:12:14 | LL | for _ in [1, 2, 3].iter().skip(4) { | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL | #![deny(clippy::iter_out_of_bounds)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^ error: this `.take()` call takes more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:15:19 + --> $DIR/iter_out_of_bounds.rs:16:19 | LL | for (i, _) in [1, 2, 3].iter().take(4).enumerate() { | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -20,7 +20,7 @@ LL | for (i, _) in [1, 2, 3].iter().take(4).enumerate() { = note: this operation is useless and the returned iterator will simply yield the same items error: this `.take()` call takes more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:21:14 + --> $DIR/iter_out_of_bounds.rs:22:14 | LL | for _ in (&&&&&&[1, 2, 3]).iter().take(4) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -28,7 +28,7 @@ LL | for _ in (&&&&&&[1, 2, 3]).iter().take(4) {} = note: this operation is useless and the returned iterator will simply yield the same items error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:24:14 + --> $DIR/iter_out_of_bounds.rs:25:14 | LL | for _ in [1, 2, 3].iter().skip(4) {} | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -36,7 +36,7 @@ LL | for _ in [1, 2, 3].iter().skip(4) {} = note: this operation is useless and will create an empty iterator error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:27:14 + --> $DIR/iter_out_of_bounds.rs:28:14 | LL | for _ in [1; 3].iter().skip(4) {} | ^^^^^^^^^^^^^^^^^^^^^ @@ -44,7 +44,7 @@ LL | for _ in [1; 3].iter().skip(4) {} = note: this operation is useless and will create an empty iterator error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:33:14 + --> $DIR/iter_out_of_bounds.rs:34:14 | LL | for _ in vec![1, 2, 3].iter().skip(4) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -52,7 +52,7 @@ LL | for _ in vec![1, 2, 3].iter().skip(4) {} = note: this operation is useless and will create an empty iterator error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:36:14 + --> $DIR/iter_out_of_bounds.rs:37:14 | LL | for _ in vec![1; 3].iter().skip(4) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -60,7 +60,7 @@ LL | for _ in vec![1; 3].iter().skip(4) {} = note: this operation is useless and will create an empty iterator error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:40:14 + --> $DIR/iter_out_of_bounds.rs:41:14 | LL | for _ in x.iter().skip(4) {} | ^^^^^^^^^^^^^^^^ @@ -68,7 +68,7 @@ LL | for _ in x.iter().skip(4) {} = note: this operation is useless and will create an empty iterator error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:44:14 + --> $DIR/iter_out_of_bounds.rs:45:14 | LL | for _ in x.iter().skip(n) {} | ^^^^^^^^^^^^^^^^ @@ -76,7 +76,7 @@ LL | for _ in x.iter().skip(n) {} = note: this operation is useless and will create an empty iterator error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:49:14 + --> $DIR/iter_out_of_bounds.rs:50:14 | LL | for _ in empty().skip(1) {} | ^^^^^^^^^^^^^^^ @@ -84,7 +84,7 @@ LL | for _ in empty().skip(1) {} = note: this operation is useless and will create an empty iterator error: this `.take()` call takes more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:52:14 + --> $DIR/iter_out_of_bounds.rs:53:14 | LL | for _ in empty().take(1) {} | ^^^^^^^^^^^^^^^ @@ -92,7 +92,7 @@ LL | for _ in empty().take(1) {} = note: this operation is useless and the returned iterator will simply yield the same items error: this `.skip()` call skips more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:55:14 + --> $DIR/iter_out_of_bounds.rs:56:14 | LL | for _ in std::iter::once(1).skip(2) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -100,7 +100,7 @@ LL | for _ in std::iter::once(1).skip(2) {} = note: this operation is useless and will create an empty iterator error: this `.take()` call takes more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:58:14 + --> $DIR/iter_out_of_bounds.rs:59:14 | LL | for _ in std::iter::once(1).take(2) {} | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -108,7 +108,7 @@ LL | for _ in std::iter::once(1).take(2) {} = note: this operation is useless and the returned iterator will simply yield the same items error: this `.take()` call takes more items than the iterator will produce - --> $DIR/iter_out_of_bounds.rs:61:14 + --> $DIR/iter_out_of_bounds.rs:62:14 | LL | for x in [].iter().take(1) { | ^^^^^^^^^^^^^^^^^ diff --git a/tests/ui/missing_asserts_for_indexing.fixed b/tests/ui/missing_asserts_for_indexing.fixed new file mode 100644 index 00000000000..a96827259f5 --- /dev/null +++ b/tests/ui/missing_asserts_for_indexing.fixed @@ -0,0 +1,121 @@ +#![allow(unused)] +#![warn(clippy::missing_asserts_for_indexing)] + +// ok +fn sum_with_assert(v: &[u8]) -> u8 { + assert!(v.len() > 4); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +// ok +fn sum_with_assert_other_way(v: &[u8]) -> u8 { + assert!(5 <= v.len()); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +// ok +fn sum_with_assert_ge(v: &[u8]) -> u8 { + assert!(v.len() >= 5); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +// ok +fn sum_with_assert_ge_other_way(v: &[u8]) -> u8 { + assert!(4 < v.len()); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +fn sum_with_assert_lt(v: &[u8]) -> u8 { + assert!(v.len() > 4); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +fn sum_with_assert_le(v: &[u8]) -> u8 { + assert!(v.len() > 4); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +fn sum_with_incorrect_assert_len(v: &[u8]) -> u8 { + assert!(v.len() > 4); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +fn sum_with_incorrect_assert_len2(v: &[u8]) -> u8 { + assert!(v.len() > 4); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +// ok, don't lint for single array access +fn single_access(v: &[u8]) -> u8 { + v[0] +} + +// ok +fn subslice_ok(v: &[u8]) { + assert!(v.len() > 3); + let _ = v[0]; + let _ = v[1..4]; +} + +fn subslice_bad(v: &[u8]) { + assert!(v.len() > 3); + let _ = v[0]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v[1..4]; +} + +// ok +fn subslice_inclusive_ok(v: &[u8]) { + assert!(v.len() > 4); + let _ = v[0]; + let _ = v[1..=4]; +} + +fn subslice_inclusive_bad(v: &[u8]) { + assert!(v.len() > 4); + let _ = v[0]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v[1..=4]; +} + +fn index_different_slices_ok(v1: &[u8], v2: &[u8]) { + assert!(v1.len() > 12); + assert!(v2.len() > 15); + let _ = v1[0] + v1[12]; + let _ = v2[5] + v2[15]; +} + +fn index_different_slices_wrong_len(v1: &[u8], v2: &[u8]) { + assert!(v1.len() > 12); + assert!(v2.len() > 15); + let _ = v1[0] + v1[12]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v2[5] + v2[15]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} +fn index_different_slices_one_wrong_len(v1: &[u8], v2: &[u8]) { + assert!(v1.len() > 12); + assert!(v2.len() > 15); + let _ = v1[0] + v1[12]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v2[5] + v2[15]; +} + +fn side_effect() -> &'static [u8] { + &[] +} + +fn index_side_effect_expr() { + let _ = side_effect()[0] + side_effect()[1]; +} + +// ok, single access for different slices +fn index_different_slice_in_same_expr(v1: &[u8], v2: &[u8]) { + let _ = v1[0] + v2[1]; +} + +fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing.rs b/tests/ui/missing_asserts_for_indexing.rs new file mode 100644 index 00000000000..0b4b883acf8 --- /dev/null +++ b/tests/ui/missing_asserts_for_indexing.rs @@ -0,0 +1,121 @@ +#![allow(unused)] +#![warn(clippy::missing_asserts_for_indexing)] + +// ok +fn sum_with_assert(v: &[u8]) -> u8 { + assert!(v.len() > 4); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +// ok +fn sum_with_assert_other_way(v: &[u8]) -> u8 { + assert!(5 <= v.len()); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +// ok +fn sum_with_assert_ge(v: &[u8]) -> u8 { + assert!(v.len() >= 5); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +// ok +fn sum_with_assert_ge_other_way(v: &[u8]) -> u8 { + assert!(4 < v.len()); + v[0] + v[1] + v[2] + v[3] + v[4] +} + +fn sum_with_assert_lt(v: &[u8]) -> u8 { + assert!(v.len() < 5); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +fn sum_with_assert_le(v: &[u8]) -> u8 { + assert!(v.len() <= 5); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +fn sum_with_incorrect_assert_len(v: &[u8]) -> u8 { + assert!(v.len() > 3); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +fn sum_with_incorrect_assert_len2(v: &[u8]) -> u8 { + assert!(v.len() >= 4); + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} + +// ok, don't lint for single array access +fn single_access(v: &[u8]) -> u8 { + v[0] +} + +// ok +fn subslice_ok(v: &[u8]) { + assert!(v.len() > 3); + let _ = v[0]; + let _ = v[1..4]; +} + +fn subslice_bad(v: &[u8]) { + assert!(v.len() >= 3); + let _ = v[0]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v[1..4]; +} + +// ok +fn subslice_inclusive_ok(v: &[u8]) { + assert!(v.len() > 4); + let _ = v[0]; + let _ = v[1..=4]; +} + +fn subslice_inclusive_bad(v: &[u8]) { + assert!(v.len() >= 4); + let _ = v[0]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v[1..=4]; +} + +fn index_different_slices_ok(v1: &[u8], v2: &[u8]) { + assert!(v1.len() > 12); + assert!(v2.len() > 15); + let _ = v1[0] + v1[12]; + let _ = v2[5] + v2[15]; +} + +fn index_different_slices_wrong_len(v1: &[u8], v2: &[u8]) { + assert!(v1.len() >= 12); + assert!(v2.len() >= 15); + let _ = v1[0] + v1[12]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v2[5] + v2[15]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the +} +fn index_different_slices_one_wrong_len(v1: &[u8], v2: &[u8]) { + assert!(v1.len() >= 12); + assert!(v2.len() > 15); + let _ = v1[0] + v1[12]; + //~^ ERROR: indexing into a slice multiple times with an `assert` that does not cover the + let _ = v2[5] + v2[15]; +} + +fn side_effect() -> &'static [u8] { + &[] +} + +fn index_side_effect_expr() { + let _ = side_effect()[0] + side_effect()[1]; +} + +// ok, single access for different slices +fn index_different_slice_in_same_expr(v1: &[u8], v2: &[u8]) { + let _ = v1[0] + v2[1]; +} + +fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing.stderr b/tests/ui/missing_asserts_for_indexing.stderr new file mode 100644 index 00000000000..75eb4a35cd4 --- /dev/null +++ b/tests/ui/missing_asserts_for_indexing.stderr @@ -0,0 +1,252 @@ +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:30:5 + | +LL | assert!(v.len() < 5); + | -------------------- help: provide the highest index that is indexed with: `assert!(v.len() > 4)` +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:30:5 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:30:12 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:30:19 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:30:26 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:30:33 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ + = note: asserting the length before indexing will elide bounds checks + = note: `-D clippy::missing-asserts-for-indexing` implied by `-D warnings` + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:36:5 + | +LL | assert!(v.len() <= 5); + | --------------------- help: provide the highest index that is indexed with: `assert!(v.len() > 4)` +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:36:5 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:36:12 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:36:19 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:36:26 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:36:33 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:42:5 + | +LL | assert!(v.len() > 3); + | -------------------- help: provide the highest index that is indexed with: `assert!(v.len() > 4)` +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:42:5 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:42:12 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:42:19 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:42:26 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:42:33 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:48:5 + | +LL | assert!(v.len() >= 4); + | --------------------- help: provide the highest index that is indexed with: `assert!(v.len() > 4)` +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:48:5 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:48:12 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:48:19 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:48:26 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:48:33 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:66:13 + | +LL | assert!(v.len() >= 3); + | --------------------- help: provide the highest index that is indexed with: `assert!(v.len() > 3)` +LL | let _ = v[0]; + | _____________^ +LL | | +LL | | let _ = v[1..4]; + | |___________________^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:66:13 + | +LL | let _ = v[0]; + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:68:13 + | +LL | let _ = v[1..4]; + | ^^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:80:13 + | +LL | assert!(v.len() >= 4); + | --------------------- help: provide the highest index that is indexed with: `assert!(v.len() > 4)` +LL | let _ = v[0]; + | _____________^ +LL | | +LL | | let _ = v[1..=4]; + | |____________________^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:80:13 + | +LL | let _ = v[0]; + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:82:13 + | +LL | let _ = v[1..=4]; + | ^^^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:95:13 + | +LL | assert!(v1.len() >= 12); + | ----------------------- help: provide the highest index that is indexed with: `assert!(v1.len() > 12)` +LL | assert!(v2.len() >= 15); +LL | let _ = v1[0] + v1[12]; + | ^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:95:13 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:95:21 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:97:13 + | +LL | assert!(v2.len() >= 15); + | ----------------------- help: provide the highest index that is indexed with: `assert!(v2.len() > 15)` +... +LL | let _ = v2[5] + v2[15]; + | ^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:97:13 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:97:21 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times with an `assert` that does not cover the highest index + --> $DIR/missing_asserts_for_indexing.rs:103:13 + | +LL | assert!(v1.len() >= 12); + | ----------------------- help: provide the highest index that is indexed with: `assert!(v1.len() > 12)` +LL | assert!(v2.len() > 15); +LL | let _ = v1[0] + v1[12]; + | ^^^^^^^^^^^^^^ + | +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:103:13 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing.rs:103:21 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: aborting due to 9 previous errors + diff --git a/tests/ui/missing_asserts_for_indexing_unfixable.rs b/tests/ui/missing_asserts_for_indexing_unfixable.rs new file mode 100644 index 00000000000..4346ed892f2 --- /dev/null +++ b/tests/ui/missing_asserts_for_indexing_unfixable.rs @@ -0,0 +1,49 @@ +#![allow(unused)] +#![warn(clippy::missing_asserts_for_indexing)] + +fn sum(v: &[u8]) -> u8 { + v[0] + v[1] + v[2] + v[3] + v[4] + //~^ ERROR: indexing into a slice multiple times without an `assert` +} + +fn subslice(v: &[u8]) { + let _ = v[0]; + //~^ ERROR: indexing into a slice multiple times without an `assert` + let _ = v[1..4]; +} + +fn variables(v: &[u8]) -> u8 { + let a = v[0]; + //~^ ERROR: indexing into a slice multiple times without an `assert` + let b = v[1]; + let c = v[2]; + a + b + c +} + +fn index_different_slices(v1: &[u8], v2: &[u8]) { + let _ = v1[0] + v1[12]; + let _ = v2[5] + v2[15]; +} + +fn index_different_slices2(v1: &[u8], v2: &[u8]) { + assert!(v1.len() > 12); + let _ = v1[0] + v1[12]; + let _ = v2[5] + v2[15]; +} + +struct Foo<'a> { + v: &'a [u8], + v2: &'a [u8], +} + +fn index_struct_field(f: &Foo<'_>) { + let _ = f.v[0] + f.v[1]; + //~^ ERROR: indexing into a slice multiple times without an `assert` +} + +fn index_struct_different_fields(f: &Foo<'_>) { + // ok, different fields + let _ = f.v[0] + f.v2[1]; +} + +fn main() {} diff --git a/tests/ui/missing_asserts_for_indexing_unfixable.stderr b/tests/ui/missing_asserts_for_indexing_unfixable.stderr new file mode 100644 index 00000000000..a26524d9a11 --- /dev/null +++ b/tests/ui/missing_asserts_for_indexing_unfixable.stderr @@ -0,0 +1,163 @@ +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:5:5 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(v.len() > 4);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:5:5 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:5:12 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:5:19 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:5:26 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:5:33 + | +LL | v[0] + v[1] + v[2] + v[3] + v[4] + | ^^^^ + = note: asserting the length before indexing will elide bounds checks + = note: `-D clippy::missing-asserts-for-indexing` implied by `-D warnings` + +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:10:13 + | +LL | let _ = v[0]; + | _____________^ +LL | | +LL | | let _ = v[1..4]; + | |___________________^ + | + = help: consider asserting the length before indexing: `assert!(v.len() > 3);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:10:13 + | +LL | let _ = v[0]; + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:12:13 + | +LL | let _ = v[1..4]; + | ^^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:16:13 + | +LL | let a = v[0]; + | _____________^ +LL | | +LL | | let b = v[1]; +LL | | let c = v[2]; + | |________________^ + | + = help: consider asserting the length before indexing: `assert!(v.len() > 2);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:16:13 + | +LL | let a = v[0]; + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:18:13 + | +LL | let b = v[1]; + | ^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:19:13 + | +LL | let c = v[2]; + | ^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:24:13 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(v1.len() > 12);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:24:13 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:24:21 + | +LL | let _ = v1[0] + v1[12]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:25:13 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(v2.len() > 15);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:25:13 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:25:21 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:31:13 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(v2.len() > 15);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:31:13 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:31:21 + | +LL | let _ = v2[5] + v2[15]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: indexing into a slice multiple times without an `assert` + --> $DIR/missing_asserts_for_indexing_unfixable.rs:40:13 + | +LL | let _ = f.v[0] + f.v[1]; + | ^^^^^^^^^^^^^^^ + | + = help: consider asserting the length before indexing: `assert!(f.v.len() > 1);` +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:40:13 + | +LL | let _ = f.v[0] + f.v[1]; + | ^^^^^^ +note: slice indexed here + --> $DIR/missing_asserts_for_indexing_unfixable.rs:40:22 + | +LL | let _ = f.v[0] + f.v[1]; + | ^^^^^^ + = note: asserting the length before indexing will elide bounds checks + +error: aborting due to 7 previous errors + diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index ee67224b4a8..0a52b25229d 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -503,3 +503,16 @@ mod issue_10535 { { } } + +mod issue_10253 { + struct S; + trait X { + fn f<T>(&self); + } + impl X for &S { + fn f<T>(&self) {} + } + fn f() { + (&S).f::<()>(); + } +} diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 1444f47d920..34a95d18463 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -503,3 +503,16 @@ mod issue_10535 { { } } + +mod issue_10253 { + struct S; + trait X { + fn f<T>(&self); + } + impl X for &S { + fn f<T>(&self) {} + } + fn f() { + (&S).f::<()>(); + } +} diff --git a/tests/ui/needless_raw_string.fixed b/tests/ui/needless_raw_string.fixed index 4db375178b4..85549810513 100644 --- a/tests/ui/needless_raw_string.fixed +++ b/tests/ui/needless_raw_string.fixed @@ -9,8 +9,13 @@ fn main() { b"aaa"; br#""aaa""#; br#"\s"#; - // currently disabled: https://github.com/rust-lang/rust/issues/113333 - // cr#"aaa"#; - // cr#""aaa""#; - // cr#"\s"#; + c"aaa"; + cr#""aaa""#; + cr#"\s"#; + + " + a + multiline + string + "; } diff --git a/tests/ui/needless_raw_string.rs b/tests/ui/needless_raw_string.rs index 59c75fda41b..06d49730387 100644 --- a/tests/ui/needless_raw_string.rs +++ b/tests/ui/needless_raw_string.rs @@ -9,8 +9,13 @@ fn main() { br#"aaa"#; br#""aaa""#; br#"\s"#; - // currently disabled: https://github.com/rust-lang/rust/issues/113333 - // cr#"aaa"#; - // cr#""aaa""#; - // cr#"\s"#; + cr#"aaa"#; + cr#""aaa""#; + cr#"\s"#; + + r#" + a + multiline + string + "#; } diff --git a/tests/ui/needless_raw_string.stderr b/tests/ui/needless_raw_string.stderr index ddc36af2e72..9c417fe1699 100644 --- a/tests/ui/needless_raw_string.stderr +++ b/tests/ui/needless_raw_string.stderr @@ -2,15 +2,57 @@ error: unnecessary raw string literal --> $DIR/needless_raw_string.rs:6:5 | LL | r#"aaa"#; - | ^^^^^^^^ help: try: `"aaa"` + | ^^^^^^^^ | = note: `-D clippy::needless-raw-strings` implied by `-D warnings` +help: try + | +LL - r#"aaa"#; +LL + "aaa"; + | error: unnecessary raw string literal --> $DIR/needless_raw_string.rs:9:5 | LL | br#"aaa"#; - | ^^^^^^^^^ help: try: `b"aaa"` + | ^^^^^^^^^ + | +help: try + | +LL - br#"aaa"#; +LL + b"aaa"; + | + +error: unnecessary raw string literal + --> $DIR/needless_raw_string.rs:12:5 + | +LL | cr#"aaa"#; + | ^^^^^^^^^ + | +help: try + | +LL - cr#"aaa"#; +LL + c"aaa"; + | + +error: unnecessary raw string literal + --> $DIR/needless_raw_string.rs:16:5 + | +LL | / r#" +LL | | a +LL | | multiline +LL | | string +LL | | "#; + | |______^ + | +help: try + | +LL ~ " +LL | a +LL | multiline +LL | string +LL ~ "; + | -error: aborting due to 2 previous errors +error: aborting due to 4 previous errors diff --git a/tests/ui/needless_raw_string_hashes.fixed b/tests/ui/needless_raw_string_hashes.fixed index 84902157ab4..e980adeeff4 100644 --- a/tests/ui/needless_raw_string_hashes.fixed +++ b/tests/ui/needless_raw_string_hashes.fixed @@ -3,17 +3,22 @@ #![feature(c_str_literals)] fn main() { - r#"aaa"#; + r"\aaa"; r#"Hello "world"!"#; r####" "### "## "# "####; r###" "aa" "# "## "###; - br#"aaa"#; + br"\aaa"; br#"Hello "world"!"#; br####" "### "## "# "####; br###" "aa" "# "## "###; - // currently disabled: https://github.com/rust-lang/rust/issues/113333 - // cr#"aaa"#; - // cr##"Hello "world"!"##; - // cr######" "### "## "# "######; - // cr######" "aa" "# "## "######; + cr"\aaa"; + cr#"Hello "world"!"#; + cr####" "### "## "# "####; + cr###" "aa" "# "## "###; + + r" + \a + multiline + string + "; } diff --git a/tests/ui/needless_raw_string_hashes.rs b/tests/ui/needless_raw_string_hashes.rs index 62abae83859..6113c5f25ae 100644 --- a/tests/ui/needless_raw_string_hashes.rs +++ b/tests/ui/needless_raw_string_hashes.rs @@ -3,17 +3,22 @@ #![feature(c_str_literals)] fn main() { - r#"aaa"#; + r#"\aaa"#; r##"Hello "world"!"##; r######" "### "## "# "######; r######" "aa" "# "## "######; - br#"aaa"#; + br#"\aaa"#; br##"Hello "world"!"##; br######" "### "## "# "######; br######" "aa" "# "## "######; - // currently disabled: https://github.com/rust-lang/rust/issues/113333 - // cr#"aaa"#; - // cr##"Hello "world"!"##; - // cr######" "### "## "# "######; - // cr######" "aa" "# "## "######; + cr#"\aaa"#; + cr##"Hello "world"!"##; + cr######" "### "## "# "######; + cr######" "aa" "# "## "######; + + r#" + \a + multiline + string + "#; } diff --git a/tests/ui/needless_raw_string_hashes.stderr b/tests/ui/needless_raw_string_hashes.stderr index 9649d59a71f..ab6a87c5e8d 100644 --- a/tests/ui/needless_raw_string_hashes.stderr +++ b/tests/ui/needless_raw_string_hashes.stderr @@ -1,40 +1,166 @@ error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:6:5 + | +LL | r#"\aaa"#; + | ^^^^^^^^^ + | + = note: `-D clippy::needless-raw-string-hashes` implied by `-D warnings` +help: remove all the hashes around the literal + | +LL - r#"\aaa"#; +LL + r"\aaa"; + | + +error: unnecessary hashes around raw string literal --> $DIR/needless_raw_string_hashes.rs:7:5 | LL | r##"Hello "world"!"##; - | ^^^^^^^^^^^^^^^^^^^^^ help: try: `r#"Hello "world"!"#` + | ^^^^^^^^^^^^^^^^^^^^^ + | +help: remove one hash from both sides of the literal + | +LL - r##"Hello "world"!"##; +LL + r#"Hello "world"!"#; | - = note: `-D clippy::needless-raw-string-hashes` implied by `-D warnings` error: unnecessary hashes around raw string literal --> $DIR/needless_raw_string_hashes.rs:8:5 | LL | r######" "### "## "# "######; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `r####" "### "## "# "####` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove 2 hashes from both sides of the literal + | +LL - r######" "### "## "# "######; +LL + r####" "### "## "# "####; + | error: unnecessary hashes around raw string literal --> $DIR/needless_raw_string_hashes.rs:9:5 | LL | r######" "aa" "# "## "######; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `r###" "aa" "# "## "###` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove 3 hashes from both sides of the literal + | +LL - r######" "aa" "# "## "######; +LL + r###" "aa" "# "## "###; + | + +error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:10:5 + | +LL | br#"\aaa"#; + | ^^^^^^^^^^ + | +help: remove all the hashes around the literal + | +LL - br#"\aaa"#; +LL + br"\aaa"; + | error: unnecessary hashes around raw string literal --> $DIR/needless_raw_string_hashes.rs:11:5 | LL | br##"Hello "world"!"##; - | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `br#"Hello "world"!"#` + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove one hash from both sides of the literal + | +LL - br##"Hello "world"!"##; +LL + br#"Hello "world"!"#; + | error: unnecessary hashes around raw string literal --> $DIR/needless_raw_string_hashes.rs:12:5 | LL | br######" "### "## "# "######; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `br####" "### "## "# "####` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove 2 hashes from both sides of the literal + | +LL - br######" "### "## "# "######; +LL + br####" "### "## "# "####; + | error: unnecessary hashes around raw string literal --> $DIR/needless_raw_string_hashes.rs:13:5 | LL | br######" "aa" "# "## "######; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `br###" "aa" "# "## "###` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove 3 hashes from both sides of the literal + | +LL - br######" "aa" "# "## "######; +LL + br###" "aa" "# "## "###; + | + +error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:14:5 + | +LL | cr#"\aaa"#; + | ^^^^^^^^^^ + | +help: remove all the hashes around the literal + | +LL - cr#"\aaa"#; +LL + cr"\aaa"; + | + +error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:15:5 + | +LL | cr##"Hello "world"!"##; + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove one hash from both sides of the literal + | +LL - cr##"Hello "world"!"##; +LL + cr#"Hello "world"!"#; + | + +error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:16:5 + | +LL | cr######" "### "## "# "######; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove 2 hashes from both sides of the literal + | +LL - cr######" "### "## "# "######; +LL + cr####" "### "## "# "####; + | + +error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:17:5 + | +LL | cr######" "aa" "# "## "######; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: remove 3 hashes from both sides of the literal + | +LL - cr######" "aa" "# "## "######; +LL + cr###" "aa" "# "## "###; + | + +error: unnecessary hashes around raw string literal + --> $DIR/needless_raw_string_hashes.rs:19:5 + | +LL | / r#" +LL | | \a +LL | | multiline +LL | | string +LL | | "#; + | |______^ + | +help: remove all the hashes around the literal + | +LL ~ r" +LL | \a +LL | multiline +LL | string +LL ~ "; + | -error: aborting due to 6 previous errors +error: aborting due to 13 previous errors diff --git a/tests/ui/never_loop.rs b/tests/ui/never_loop.rs index 001b94391ca..c67a6d4494e 100644 --- a/tests/ui/never_loop.rs +++ b/tests/ui/never_loop.rs @@ -337,10 +337,8 @@ pub fn test26() { pub fn test27() { loop { - //~^ ERROR: this loop never actually loops 'label: { let x = true; - // Lints because we cannot prove it's always `true` if x { break 'label; } @@ -349,6 +347,59 @@ pub fn test27() { } } +// issue 11004 +pub fn test29() { + loop { + 'label: { + if true { + break 'label; + } + return; + } + } +} + +pub fn test30() { + 'a: loop { + 'b: { + for j in 0..2 { + if j == 1 { + break 'b; + } + } + break 'a; + } + } +} + +pub fn test31(b: bool) { + 'a: loop { + 'b: { + 'c: loop { + //~^ ERROR: this loop never actually loops + if b { break 'c } else { break 'b } + } + continue 'a; + } + break 'a; + } +} + +pub fn test32() { + loop { + //~^ ERROR: this loop never actually loops + panic!("oh no"); + } + loop { + //~^ ERROR: this loop never actually loops + unimplemented!("not yet"); + } + loop { + // no error + todo!("maybe later"); + } +} + fn main() { test1(); test2(); diff --git a/tests/ui/never_loop.stderr b/tests/ui/never_loop.stderr index 6d6d2c8ac52..50cec32cabd 100644 --- a/tests/ui/never_loop.stderr +++ b/tests/ui/never_loop.stderr @@ -153,16 +153,31 @@ LL | if let Some(_) = (0..20).next() { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: this loop never actually loops - --> $DIR/never_loop.rs:339:5 + --> $DIR/never_loop.rs:378:13 + | +LL | / 'c: loop { +LL | | +LL | | if b { break 'c } else { break 'b } +LL | | } + | |_____________^ + +error: this loop never actually loops + --> $DIR/never_loop.rs:389:5 | LL | / loop { LL | | -LL | | 'label: { -LL | | let x = true; -... | -LL | | } +LL | | panic!("oh no"); +LL | | } + | |_____^ + +error: this loop never actually loops + --> $DIR/never_loop.rs:393:5 + | +LL | / loop { +LL | | +LL | | unimplemented!("not yet"); LL | | } | |_____^ -error: aborting due to 14 previous errors +error: aborting due to 16 previous errors diff --git a/tests/ui/similar_names.rs b/tests/ui/similar_names.rs index c5a941316da..f46af56c6e2 100644 --- a/tests/ui/similar_names.rs +++ b/tests/ui/similar_names.rs @@ -3,6 +3,7 @@ unused, clippy::println_empty_string, clippy::empty_loop, + clippy::never_loop, clippy::diverging_sub_expression, clippy::let_unit_value )] diff --git a/tests/ui/similar_names.stderr b/tests/ui/similar_names.stderr index 059e26d5891..00735617bc2 100644 --- a/tests/ui/similar_names.stderr +++ b/tests/ui/similar_names.stderr @@ -1,84 +1,84 @@ error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:21:9 + --> $DIR/similar_names.rs:22:9 | LL | let bpple: i32; | ^^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:19:9 + --> $DIR/similar_names.rs:20:9 | LL | let apple: i32; | ^^^^^ = note: `-D clippy::similar-names` implied by `-D warnings` error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:24:9 + --> $DIR/similar_names.rs:25:9 | LL | let cpple: i32; | ^^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:19:9 + --> $DIR/similar_names.rs:20:9 | LL | let apple: i32; | ^^^^^ error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:49:9 + --> $DIR/similar_names.rs:50:9 | LL | let bluby: i32; | ^^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:48:9 + --> $DIR/similar_names.rs:49:9 | LL | let blubx: i32; | ^^^^^ error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:54:9 + --> $DIR/similar_names.rs:55:9 | LL | let coke: i32; | ^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:52:9 + --> $DIR/similar_names.rs:53:9 | LL | let cake: i32; | ^^^^ error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:73:9 + --> $DIR/similar_names.rs:74:9 | LL | let xyzeabc: i32; | ^^^^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:71:9 + --> $DIR/similar_names.rs:72:9 | LL | let xyz1abc: i32; | ^^^^^^^ error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:78:9 + --> $DIR/similar_names.rs:79:9 | LL | let parsee: i32; | ^^^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:76:9 + --> $DIR/similar_names.rs:77:9 | LL | let parser: i32; | ^^^^^^ error: binding's name is too similar to existing binding - --> $DIR/similar_names.rs:100:16 + --> $DIR/similar_names.rs:101:16 | LL | bpple: sprang, | ^^^^^^ | note: existing binding defined here - --> $DIR/similar_names.rs:99:16 + --> $DIR/similar_names.rs:100:16 | LL | apple: spring, | ^^^^^^ diff --git a/tests/ui/vec.fixed b/tests/ui/vec.fixed index 3ff2acbe28f..bcbca971a78 100644 --- a/tests/ui/vec.fixed +++ b/tests/ui/vec.fixed @@ -120,6 +120,7 @@ fn issue11075() { stringify!($e) }; } + #[allow(clippy::never_loop)] for _string in [repro!(true), repro!(null)] { unimplemented!(); } diff --git a/tests/ui/vec.rs b/tests/ui/vec.rs index 2ab025f424a..087425585de 100644 --- a/tests/ui/vec.rs +++ b/tests/ui/vec.rs @@ -120,6 +120,7 @@ fn issue11075() { stringify!($e) }; } + #[allow(clippy::never_loop)] for _string in vec![repro!(true), repro!(null)] { unimplemented!(); } diff --git a/tests/ui/vec.stderr b/tests/ui/vec.stderr index 5cd6d9fa8c7..a133b4041d4 100644 --- a/tests/ui/vec.stderr +++ b/tests/ui/vec.stderr @@ -85,31 +85,31 @@ LL | for _ in vec![1, 2, 3] {} | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:123:20 + --> $DIR/vec.rs:124:20 | LL | for _string in vec![repro!(true), repro!(null)] { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[repro!(true), repro!(null)]` error: useless use of `vec!` - --> $DIR/vec.rs:140:18 + --> $DIR/vec.rs:141:18 | LL | in_macro!(1, vec![1, 2], vec![1; 2]); | ^^^^^^^^^^ help: you can use an array directly: `[1, 2]` error: useless use of `vec!` - --> $DIR/vec.rs:140:30 + --> $DIR/vec.rs:141:30 | LL | in_macro!(1, vec![1, 2], vec![1; 2]); | ^^^^^^^^^^ help: you can use an array directly: `[1; 2]` error: useless use of `vec!` - --> $DIR/vec.rs:159:14 + --> $DIR/vec.rs:160:14 | LL | for a in vec![1, 2, 3] { | ^^^^^^^^^^^^^ help: you can use an array directly: `[1, 2, 3]` error: useless use of `vec!` - --> $DIR/vec.rs:163:14 + --> $DIR/vec.rs:164:14 | LL | for a in vec![String::new(), String::new()] { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: you can use an array directly: `[String::new(), String::new()]` diff --git a/tests/ui/write_literal_2.stderr b/tests/ui/write_literal_2.stderr index 84b302d8d3b..20b1f0e2a45 100644 --- a/tests/ui/write_literal_2.stderr +++ b/tests/ui/write_literal_2.stderr @@ -2,9 +2,14 @@ error: unnecessary raw string literal --> $DIR/write_literal_2.rs:13:24 | LL | writeln!(v, r"{}", r"{hello}"); - | ^^^^^^^^^^ help: try: `"{hello}"` + | ^^^^^^^^^^ | = note: `-D clippy::needless-raw-strings` implied by `-D warnings` +help: try + | +LL - writeln!(v, r"{}", r"{hello}"); +LL + writeln!(v, r"{}", "{hello}"); + | error: literal with an empty format string --> $DIR/write_literal_2.rs:10:23 |