From 98c5f37ad2f0b40721805b3af21d26109297ce95 Mon Sep 17 00:00:00 2001 From: "A.A.Abroskin" Date: Wed, 26 Dec 2018 01:29:03 +0300 Subject: [PATCH 01/15] Add assert(true) and assert(false) lints --- clippy_lints/src/assert_checks.rs | 78 ++++++++++++++++++++ clippy_lints/src/lib.rs | 6 ++ tests/ui/assert_checks.rs | 13 ++++ tests/ui/assert_checks.stderr | 18 +++++ tests/ui/attrs.rs | 2 +- tests/ui/empty_line_after_outer_attribute.rs | 2 +- tests/ui/panic_unimplemented.rs | 2 +- 7 files changed, 118 insertions(+), 3 deletions(-) create mode 100644 clippy_lints/src/assert_checks.rs create mode 100644 tests/ui/assert_checks.rs create mode 100644 tests/ui/assert_checks.stderr diff --git a/clippy_lints/src/assert_checks.rs b/clippy_lints/src/assert_checks.rs new file mode 100644 index 000000000000..ecd71952b93c --- /dev/null +++ b/clippy_lints/src/assert_checks.rs @@ -0,0 +1,78 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use crate::rustc::hir::{Expr, ExprKind}; +use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use crate::rustc::{declare_tool_lint, lint_array}; +use crate::syntax::ast::LitKind; +use crate::utils::{is_direct_expn_of, span_lint}; +use if_chain::if_chain; + +/// **What it does:** Check explicit call assert!(true) +/// +/// **Why is this bad?** Will be optimized out by the compiler +/// +/// **Known problems:** None +/// +/// **Example:** +/// ```rust +/// assert!(true) +/// ``` +declare_clippy_lint! { + pub EXPLICIT_TRUE, + correctness, + "assert!(true) will be optimized out by the compiler" +} + +/// **What it does:** Check explicit call assert!(false) +/// +/// **Why is this bad?** Should probably be replaced by a panic!() +/// +/// **Known problems:** None +/// +/// **Example:** +/// ```rust +/// assert!(false) +/// ``` +declare_clippy_lint! { + pub EXPLICIT_FALSE, + correctness, + "assert!(false) should probably be replaced by a panic!()r" +} + +pub struct AssertChecks; + +impl LintPass for AssertChecks { + fn get_lints(&self) -> LintArray { + lint_array![EXPLICIT_TRUE, EXPLICIT_FALSE] + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssertChecks { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { + if_chain! { + if is_direct_expn_of(e.span, "assert").is_some(); + if let ExprKind::Unary(_, ref lit) = e.node; + if let ExprKind::Lit(ref inner) = lit.node; + then { + match inner.node { + LitKind::Bool(true) => { + span_lint(cx, EXPLICIT_TRUE, e.span, + "assert!(true) will be optimized out by the compiler"); + }, + LitKind::Bool(false) => { + span_lint(cx, EXPLICIT_FALSE, e.span, + "assert!(false) should probably be replaced by a panic!()"); + }, + _ => (), + } + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2e515cc8aea6..1f4cb27891b9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -88,6 +88,7 @@ mod utils; // begin lints modules, do not remove this comment, it’s used in `update_lints` pub mod approx_const; pub mod arithmetic; +pub mod assert_checks; pub mod assign_ops; pub mod attrs; pub mod bit_mask; @@ -486,6 +487,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box ptr_offset_with_cast::Pass); reg.register_late_lint_pass(box redundant_clone::RedundantClone); reg.register_late_lint_pass(box slow_vector_initialization::Pass); + reg.register_late_lint_pass(box assert_checks::AssertChecks); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -563,6 +565,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::all", Some("clippy"), vec![ approx_const::APPROX_CONSTANT, + assert_checks::EXPLICIT_TRUE, + assert_checks::EXPLICIT_FALSE, assign_ops::ASSIGN_OP_PATTERN, assign_ops::MISREFACTORED_ASSIGN_OP, attrs::DEPRECATED_CFG_ATTR, @@ -940,6 +944,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::correctness", Some("clippy_correctness"), vec![ approx_const::APPROX_CONSTANT, + assert_checks::EXPLICIT_TRUE, + assert_checks::EXPLICIT_FALSE, attrs::DEPRECATED_SEMVER, attrs::USELESS_ATTRIBUTE, bit_mask::BAD_BIT_MASK, diff --git a/tests/ui/assert_checks.rs b/tests/ui/assert_checks.rs new file mode 100644 index 000000000000..811046d060a2 --- /dev/null +++ b/tests/ui/assert_checks.rs @@ -0,0 +1,13 @@ +// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +fn main() { + assert!(true); + assert!(false); +} diff --git a/tests/ui/assert_checks.stderr b/tests/ui/assert_checks.stderr new file mode 100644 index 000000000000..fd7e4e014201 --- /dev/null +++ b/tests/ui/assert_checks.stderr @@ -0,0 +1,18 @@ +error: assert!(true) will be optimized out by the compiler + --> $DIR/assert_checks.rs:11:5 + | +11 | assert!(true); + | ^^^^^^^^^^^^^^ + | + = note: #[deny(clippy::explicit_true)] on by default + +error: assert!(false) should probably be replaced by a panic!() + --> $DIR/assert_checks.rs:12:5 + | +12 | assert!(false); + | ^^^^^^^^^^^^^^^ + | + = note: #[deny(clippy::explicit_false)] on by default + +error: aborting due to 2 previous errors + diff --git a/tests/ui/attrs.rs b/tests/ui/attrs.rs index 413c30a19453..c0ea73297181 100644 --- a/tests/ui/attrs.rs +++ b/tests/ui/attrs.rs @@ -8,7 +8,7 @@ // except according to those terms. #![warn(clippy::inline_always, clippy::deprecated_semver)] - +#![allow(clippy::assert_checks::explicit_true)] #[inline(always)] fn test_attr_lint() { assert!(true) diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs index ede1244df7ed..a6e6adcac5c1 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -8,7 +8,7 @@ // except according to those terms. #![warn(clippy::empty_line_after_outer_attr)] - +#![allow(clippy::assert_checks::explicit_true)] // This should produce a warning #[crate_type = "lib"] diff --git a/tests/ui/panic_unimplemented.rs b/tests/ui/panic_unimplemented.rs index 93dec197ff5f..3c568658a6cf 100644 --- a/tests/ui/panic_unimplemented.rs +++ b/tests/ui/panic_unimplemented.rs @@ -8,7 +8,7 @@ // except according to those terms. #![warn(clippy::panic_params, clippy::unimplemented)] - +#![allow(clippy::assert_checks::explicit_true)] fn missing() { if true { panic!("{}"); From 3d9535a1067ad2913eab4bfd44b220bab0655b47 Mon Sep 17 00:00:00 2001 From: "A.A.Abroskin" Date: Thu, 27 Dec 2018 16:12:11 +0300 Subject: [PATCH 02/15] Add unreachable!() as option --- clippy_lints/src/assert_checks.rs | 6 +++--- tests/ui/assert_checks.stderr | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/assert_checks.rs b/clippy_lints/src/assert_checks.rs index ecd71952b93c..dcc11951b267 100644 --- a/clippy_lints/src/assert_checks.rs +++ b/clippy_lints/src/assert_checks.rs @@ -32,7 +32,7 @@ declare_clippy_lint! { /// **What it does:** Check explicit call assert!(false) /// -/// **Why is this bad?** Should probably be replaced by a panic!() +/// **Why is this bad?** Should probably be replaced by a panic!() or unreachable!() /// /// **Known problems:** None /// @@ -43,7 +43,7 @@ declare_clippy_lint! { declare_clippy_lint! { pub EXPLICIT_FALSE, correctness, - "assert!(false) should probably be replaced by a panic!()r" + "assert!(false) should probably be replaced by a panic!() or unreachable!()" } pub struct AssertChecks; @@ -68,7 +68,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssertChecks { }, LitKind::Bool(false) => { span_lint(cx, EXPLICIT_FALSE, e.span, - "assert!(false) should probably be replaced by a panic!()"); + "assert!(false) should probably be replaced by a panic!() or unreachable!()"); }, _ => (), } diff --git a/tests/ui/assert_checks.stderr b/tests/ui/assert_checks.stderr index fd7e4e014201..e20399416506 100644 --- a/tests/ui/assert_checks.stderr +++ b/tests/ui/assert_checks.stderr @@ -6,7 +6,7 @@ error: assert!(true) will be optimized out by the compiler | = note: #[deny(clippy::explicit_true)] on by default -error: assert!(false) should probably be replaced by a panic!() +error: assert!(false) should probably be replaced by a panic!() or unreachable!() --> $DIR/assert_checks.rs:12:5 | 12 | assert!(false); From 96058616e2a5d8af709da67cb03c35715437a990 Mon Sep 17 00:00:00 2001 From: "A.A.Abroskin" Date: Thu, 27 Dec 2018 16:48:11 +0300 Subject: [PATCH 03/15] run ./util/dev update_lints --- CHANGELOG.md | 2 ++ README.md | 6 ++++++ clippy_lints/src/lib.rs | 4 ++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index efa637b185cd..603fb0b5b7bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -679,8 +679,10 @@ All notable changes to this project will be documented in this file. [`expect_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call [`expl_impl_clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy [`explicit_counter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_counter_loop +[`explicit_false`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_false [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop [`explicit_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop +[`explicit_true`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_true [`explicit_write`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write [`extend_from_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_from_slice [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes diff --git a/README.md b/README.md index be54424dc381..658b2a70adc2 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,13 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. +<<<<<<< HEAD [There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +||||||| merged common ancestors +[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +======= +[There are 293 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +>>>>>>> run ./util/dev update_lints We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 1f4cb27891b9..8af73a4f99b2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -565,8 +565,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::all", Some("clippy"), vec![ approx_const::APPROX_CONSTANT, - assert_checks::EXPLICIT_TRUE, assert_checks::EXPLICIT_FALSE, + assert_checks::EXPLICIT_TRUE, assign_ops::ASSIGN_OP_PATTERN, assign_ops::MISREFACTORED_ASSIGN_OP, attrs::DEPRECATED_CFG_ATTR, @@ -944,8 +944,8 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::correctness", Some("clippy_correctness"), vec![ approx_const::APPROX_CONSTANT, - assert_checks::EXPLICIT_TRUE, assert_checks::EXPLICIT_FALSE, + assert_checks::EXPLICIT_TRUE, attrs::DEPRECATED_SEMVER, attrs::USELESS_ATTRIBUTE, bit_mask::BAD_BIT_MASK, From 906b51637ca4bfa0cf68b909160937546234a2e2 Mon Sep 17 00:00:00 2001 From: "A.A.Abroskin" Date: Wed, 9 Jan 2019 13:38:38 +0300 Subject: [PATCH 04/15] change assert_checks to assertions_on_constants --- CHANGELOG.md | 3 +- README.md | 4 +- ...t_checks.rs => assertions_on_constants.rs} | 54 +++++++++---------- clippy_lints/src/lib.rs | 10 ++-- tests/ui/assert_checks.stderr | 18 ------- ...t_checks.rs => assertions_on_constants.rs} | 0 tests/ui/assertions_on_constants.stderr | 16 ++++++ tests/ui/attrs.rs | 2 +- tests/ui/empty_line_after_outer_attribute.rs | 2 +- tests/ui/panic_unimplemented.rs | 2 +- 10 files changed, 51 insertions(+), 60 deletions(-) rename clippy_lints/src/{assert_checks.rs => assertions_on_constants.rs} (58%) delete mode 100644 tests/ui/assert_checks.stderr rename tests/ui/{assert_checks.rs => assertions_on_constants.rs} (100%) create mode 100644 tests/ui/assertions_on_constants.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 603fb0b5b7bd..69d0350eb09f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -616,6 +616,7 @@ All notable changes to this project will be documented in this file. [`absurd_extreme_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#absurd_extreme_comparisons [`almost_swapped`]: https://rust-lang.github.io/rust-clippy/master/index.html#almost_swapped [`approx_constant`]: https://rust-lang.github.io/rust-clippy/master/index.html#approx_constant +[`assertions_on_constants`]: https://rust-lang.github.io/rust-clippy/master/index.html#assertions_on_constants [`assign_op_pattern`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_op_pattern [`assign_ops`]: https://rust-lang.github.io/rust-clippy/master/index.html#assign_ops [`bad_bit_mask`]: https://rust-lang.github.io/rust-clippy/master/index.html#bad_bit_mask @@ -679,10 +680,8 @@ All notable changes to this project will be documented in this file. [`expect_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call [`expl_impl_clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy [`explicit_counter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_counter_loop -[`explicit_false`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_false [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop [`explicit_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_iter_loop -[`explicit_true`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_true [`explicit_write`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_write [`extend_from_slice`]: https://rust-lang.github.io/rust-clippy/master/index.html#extend_from_slice [`extra_unused_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#extra_unused_lifetimes diff --git a/README.md b/README.md index 658b2a70adc2..751f0add8eb5 100644 --- a/README.md +++ b/README.md @@ -8,11 +8,11 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. <<<<<<< HEAD -[There are 290 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) ||||||| merged common ancestors [There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) ======= -[There are 293 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) >>>>>>> run ./util/dev update_lints We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/assert_checks.rs b/clippy_lints/src/assertions_on_constants.rs similarity index 58% rename from clippy_lints/src/assert_checks.rs rename to clippy_lints/src/assertions_on_constants.rs index dcc11951b267..0068d6fa1578 100644 --- a/clippy_lints/src/assert_checks.rs +++ b/clippy_lints/src/assertions_on_constants.rs @@ -11,50 +11,40 @@ use crate::rustc::hir::{Expr, ExprKind}; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::syntax::ast::LitKind; -use crate::utils::{is_direct_expn_of, span_lint}; +use crate::utils::{is_direct_expn_of, span_lint, span_lint_and_sugg}; +use rustc_errors::Applicability; use if_chain::if_chain; -/// **What it does:** Check explicit call assert!(true) +/// **What it does:** Check explicit call assert!(true/false) /// -/// **Why is this bad?** Will be optimized out by the compiler -/// -/// **Known problems:** None -/// -/// **Example:** -/// ```rust -/// assert!(true) -/// ``` -declare_clippy_lint! { - pub EXPLICIT_TRUE, - correctness, - "assert!(true) will be optimized out by the compiler" -} - -/// **What it does:** Check explicit call assert!(false) -/// -/// **Why is this bad?** Should probably be replaced by a panic!() or unreachable!() +/// **Why is this bad?** Will be optimized out by the compiler or should probably be replaced by a panic!() or unreachable!() /// /// **Known problems:** None /// /// **Example:** /// ```rust /// assert!(false) +/// // or +/// assert!(true) +/// // or +/// const B: bool = false; +/// assert!(B) /// ``` declare_clippy_lint! { - pub EXPLICIT_FALSE, - correctness, - "assert!(false) should probably be replaced by a panic!() or unreachable!()" + pub ASSERTIONS_ON_CONSTANTS, + style, + "assert!(true/false) will be optimized out by the compiler/should probably be replaced by a panic!() or unreachable!()" } -pub struct AssertChecks; +pub struct AssertionsOnConstants; -impl LintPass for AssertChecks { +impl LintPass for AssertionsOnConstants { fn get_lints(&self) -> LintArray { - lint_array![EXPLICIT_TRUE, EXPLICIT_FALSE] + lint_array![ASSERTIONS_ON_CONSTANTS] } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssertChecks { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssertionsOnConstants { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { if_chain! { if is_direct_expn_of(e.span, "assert").is_some(); @@ -63,12 +53,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssertChecks { then { match inner.node { LitKind::Bool(true) => { - span_lint(cx, EXPLICIT_TRUE, e.span, + span_lint(cx, ASSERTIONS_ON_CONSTANTS, e.span, "assert!(true) will be optimized out by the compiler"); }, LitKind::Bool(false) => { - span_lint(cx, EXPLICIT_FALSE, e.span, - "assert!(false) should probably be replaced by a panic!() or unreachable!()"); + span_lint_and_sugg( + cx, + ASSERTIONS_ON_CONSTANTS, + e.span, + "assert!(false) should probably be replaced", + "try", + "panic!()".to_string(), + Applicability::MachineApplicable); }, _ => (), } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8af73a4f99b2..192c9226b699 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -88,7 +88,7 @@ mod utils; // begin lints modules, do not remove this comment, it’s used in `update_lints` pub mod approx_const; pub mod arithmetic; -pub mod assert_checks; +pub mod assertions_on_constants; pub mod assign_ops; pub mod attrs; pub mod bit_mask; @@ -487,7 +487,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_late_lint_pass(box ptr_offset_with_cast::Pass); reg.register_late_lint_pass(box redundant_clone::RedundantClone); reg.register_late_lint_pass(box slow_vector_initialization::Pass); - reg.register_late_lint_pass(box assert_checks::AssertChecks); + reg.register_late_lint_pass(box assertions_on_constants::AssertionsOnConstants); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -565,8 +565,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::all", Some("clippy"), vec![ approx_const::APPROX_CONSTANT, - assert_checks::EXPLICIT_FALSE, - assert_checks::EXPLICIT_TRUE, + assertions_on_constants::ASSERTIONS_ON_CONSTANTS, assign_ops::ASSIGN_OP_PATTERN, assign_ops::MISREFACTORED_ASSIGN_OP, attrs::DEPRECATED_CFG_ATTR, @@ -788,6 +787,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { ]); reg.register_lint_group("clippy::style", Some("clippy_style"), vec![ + assertions_on_constants::ASSERTIONS_ON_CONSTANTS, assign_ops::ASSIGN_OP_PATTERN, attrs::UNKNOWN_CLIPPY_LINTS, bit_mask::VERBOSE_BIT_MASK, @@ -944,8 +944,6 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) { reg.register_lint_group("clippy::correctness", Some("clippy_correctness"), vec![ approx_const::APPROX_CONSTANT, - assert_checks::EXPLICIT_FALSE, - assert_checks::EXPLICIT_TRUE, attrs::DEPRECATED_SEMVER, attrs::USELESS_ATTRIBUTE, bit_mask::BAD_BIT_MASK, diff --git a/tests/ui/assert_checks.stderr b/tests/ui/assert_checks.stderr deleted file mode 100644 index e20399416506..000000000000 --- a/tests/ui/assert_checks.stderr +++ /dev/null @@ -1,18 +0,0 @@ -error: assert!(true) will be optimized out by the compiler - --> $DIR/assert_checks.rs:11:5 - | -11 | assert!(true); - | ^^^^^^^^^^^^^^ - | - = note: #[deny(clippy::explicit_true)] on by default - -error: assert!(false) should probably be replaced by a panic!() or unreachable!() - --> $DIR/assert_checks.rs:12:5 - | -12 | assert!(false); - | ^^^^^^^^^^^^^^^ - | - = note: #[deny(clippy::explicit_false)] on by default - -error: aborting due to 2 previous errors - diff --git a/tests/ui/assert_checks.rs b/tests/ui/assertions_on_constants.rs similarity index 100% rename from tests/ui/assert_checks.rs rename to tests/ui/assertions_on_constants.rs diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr new file mode 100644 index 000000000000..33104ed2066a --- /dev/null +++ b/tests/ui/assertions_on_constants.stderr @@ -0,0 +1,16 @@ +error: assert!(true) will be optimized out by the compiler + --> $DIR/assertions_on_constants.rs:11:5 + | +LL | assert!(true); + | ^^^^^^^^^^^^^^ + | + = note: `-D clippy::assertions-on-constants` implied by `-D warnings` + +error: assert!(false) should probably be replaced + --> $DIR/assertions_on_constants.rs:12:5 + | +LL | assert!(false); + | ^^^^^^^^^^^^^^^ help: try: `panic!()` + +error: aborting due to 2 previous errors + diff --git a/tests/ui/attrs.rs b/tests/ui/attrs.rs index c0ea73297181..2c7f67d4505a 100644 --- a/tests/ui/attrs.rs +++ b/tests/ui/attrs.rs @@ -8,7 +8,7 @@ // except according to those terms. #![warn(clippy::inline_always, clippy::deprecated_semver)] -#![allow(clippy::assert_checks::explicit_true)] +#![allow(clippy::assertions_on_constants::assertions_on_constants)] #[inline(always)] fn test_attr_lint() { assert!(true) diff --git a/tests/ui/empty_line_after_outer_attribute.rs b/tests/ui/empty_line_after_outer_attribute.rs index a6e6adcac5c1..2bb5037614db 100644 --- a/tests/ui/empty_line_after_outer_attribute.rs +++ b/tests/ui/empty_line_after_outer_attribute.rs @@ -8,7 +8,7 @@ // except according to those terms. #![warn(clippy::empty_line_after_outer_attr)] -#![allow(clippy::assert_checks::explicit_true)] +#![allow(clippy::assertions_on_constants::assertions_on_constants)] // This should produce a warning #[crate_type = "lib"] diff --git a/tests/ui/panic_unimplemented.rs b/tests/ui/panic_unimplemented.rs index 3c568658a6cf..309a22dc2151 100644 --- a/tests/ui/panic_unimplemented.rs +++ b/tests/ui/panic_unimplemented.rs @@ -8,7 +8,7 @@ // except according to those terms. #![warn(clippy::panic_params, clippy::unimplemented)] -#![allow(clippy::assert_checks::explicit_true)] +#![allow(clippy::assertions_on_constants::assertions_on_constants)] fn missing() { if true { panic!("{}"); From a9f8d3c8fdc18080df800e65d73a737ccfd08933 Mon Sep 17 00:00:00 2001 From: "A.A.Abroskin" Date: Wed, 9 Jan 2019 21:30:47 +0300 Subject: [PATCH 05/15] add assert(true/false, some message) tests --- clippy_lints/src/assertions_on_constants.rs | 55 +++++++++++++-------- tests/ui/assertions_on_constants.rs | 8 +++ tests/ui/assertions_on_constants.stderr | 39 ++++++++++++++- 3 files changed, 79 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/assertions_on_constants.rs b/clippy_lints/src/assertions_on_constants.rs index 0068d6fa1578..f88ef8e83edb 100644 --- a/clippy_lints/src/assertions_on_constants.rs +++ b/clippy_lints/src/assertions_on_constants.rs @@ -7,17 +7,18 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use crate::consts::{constant, Constant}; use crate::rustc::hir::{Expr, ExprKind}; use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use crate::rustc::{declare_tool_lint, lint_array}; use crate::syntax::ast::LitKind; -use crate::utils::{is_direct_expn_of, span_lint, span_lint_and_sugg}; -use rustc_errors::Applicability; +use crate::utils::{is_direct_expn_of, span_help_and_lint}; use if_chain::if_chain; -/// **What it does:** Check explicit call assert!(true/false) +/// **What it does:** Check to call assert!(true/false) /// -/// **Why is this bad?** Will be optimized out by the compiler or should probably be replaced by a panic!() or unreachable!() +/// **Why is this bad?** Will be optimized out by the compiler or should probably be replaced by a +/// panic!() or unreachable!() /// /// **Known problems:** None /// @@ -49,24 +50,36 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for AssertionsOnConstants { if_chain! { if is_direct_expn_of(e.span, "assert").is_some(); if let ExprKind::Unary(_, ref lit) = e.node; - if let ExprKind::Lit(ref inner) = lit.node; then { - match inner.node { - LitKind::Bool(true) => { - span_lint(cx, ASSERTIONS_ON_CONSTANTS, e.span, - "assert!(true) will be optimized out by the compiler"); - }, - LitKind::Bool(false) => { - span_lint_and_sugg( - cx, - ASSERTIONS_ON_CONSTANTS, - e.span, - "assert!(false) should probably be replaced", - "try", - "panic!()".to_string(), - Applicability::MachineApplicable); - }, - _ => (), + if let ExprKind::Lit(ref inner) = lit.node { + match inner.node { + LitKind::Bool(true) => { + span_help_and_lint(cx, ASSERTIONS_ON_CONSTANTS, e.span, + "assert!(true) will be optimized out by the compiler", + "remove it"); + }, + LitKind::Bool(false) => { + span_help_and_lint( + cx, ASSERTIONS_ON_CONSTANTS, e.span, + "assert!(false) should probably be replaced", + "use panic!() or unreachable!()"); + }, + _ => (), + } + } else if let Some(bool_const) = constant(cx, cx.tables, lit) { + match bool_const.0 { + Constant::Bool(true) => { + span_help_and_lint(cx, ASSERTIONS_ON_CONSTANTS, e.span, + "assert!(const: true) will be optimized out by the compiler", + "remove it"); + }, + Constant::Bool(false) => { + span_help_and_lint(cx, ASSERTIONS_ON_CONSTANTS, e.span, + "assert!(const: false) should probably be replaced", + "use panic!() or unreachable!()"); + }, + _ => (), + } } } } diff --git a/tests/ui/assertions_on_constants.rs b/tests/ui/assertions_on_constants.rs index 811046d060a2..dcefe83f8c2f 100644 --- a/tests/ui/assertions_on_constants.rs +++ b/tests/ui/assertions_on_constants.rs @@ -10,4 +10,12 @@ fn main() { assert!(true); assert!(false); + assert!(true, "true message"); + assert!(false, "false message"); + + const B: bool = true; + assert!(B); + + const C: bool = false; + assert!(C); } diff --git a/tests/ui/assertions_on_constants.stderr b/tests/ui/assertions_on_constants.stderr index 33104ed2066a..1f1a80e0e77b 100644 --- a/tests/ui/assertions_on_constants.stderr +++ b/tests/ui/assertions_on_constants.stderr @@ -5,12 +5,47 @@ LL | assert!(true); | ^^^^^^^^^^^^^^ | = note: `-D clippy::assertions-on-constants` implied by `-D warnings` + = help: remove it error: assert!(false) should probably be replaced --> $DIR/assertions_on_constants.rs:12:5 | LL | assert!(false); - | ^^^^^^^^^^^^^^^ help: try: `panic!()` + | ^^^^^^^^^^^^^^^ + | + = help: use panic!() or unreachable!() + +error: assert!(true) will be optimized out by the compiler + --> $DIR/assertions_on_constants.rs:13:5 + | +LL | assert!(true, "true message"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: remove it + +error: assert!(false) should probably be replaced + --> $DIR/assertions_on_constants.rs:14:5 + | +LL | assert!(false, "false message"); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = help: use panic!() or unreachable!() + +error: assert!(const: true) will be optimized out by the compiler + --> $DIR/assertions_on_constants.rs:17:5 + | +LL | assert!(B); + | ^^^^^^^^^^^ + | + = help: remove it + +error: assert!(const: false) should probably be replaced + --> $DIR/assertions_on_constants.rs:20:5 + | +LL | assert!(C); + | ^^^^^^^^^^^ + | + = help: use panic!() or unreachable!() -error: aborting due to 2 previous errors +error: aborting due to 6 previous errors From 58abdb591884bcde77d53a5933a78c89e26c6752 Mon Sep 17 00:00:00 2001 From: "A.A.Abroskin" Date: Wed, 9 Jan 2019 21:31:29 +0300 Subject: [PATCH 06/15] run ./util/dev update_lints --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8ca10da416dd..3a7e9a165c16 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 291 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 292 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: From 13b5ea4223480265059356ed80e233e9f1b8a570 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Sun, 20 Jan 2019 14:50:26 +0100 Subject: [PATCH 07/15] Fix automatic suggestion on `use_self`. --- clippy_lints/src/use_self.rs | 12 ++++++------ tests/ui/use_self.stderr | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index b72401e1cca7..696f87854a40 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -7,7 +7,7 @@ use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintC use rustc::ty; use rustc::{declare_tool_lint, lint_array}; use rustc_errors::Applicability; -use syntax_pos::symbol::keywords::SelfUpper; +use syntax_pos::{symbol::keywords::SelfUpper, Span}; /// **What it does:** Checks for unnecessary repetition of structure name when a /// replacement with `Self` is applicable. @@ -55,11 +55,11 @@ impl LintPass for UseSelf { const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; -fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path) { +fn span_use_self_lint(cx: &LateContext<'_, '_>, span: Span) { span_lint_and_sugg( cx, USE_SELF, - path.span, + span, "unnecessary structure name repetition", "use the applicable keyword", "Self".to_owned(), @@ -92,7 +92,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> { }; if !is_self_ty { - span_use_self_lint(self.cx, path); + span_use_self_lint(self.cx, path.span); } } } @@ -221,10 +221,10 @@ impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { fn visit_path(&mut self, path: &'tcx Path, _id: HirId) { if path.segments.last().expect(SEGMENTS_MSG).ident.name != SelfUpper.name() { if self.item_path.def == path.def { - span_use_self_lint(self.cx, path); + span_use_self_lint(self.cx, path.segments.first().expect(SEGMENTS_MSG).ident.span); } else if let Def::StructCtor(ctor_did, CtorKind::Fn) = path.def { if self.item_path.def.opt_def_id() == self.cx.tcx.parent_def_id(ctor_did) { - span_use_self_lint(self.cx, path); + span_use_self_lint(self.cx, path.span); } } } diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 9d23433ba645..649fcdbfff95 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -22,7 +22,7 @@ error: unnecessary structure name repetition --> $DIR/use_self.rs:15:13 | LL | Foo::new() - | ^^^^^^^^ help: use the applicable keyword: `Self` + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:20:25 @@ -34,7 +34,7 @@ error: unnecessary structure name repetition --> $DIR/use_self.rs:21:13 | LL | Foo::new() - | ^^^^^^^^ help: use the applicable keyword: `Self` + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:86:22 @@ -100,7 +100,7 @@ error: unnecessary structure name repetition --> $DIR/use_self.rs:101:13 | LL | Bad::default() - | ^^^^^^^^^^^^ help: use the applicable keyword: `Self` + | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition --> $DIR/use_self.rs:106:23 From 2e0977f3b4061ed626fe9bfe29c703d13346abc1 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Mon, 21 Jan 2019 13:06:32 +0100 Subject: [PATCH 08/15] Fixed potential mistakes with nesting. Added tests. --- clippy_lints/src/use_self.rs | 16 +++++++++++----- tests/ui/use_self.rs | 20 ++++++++++++++++++++ tests/ui/use_self.stderr | 20 +++++++++++++++++++- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 696f87854a40..b57847d8f4b0 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -7,7 +7,7 @@ use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintC use rustc::ty; use rustc::{declare_tool_lint, lint_array}; use rustc_errors::Applicability; -use syntax_pos::{symbol::keywords::SelfUpper, Span}; +use syntax_pos::symbol::keywords::SelfUpper; /// **What it does:** Checks for unnecessary repetition of structure name when a /// replacement with `Self` is applicable. @@ -55,7 +55,13 @@ impl LintPass for UseSelf { const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; -fn span_use_self_lint(cx: &LateContext<'_, '_>, span: Span) { +fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path) { + // path segments only include actual path, no methods or fields + let last_path_span = path.segments.last().expect(SEGMENTS_MSG).ident.span; + // `to()` doesn't shorten span, so we shorten it with `until(..)` + // and then include it with `to(..)` + let span = path.span.until(last_path_span).to(last_path_span); + span_lint_and_sugg( cx, USE_SELF, @@ -92,7 +98,7 @@ impl<'a, 'tcx> Visitor<'tcx> for TraitImplTyVisitor<'a, 'tcx> { }; if !is_self_ty { - span_use_self_lint(self.cx, path.span); + span_use_self_lint(self.cx, path); } } } @@ -221,10 +227,10 @@ impl<'a, 'tcx> Visitor<'tcx> for UseSelfVisitor<'a, 'tcx> { fn visit_path(&mut self, path: &'tcx Path, _id: HirId) { if path.segments.last().expect(SEGMENTS_MSG).ident.name != SelfUpper.name() { if self.item_path.def == path.def { - span_use_self_lint(self.cx, path.segments.first().expect(SEGMENTS_MSG).ident.span); + span_use_self_lint(self.cx, path); } else if let Def::StructCtor(ctor_did, CtorKind::Fn) = path.def { if self.item_path.def.opt_def_id() == self.cx.tcx.parent_def_id(ctor_did) { - span_use_self_lint(self.cx, path.span); + span_use_self_lint(self.cx, path); } } } diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index b839aead95a0..0cf406b18ce8 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -274,3 +274,23 @@ mod issue3410 { fn a(_: Vec) {} } } + +#[allow(clippy::no_effect)] +mod rustfix { + mod nested { + pub struct A {} + } + + impl nested::A { + const A: bool = true; + + fn fun_1() {} + + fn fun_2() { + nested::A::fun_1(); + nested::A::A; + + nested::A {}; + } + } +} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 649fcdbfff95..68ce7221d039 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -162,5 +162,23 @@ error: unnecessary structure name repetition LL | Bar { foo: Foo {} } | ^^^ help: use the applicable keyword: `Self` -error: aborting due to 26 previous errors +error: unnecessary structure name repetition + --> $DIR/use_self.rs:290:13 + | +LL | nested::A::fun_1(); + | ^^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:291:13 + | +LL | nested::A::A; + | ^^^^^^^^^ help: use the applicable keyword: `Self` + +error: unnecessary structure name repetition + --> $DIR/use_self.rs:293:13 + | +LL | nested::A {}; + | ^^^^^^^^^ help: use the applicable keyword: `Self` + +error: aborting due to 29 previous errors From 0f5c43a7227f830f1c083b00d89ceb2f57ccb923 Mon Sep 17 00:00:00 2001 From: Grzegorz Bartoszek Date: Tue, 22 Jan 2019 12:33:47 +0100 Subject: [PATCH 09/15] Added "make_return" and "blockify" convenience methods in Sugg and used them in "needless_bool". --- clippy_lints/src/needless_bool.rs | 14 ++++++-------- clippy_lints/src/utils/sugg.rs | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index a898a740cd81..3b1fea465f51 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -70,16 +70,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessBool { let reduce = |ret, not| { let mut applicability = Applicability::MachineApplicable; let snip = Sugg::hir_with_applicability(cx, pred, "", &mut applicability); - let snip = if not { !snip } else { snip }; + let mut snip = if not { !snip } else { snip }; - let mut hint = if ret { - format!("return {}", snip) - } else { - snip.to_string() - }; + if ret { + snip = snip.make_return(); + } if parent_node_is_if_expr(&e, &cx) { - hint = format!("{{ {} }}", hint); + snip = snip.blockify() } span_lint_and_sugg( @@ -88,7 +86,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessBool { e.span, "this if-then-else expression returns a bool literal", "you can reduce it to", - hint, + snip.to_string(), applicability, ); }; diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index d42af5fde3ae..b95ce17ed93d 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -206,6 +206,17 @@ impl<'a> Sugg<'a> { make_unop("&mut *", self) } + /// Convenience method to transform suggestion into a return call + pub fn make_return(self) -> Sugg<'static> { + Sugg::NonParen(Cow::Owned(format!("return {}", self))) + } + + /// Convenience method to transform suggestion into a block + /// where the suggestion is a trailing expression + pub fn blockify(self) -> Sugg<'static> { + Sugg::NonParen(Cow::Owned(format!("{{ {} }}", self))) + } + /// Convenience method to create the `..` or `...` /// suggestion. #[allow(dead_code)] @@ -578,3 +589,21 @@ impl<'a, 'b, 'c, T: LintContext<'c>> DiagnosticBuilderExt<'c, T> for rustc_error self.span_suggestion_with_applicability(remove_span, msg, String::new(), applicability); } } + +#[cfg(test)] +mod test { + use super::Sugg; + use std::borrow::Cow; + + const SUGGESTION: Sugg<'static> = Sugg::NonParen(Cow::Borrowed("function_call()")); + + #[test] + fn make_return_transform_sugg_into_a_return_call() { + assert_eq!("return function_call()", SUGGESTION.make_return().to_string()); + } + + #[test] + fn blockify_transforms_sugg_into_a_block() { + assert_eq!("{ function_call() }", SUGGESTION.blockify().to_string()); + } +} From e70f9456fcc2cafae9a94611e0e0dd6bc2fa6cb9 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Tue, 22 Jan 2019 14:43:59 +0100 Subject: [PATCH 10/15] Improve span shortening. Co-Authored-By: daxpedda <1645124+daxpedda@users.noreply.github.com> --- clippy_lints/src/use_self.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index b57847d8f4b0..5d4d5bac4c7c 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -60,7 +60,7 @@ fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path) { let last_path_span = path.segments.last().expect(SEGMENTS_MSG).ident.span; // `to()` doesn't shorten span, so we shorten it with `until(..)` // and then include it with `to(..)` - let span = path.span.until(last_path_span).to(last_path_span); + let span = path.span.with_hi(last_path_span.hi()); span_lint_and_sugg( cx, From e6f2239bc3bfec1cf37a0e28bd83b6ba9b809520 Mon Sep 17 00:00:00 2001 From: daxpedda Date: Tue, 22 Jan 2019 15:16:54 +0100 Subject: [PATCH 11/15] Added rustfix to the test. --- tests/ui/use_self.fixed | 299 +++++++++++++++++++++++++++++++++++++++ tests/ui/use_self.rs | 5 +- tests/ui/use_self.stderr | 58 ++++---- 3 files changed, 332 insertions(+), 30 deletions(-) create mode 100644 tests/ui/use_self.fixed diff --git a/tests/ui/use_self.fixed b/tests/ui/use_self.fixed new file mode 100644 index 000000000000..5eae9a7a8069 --- /dev/null +++ b/tests/ui/use_self.fixed @@ -0,0 +1,299 @@ +// run-rustfix + +#![warn(clippy::use_self)] +#![allow(dead_code)] +#![allow(clippy::should_implement_trait)] + +fn main() {} + +mod use_self { + struct Foo {} + + impl Foo { + fn new() -> Self { + Self {} + } + fn test() -> Self { + Self::new() + } + } + + impl Default for Foo { + fn default() -> Self { + Self::new() + } + } +} + +mod better { + struct Foo {} + + impl Foo { + fn new() -> Self { + Self {} + } + fn test() -> Self { + Self::new() + } + } + + impl Default for Foo { + fn default() -> Self { + Self::new() + } + } +} + +mod lifetimes { + struct Foo<'a> { + foo_str: &'a str, + } + + impl<'a> Foo<'a> { + // Cannot use `Self` as return type, because the function is actually `fn foo<'b>(s: &'b str) -> + // Foo<'b>` + fn foo(s: &str) -> Foo { + Foo { foo_str: s } + } + // cannot replace with `Self`, because that's `Foo<'a>` + fn bar() -> Foo<'static> { + Foo { foo_str: "foo" } + } + + // FIXME: the lint does not handle lifetimed struct + // `Self` should be applicable here + fn clone(&self) -> Foo<'a> { + Foo { foo_str: self.foo_str } + } + } +} + +#[allow(clippy::boxed_local)] +mod traits { + + use std::ops::Mul; + + trait SelfTrait { + fn refs(p1: &Self) -> &Self; + fn ref_refs<'a>(p1: &'a &'a Self) -> &'a &'a Self; + fn mut_refs(p1: &mut Self) -> &mut Self; + fn nested(p1: Box, p2: (&u8, &Self)); + fn vals(r: Self) -> Self; + } + + #[derive(Default)] + struct Bad; + + impl SelfTrait for Bad { + fn refs(p1: &Self) -> &Self { + p1 + } + + fn ref_refs<'a>(p1: &'a &'a Self) -> &'a &'a Self { + p1 + } + + fn mut_refs(p1: &mut Self) -> &mut Self { + p1 + } + + fn nested(_p1: Box, _p2: (&u8, &Self)) {} + + fn vals(_: Self) -> Self { + Self::default() + } + } + + impl Mul for Bad { + type Output = Self; + + fn mul(self, rhs: Self) -> Self { + rhs + } + } + + #[derive(Default)] + struct Good; + + impl SelfTrait for Good { + fn refs(p1: &Self) -> &Self { + p1 + } + + fn ref_refs<'a>(p1: &'a &'a Self) -> &'a &'a Self { + p1 + } + + fn mut_refs(p1: &mut Self) -> &mut Self { + p1 + } + + fn nested(_p1: Box, _p2: (&u8, &Self)) {} + + fn vals(_: Self) -> Self { + Self::default() + } + } + + impl Mul for Good { + type Output = Self; + + fn mul(self, rhs: Self) -> Self { + rhs + } + } + + trait NameTrait { + fn refs(p1: &u8) -> &u8; + fn ref_refs<'a>(p1: &'a &'a u8) -> &'a &'a u8; + fn mut_refs(p1: &mut u8) -> &mut u8; + fn nested(p1: Box, p2: (&u8, &u8)); + fn vals(p1: u8) -> u8; + } + + // Using `Self` instead of the type name is OK + impl NameTrait for u8 { + fn refs(p1: &Self) -> &Self { + p1 + } + + fn ref_refs<'a>(p1: &'a &'a Self) -> &'a &'a Self { + p1 + } + + fn mut_refs(p1: &mut Self) -> &mut Self { + p1 + } + + fn nested(_p1: Box, _p2: (&Self, &Self)) {} + + fn vals(_: Self) -> Self { + Self::default() + } + } + + // Check that self arg isn't linted + impl Clone for Good { + fn clone(&self) -> Self { + // Note: Not linted and it wouldn't be valid + // because "can't use `Self` as a constructor`" + Good + } + } +} + +mod issue2894 { + trait IntoBytes { + fn into_bytes(&self) -> Vec; + } + + // This should not be linted + impl IntoBytes for u8 { + fn into_bytes(&self) -> Vec { + vec![*self] + } + } +} + +mod existential { + struct Foo; + + impl Foo { + fn bad(foos: &[Self]) -> impl Iterator { + foos.iter() + } + + fn good(foos: &[Self]) -> impl Iterator { + foos.iter() + } + } +} + +mod tuple_structs { + pub struct TS(i32); + + impl TS { + pub fn ts() -> Self { + Self(0) + } + } +} + +mod macros { + macro_rules! use_self_expand { + () => { + fn new() -> Self { + Self {} + } + }; + } + + struct Foo {} + + impl Foo { + use_self_expand!(); // Should lint in local macros + } +} + +mod nesting { + struct Foo {} + impl Foo { + fn foo() { + use self::Foo; // Can't use Self here + struct Bar { + foo: Foo, // Foo != Self + } + + impl Bar { + fn bar() -> Self { + Self { foo: Foo {} } + } + } + } + } + + enum Enum { + A, + } + impl Enum { + fn method() { + #[allow(unused_imports)] + use self::Enum::*; // Issue 3425 + static STATIC: Enum = Enum::A; // Can't use Self as type + } + } +} + +mod issue3410 { + + struct A; + struct B; + + trait Trait { + fn a(v: T); + } + + impl Trait> for Vec { + fn a(_: Vec) {} + } +} + +#[allow(clippy::no_effect, path_statements)] +mod rustfix { + mod nested { + pub struct A {} + } + + impl nested::A { + const A: bool = true; + + fn fun_1() {} + + fn fun_2() { + Self::fun_1(); + Self::A; + + Self {}; + } + } +} diff --git a/tests/ui/use_self.rs b/tests/ui/use_self.rs index 0cf406b18ce8..8e28bbbeb9c6 100644 --- a/tests/ui/use_self.rs +++ b/tests/ui/use_self.rs @@ -1,3 +1,5 @@ +// run-rustfix + #![warn(clippy::use_self)] #![allow(dead_code)] #![allow(clippy::should_implement_trait)] @@ -255,6 +257,7 @@ mod nesting { } impl Enum { fn method() { + #[allow(unused_imports)] use self::Enum::*; // Issue 3425 static STATIC: Enum = Enum::A; // Can't use Self as type } @@ -275,7 +278,7 @@ mod issue3410 { } } -#[allow(clippy::no_effect)] +#[allow(clippy::no_effect, path_statements)] mod rustfix { mod nested { pub struct A {} diff --git a/tests/ui/use_self.stderr b/tests/ui/use_self.stderr index 68ce7221d039..af9e15edb6cf 100644 --- a/tests/ui/use_self.stderr +++ b/tests/ui/use_self.stderr @@ -1,5 +1,5 @@ error: unnecessary structure name repetition - --> $DIR/use_self.rs:11:21 + --> $DIR/use_self.rs:13:21 | LL | fn new() -> Foo { | ^^^ help: use the applicable keyword: `Self` @@ -7,133 +7,133 @@ LL | fn new() -> Foo { = note: `-D clippy::use-self` implied by `-D warnings` error: unnecessary structure name repetition - --> $DIR/use_self.rs:12:13 + --> $DIR/use_self.rs:14:13 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:14:22 + --> $DIR/use_self.rs:16:22 | LL | fn test() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:15:13 + --> $DIR/use_self.rs:17:13 | LL | Foo::new() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:20:25 + --> $DIR/use_self.rs:22:25 | LL | fn default() -> Foo { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:21:13 + --> $DIR/use_self.rs:23:13 | LL | Foo::new() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:86:22 + --> $DIR/use_self.rs:88:22 | LL | fn refs(p1: &Bad) -> &Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:86:31 + --> $DIR/use_self.rs:88:31 | LL | fn refs(p1: &Bad) -> &Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:90:37 + --> $DIR/use_self.rs:92:37 | LL | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:90:53 + --> $DIR/use_self.rs:92:53 | LL | fn ref_refs<'a>(p1: &'a &'a Bad) -> &'a &'a Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:94:30 + --> $DIR/use_self.rs:96:30 | LL | fn mut_refs(p1: &mut Bad) -> &mut Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:94:43 + --> $DIR/use_self.rs:96:43 | LL | fn mut_refs(p1: &mut Bad) -> &mut Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:98:28 + --> $DIR/use_self.rs:100:28 | LL | fn nested(_p1: Box, _p2: (&u8, &Bad)) {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:98:46 + --> $DIR/use_self.rs:100:46 | LL | fn nested(_p1: Box, _p2: (&u8, &Bad)) {} | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:100:20 + --> $DIR/use_self.rs:102:20 | LL | fn vals(_: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:100:28 + --> $DIR/use_self.rs:102:28 | LL | fn vals(_: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:101:13 + --> $DIR/use_self.rs:103:13 | LL | Bad::default() | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:106:23 + --> $DIR/use_self.rs:108:23 | LL | type Output = Bad; | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:108:27 + --> $DIR/use_self.rs:110:27 | LL | fn mul(self, rhs: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:108:35 + --> $DIR/use_self.rs:110:35 | LL | fn mul(self, rhs: Bad) -> Bad { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:200:56 + --> $DIR/use_self.rs:202:56 | LL | fn bad(foos: &[Self]) -> impl Iterator { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:215:13 + --> $DIR/use_self.rs:217:13 | LL | TS(0) | ^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:223:25 + --> $DIR/use_self.rs:225:25 | LL | fn new() -> Foo { | ^^^ help: use the applicable keyword: `Self` @@ -142,7 +142,7 @@ LL | use_self_expand!(); // Should lint in local macros | ------------------- in this macro invocation error: unnecessary structure name repetition - --> $DIR/use_self.rs:224:17 + --> $DIR/use_self.rs:226:17 | LL | Foo {} | ^^^ help: use the applicable keyword: `Self` @@ -151,31 +151,31 @@ LL | use_self_expand!(); // Should lint in local macros | ------------------- in this macro invocation error: unnecessary structure name repetition - --> $DIR/use_self.rs:246:29 + --> $DIR/use_self.rs:248:29 | LL | fn bar() -> Bar { | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:247:21 + --> $DIR/use_self.rs:249:21 | LL | Bar { foo: Foo {} } | ^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:290:13 + --> $DIR/use_self.rs:293:13 | LL | nested::A::fun_1(); | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:291:13 + --> $DIR/use_self.rs:294:13 | LL | nested::A::A; | ^^^^^^^^^ help: use the applicable keyword: `Self` error: unnecessary structure name repetition - --> $DIR/use_self.rs:293:13 + --> $DIR/use_self.rs:296:13 | LL | nested::A {}; | ^^^^^^^^^ help: use the applicable keyword: `Self` From 3168023cc8027c3fa53c7777d4f27be6a06bf168 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 22 Jan 2019 15:17:05 +0100 Subject: [PATCH 12/15] Rustup --- clippy_lints/src/arithmetic.rs | 2 +- clippy_lints/src/utils/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/arithmetic.rs b/clippy_lints/src/arithmetic.rs index 473db1869ac0..efa53ff94c39 100644 --- a/clippy_lints/src/arithmetic.rs +++ b/clippy_lints/src/arithmetic.rs @@ -128,7 +128,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Arithmetic { } self.const_span = Some(body_span); }, - hir::BodyOwnerKind::Fn => (), + hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => (), } } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 5d94f0f3f051..ab394cc47eaa 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -65,7 +65,7 @@ pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool { pub fn in_constant(cx: &LateContext<'_, '_>, id: NodeId) -> bool { let parent_id = cx.tcx.hir().get_parent(id); match cx.tcx.hir().body_owner_kind(parent_id) { - hir::BodyOwnerKind::Fn => false, + hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => false, hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(..) => true, } } From 42d5a07f0ca4d0a3d17f2d2634862dc73bf82d2f Mon Sep 17 00:00:00 2001 From: daxpedda Date: Tue, 22 Jan 2019 15:23:45 +0100 Subject: [PATCH 13/15] Improving comments. --- clippy_lints/src/use_self.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clippy_lints/src/use_self.rs b/clippy_lints/src/use_self.rs index 5d4d5bac4c7c..88cf01987b55 100644 --- a/clippy_lints/src/use_self.rs +++ b/clippy_lints/src/use_self.rs @@ -58,8 +58,7 @@ const SEGMENTS_MSG: &str = "segments should be composed of at least 1 element"; fn span_use_self_lint(cx: &LateContext<'_, '_>, path: &Path) { // path segments only include actual path, no methods or fields let last_path_span = path.segments.last().expect(SEGMENTS_MSG).ident.span; - // `to()` doesn't shorten span, so we shorten it with `until(..)` - // and then include it with `to(..)` + // only take path up to the end of last_path_span let span = path.span.with_hi(last_path_span.hi()); span_lint_and_sugg( From 38cdf63acfd8a46ce5753a8767feab43c6382aa4 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 22 Jan 2019 15:28:51 +0100 Subject: [PATCH 14/15] Don't make decisions on values that don't represent the decision --- clippy_lints/src/utils/mod.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index ab394cc47eaa..a7af4a52714a 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -64,9 +64,14 @@ pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool { /// ``` pub fn in_constant(cx: &LateContext<'_, '_>, id: NodeId) -> bool { let parent_id = cx.tcx.hir().get_parent(id); - match cx.tcx.hir().body_owner_kind(parent_id) { - hir::BodyOwnerKind::Fn | hir::BodyOwnerKind::Closure => false, - hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(..) => true, + match cx.tcx.hir().get(parent_id) { + | Node::Item(&Item { node: ItemKind::Const(..), .. }) + | Node::TraitItem(&TraitItem { node: TraitItemKind::Const(..), .. }) + | Node::ImplItem(&ImplItem { node: ImplItemKind::Const(..), .. }) + | Node::AnonConst(_) + | Node::Item(&Item { node: ItemKind::Static(..), .. }) + => true, + _ => false, } } From d6c806378e6901b83d5ab8594dcd2c6347e0196a Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 22 Jan 2019 16:27:42 +0100 Subject: [PATCH 15/15] Rustfmt all the things --- clippy_lints/src/utils/mod.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index a7af4a52714a..c83b0f155fca 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -65,12 +65,23 @@ pub fn differing_macro_contexts(lhs: Span, rhs: Span) -> bool { pub fn in_constant(cx: &LateContext<'_, '_>, id: NodeId) -> bool { let parent_id = cx.tcx.hir().get_parent(id); match cx.tcx.hir().get(parent_id) { - | Node::Item(&Item { node: ItemKind::Const(..), .. }) - | Node::TraitItem(&TraitItem { node: TraitItemKind::Const(..), .. }) - | Node::ImplItem(&ImplItem { node: ImplItemKind::Const(..), .. }) + Node::Item(&Item { + node: ItemKind::Const(..), + .. + }) + | Node::TraitItem(&TraitItem { + node: TraitItemKind::Const(..), + .. + }) + | Node::ImplItem(&ImplItem { + node: ImplItemKind::Const(..), + .. + }) | Node::AnonConst(_) - | Node::Item(&Item { node: ItemKind::Static(..), .. }) - => true, + | Node::Item(&Item { + node: ItemKind::Static(..), + .. + }) => true, _ => false, } }