From a451b2af30c173b081f5d6150e558a65062a1dc8 Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sat, 19 Dec 2020 15:49:42 +0100 Subject: [PATCH 01/33] Added from_over_into lint --- CHANGELOG.md | 1 + clippy_lints/src/from_over_into.rs | 63 ++++++++++++++++++++++++++++++ clippy_lints/src/lib.rs | 5 +++ tests/ui/from_over_into.rs | 21 ++++++++++ tests/ui/from_over_into.stderr | 15 +++++++ tests/ui/unused_unit.fixed | 1 + tests/ui/unused_unit.rs | 1 + tests/ui/unused_unit.stderr | 38 +++++++++--------- 8 files changed, 126 insertions(+), 19 deletions(-) create mode 100644 clippy_lints/src/from_over_into.rs create mode 100644 tests/ui/from_over_into.rs create mode 100644 tests/ui/from_over_into.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index af3b1c1db2aed..de8da99cdee12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1841,6 +1841,7 @@ Released 2018-09-13 [`forget_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_copy [`forget_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#forget_ref [`from_iter_instead_of_collect`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_iter_instead_of_collect +[`from_over_into`]: https://rust-lang.github.io/rust-clippy/master/index.html#from_over_into [`future_not_send`]: https://rust-lang.github.io/rust-clippy/master/index.html#future_not_send [`get_last_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_last_with_len [`get_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#get_unwrap diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs new file mode 100644 index 0000000000000..fe7120b0f9a25 --- /dev/null +++ b/clippy_lints/src/from_over_into.rs @@ -0,0 +1,63 @@ +use crate::utils::paths::INTO; +use crate::utils::{match_def_path, span_lint_and_help}; +use if_chain::if_chain; +use rustc_hir as hir; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Searches for implementations of the `Into<..>` trait and suggests to implement `From<..>` instead. + /// + /// **Why is this bad?** According the std docs implementing `From<..>` is preferred since it gives you `Into<..>` for free where the reverse isn't true. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```rust + /// struct StringWrapper(String); + /// + /// impl Into for String { + /// fn into(self) -> StringWrapper { + /// StringWrapper(self) + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// struct StringWrapper(String); + /// + /// impl From for StringWrapper { + /// fn from(s: String) -> StringWrapper { + /// StringWrapper(s) + /// } + /// } + /// ``` + pub FROM_OVER_INTO, + style, + "Warns on implementations of `Into<..>` to use `From<..>`" +} + +declare_lint_pass!(FromOverInto => [FROM_OVER_INTO]); + +impl LateLintPass<'_> for FromOverInto { + fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id); + if_chain! { + if let hir::ItemKind::Impl{ .. } = &item.kind; + if let Some(impl_trait_ref) = cx.tcx.impl_trait_ref(impl_def_id); + if match_def_path(cx, impl_trait_ref.def_id, &INTO); + + then { + span_lint_and_help( + cx, + FROM_OVER_INTO, + item.span, + "An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true.", + None, + "consider implement From instead", + ); + } + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 02ba422a2f5b8..58587481922cc 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -207,6 +207,7 @@ mod float_literal; mod floating_point_arithmetic; mod format; mod formatting; +mod from_over_into; mod functions; mod future_not_send; mod get_last_with_len; @@ -614,6 +615,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING, &formatting::SUSPICIOUS_ELSE_FORMATTING, &formatting::SUSPICIOUS_UNARY_OP_FORMATTING, + &from_over_into::FROM_OVER_INTO, &functions::DOUBLE_MUST_USE, &functions::MUST_USE_CANDIDATE, &functions::MUST_USE_UNIT, @@ -1203,6 +1205,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box manual_unwrap_or::ManualUnwrapOr); store.register_late_pass(|| box manual_ok_or::ManualOkOr); store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs); + store.register_late_pass(|| box from_over_into::FromOverInto); store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync); let disallowed_methods = conf.disallowed_methods.iter().cloned().collect::>(); store.register_late_pass(move || box disallowed_method::DisallowedMethod::new(&disallowed_methods)); @@ -1417,6 +1420,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING), LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING), LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING), + LintId::of(&from_over_into::FROM_OVER_INTO), LintId::of(&functions::DOUBLE_MUST_USE), LintId::of(&functions::MUST_USE_UNIT), LintId::of(&functions::NOT_UNSAFE_PTR_ARG_DEREF), @@ -1663,6 +1667,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&formatting::SUSPICIOUS_ASSIGNMENT_FORMATTING), LintId::of(&formatting::SUSPICIOUS_ELSE_FORMATTING), LintId::of(&formatting::SUSPICIOUS_UNARY_OP_FORMATTING), + LintId::of(&from_over_into::FROM_OVER_INTO), LintId::of(&functions::DOUBLE_MUST_USE), LintId::of(&functions::MUST_USE_UNIT), LintId::of(&functions::RESULT_UNIT_ERR), diff --git a/tests/ui/from_over_into.rs b/tests/ui/from_over_into.rs new file mode 100644 index 0000000000000..292d0924fb17a --- /dev/null +++ b/tests/ui/from_over_into.rs @@ -0,0 +1,21 @@ +#![warn(clippy::from_over_into)] + +// this should throw an error +struct StringWrapper(String); + +impl Into for String { + fn into(self) -> StringWrapper { + StringWrapper(self) + } +} + +// this is fine +struct A(String); + +impl From for A { + fn from(s: String) -> A { + A(s) + } +} + +fn main() {} diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr new file mode 100644 index 0000000000000..17f30fa837ec4 --- /dev/null +++ b/tests/ui/from_over_into.stderr @@ -0,0 +1,15 @@ +error: An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true. + --> $DIR/from_over_into.rs:6:1 + | +LL | / impl Into for String { +LL | | fn into(self) -> StringWrapper { +LL | | StringWrapper(self) +LL | | } +LL | | } + | |_^ + | + = note: `-D clippy::from-over-into` implied by `-D warnings` + = help: consider implement From instead + +error: aborting due to previous error + diff --git a/tests/ui/unused_unit.fixed b/tests/ui/unused_unit.fixed index 7afc536135633..a192ebde3ebf4 100644 --- a/tests/ui/unused_unit.fixed +++ b/tests/ui/unused_unit.fixed @@ -11,6 +11,7 @@ #![deny(clippy::unused_unit)] #![allow(dead_code)] +#![allow(clippy::from_over_into)] struct Unitter; impl Unitter { diff --git a/tests/ui/unused_unit.rs b/tests/ui/unused_unit.rs index 96cef1ed5a5f2..96041a7dd850e 100644 --- a/tests/ui/unused_unit.rs +++ b/tests/ui/unused_unit.rs @@ -11,6 +11,7 @@ #![deny(clippy::unused_unit)] #![allow(dead_code)] +#![allow(clippy::from_over_into)] struct Unitter; impl Unitter { diff --git a/tests/ui/unused_unit.stderr b/tests/ui/unused_unit.stderr index c45634c2b6df7..02038b5fb6b5a 100644 --- a/tests/ui/unused_unit.stderr +++ b/tests/ui/unused_unit.stderr @@ -1,5 +1,5 @@ error: unneeded unit return type - --> $DIR/unused_unit.rs:18:28 + --> $DIR/unused_unit.rs:19:28 | LL | pub fn get_unit (), G>(&self, f: F, _g: G) -> () | ^^^^^^ help: remove the `-> ()` @@ -11,109 +11,109 @@ LL | #![deny(clippy::unused_unit)] | ^^^^^^^^^^^^^^^^^^^ error: unneeded unit return type - --> $DIR/unused_unit.rs:19:18 + --> $DIR/unused_unit.rs:20:18 | LL | where G: Fn() -> () { | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:18:58 + --> $DIR/unused_unit.rs:19:58 | LL | pub fn get_unit (), G>(&self, f: F, _g: G) -> () | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:20:26 + --> $DIR/unused_unit.rs:21:26 | LL | let _y: &dyn Fn() -> () = &f; | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:27:18 + --> $DIR/unused_unit.rs:28:18 | LL | fn into(self) -> () { | ^^^^^^ help: remove the `-> ()` error: unneeded unit expression - --> $DIR/unused_unit.rs:28:9 + --> $DIR/unused_unit.rs:29:9 | LL | () | ^^ help: remove the final `()` error: unneeded unit return type - --> $DIR/unused_unit.rs:33:29 + --> $DIR/unused_unit.rs:34:29 | LL | fn redundant (), G, H>(&self, _f: F, _g: G, _h: H) | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:35:19 + --> $DIR/unused_unit.rs:36:19 | LL | G: FnMut() -> (), | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:36:16 + --> $DIR/unused_unit.rs:37:16 | LL | H: Fn() -> (); | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:40:29 + --> $DIR/unused_unit.rs:41:29 | LL | fn redundant (), G, H>(&self, _f: F, _g: G, _h: H) | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:42:19 + --> $DIR/unused_unit.rs:43:19 | LL | G: FnMut() -> (), | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:43:16 + --> $DIR/unused_unit.rs:44:16 | LL | H: Fn() -> () {} | ^^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:46:17 + --> $DIR/unused_unit.rs:47:17 | LL | fn return_unit() -> () { () } | ^^^^^^ help: remove the `-> ()` error: unneeded unit expression - --> $DIR/unused_unit.rs:46:26 + --> $DIR/unused_unit.rs:47:26 | LL | fn return_unit() -> () { () } | ^^ help: remove the final `()` error: unneeded `()` - --> $DIR/unused_unit.rs:56:14 + --> $DIR/unused_unit.rs:57:14 | LL | break(); | ^^ help: remove the `()` error: unneeded `()` - --> $DIR/unused_unit.rs:58:11 + --> $DIR/unused_unit.rs:59:11 | LL | return(); | ^^ help: remove the `()` error: unneeded unit return type - --> $DIR/unused_unit.rs:75:10 + --> $DIR/unused_unit.rs:76:10 | LL | fn test()->(){} | ^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:78:11 + --> $DIR/unused_unit.rs:79:11 | LL | fn test2() ->(){} | ^^^^^ help: remove the `-> ()` error: unneeded unit return type - --> $DIR/unused_unit.rs:81:11 + --> $DIR/unused_unit.rs:82:11 | LL | fn test3()-> (){} | ^^^^^ help: remove the `-> ()` From dd005c17e72a48f8579cd26a7165c960b04f6aa3 Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sun, 20 Dec 2020 13:00:17 +0100 Subject: [PATCH 02/33] Added MSRV and fixed typo --- clippy_lints/src/from_over_into.rs | 30 ++++++++++++++++++++++----- clippy_lints/src/lib.rs | 2 +- tests/ui/from_over_into.stderr | 2 +- tests/ui/min_rust_version_attr.rs | 8 +++++++ tests/ui/min_rust_version_attr.stderr | 8 +++---- 5 files changed, 39 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index fe7120b0f9a25..c7988d6f01fa5 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -1,9 +1,12 @@ use crate::utils::paths::INTO; -use crate::utils::{match_def_path, span_lint_and_help}; +use crate::utils::{match_def_path, meets_msrv, span_lint_and_help}; use if_chain::if_chain; use rustc_hir as hir; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_semver::RustcVersion; +use rustc_session::{declare_tool_lint, impl_lint_pass}; + +const FROM_OVER_INTO_MSRV: RustcVersion = RustcVersion::new(1, 41, 0); declare_clippy_lint! { /// **What it does:** Searches for implementations of the `Into<..>` trait and suggests to implement `From<..>` instead. @@ -38,10 +41,25 @@ declare_clippy_lint! { "Warns on implementations of `Into<..>` to use `From<..>`" } -declare_lint_pass!(FromOverInto => [FROM_OVER_INTO]); +pub struct FromOverInto { + msrv: Option, +} + +impl FromOverInto { + #[must_use] + pub fn new(msrv: Option) -> Self { + FromOverInto { msrv } + } +} + +impl_lint_pass!(FromOverInto => [FROM_OVER_INTO]); impl LateLintPass<'_> for FromOverInto { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + if !meets_msrv(self.msrv.as_ref(), &FROM_OVER_INTO_MSRV) { + return; + } + let impl_def_id = cx.tcx.hir().local_def_id(item.hir_id); if_chain! { if let hir::ItemKind::Impl{ .. } = &item.kind; @@ -55,9 +73,11 @@ impl LateLintPass<'_> for FromOverInto { item.span, "An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true.", None, - "consider implement From instead", + "consider to implement From instead", ); } } } + + extract_msrv_attr!(LateContext); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 58587481922cc..35b057d7b6a41 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1016,6 +1016,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || box checked_conversions::CheckedConversions::new(msrv)); store.register_late_pass(move || box mem_replace::MemReplace::new(msrv)); store.register_late_pass(move || box ranges::Ranges::new(msrv)); + store.register_late_pass(move || box from_over_into::FromOverInto::new(msrv)); store.register_late_pass(move || box use_self::UseSelf::new(msrv)); store.register_late_pass(move || box missing_const_for_fn::MissingConstForFn::new(msrv)); @@ -1205,7 +1206,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box manual_unwrap_or::ManualUnwrapOr); store.register_late_pass(|| box manual_ok_or::ManualOkOr); store.register_late_pass(|| box float_equality_without_abs::FloatEqualityWithoutAbs); - store.register_late_pass(|| box from_over_into::FromOverInto); store.register_late_pass(|| box async_yields_async::AsyncYieldsAsync); let disallowed_methods = conf.disallowed_methods.iter().cloned().collect::>(); store.register_late_pass(move || box disallowed_method::DisallowedMethod::new(&disallowed_methods)); diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr index 17f30fa837ec4..c9c8e7c4b538b 100644 --- a/tests/ui/from_over_into.stderr +++ b/tests/ui/from_over_into.stderr @@ -9,7 +9,7 @@ LL | | } | |_^ | = note: `-D clippy::from-over-into` implied by `-D warnings` - = help: consider implement From instead + = help: consider to implement From instead error: aborting due to previous error diff --git a/tests/ui/min_rust_version_attr.rs b/tests/ui/min_rust_version_attr.rs index 3848bca320759..0f47f1cbc4026 100644 --- a/tests/ui/min_rust_version_attr.rs +++ b/tests/ui/min_rust_version_attr.rs @@ -57,6 +57,14 @@ pub fn checked_conversion() { let _ = value <= (u32::MAX as i64) && value >= 0; } +pub struct FromOverInto(String); + +impl Into for String { + fn into(self) -> FromOverInto { + FromOverInto(self) + } +} + pub fn filter_map_next() { let a = ["1", "lol", "3", "NaN", "5"]; diff --git a/tests/ui/min_rust_version_attr.stderr b/tests/ui/min_rust_version_attr.stderr index 348052631049b..e3e3b335cbe16 100644 --- a/tests/ui/min_rust_version_attr.stderr +++ b/tests/ui/min_rust_version_attr.stderr @@ -1,12 +1,12 @@ error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:142:24 + --> $DIR/min_rust_version_attr.rs:150:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | = note: `-D clippy::manual-strip` implied by `-D warnings` note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:141:9 + --> $DIR/min_rust_version_attr.rs:149:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -17,13 +17,13 @@ LL | assert_eq!(.to_uppercase(), "WORLD!"); | error: stripping a prefix manually - --> $DIR/min_rust_version_attr.rs:154:24 + --> $DIR/min_rust_version_attr.rs:162:24 | LL | assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!"); | ^^^^^^^^^^^^^^^^^^^^ | note: the prefix was tested here - --> $DIR/min_rust_version_attr.rs:153:9 + --> $DIR/min_rust_version_attr.rs:161:9 | LL | if s.starts_with("hello, ") { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ From 97f5db97c464bfa44467ccff5c0ff6ac2a681d85 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Sun, 20 Dec 2020 15:53:23 +0000 Subject: [PATCH 03/33] Website issue tracker link and better search performance * last minor improvements --- util/gh-pages/index.html | 54 +++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 23 deletions(-) diff --git a/util/gh-pages/index.html b/util/gh-pages/index.html index 428708136cb65..ad48294b84437 100644 --- a/util/gh-pages/index.html +++ b/util/gh-pages/index.html @@ -77,7 +77,7 @@

ALL the Clippy Lints

- +
+ ng-repeat="lint in data | filter:byLevels | filter:byGroups | filter:bySearch | orderBy:'id' track by lint.id">

@@ -215,6 +215,46 @@

return $scope.groups[lint.group]; }; + $scope.bySearch = function (lint, index, array) { + let search_str = $scope.search; + // It can be `null` I haven't missed this value + if (search_str == null || search_str.length == 0) { + return true; + } + search_str = search_str.toLowerCase(); + + // Search by id + let id_search = search_str.trim().replace(/(\-| )/g, "_"); + if (lint.id.includes(id_search)) { + return true; + } + + // Search the description + // The use of `for`-loops instead of `foreach` enables us to return early + let search_lint = (lint, therm) => { + for (const field in lint.docs) { + // Continue if it's not a property + if (!lint.docs.hasOwnProperty(field)) { + continue; + } + + // Return if not found + if (lint.docs[field].toLowerCase().includes(therm)) { + return true; + } + } + return false; + }; + let therms = search_str.split(" "); + for (index = 0; index < therms.length; index++) { + if (!search_lint(lint, therms[index])) { + return false; + } + } + + return true; + } + // Get data $scope.open = {}; $scope.loading = true; From 7e641c8be774bba047d7d9ee57455957fd4e2a3b Mon Sep 17 00:00:00 2001 From: Bastian Kersting Date: Sun, 20 Dec 2020 20:10:00 +0100 Subject: [PATCH 05/33] Fixed error messages --- clippy_lints/src/from_over_into.rs | 4 ++-- tests/ui/from_over_into.stderr | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index c7988d6f01fa5..083101ab1c649 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -71,9 +71,9 @@ impl LateLintPass<'_> for FromOverInto { cx, FROM_OVER_INTO, item.span, - "An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true.", + "An implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true.", None, - "consider to implement From instead", + "consider to implement `From` instead", ); } } diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr index c9c8e7c4b538b..0825d47add248 100644 --- a/tests/ui/from_over_into.stderr +++ b/tests/ui/from_over_into.stderr @@ -1,4 +1,4 @@ -error: An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true. +error: An implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true. --> $DIR/from_over_into.rs:6:1 | LL | / impl Into for String { @@ -9,7 +9,7 @@ LL | | } | |_^ | = note: `-D clippy::from-over-into` implied by `-D warnings` - = help: consider to implement From instead + = help: consider to implement `From` instead error: aborting due to previous error From 0d6c128bfd41b61f246f603cfcd9ee4fc97ca3a0 Mon Sep 17 00:00:00 2001 From: llogiq Date: Mon, 21 Dec 2020 09:18:30 +0100 Subject: [PATCH 06/33] Update clippy_lints/src/from_over_into.rs Co-authored-by: Takayuki Nakata --- clippy_lints/src/from_over_into.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/from_over_into.rs b/clippy_lints/src/from_over_into.rs index 083101ab1c649..1e7e5f53cc2a3 100644 --- a/clippy_lints/src/from_over_into.rs +++ b/clippy_lints/src/from_over_into.rs @@ -71,7 +71,7 @@ impl LateLintPass<'_> for FromOverInto { cx, FROM_OVER_INTO, item.span, - "An implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true.", + "an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true", None, "consider to implement `From` instead", ); From 12bd244caa4547b4b84108d892e85bf953b1a408 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Mon, 21 Dec 2020 11:09:49 +0100 Subject: [PATCH 07/33] Don't trigger large_enum_variant in external macros --- clippy_lints/src/large_enum_variant.rs | 4 ++++ tests/ui/auxiliary/macro_rules.rs | 10 ++++++++++ tests/ui/large_enum_variant.rs | 9 ++++++++- tests/ui/large_enum_variant.stderr | 18 +++++++++--------- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/clippy_lints/src/large_enum_variant.rs b/clippy_lints/src/large_enum_variant.rs index 3c7880d74ee64..ad9b4f357a74d 100644 --- a/clippy_lints/src/large_enum_variant.rs +++ b/clippy_lints/src/large_enum_variant.rs @@ -4,6 +4,7 @@ use crate::utils::{snippet_opt, span_lint_and_then}; use rustc_errors::Applicability; use rustc_hir::{Item, ItemKind, VariantData}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::lint::in_external_macro; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_target::abi::LayoutOf; @@ -58,6 +59,9 @@ impl_lint_pass!(LargeEnumVariant => [LARGE_ENUM_VARIANT]); impl<'tcx> LateLintPass<'tcx> for LargeEnumVariant { fn check_item(&mut self, cx: &LateContext<'_>, item: &Item<'_>) { + if in_external_macro(cx.tcx.sess, item.span) { + return; + } let did = cx.tcx.hir().local_def_id(item.hir_id); if let ItemKind::Enum(ref def, _) = item.kind { let ty = cx.tcx.type_of(did); diff --git a/tests/ui/auxiliary/macro_rules.rs b/tests/ui/auxiliary/macro_rules.rs index f985a15eda2bb..1832482346820 100644 --- a/tests/ui/auxiliary/macro_rules.rs +++ b/tests/ui/auxiliary/macro_rules.rs @@ -84,3 +84,13 @@ macro_rules! as_conv { 0u32 as u64 }; } + +#[macro_export] +macro_rules! large_enum_variant { + () => { + enum LargeEnumInMacro { + A(i32), + B([i32; 8000]), + } + }; +} diff --git a/tests/ui/large_enum_variant.rs b/tests/ui/large_enum_variant.rs index 852ef5fec0e7b..d22fee3f27b05 100644 --- a/tests/ui/large_enum_variant.rs +++ b/tests/ui/large_enum_variant.rs @@ -1,7 +1,12 @@ +// aux-build:macro_rules.rs + #![allow(dead_code)] #![allow(unused_variables)] #![warn(clippy::large_enum_variant)] +#[macro_use] +extern crate macro_rules; + enum LargeEnum { A(i32), B([i32; 8000]), @@ -51,4 +56,6 @@ enum LargeEnumOk { LargeB([i32; 8001]), } -fn main() {} +fn main() { + large_enum_variant!(); +} diff --git a/tests/ui/large_enum_variant.stderr b/tests/ui/large_enum_variant.stderr index 8ce641a81f297..d39a4d462aabd 100644 --- a/tests/ui/large_enum_variant.stderr +++ b/tests/ui/large_enum_variant.stderr @@ -1,12 +1,12 @@ error: large size difference between variants - --> $DIR/large_enum_variant.rs:7:5 + --> $DIR/large_enum_variant.rs:12:5 | LL | B([i32; 8000]), | ^^^^^^^^^^^^^^ this variant is 32000 bytes | = note: `-D clippy::large-enum-variant` implied by `-D warnings` note: and the second-largest variant is 4 bytes: - --> $DIR/large_enum_variant.rs:6:5 + --> $DIR/large_enum_variant.rs:11:5 | LL | A(i32), | ^^^^^^ @@ -16,13 +16,13 @@ LL | B(Box<[i32; 8000]>), | ^^^^^^^^^^^^^^^^ error: large size difference between variants - --> $DIR/large_enum_variant.rs:31:5 + --> $DIR/large_enum_variant.rs:36:5 | LL | ContainingLargeEnum(LargeEnum), | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 32004 bytes | note: and the second-largest variant is 8 bytes: - --> $DIR/large_enum_variant.rs:30:5 + --> $DIR/large_enum_variant.rs:35:5 | LL | VariantOk(i32, u32), | ^^^^^^^^^^^^^^^^^^^ @@ -32,30 +32,30 @@ LL | ContainingLargeEnum(Box), | ^^^^^^^^^^^^^^ error: large size difference between variants - --> $DIR/large_enum_variant.rs:41:5 + --> $DIR/large_enum_variant.rs:46:5 | LL | StructLikeLarge { x: [i32; 8000], y: i32 }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 32004 bytes | note: and the second-largest variant is 8 bytes: - --> $DIR/large_enum_variant.rs:40:5 + --> $DIR/large_enum_variant.rs:45:5 | LL | VariantOk(i32, u32), | ^^^^^^^^^^^^^^^^^^^ help: consider boxing the large fields to reduce the total size of the enum - --> $DIR/large_enum_variant.rs:41:5 + --> $DIR/large_enum_variant.rs:46:5 | LL | StructLikeLarge { x: [i32; 8000], y: i32 }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: large size difference between variants - --> $DIR/large_enum_variant.rs:46:5 + --> $DIR/large_enum_variant.rs:51:5 | LL | StructLikeLarge2 { x: [i32; 8000] }, | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this variant is 32000 bytes | note: and the second-largest variant is 8 bytes: - --> $DIR/large_enum_variant.rs:45:5 + --> $DIR/large_enum_variant.rs:50:5 | LL | VariantOk(i32, u32), | ^^^^^^^^^^^^^^^^^^^ From 19ace28af4678cd3526a8a08bc48872b71d10d4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Mon, 21 Dec 2020 16:11:40 +0100 Subject: [PATCH 08/33] readme: remove paragraph about installing clippy manually on ci, if it is missing on a nightly Clippy should always be available on nightly because we are gating on it in rustcs CI. --- README.md | 32 ++++++++++---------------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index dc931963726b2..65e0864be904c 100644 --- a/README.md +++ b/README.md @@ -10,16 +10,16 @@ A collection of lints to catch common mistakes and improve your [Rust](https://g Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category. -Category | Description | Default level --- | -- | -- -`clippy::all` | all lints that are on by default (correctness, style, complexity, perf) | **warn/deny** -`clippy::correctness` | code that is outright wrong or very useless | **deny** -`clippy::style` | code that should be written in a more idiomatic way | **warn** -`clippy::complexity` | code that does something simple but in a complex way | **warn** -`clippy::perf` | code that can be written to run faster | **warn** -`clippy::pedantic` | lints which are rather strict or might have false positives | allow -`clippy::nursery` | new lints that are still under development | allow -`clippy::cargo` | lints for the cargo manifest | allow +| Category | Description | Default level | +| --------------------- | ----------------------------------------------------------------------- | ------------- | +| `clippy::all` | all lints that are on by default (correctness, style, complexity, perf) | **warn/deny** | +| `clippy::correctness` | code that is outright wrong or very useless | **deny** | +| `clippy::style` | code that should be written in a more idiomatic way | **warn** | +| `clippy::complexity` | code that does something simple but in a complex way | **warn** | +| `clippy::perf` | code that can be written to run faster | **warn** | +| `clippy::pedantic` | lints which are rather strict or might have false positives | allow | +| `clippy::nursery` | new lints that are still under development | allow | +| `clippy::cargo` | lints for the cargo manifest | allow | More to come, please [file an issue](https://github.com/rust-lang/rust-clippy/issues) if you have ideas! @@ -130,18 +130,6 @@ script: # etc. ``` -If you are on nightly, It might happen that Clippy is not available for a certain nightly release. -In this case you can try to conditionally install Clippy from the Git repo. - -```yaml -language: rust -rust: - - nightly -before_script: - - rustup component add clippy --toolchain=nightly || cargo install --git https://github.com/rust-lang/rust-clippy/ --force clippy - # etc. -``` - Note that adding `-D warnings` will cause your build to fail if **any** warnings are found in your code. That includes warnings found by rustc (e.g. `dead_code`, etc.). If you want to avoid this and only cause an error for Clippy warnings, use `#![deny(clippy::all)]` in your code or `-D clippy::all` on the command From 53f4d437f4c448c4382a99c2b7ff03f104cf7368 Mon Sep 17 00:00:00 2001 From: Bastian Kersting <45260993+1c3t3a@users.noreply.github.com> Date: Mon, 21 Dec 2020 17:15:05 +0100 Subject: [PATCH 09/33] Update from_over_into.stderr --- tests/ui/from_over_into.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/from_over_into.stderr b/tests/ui/from_over_into.stderr index 0825d47add248..18f56f854329e 100644 --- a/tests/ui/from_over_into.stderr +++ b/tests/ui/from_over_into.stderr @@ -1,4 +1,4 @@ -error: An implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true. +error: an implementation of `From` is preferred since it gives you `Into<_>` for free where the reverse isn't true --> $DIR/from_over_into.rs:6:1 | LL | / impl Into for String { From 04d30448374277e34ce77ed83cfcea4ba8b6ecb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Tue, 22 Dec 2020 01:24:59 +0100 Subject: [PATCH 10/33] readme: remove paragraph about executing clippy via "cargo run .." This most likely no longer works since we are pinning clippy on a specific nightly now. "cargo run" would try to compile clippy with whatever version the project we want to check demands. Also building clippy yourself to run it on a project is not really needed anymore since clippy is shipped with official rust releases. Fixes #6489 --- README.md | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/README.md b/README.md index 65e0864be904c..74719f02fe086 100644 --- a/README.md +++ b/README.md @@ -98,17 +98,6 @@ If you want to run Clippy **only** on the given crate, use the `--no-deps` optio cargo clippy -p example -- --no-deps ``` -### Running Clippy from the command line without installing it - -To have cargo compile your crate with Clippy without Clippy installation -in your code, you can use: - -```terminal -cargo run --bin cargo-clippy --manifest-path=path_to_clippys_Cargo.toml -``` - -*Note:* Be sure that Clippy was compiled with the same version of rustc that cargo invokes here! - ### Travis CI You can add Clippy to Travis CI in the same way you use it locally: From d6a7ebcdd6dc29fcddc843486b7a9bc4f7be7a42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Tue, 22 Dec 2020 01:57:55 +0100 Subject: [PATCH 11/33] ci: test cargo clippy --fix -Zunstable-options --- .github/workflows/clippy.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml index 530e60001f726..17812b74ab2fd 100644 --- a/.github/workflows/clippy.yml +++ b/.github/workflows/clippy.yml @@ -57,6 +57,10 @@ jobs: run: cargo test --features deny-warnings,internal-lints working-directory: clippy_lints + - name: Test --fix -Zunstable-options + run: cargo run --bin cargo-clippy -- clippy --fix -Zunstable-options + working-directory: clippy_lints + - name: Test rustc_tools_util run: cargo test --features deny-warnings working-directory: rustc_tools_util From af22613a25ad866a600bec13995ec16c993e5aac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Tue, 22 Dec 2020 03:11:35 +0100 Subject: [PATCH 12/33] remove clone in manual_async_fn lint --- clippy_lints/src/manual_async_fn.rs | 2 +- clippy_lints/src/unused_unit.rs | 2 +- clippy_lints/src/utils/mod.rs | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/manual_async_fn.rs b/clippy_lints/src/manual_async_fn.rs index 7b3b450ef93e9..29439e52c48e1 100644 --- a/clippy_lints/src/manual_async_fn.rs +++ b/clippy_lints/src/manual_async_fn.rs @@ -69,7 +69,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { |diag| { if_chain! { if let Some(header_snip) = snippet_opt(cx, header_span); - if let Some(ret_pos) = position_before_rarrow(header_snip.clone()); + if let Some(ret_pos) = position_before_rarrow(&header_snip); if let Some((ret_sugg, ret_snip)) = suggested_ret(cx, output); then { let help = format!("make the function `async` and {}", ret_sugg); diff --git a/clippy_lints/src/unused_unit.rs b/clippy_lints/src/unused_unit.rs index f61fd2ecd735d..a31cd5fda849e 100644 --- a/clippy_lints/src/unused_unit.rs +++ b/clippy_lints/src/unused_unit.rs @@ -120,7 +120,7 @@ fn is_unit_expr(expr: &ast::Expr) -> bool { fn lint_unneeded_unit_return(cx: &EarlyContext<'_>, ty: &ast::Ty, span: Span) { let (ret_span, appl) = if let Ok(fn_source) = cx.sess().source_map().span_to_snippet(span.with_hi(ty.span.hi())) { - position_before_rarrow(fn_source).map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { + position_before_rarrow(&fn_source).map_or((ty.span, Applicability::MaybeIncorrect), |rpos| { ( #[allow(clippy::cast_possible_truncation)] ty.span.with_lo(BytePos(span.lo().0 + rpos as u32)), diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 424856090f261..1c68e837c4ab9 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -788,8 +788,7 @@ pub fn indent_of(cx: &T, span: Span) -> Option { /// fn into3(self) -> () {} /// ^ /// ``` -#[allow(clippy::needless_pass_by_value)] -pub fn position_before_rarrow(s: String) -> Option { +pub fn position_before_rarrow(s: &str) -> Option { s.rfind("->").map(|rpos| { let mut rpos = rpos; let chars: Vec = s.chars().collect(); From 8bdf34e10cfa47b6e0cfccd5d0f8e6a5c079bc30 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 23 Nov 2020 12:26:35 -0600 Subject: [PATCH 13/33] Fix default initialized fields in suggestion The default value for a field type does not necessarily match the default value for that field in the struct Default. --- clippy_lints/src/default.rs | 6 ------ tests/ui/field_reassign_with_default.stderr | 4 ++-- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs index f69f6f1412af9..adcc17266d22e 100644 --- a/clippy_lints/src/default.rs +++ b/clippy_lints/src/default.rs @@ -165,12 +165,6 @@ impl LateLintPass<'_> for Default { let stmt = &block.stmts[stmt_idx]; if let StmtKind::Local(preceding_local) = &stmt.kind { - // filter out fields like `= Default::default()`, because the FRU already covers them - let assigned_fields = assigned_fields - .into_iter() - .filter(|(_, rhs)| !is_expr_default(rhs, cx)) - .collect::)>>(); - // if all fields of the struct are not assigned, add `.. Default::default()` to the suggestion. let ext_with_default = !fields_of_type(binding_type) .iter() diff --git a/tests/ui/field_reassign_with_default.stderr b/tests/ui/field_reassign_with_default.stderr index c788ebae5526c..9a2bc778c3ff7 100644 --- a/tests/ui/field_reassign_with_default.stderr +++ b/tests/ui/field_reassign_with_default.stderr @@ -53,7 +53,7 @@ error: field assignment outside of initializer for an instance created with Defa LL | a.i = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ | -note: consider initializing the variable with `A::default()` and removing relevant reassignments +note: consider initializing the variable with `A { i: Default::default(), ..Default::default() }` and removing relevant reassignments --> $DIR/field_reassign_with_default.rs:90:5 | LL | let mut a: A = Default::default(); @@ -65,7 +65,7 @@ error: field assignment outside of initializer for an instance created with Defa LL | a.i = Default::default(); | ^^^^^^^^^^^^^^^^^^^^^^^^^ | -note: consider initializing the variable with `A { j: 45, ..Default::default() }` and removing relevant reassignments +note: consider initializing the variable with `A { i: Default::default(), j: 45 }` and removing relevant reassignments --> $DIR/field_reassign_with_default.rs:94:5 | LL | let mut a: A = Default::default(); From 73ce34a1970e860d14111127dd11d9ba56c41c3a Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 23 Nov 2020 13:20:53 -0600 Subject: [PATCH 14/33] Remove redundant shadow check There is already an assertion that consecutive lines assign to a struct field. --- clippy_lints/src/default.rs | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs index adcc17266d22e..c3fe77f625017 100644 --- a/clippy_lints/src/default.rs +++ b/clippy_lints/src/default.rs @@ -122,15 +122,8 @@ impl LateLintPass<'_> for Default { let mut assigned_fields = Vec::new(); let mut cancel_lint = false; for consecutive_statement in &block.stmts[stmt_idx + 1..] { - // interrupt if the statement is a let binding (`Local`) that shadows the original - // binding - if stmt_shadows_binding(consecutive_statement, binding_name) { - break; - } // find out if and which field was set by this `consecutive_statement` - else if let Some((field_ident, assign_rhs)) = - field_reassigned_by_stmt(consecutive_statement, binding_name) - { + if let Some((field_ident, assign_rhs)) = field_reassigned_by_stmt(consecutive_statement, binding_name) { // interrupt and cancel lint if assign_rhs references the original binding if contains_name(binding_name, assign_rhs) { cancel_lint = true; @@ -152,7 +145,7 @@ impl LateLintPass<'_> for Default { first_assign = Some(consecutive_statement); } } - // interrupt also if no field was assigned, since we only want to look at consecutive statements + // interrupt if no field was assigned, since we only want to look at consecutive statements else { break; } @@ -256,15 +249,6 @@ fn enumerate_bindings_using_default<'tcx>( .collect() } -fn stmt_shadows_binding(this: &Stmt<'_>, shadowed: Symbol) -> bool { - if let StmtKind::Local(local) = &this.kind { - if let PatKind::Binding(_, _, ident, _) = local.pat.kind { - return ident.name == shadowed; - } - } - false -} - /// Returns the reassigned field and the assigning expression (right-hand side of assign). fn field_reassigned_by_stmt<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Option<(Ident, &'tcx Expr<'tcx>)> { if_chain! { From c6450c70ddb5354bc2218bff20a325ded9682613 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 23 Nov 2020 09:23:24 -0600 Subject: [PATCH 15/33] Fix field_reassign_with_default for private fields --- clippy_lints/src/default.rs | 162 ++++++++++-------------- tests/ui/field_reassign_with_default.rs | 12 ++ 2 files changed, 81 insertions(+), 93 deletions(-) diff --git a/clippy_lints/src/default.rs b/clippy_lints/src/default.rs index c3fe77f625017..b0d7c7b3baab1 100644 --- a/clippy_lints/src/default.rs +++ b/clippy_lints/src/default.rs @@ -6,7 +6,7 @@ use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::{Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty::{self, Adt, Ty}; +use rustc_middle::ty; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::symbol::{Ident, Symbol}; use rustc_span::Span; @@ -103,18 +103,41 @@ impl LateLintPass<'_> for Default { } fn check_block<'tcx>(&mut self, cx: &LateContext<'tcx>, block: &Block<'tcx>) { - // find all binding statements like `let mut _ = T::default()` where `T::default()` is the - // `default` method of the `Default` trait, and store statement index in current block being - // checked and the name of the bound variable - let binding_statements_using_default = enumerate_bindings_using_default(cx, block); - // start from the `let mut _ = _::default();` and look at all the following // statements, see if they re-assign the fields of the binding - for (stmt_idx, binding_name, binding_type, span) in binding_statements_using_default { - // the last statement of a block cannot trigger the lint - if stmt_idx == block.stmts.len() - 1 { - break; - } + let stmts_head = match block.stmts { + // Skip the last statement since there cannot possibly be any following statements that re-assign fields. + [head @ .., _] if !head.is_empty() => head, + _ => return, + }; + for (stmt_idx, stmt) in stmts_head.iter().enumerate() { + // find all binding statements like `let mut _ = T::default()` where `T::default()` is the + // `default` method of the `Default` trait, and store statement index in current block being + // checked and the name of the bound variable + let (local, variant, binding_name, binding_type, span) = if_chain! { + // only take `let ...` statements + if let StmtKind::Local(local) = stmt.kind; + if let Some(expr) = local.init; + // only take bindings to identifiers + if let PatKind::Binding(_, binding_id, ident, _) = local.pat.kind; + // only when assigning `... = Default::default()` + if is_expr_default(expr, cx); + let binding_type = cx.typeck_results().node_type(binding_id); + if let Some(adt) = binding_type.ty_adt_def(); + if adt.is_struct(); + let variant = adt.non_enum_variant(); + if adt.did.is_local() || !variant.is_field_list_non_exhaustive(); + let module_did = cx.tcx.parent_module(stmt.hir_id).to_def_id(); + if variant + .fields + .iter() + .all(|field| field.vis.is_accessible_from(module_did, cx.tcx)); + then { + (local, variant, ident.name, binding_type, expr.span) + } else { + continue; + } + }; // find all "later statement"'s where the fields of the binding set as // Default::default() get reassigned, unless the reassignment refers to the original binding @@ -154,49 +177,45 @@ impl LateLintPass<'_> for Default { // if there are incorrectly assigned fields, do a span_lint_and_note to suggest // construction using `Ty { fields, ..Default::default() }` if !assigned_fields.is_empty() && !cancel_lint { - // take the original assignment as span - let stmt = &block.stmts[stmt_idx]; - - if let StmtKind::Local(preceding_local) = &stmt.kind { - // if all fields of the struct are not assigned, add `.. Default::default()` to the suggestion. - let ext_with_default = !fields_of_type(binding_type) - .iter() - .all(|field| assigned_fields.iter().any(|(a, _)| a == &field.name)); + // if all fields of the struct are not assigned, add `.. Default::default()` to the suggestion. + let ext_with_default = !variant + .fields + .iter() + .all(|field| assigned_fields.iter().any(|(a, _)| a == &field.ident.name)); - let field_list = assigned_fields - .into_iter() - .map(|(field, rhs)| { - // extract and store the assigned value for help message - let value_snippet = snippet(cx, rhs.span, ".."); - format!("{}: {}", field, value_snippet) - }) - .collect::>() - .join(", "); + let field_list = assigned_fields + .into_iter() + .map(|(field, rhs)| { + // extract and store the assigned value for help message + let value_snippet = snippet(cx, rhs.span, ".."); + format!("{}: {}", field, value_snippet) + }) + .collect::>() + .join(", "); - let sugg = if ext_with_default { - if field_list.is_empty() { - format!("{}::default()", binding_type) - } else { - format!("{} {{ {}, ..Default::default() }}", binding_type, field_list) - } + let sugg = if ext_with_default { + if field_list.is_empty() { + format!("{}::default()", binding_type) } else { - format!("{} {{ {} }}", binding_type, field_list) - }; + format!("{} {{ {}, ..Default::default() }}", binding_type, field_list) + } + } else { + format!("{} {{ {} }}", binding_type, field_list) + }; - // span lint once per statement that binds default - span_lint_and_note( - cx, - FIELD_REASSIGN_WITH_DEFAULT, - first_assign.unwrap().span, - "field assignment outside of initializer for an instance created with Default::default()", - Some(preceding_local.span), - &format!( - "consider initializing the variable with `{}` and removing relevant reassignments", - sugg - ), - ); - self.reassigned_linted.insert(span); - } + // span lint once per statement that binds default + span_lint_and_note( + cx, + FIELD_REASSIGN_WITH_DEFAULT, + first_assign.unwrap().span, + "field assignment outside of initializer for an instance created with Default::default()", + Some(local.span), + &format!( + "consider initializing the variable with `{}` and removing relevant reassignments", + sugg + ), + ); + self.reassigned_linted.insert(span); } } } @@ -217,38 +236,6 @@ fn is_expr_default<'tcx>(expr: &'tcx Expr<'tcx>, cx: &LateContext<'tcx>) -> bool } } -/// Returns the block indices, identifiers and types of bindings set as `Default::default()`, except -/// for when the pattern type is a tuple. -fn enumerate_bindings_using_default<'tcx>( - cx: &LateContext<'tcx>, - block: &Block<'tcx>, -) -> Vec<(usize, Symbol, Ty<'tcx>, Span)> { - block - .stmts - .iter() - .enumerate() - .filter_map(|(idx, stmt)| { - if_chain! { - // only take `let ...` statements - if let StmtKind::Local(ref local) = stmt.kind; - // only take bindings to identifiers - if let PatKind::Binding(_, _, ident, _) = local.pat.kind; - // that are not tuples - let ty = cx.typeck_results().pat_ty(local.pat); - if !matches!(ty.kind(), ty::Tuple(_)); - // only when assigning `... = Default::default()` - if let Some(ref expr) = local.init; - if is_expr_default(expr, cx); - then { - Some((idx, ident.name, ty, expr.span)) - } else { - None - } - } - }) - .collect() -} - /// Returns the reassigned field and the assigning expression (right-hand side of assign). fn field_reassigned_by_stmt<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Option<(Ident, &'tcx Expr<'tcx>)> { if_chain! { @@ -268,14 +255,3 @@ fn field_reassigned_by_stmt<'tcx>(this: &Stmt<'tcx>, binding_name: Symbol) -> Op } } } - -/// Returns the vec of fields for a struct and an empty vec for non-struct ADTs. -fn fields_of_type(ty: Ty<'_>) -> Vec { - if let Adt(adt, _) = ty.kind() { - if adt.is_struct() { - let variant = &adt.non_enum_variant(); - return variant.fields.iter().map(|f| f.ident).collect(); - } - } - vec![] -} diff --git a/tests/ui/field_reassign_with_default.rs b/tests/ui/field_reassign_with_default.rs index 79a30c22f9536..3e0921022b417 100644 --- a/tests/ui/field_reassign_with_default.rs +++ b/tests/ui/field_reassign_with_default.rs @@ -107,4 +107,16 @@ fn main() { x.i = side_effect.next(); x.j = 2; x.i = side_effect.next(); + + // don't lint - some private fields + let mut x = m::F::default(); + x.a = 1; +} + +mod m { + #[derive(Default)] + pub struct F { + pub a: u64, + b: u64, + } } From 7fa1d78c89f74c73d645901d6ee4728bcd6a72bf Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Tue, 22 Dec 2020 19:17:59 +0100 Subject: [PATCH 16/33] Revert "Pass Clippy args also trough RUSTFLAGS" --- Cargo.toml | 1 + README.md | 1 + src/driver.rs | 116 ++++++++++++----------------------------------- src/main.rs | 98 ++++++--------------------------------- tests/dogfood.rs | 2 +- 5 files changed, 47 insertions(+), 171 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7f9d22e594b9c..a765390c6032d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ publish = false [[bin]] name = "cargo-clippy" +test = false path = "src/main.rs" [[bin]] diff --git a/README.md b/README.md index 74719f02fe086..a4928e17e6a94 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,7 @@ the lint(s) you are interested in: ```terminal cargo clippy -- -A clippy::all -W clippy::useless_format -W clippy::... ``` +Note that if you've run clippy before, this may only take effect after you've modified a file or ran `cargo clean`. ### Specifying the minimum supported Rust version diff --git a/src/driver.rs b/src/driver.rs index 40f1b802e60e6..e490ee54c0be0 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -1,6 +1,5 @@ #![feature(rustc_private)] #![feature(once_cell)] -#![feature(bool_to_option)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -20,7 +19,6 @@ use rustc_tools_util::VersionInfo; use std::borrow::Cow; use std::env; -use std::iter; use std::lazy::SyncLazy; use std::ops::Deref; use std::panic; @@ -49,6 +47,20 @@ fn arg_value<'a, T: Deref>( None } +#[test] +fn test_arg_value() { + let args = &["--bar=bar", "--foobar", "123", "--foo"]; + + assert_eq!(arg_value(&[] as &[&str], "--foobar", |_| true), None); + assert_eq!(arg_value(args, "--bar", |_| false), None); + assert_eq!(arg_value(args, "--bar", |_| true), Some("bar")); + assert_eq!(arg_value(args, "--bar", |p| p == "bar"), Some("bar")); + assert_eq!(arg_value(args, "--bar", |p| p == "foo"), None); + assert_eq!(arg_value(args, "--foobar", |p| p == "foo"), None); + assert_eq!(arg_value(args, "--foobar", |p| p == "123"), Some("123")); + assert_eq!(arg_value(args, "--foo", |_| true), None); +} + struct DefaultCallbacks; impl rustc_driver::Callbacks for DefaultCallbacks {} @@ -170,28 +182,6 @@ fn toolchain_path(home: Option, toolchain: Option) -> Option(args: &mut Vec, clippy_args: I) -where - T: AsRef, - U: AsRef + ?Sized + 'a, - I: Iterator + Clone, -{ - let args_iter = clippy_args.map(AsRef::as_ref); - let args_count = args_iter.clone().count(); - - if args_count > 0 { - if let Some(start) = args.windows(args_count).enumerate().find_map(|(current, window)| { - window - .iter() - .map(AsRef::as_ref) - .eq(args_iter.clone()) - .then_some(current) - }) { - args.drain(start..start + args_count); - } - } -} - #[allow(clippy::too_many_lines)] pub fn main() { rustc_driver::init_rustc_env_logger(); @@ -288,9 +278,20 @@ pub fn main() { args.extend(vec!["--sysroot".into(), sys_root]); }; - let clippy_args = env::var("CLIPPY_ARGS").unwrap_or_default(); - let clippy_args = clippy_args.split_whitespace(); - let no_deps = clippy_args.clone().any(|flag| flag == "--no-deps"); + let mut no_deps = false; + let clippy_args = env::var("CLIPPY_ARGS") + .unwrap_or_default() + .split("__CLIPPY_HACKERY__") + .filter_map(|s| match s { + "" => None, + "--no-deps" => { + no_deps = true; + None + }, + _ => Some(s.to_string()), + }) + .chain(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]) + .collect::>(); // We enable Clippy if one of the following conditions is met // - IF Clippy is run on its test suite OR @@ -303,11 +304,7 @@ pub fn main() { let clippy_enabled = clippy_tests_set || (!cap_lints_allow && (!no_deps || in_primary_package)); if clippy_enabled { - remove_clippy_args(&mut args, iter::once("--no-deps")); - args.extend(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]); - } else { - // Remove all flags passed through RUSTFLAGS if Clippy is not enabled. - remove_clippy_args(&mut args, clippy_args); + args.extend(clippy_args); } let mut clippy = ClippyCallbacks; @@ -318,58 +315,3 @@ pub fn main() { rustc_driver::RunCompiler::new(&args, callbacks).run() })) } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_arg_value() { - let args = &["--bar=bar", "--foobar", "123", "--foo"]; - - assert_eq!(arg_value(&[] as &[&str], "--foobar", |_| true), None); - assert_eq!(arg_value(args, "--bar", |_| false), None); - assert_eq!(arg_value(args, "--bar", |_| true), Some("bar")); - assert_eq!(arg_value(args, "--bar", |p| p == "bar"), Some("bar")); - assert_eq!(arg_value(args, "--bar", |p| p == "foo"), None); - assert_eq!(arg_value(args, "--foobar", |p| p == "foo"), None); - assert_eq!(arg_value(args, "--foobar", |p| p == "123"), Some("123")); - assert_eq!(arg_value(args, "--foo", |_| true), None); - } - - #[test] - fn removes_clippy_args_from_start() { - let mut args = vec!["-D", "clippy::await_holding_lock", "--cfg", r#"feature="some_feat""#]; - let clippy_args = ["-D", "clippy::await_holding_lock"].iter(); - - remove_clippy_args(&mut args, clippy_args); - assert_eq!(args, &["--cfg", r#"feature="some_feat""#]); - } - - #[test] - fn removes_clippy_args_from_end() { - let mut args = vec!["-Zui-testing", "-A", "clippy::empty_loop", "--no-deps"]; - let clippy_args = ["-A", "clippy::empty_loop", "--no-deps"].iter(); - - remove_clippy_args(&mut args, clippy_args); - assert_eq!(args, &["-Zui-testing"]); - } - - #[test] - fn removes_clippy_args_from_middle() { - let mut args = vec!["-Zui-testing", "-W", "clippy::filter_map", "-L", "serde"]; - let clippy_args = ["-W", "clippy::filter_map"].iter(); - - remove_clippy_args(&mut args, clippy_args); - assert_eq!(args, &["-Zui-testing", "-L", "serde"]); - } - - #[test] - fn no_clippy_args_to_remove() { - let mut args = vec!["-Zui-testing", "-L", "serde"]; - let clippy_args: [&str; 0] = []; - - remove_clippy_args(&mut args, clippy_args.iter()); - assert_eq!(args, &["-Zui-testing", "-L", "serde"]); - } -} diff --git a/src/main.rs b/src/main.rs index 1c0e04689a9fe..ea06743394d10 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,3 @@ -#![feature(bool_to_option)] -#![feature(command_access)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -64,7 +62,7 @@ struct ClippyCmd { unstable_options: bool, cargo_subcommand: &'static str, args: Vec, - clippy_args: Option, + clippy_args: Vec, } impl ClippyCmd { @@ -101,17 +99,16 @@ impl ClippyCmd { args.insert(0, "+nightly".to_string()); } - let mut clippy_args = old_args.collect::>().join(" "); - if cargo_subcommand == "fix" && !clippy_args.contains("--no-deps") { - clippy_args = format!("{} --no-deps", clippy_args); + let mut clippy_args: Vec = old_args.collect(); + if cargo_subcommand == "fix" && !clippy_args.iter().any(|arg| arg == "--no-deps") { + clippy_args.push("--no-deps".into()); } - let has_args = !clippy_args.is_empty(); ClippyCmd { unstable_options, cargo_subcommand, args, - clippy_args: has_args.then_some(clippy_args), + clippy_args, } } @@ -151,24 +148,20 @@ impl ClippyCmd { .map(|p| ("CARGO_TARGET_DIR", p)) } - fn into_std_cmd(self, rustflags: Option) -> Command { + fn into_std_cmd(self) -> Command { let mut cmd = Command::new("cargo"); + let clippy_args: String = self + .clippy_args + .iter() + .map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) + .collect(); cmd.env(self.path_env(), Self::path()) .envs(ClippyCmd::target_dir()) + .env("CLIPPY_ARGS", clippy_args) .arg(self.cargo_subcommand) .args(&self.args); - // HACK: pass Clippy args to the driver *also* through RUSTFLAGS. - // This guarantees that new builds will be triggered when Clippy flags change. - if let Some(clippy_args) = self.clippy_args { - cmd.env( - "RUSTFLAGS", - rustflags.map_or(clippy_args.clone(), |flags| format!("{} {}", clippy_args, flags)), - ); - cmd.env("CLIPPY_ARGS", clippy_args); - } - cmd } } @@ -179,7 +172,7 @@ where { let cmd = ClippyCmd::new(old_args); - let mut cmd = cmd.into_std_cmd(env::var("RUSTFLAGS").ok()); + let mut cmd = cmd.into_std_cmd(); let exit_status = cmd .spawn() @@ -197,7 +190,6 @@ where #[cfg(test)] mod tests { use super::ClippyCmd; - use std::ffi::OsStr; #[test] #[should_panic] @@ -212,7 +204,6 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - assert_eq!("fix", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); assert!(cmd.args.iter().any(|arg| arg.ends_with("unstable-options"))); @@ -224,8 +215,7 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - - assert!(cmd.clippy_args.unwrap().contains("--no-deps")); + assert!(cmd.clippy_args.iter().any(|arg| arg == "--no-deps")); } #[test] @@ -234,15 +224,13 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - - assert_eq!(1, cmd.clippy_args.unwrap().matches("--no-deps").count()); + assert_eq!(cmd.clippy_args.iter().filter(|arg| *arg == "--no-deps").count(), 1); } #[test] fn check() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); let cmd = ClippyCmd::new(args); - assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WRAPPER", cmd.path_env()); } @@ -253,63 +241,7 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); } - - #[test] - fn clippy_args_into_rustflags() { - let args = "cargo clippy -- -W clippy::as_conversions" - .split_whitespace() - .map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = None; - let cmd = cmd.into_std_cmd(rustflags); - - assert!(cmd - .get_envs() - .any(|(key, val)| key == "RUSTFLAGS" && val == Some(OsStr::new("-W clippy::as_conversions")))); - } - - #[test] - fn clippy_args_respect_existing_rustflags() { - let args = "cargo clippy -- -D clippy::await_holding_lock" - .split_whitespace() - .map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = Some(r#"--cfg feature="some_feat""#.into()); - let cmd = cmd.into_std_cmd(rustflags); - - assert!(cmd.get_envs().any(|(key, val)| key == "RUSTFLAGS" - && val == Some(OsStr::new(r#"-D clippy::await_holding_lock --cfg feature="some_feat""#)))); - } - - #[test] - fn no_env_change_if_no_clippy_args() { - let args = "cargo clippy".split_whitespace().map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = Some(r#"--cfg feature="some_feat""#.into()); - let cmd = cmd.into_std_cmd(rustflags); - - assert!(!cmd - .get_envs() - .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS")); - } - - #[test] - fn no_env_change_if_no_clippy_args_nor_rustflags() { - let args = "cargo clippy".split_whitespace().map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = None; - let cmd = cmd.into_std_cmd(rustflags); - - assert!(!cmd - .get_envs() - .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS")) - } } diff --git a/tests/dogfood.rs b/tests/dogfood.rs index fda1413868e82..052223d6d6ff7 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -23,7 +23,7 @@ fn dogfood_clippy() { .current_dir(root_dir) .env("CLIPPY_DOGFOOD", "1") .env("CARGO_INCREMENTAL", "0") - .arg("clippy") + .arg("clippy-preview") .arg("--all-targets") .arg("--all-features") .arg("--") From f055e7f2e9cc7233dd31384afdb303d362578012 Mon Sep 17 00:00:00 2001 From: xFrednet Date: Tue, 22 Dec 2020 20:02:59 +0000 Subject: [PATCH 17/33] Fixed a value spelling mistake --- util/gh-pages/index.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/util/gh-pages/index.html b/util/gh-pages/index.html index ad48294b84437..1852fb6640ecc 100644 --- a/util/gh-pages/index.html +++ b/util/gh-pages/index.html @@ -181,7 +181,7 @@

} } - function searchLint(lint, therm) { + function searchLint(lint, term) { for (const field in lint.docs) { // Continue if it's not a property if (!lint.docs.hasOwnProperty(field)) { @@ -189,7 +189,7 @@

} // Return if not found - if (lint.docs[field].toLowerCase().indexOf(therm) !== -1) { + if (lint.docs[field].toLowerCase().indexOf(term) !== -1) { return true; } } @@ -247,13 +247,13 @@

// Search the description // The use of `for`-loops instead of `foreach` enables us to return early - let therms = searchStr.split(" "); - for (index = 0; index < therms.length; index++) { - if (lint.id.indexOf(therms[index]) !== -1) { + let terms = searchStr.split(" "); + for (index = 0; index < terms.length; index++) { + if (lint.id.indexOf(terms[index]) !== -1) { continue; } - if (searchLint(lint, therms[index])) { + if (searchLint(lint, terms[index])) { continue; } From 88491e2a5168b2266473f476855024200b6df376 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Wed, 23 Dec 2020 10:52:53 +0100 Subject: [PATCH 18/33] Special sync of 'e89801553ddbaccdeb2eac4db08900edb51ac7ff' --- Cargo.toml | 1 + README.md | 1 + src/driver.rs | 116 ++++++++++++----------------------------------- src/main.rs | 98 ++++++--------------------------------- tests/dogfood.rs | 2 +- 5 files changed, 47 insertions(+), 171 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7f9d22e594b9c..a765390c6032d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ publish = false [[bin]] name = "cargo-clippy" +test = false path = "src/main.rs" [[bin]] diff --git a/README.md b/README.md index dc931963726b2..aaa55e11c7db1 100644 --- a/README.md +++ b/README.md @@ -208,6 +208,7 @@ the lint(s) you are interested in: ```terminal cargo clippy -- -A clippy::all -W clippy::useless_format -W clippy::... ``` +Note that if you've run clippy before, this may only take effect after you've modified a file or ran `cargo clean`. ### Specifying the minimum supported Rust version diff --git a/src/driver.rs b/src/driver.rs index 40f1b802e60e6..e490ee54c0be0 100644 --- a/src/driver.rs +++ b/src/driver.rs @@ -1,6 +1,5 @@ #![feature(rustc_private)] #![feature(once_cell)] -#![feature(bool_to_option)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -20,7 +19,6 @@ use rustc_tools_util::VersionInfo; use std::borrow::Cow; use std::env; -use std::iter; use std::lazy::SyncLazy; use std::ops::Deref; use std::panic; @@ -49,6 +47,20 @@ fn arg_value<'a, T: Deref>( None } +#[test] +fn test_arg_value() { + let args = &["--bar=bar", "--foobar", "123", "--foo"]; + + assert_eq!(arg_value(&[] as &[&str], "--foobar", |_| true), None); + assert_eq!(arg_value(args, "--bar", |_| false), None); + assert_eq!(arg_value(args, "--bar", |_| true), Some("bar")); + assert_eq!(arg_value(args, "--bar", |p| p == "bar"), Some("bar")); + assert_eq!(arg_value(args, "--bar", |p| p == "foo"), None); + assert_eq!(arg_value(args, "--foobar", |p| p == "foo"), None); + assert_eq!(arg_value(args, "--foobar", |p| p == "123"), Some("123")); + assert_eq!(arg_value(args, "--foo", |_| true), None); +} + struct DefaultCallbacks; impl rustc_driver::Callbacks for DefaultCallbacks {} @@ -170,28 +182,6 @@ fn toolchain_path(home: Option, toolchain: Option) -> Option(args: &mut Vec, clippy_args: I) -where - T: AsRef, - U: AsRef + ?Sized + 'a, - I: Iterator + Clone, -{ - let args_iter = clippy_args.map(AsRef::as_ref); - let args_count = args_iter.clone().count(); - - if args_count > 0 { - if let Some(start) = args.windows(args_count).enumerate().find_map(|(current, window)| { - window - .iter() - .map(AsRef::as_ref) - .eq(args_iter.clone()) - .then_some(current) - }) { - args.drain(start..start + args_count); - } - } -} - #[allow(clippy::too_many_lines)] pub fn main() { rustc_driver::init_rustc_env_logger(); @@ -288,9 +278,20 @@ pub fn main() { args.extend(vec!["--sysroot".into(), sys_root]); }; - let clippy_args = env::var("CLIPPY_ARGS").unwrap_or_default(); - let clippy_args = clippy_args.split_whitespace(); - let no_deps = clippy_args.clone().any(|flag| flag == "--no-deps"); + let mut no_deps = false; + let clippy_args = env::var("CLIPPY_ARGS") + .unwrap_or_default() + .split("__CLIPPY_HACKERY__") + .filter_map(|s| match s { + "" => None, + "--no-deps" => { + no_deps = true; + None + }, + _ => Some(s.to_string()), + }) + .chain(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]) + .collect::>(); // We enable Clippy if one of the following conditions is met // - IF Clippy is run on its test suite OR @@ -303,11 +304,7 @@ pub fn main() { let clippy_enabled = clippy_tests_set || (!cap_lints_allow && (!no_deps || in_primary_package)); if clippy_enabled { - remove_clippy_args(&mut args, iter::once("--no-deps")); - args.extend(vec!["--cfg".into(), r#"feature="cargo-clippy""#.into()]); - } else { - // Remove all flags passed through RUSTFLAGS if Clippy is not enabled. - remove_clippy_args(&mut args, clippy_args); + args.extend(clippy_args); } let mut clippy = ClippyCallbacks; @@ -318,58 +315,3 @@ pub fn main() { rustc_driver::RunCompiler::new(&args, callbacks).run() })) } - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_arg_value() { - let args = &["--bar=bar", "--foobar", "123", "--foo"]; - - assert_eq!(arg_value(&[] as &[&str], "--foobar", |_| true), None); - assert_eq!(arg_value(args, "--bar", |_| false), None); - assert_eq!(arg_value(args, "--bar", |_| true), Some("bar")); - assert_eq!(arg_value(args, "--bar", |p| p == "bar"), Some("bar")); - assert_eq!(arg_value(args, "--bar", |p| p == "foo"), None); - assert_eq!(arg_value(args, "--foobar", |p| p == "foo"), None); - assert_eq!(arg_value(args, "--foobar", |p| p == "123"), Some("123")); - assert_eq!(arg_value(args, "--foo", |_| true), None); - } - - #[test] - fn removes_clippy_args_from_start() { - let mut args = vec!["-D", "clippy::await_holding_lock", "--cfg", r#"feature="some_feat""#]; - let clippy_args = ["-D", "clippy::await_holding_lock"].iter(); - - remove_clippy_args(&mut args, clippy_args); - assert_eq!(args, &["--cfg", r#"feature="some_feat""#]); - } - - #[test] - fn removes_clippy_args_from_end() { - let mut args = vec!["-Zui-testing", "-A", "clippy::empty_loop", "--no-deps"]; - let clippy_args = ["-A", "clippy::empty_loop", "--no-deps"].iter(); - - remove_clippy_args(&mut args, clippy_args); - assert_eq!(args, &["-Zui-testing"]); - } - - #[test] - fn removes_clippy_args_from_middle() { - let mut args = vec!["-Zui-testing", "-W", "clippy::filter_map", "-L", "serde"]; - let clippy_args = ["-W", "clippy::filter_map"].iter(); - - remove_clippy_args(&mut args, clippy_args); - assert_eq!(args, &["-Zui-testing", "-L", "serde"]); - } - - #[test] - fn no_clippy_args_to_remove() { - let mut args = vec!["-Zui-testing", "-L", "serde"]; - let clippy_args: [&str; 0] = []; - - remove_clippy_args(&mut args, clippy_args.iter()); - assert_eq!(args, &["-Zui-testing", "-L", "serde"]); - } -} diff --git a/src/main.rs b/src/main.rs index 1c0e04689a9fe..ea06743394d10 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,3 @@ -#![feature(bool_to_option)] -#![feature(command_access)] #![cfg_attr(feature = "deny-warnings", deny(warnings))] // warn on lints, that are included in `rust-lang/rust`s bootstrap #![warn(rust_2018_idioms, unused_lifetimes)] @@ -64,7 +62,7 @@ struct ClippyCmd { unstable_options: bool, cargo_subcommand: &'static str, args: Vec, - clippy_args: Option, + clippy_args: Vec, } impl ClippyCmd { @@ -101,17 +99,16 @@ impl ClippyCmd { args.insert(0, "+nightly".to_string()); } - let mut clippy_args = old_args.collect::>().join(" "); - if cargo_subcommand == "fix" && !clippy_args.contains("--no-deps") { - clippy_args = format!("{} --no-deps", clippy_args); + let mut clippy_args: Vec = old_args.collect(); + if cargo_subcommand == "fix" && !clippy_args.iter().any(|arg| arg == "--no-deps") { + clippy_args.push("--no-deps".into()); } - let has_args = !clippy_args.is_empty(); ClippyCmd { unstable_options, cargo_subcommand, args, - clippy_args: has_args.then_some(clippy_args), + clippy_args, } } @@ -151,24 +148,20 @@ impl ClippyCmd { .map(|p| ("CARGO_TARGET_DIR", p)) } - fn into_std_cmd(self, rustflags: Option) -> Command { + fn into_std_cmd(self) -> Command { let mut cmd = Command::new("cargo"); + let clippy_args: String = self + .clippy_args + .iter() + .map(|arg| format!("{}__CLIPPY_HACKERY__", arg)) + .collect(); cmd.env(self.path_env(), Self::path()) .envs(ClippyCmd::target_dir()) + .env("CLIPPY_ARGS", clippy_args) .arg(self.cargo_subcommand) .args(&self.args); - // HACK: pass Clippy args to the driver *also* through RUSTFLAGS. - // This guarantees that new builds will be triggered when Clippy flags change. - if let Some(clippy_args) = self.clippy_args { - cmd.env( - "RUSTFLAGS", - rustflags.map_or(clippy_args.clone(), |flags| format!("{} {}", clippy_args, flags)), - ); - cmd.env("CLIPPY_ARGS", clippy_args); - } - cmd } } @@ -179,7 +172,7 @@ where { let cmd = ClippyCmd::new(old_args); - let mut cmd = cmd.into_std_cmd(env::var("RUSTFLAGS").ok()); + let mut cmd = cmd.into_std_cmd(); let exit_status = cmd .spawn() @@ -197,7 +190,6 @@ where #[cfg(test)] mod tests { use super::ClippyCmd; - use std::ffi::OsStr; #[test] #[should_panic] @@ -212,7 +204,6 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - assert_eq!("fix", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); assert!(cmd.args.iter().any(|arg| arg.ends_with("unstable-options"))); @@ -224,8 +215,7 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - - assert!(cmd.clippy_args.unwrap().contains("--no-deps")); + assert!(cmd.clippy_args.iter().any(|arg| arg == "--no-deps")); } #[test] @@ -234,15 +224,13 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - - assert_eq!(1, cmd.clippy_args.unwrap().matches("--no-deps").count()); + assert_eq!(cmd.clippy_args.iter().filter(|arg| *arg == "--no-deps").count(), 1); } #[test] fn check() { let args = "cargo clippy".split_whitespace().map(ToString::to_string); let cmd = ClippyCmd::new(args); - assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WRAPPER", cmd.path_env()); } @@ -253,63 +241,7 @@ mod tests { .split_whitespace() .map(ToString::to_string); let cmd = ClippyCmd::new(args); - assert_eq!("check", cmd.cargo_subcommand); assert_eq!("RUSTC_WORKSPACE_WRAPPER", cmd.path_env()); } - - #[test] - fn clippy_args_into_rustflags() { - let args = "cargo clippy -- -W clippy::as_conversions" - .split_whitespace() - .map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = None; - let cmd = cmd.into_std_cmd(rustflags); - - assert!(cmd - .get_envs() - .any(|(key, val)| key == "RUSTFLAGS" && val == Some(OsStr::new("-W clippy::as_conversions")))); - } - - #[test] - fn clippy_args_respect_existing_rustflags() { - let args = "cargo clippy -- -D clippy::await_holding_lock" - .split_whitespace() - .map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = Some(r#"--cfg feature="some_feat""#.into()); - let cmd = cmd.into_std_cmd(rustflags); - - assert!(cmd.get_envs().any(|(key, val)| key == "RUSTFLAGS" - && val == Some(OsStr::new(r#"-D clippy::await_holding_lock --cfg feature="some_feat""#)))); - } - - #[test] - fn no_env_change_if_no_clippy_args() { - let args = "cargo clippy".split_whitespace().map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = Some(r#"--cfg feature="some_feat""#.into()); - let cmd = cmd.into_std_cmd(rustflags); - - assert!(!cmd - .get_envs() - .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS")); - } - - #[test] - fn no_env_change_if_no_clippy_args_nor_rustflags() { - let args = "cargo clippy".split_whitespace().map(ToString::to_string); - let cmd = ClippyCmd::new(args); - - let rustflags = None; - let cmd = cmd.into_std_cmd(rustflags); - - assert!(!cmd - .get_envs() - .any(|(key, _)| key == "RUSTFLAGS" || key == "CLIPPY_ARGS")) - } } diff --git a/tests/dogfood.rs b/tests/dogfood.rs index fda1413868e82..052223d6d6ff7 100644 --- a/tests/dogfood.rs +++ b/tests/dogfood.rs @@ -23,7 +23,7 @@ fn dogfood_clippy() { .current_dir(root_dir) .env("CLIPPY_DOGFOOD", "1") .env("CARGO_INCREMENTAL", "0") - .arg("clippy") + .arg("clippy-preview") .arg("--all-targets") .arg("--all-features") .arg("--") From 3687de21597a3581e92d56e130e9ec4dd4407016 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Tue, 22 Dec 2020 01:37:49 +0100 Subject: [PATCH 19/33] ISSUE_TEMPLATE: add templates for false negative and false positive tickets. --- .github/ISSUE_TEMPLATE/false_negative.md | 35 ++++++++++++++++++++++++ .github/ISSUE_TEMPLATE/false_positive.md | 35 ++++++++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/false_negative.md create mode 100644 .github/ISSUE_TEMPLATE/false_positive.md diff --git a/.github/ISSUE_TEMPLATE/false_negative.md b/.github/ISSUE_TEMPLATE/false_negative.md new file mode 100644 index 0000000000000..f46828fec91b2 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/false_negative.md @@ -0,0 +1,35 @@ +--- +name: Bug Report (False Negative) +about: Create a bug report about missing warnings from a lint +labels: L-bug, L-false-negative +--- + +Lint name: + + +I tried this code: + +```rust + +``` + +I expected to see this happen: *explanation* + +Instead, this happened: *explanation* + +### Meta + +- `cargo clippy -V`: e.g. clippy 0.0.212 (f455e46 2020-06-20) +- `rustc -Vv`: + ``` + rustc 1.46.0-nightly (f455e46ea 2020-06-20) + binary: rustc + commit-hash: f455e46eae1a227d735091091144601b467e1565 + commit-date: 2020-06-20 + host: x86_64-unknown-linux-gnu + release: 1.46.0-nightly + LLVM version: 10.0 + ``` diff --git a/.github/ISSUE_TEMPLATE/false_positive.md b/.github/ISSUE_TEMPLATE/false_positive.md new file mode 100644 index 0000000000000..92a7373fc27d5 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/false_positive.md @@ -0,0 +1,35 @@ +--- +name: Bug Report (False Positive) +about: Create a bug report about a wrongly emitted lint warning +labels: L-bug, L-false-positive +--- + +Lint name: + + +I tried this code: + +```rust + +``` + +I expected to see this happen: *explanation* + +Instead, this happened: *explanation* + +### Meta + +- `cargo clippy -V`: e.g. clippy 0.0.212 (f455e46 2020-06-20) +- `rustc -Vv`: + ``` + rustc 1.46.0-nightly (f455e46ea 2020-06-20) + binary: rustc + commit-hash: f455e46eae1a227d735091091144601b467e1565 + commit-date: 2020-06-20 + host: x86_64-unknown-linux-gnu + release: 1.46.0-nightly + LLVM version: 10.0 + ``` From 2218dd65547bb5a16efe61b03c04285d2691bbb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Thu, 24 Dec 2020 13:11:34 +0100 Subject: [PATCH 20/33] ci: run cargo clippy --fix -Zunstable-options in the correct directory. --- .github/workflows/clippy.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/clippy.yml b/.github/workflows/clippy.yml index 17812b74ab2fd..9d5e12aac5f7b 100644 --- a/.github/workflows/clippy.yml +++ b/.github/workflows/clippy.yml @@ -50,6 +50,9 @@ jobs: - name: Build run: cargo build --features deny-warnings,internal-lints + - name: Test "--fix -Zunstable-options" + run: cargo run --features deny-warnings,internal-lints --bin cargo-clippy -- clippy --fix -Zunstable-options + - name: Test run: cargo test --features deny-warnings,internal-lints @@ -57,10 +60,6 @@ jobs: run: cargo test --features deny-warnings,internal-lints working-directory: clippy_lints - - name: Test --fix -Zunstable-options - run: cargo run --bin cargo-clippy -- clippy --fix -Zunstable-options - working-directory: clippy_lints - - name: Test rustc_tools_util run: cargo test --features deny-warnings working-directory: rustc_tools_util From e1743ef5250d67260a2a1b0126708d969f80909f Mon Sep 17 00:00:00 2001 From: Takayuki Nakata Date: Fri, 25 Dec 2020 08:59:34 +0900 Subject: [PATCH 21/33] Fix a style of texts in `map_err_ignore` --- clippy_lints/src/map_err_ignore.rs | 4 ++-- tests/ui/map_err.stderr | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/map_err_ignore.rs b/clippy_lints/src/map_err_ignore.rs index f3c0515b9bcde..76fe8e776eafd 100644 --- a/clippy_lints/src/map_err_ignore.rs +++ b/clippy_lints/src/map_err_ignore.rs @@ -7,7 +7,7 @@ use rustc_session::{declare_lint_pass, declare_tool_lint}; declare_clippy_lint! { /// **What it does:** Checks for instances of `map_err(|_| Some::Enum)` /// - /// **Why is this bad?** This map_err throws away the original error rather than allowing the enum to contain and report the cause of the error + /// **Why is this bad?** This `map_err` throws away the original error rather than allowing the enum to contain and report the cause of the error /// /// **Known problems:** None. /// @@ -135,7 +135,7 @@ impl<'tcx> LateLintPass<'tcx> for MapErrIgnore { body_span, "`map_err(|_|...` wildcard pattern discards the original error", None, - "Consider storing the original error as a source in the new error, or silence this warning using an ignored identifier (`.map_err(|_foo| ...`)", + "consider storing the original error as a source in the new error, or silence this warning using an ignored identifier (`.map_err(|_foo| ...`)", ); } } diff --git a/tests/ui/map_err.stderr b/tests/ui/map_err.stderr index 8ee2941790d35..37e87e64de28f 100644 --- a/tests/ui/map_err.stderr +++ b/tests/ui/map_err.stderr @@ -5,7 +5,7 @@ LL | println!("{:?}", x.map_err(|_| Errors::Ignored)); | ^^^ | = note: `-D clippy::map-err-ignore` implied by `-D warnings` - = help: Consider storing the original error as a source in the new error, or silence this warning using an ignored identifier (`.map_err(|_foo| ...`) + = help: consider storing the original error as a source in the new error, or silence this warning using an ignored identifier (`.map_err(|_foo| ...`) error: aborting due to previous error From dfaea9c9677f97656ad75f6bacd78f8f87e1d339 Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Fri, 25 Dec 2020 14:45:04 +0300 Subject: [PATCH 22/33] lint &PathBuf instead of &Path in PTR_ARG - extract get_only_generic_arg_snippet to improve readability --- clippy_dev/src/ra_setup.rs | 4 +-- clippy_lints/src/ptr.rs | 60 ++++++++++++++++++++++++++++---------- tests/ui/ptr_arg.rs | 22 ++++++++++++++ tests/ui/ptr_arg.stderr | 45 +++++++++++++++++++++++----- 4 files changed, 106 insertions(+), 25 deletions(-) diff --git a/clippy_dev/src/ra_setup.rs b/clippy_dev/src/ra_setup.rs index 40bf4a9505a88..5f5048e79e782 100644 --- a/clippy_dev/src/ra_setup.rs +++ b/clippy_dev/src/ra_setup.rs @@ -3,7 +3,7 @@ use std::fs; use std::fs::File; use std::io::prelude::*; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; // This module takes an absolute path to a rustc repo and alters the dependencies to point towards // the respective rustc subcrates instead of using extern crate xyz. @@ -44,7 +44,7 @@ pub fn run(rustc_path: Option<&str>) { } fn inject_deps_into_manifest( - rustc_source_dir: &PathBuf, + rustc_source_dir: &Path, manifest_path: &str, cargo_toml: &str, lib_rs: &str, diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index dcb643a28aebc..c494a71363131 100644 --- a/clippy_lints/src/ptr.rs +++ b/clippy_lints/src/ptr.rs @@ -182,20 +182,6 @@ fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, fn_id: HirId, opt_body_id: if let ty::Ref(_, ty, Mutability::Not) = ty.kind() { if is_type_diagnostic_item(cx, ty, sym::vec_type) { - let mut ty_snippet = None; - if_chain! { - if let TyKind::Path(QPath::Resolved(_, ref path)) = walk_ptrs_hir_ty(arg).kind; - if let Some(&PathSegment{args: Some(ref parameters), ..}) = path.segments.last(); - then { - let types: Vec<_> = parameters.args.iter().filter_map(|arg| match arg { - GenericArg::Type(ty) => Some(ty), - _ => None, - }).collect(); - if types.len() == 1 { - ty_snippet = snippet_opt(cx, types[0].span); - } - } - }; if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_owned()")]) { span_lint_and_then( cx, @@ -204,7 +190,7 @@ fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, fn_id: HirId, opt_body_id: "writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used \ with non-Vec-based slices.", |diag| { - if let Some(ref snippet) = ty_snippet { + if let Some(ref snippet) = get_only_generic_arg_snippet(cx, arg) { diag.span_suggestion( arg.span, "change this to", @@ -247,6 +233,33 @@ fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, fn_id: HirId, opt_body_id: }, ); } + } else if match_type(cx, ty, &paths::PATH_BUF) { + if let Some(spans) = get_spans(cx, opt_body_id, idx, &[("clone", ".to_path_buf()"), ("as_path", "")]) { + span_lint_and_then( + cx, + PTR_ARG, + arg.span, + "writing `&PathBuf` instead of `&Path` involves a new object where a slice will do.", + |diag| { + diag.span_suggestion( + arg.span, + "change this to", + "&Path".into(), + Applicability::Unspecified, + ); + for (clonespan, suggestion) in spans { + diag.span_suggestion_short( + clonespan, + &snippet_opt(cx, clonespan).map_or("change the call to".into(), |x| { + Cow::Owned(format!("change `{}` to", x)) + }), + suggestion.into(), + Applicability::Unspecified, + ); + } + }, + ); + } } else if match_type(cx, ty, &paths::COW) { if_chain! { if let TyKind::Rptr(_, MutTy { ref ty, ..} ) = arg.kind; @@ -309,6 +322,23 @@ fn check_fn(cx: &LateContext<'_>, decl: &FnDecl<'_>, fn_id: HirId, opt_body_id: } } +fn get_only_generic_arg_snippet(cx: &LateContext<'_>, arg: &Ty<'_>) -> Option { + if_chain! { + if let TyKind::Path(QPath::Resolved(_, ref path)) = walk_ptrs_hir_ty(arg).kind; + if let Some(&PathSegment{args: Some(ref parameters), ..}) = path.segments.last(); + let types: Vec<_> = parameters.args.iter().filter_map(|arg| match arg { + GenericArg::Type(ty) => Some(ty), + _ => None, + }).collect(); + if types.len() == 1; + then { + snippet_opt(cx, types[0].span) + } else { + None + } + } +} + fn get_rptr_lm<'tcx>(ty: &'tcx Ty<'tcx>) -> Option<(&'tcx Lifetime, Mutability, Span)> { if let TyKind::Rptr(ref lt, ref m) = ty.kind { Some((lt, m.mutbl, ty.span)) diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index 541225e635102..e8854fb73d966 100644 --- a/tests/ui/ptr_arg.rs +++ b/tests/ui/ptr_arg.rs @@ -2,6 +2,7 @@ #![warn(clippy::ptr_arg)] use std::borrow::Cow; +use std::path::PathBuf; fn do_vec(x: &Vec) { //Nothing here @@ -21,6 +22,15 @@ fn do_str_mut(x: &mut String) { //Nothing here either } +fn do_path(x: &PathBuf) { + //Nothing here either +} + +fn do_path_mut(x: &mut PathBuf) { + // no error here + //Nothing here either +} + fn main() {} trait Foo { @@ -55,6 +65,14 @@ fn str_cloned(x: &String) -> String { x.clone() } +fn path_cloned(x: &PathBuf) -> PathBuf { + let a = x.clone(); + let b = x.clone(); + let c = b.clone(); + let d = a.clone().clone().clone(); + x.clone() +} + fn false_positive_capacity(x: &Vec, y: &String) { let a = x.capacity(); let b = y.clone(); @@ -87,10 +105,12 @@ impl Foo2 for String { // Check that the allow attribute on parameters is honored mod issue_5644 { use std::borrow::Cow; + use std::path::PathBuf; fn allowed( #[allow(clippy::ptr_arg)] _v: &Vec, #[allow(clippy::ptr_arg)] _s: &String, + #[allow(clippy::ptr_arg)] _p: &PathBuf, #[allow(clippy::ptr_arg)] _c: &Cow<[i32]>, ) { } @@ -100,6 +120,7 @@ mod issue_5644 { fn allowed( #[allow(clippy::ptr_arg)] _v: &Vec, #[allow(clippy::ptr_arg)] _s: &String, + #[allow(clippy::ptr_arg)] _p: &PathBuf, #[allow(clippy::ptr_arg)] _c: &Cow<[i32]>, ) { } @@ -109,6 +130,7 @@ mod issue_5644 { fn allowed( #[allow(clippy::ptr_arg)] _v: &Vec, #[allow(clippy::ptr_arg)] _s: &String, + #[allow(clippy::ptr_arg)] _p: &PathBuf, #[allow(clippy::ptr_arg)] _c: &Cow<[i32]>, ) { } diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr index 314f23497f971..70d1b2f5258eb 100644 --- a/tests/ui/ptr_arg.stderr +++ b/tests/ui/ptr_arg.stderr @@ -1,5 +1,5 @@ error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices. - --> $DIR/ptr_arg.rs:6:14 + --> $DIR/ptr_arg.rs:7:14 | LL | fn do_vec(x: &Vec) { | ^^^^^^^^^ help: change this to: `&[i64]` @@ -7,19 +7,25 @@ LL | fn do_vec(x: &Vec) { = note: `-D clippy::ptr-arg` implied by `-D warnings` error: writing `&String` instead of `&str` involves a new object where a slice will do. - --> $DIR/ptr_arg.rs:15:14 + --> $DIR/ptr_arg.rs:16:14 | LL | fn do_str(x: &String) { | ^^^^^^^ help: change this to: `&str` +error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do. + --> $DIR/ptr_arg.rs:25:15 + | +LL | fn do_path(x: &PathBuf) { + | ^^^^^^^^ help: change this to: `&Path` + error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices. - --> $DIR/ptr_arg.rs:28:18 + --> $DIR/ptr_arg.rs:38:18 | LL | fn do_vec(x: &Vec); | ^^^^^^^^^ help: change this to: `&[i64]` error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices. - --> $DIR/ptr_arg.rs:41:14 + --> $DIR/ptr_arg.rs:51:14 | LL | fn cloned(x: &Vec) -> Vec { | ^^^^^^^^ @@ -38,7 +44,7 @@ LL | x.to_owned() | error: writing `&String` instead of `&str` involves a new object where a slice will do. - --> $DIR/ptr_arg.rs:50:18 + --> $DIR/ptr_arg.rs:60:18 | LL | fn str_cloned(x: &String) -> String { | ^^^^^^^ @@ -60,8 +66,31 @@ help: change `x.clone()` to LL | x.to_string() | +error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do. + --> $DIR/ptr_arg.rs:68:19 + | +LL | fn path_cloned(x: &PathBuf) -> PathBuf { + | ^^^^^^^^ + | +help: change this to + | +LL | fn path_cloned(x: &Path) -> PathBuf { + | ^^^^^ +help: change `x.clone()` to + | +LL | let a = x.to_path_buf(); + | ^^^^^^^^^^^^^^^ +help: change `x.clone()` to + | +LL | let b = x.to_path_buf(); + | ^^^^^^^^^^^^^^^ +help: change `x.clone()` to + | +LL | x.to_path_buf() + | + error: writing `&String` instead of `&str` involves a new object where a slice will do. - --> $DIR/ptr_arg.rs:58:44 + --> $DIR/ptr_arg.rs:76:44 | LL | fn false_positive_capacity(x: &Vec, y: &String) { | ^^^^^^^ @@ -80,10 +109,10 @@ LL | let c = y; | ^ error: using a reference to `Cow` is not recommended. - --> $DIR/ptr_arg.rs:72:25 + --> $DIR/ptr_arg.rs:90:25 | LL | fn test_cow_with_ref(c: &Cow<[i32]>) {} | ^^^^^^^^^^^ help: change this to: `&[i32]` -error: aborting due to 7 previous errors +error: aborting due to 9 previous errors From 203715aa4ecebb167868100fe14b0eaa05133718 Mon Sep 17 00:00:00 2001 From: Aleksei Latyshev Date: Mon, 28 Dec 2020 01:09:04 +0300 Subject: [PATCH 23/33] don't ignore expression after first not matched method call in PtrCloneVisitor --- clippy_lints/src/utils/ptr.rs | 1 - tests/ui/ptr_arg.rs | 19 +++++++++++ tests/ui/ptr_arg.stderr | 59 ++++++++++++++++++++++++++++++++++- 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/utils/ptr.rs b/clippy_lints/src/utils/ptr.rs index bd2c619f00028..b330f3d890e9c 100644 --- a/clippy_lints/src/utils/ptr.rs +++ b/clippy_lints/src/utils/ptr.rs @@ -72,7 +72,6 @@ impl<'a, 'tcx> Visitor<'tcx> for PtrCloneVisitor<'a, 'tcx> { } } } - return; } walk_expr(self, expr); } diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index e8854fb73d966..06370dfce6518 100644 --- a/tests/ui/ptr_arg.rs +++ b/tests/ui/ptr_arg.rs @@ -136,3 +136,22 @@ mod issue_5644 { } } } + +mod issue6509 { + use std::path::PathBuf; + + fn foo_vec(vec: &Vec) { + let _ = vec.clone().pop(); + let _ = vec.clone().clone(); + } + + fn foo_path(path: &PathBuf) { + let _ = path.clone().pop(); + let _ = path.clone().clone(); + } + + fn foo_str(str: &PathBuf) { + let _ = str.clone().pop(); + let _ = str.clone().clone(); + } +} diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr index 70d1b2f5258eb..708318bbe295c 100644 --- a/tests/ui/ptr_arg.stderr +++ b/tests/ui/ptr_arg.stderr @@ -114,5 +114,62 @@ error: using a reference to `Cow` is not recommended. LL | fn test_cow_with_ref(c: &Cow<[i32]>) {} | ^^^^^^^^^^^ help: change this to: `&[i32]` -error: aborting due to 9 previous errors +error: writing `&Vec<_>` instead of `&[_]` involves one more reference and cannot be used with non-Vec-based slices. + --> $DIR/ptr_arg.rs:143:21 + | +LL | fn foo_vec(vec: &Vec) { + | ^^^^^^^^ + | +help: change this to + | +LL | fn foo_vec(vec: &[u8]) { + | ^^^^^ +help: change `vec.clone()` to + | +LL | let _ = vec.to_owned().pop(); + | ^^^^^^^^^^^^^^ +help: change `vec.clone()` to + | +LL | let _ = vec.to_owned().clone(); + | ^^^^^^^^^^^^^^ + +error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do. + --> $DIR/ptr_arg.rs:148:23 + | +LL | fn foo_path(path: &PathBuf) { + | ^^^^^^^^ + | +help: change this to + | +LL | fn foo_path(path: &Path) { + | ^^^^^ +help: change `path.clone()` to + | +LL | let _ = path.to_path_buf().pop(); + | ^^^^^^^^^^^^^^^^^^ +help: change `path.clone()` to + | +LL | let _ = path.to_path_buf().clone(); + | ^^^^^^^^^^^^^^^^^^ + +error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do. + --> $DIR/ptr_arg.rs:153:21 + | +LL | fn foo_str(str: &PathBuf) { + | ^^^^^^^^ + | +help: change this to + | +LL | fn foo_str(str: &Path) { + | ^^^^^ +help: change `str.clone()` to + | +LL | let _ = str.to_path_buf().pop(); + | ^^^^^^^^^^^^^^^^^ +help: change `str.clone()` to + | +LL | let _ = str.to_path_buf().clone(); + | ^^^^^^^^^^^^^^^^^ + +error: aborting due to 12 previous errors From 6fd18f95ca1ab6161790a5274000517ea345e75b Mon Sep 17 00:00:00 2001 From: Aaron Hill Date: Tue, 29 Dec 2020 17:15:53 -0500 Subject: [PATCH 24/33] Remove unnecessary semicolon from Clippy test --- tests/ui/match_expr_like_matches_macro.fixed | 2 +- tests/ui/match_expr_like_matches_macro.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ui/match_expr_like_matches_macro.fixed b/tests/ui/match_expr_like_matches_macro.fixed index 7f4ebf566733a..84981a5259732 100644 --- a/tests/ui/match_expr_like_matches_macro.fixed +++ b/tests/ui/match_expr_like_matches_macro.fixed @@ -39,7 +39,7 @@ fn main() { B(i32), C, D, - }; + } let x = E::A(2); { // lint diff --git a/tests/ui/match_expr_like_matches_macro.rs b/tests/ui/match_expr_like_matches_macro.rs index aee56dd4a5ef4..94c7c3cadacf7 100644 --- a/tests/ui/match_expr_like_matches_macro.rs +++ b/tests/ui/match_expr_like_matches_macro.rs @@ -51,7 +51,7 @@ fn main() { B(i32), C, D, - }; + } let x = E::A(2); { // lint From 5479bbaf72cf27cb768079eff199eb71adf2a368 Mon Sep 17 00:00:00 2001 From: Joshua Nelson Date: Tue, 29 Dec 2020 20:28:08 -0500 Subject: [PATCH 25/33] Rename kw::Invalid -> kw::Empty See https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Is.20there.20a.20symbol.20for.20the.20empty.20string.3F/near/220054471 for context. --- clippy_lints/src/lifetimes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 4d737b3f49b03..e84c8b4e5b3e0 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -501,7 +501,7 @@ impl<'tcx> Visitor<'tcx> for BodyLifetimeChecker { // for lifetimes as parameters of generics fn visit_lifetime(&mut self, lifetime: &'tcx Lifetime) { - if lifetime.name.ident().name != kw::Invalid && lifetime.name.ident().name != kw::StaticLifetime { + if lifetime.name.ident().name != kw::Empty && lifetime.name.ident().name != kw::StaticLifetime { self.lifetimes_used_in_body = true; } } From 59397d6abb55116e778e0b9667b7ac74f0356704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matthias=20Kr=C3=BCger?= Date: Thu, 31 Dec 2020 16:07:20 +0100 Subject: [PATCH 26/33] make clippy version number correspond to rustc version number. clippy 0.1.50 corresponds to rustc 1.50.x This bumps the clippy version number from 0.0.212 to 0.1.50 Fixes #6499 --- Cargo.toml | 4 +-- clippy_lints/Cargo.toml | 2 +- tests/versioncheck.rs | 56 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a765390c6032d..93a1e71ecabe1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clippy" -version = "0.0.212" +version = "0.1.50" authors = [ "Manish Goregaokar ", "Andre Bogus ", @@ -29,7 +29,7 @@ path = "src/driver.rs" [dependencies] # begin automatic update -clippy_lints = { version = "0.0.212", path = "clippy_lints" } +clippy_lints = { version = "0.1.50", path = "clippy_lints" } # end automatic update semver = "0.11" rustc_tools_util = { version = "0.2.0", path = "rustc_tools_util" } diff --git a/clippy_lints/Cargo.toml b/clippy_lints/Cargo.toml index 7697eba650aca..7e3eaf3ae7447 100644 --- a/clippy_lints/Cargo.toml +++ b/clippy_lints/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "clippy_lints" # begin automatic update -version = "0.0.212" +version = "0.1.50" # end automatic update authors = [ "Manish Goregaokar ", diff --git a/tests/versioncheck.rs b/tests/versioncheck.rs index f5d03c645df04..ec54e11dc06c5 100644 --- a/tests/versioncheck.rs +++ b/tests/versioncheck.rs @@ -1,3 +1,6 @@ +#![allow(clippy::single_match_else)] +use rustc_tools_util::VersionInfo; + #[test] fn check_that_clippy_lints_has_the_same_version_as_clippy() { let clippy_meta = cargo_metadata::MetadataCommand::new() @@ -17,3 +20,56 @@ fn check_that_clippy_lints_has_the_same_version_as_clippy() { } } } + +#[test] +fn check_that_clippy_has_the_same_major_version_as_rustc() { + let clippy_version = rustc_tools_util::get_version_info!(); + let clippy_major = clippy_version.major; + let clippy_minor = clippy_version.minor; + let clippy_patch = clippy_version.patch; + + // get the rustc version from cargo + // this way the rust-toolchain file version is honored + let rustc_version = String::from_utf8( + std::process::Command::new("cargo") + .arg("--version") + .output() + .expect("failed to run 'cargo --version'") + .stdout, + ) + .unwrap(); + // extract "1 50 0" from "cargo 1.50.0-nightly (a3c2627fb 2020-12-14)" + let vsplit: Vec<&str> = rustc_version + .split(' ') + .nth(1) + .unwrap() + .split('-') + .next() + .unwrap() + .split('.') + .collect(); + match vsplit.as_slice() { + [rustc_major, rustc_minor, _rustc_patch] => { + // clippy 0.1.50 should correspond to rustc 1.50.0 + dbg!(&rustc_version); + dbg!(&clippy_version); + assert_eq!(clippy_major, 0); // this will probably stay the same for a long time + assert_eq!( + clippy_minor.to_string(), + *rustc_major, + "clippy minor version does not equal rustc major version" + ); + assert_eq!( + clippy_patch.to_string(), + *rustc_minor, + "clippy patch version does not equal rustc minor version" + ); + // do not check rustc_patch because when a stable-patch-release is made (like 1.50.2), + // we don't want our tests failing suddenly + }, + _ => { + dbg!(vsplit); + panic!("Failed to parse rustc version"); + }, + }; +} From 48dec842f2306cb8a1493308f95cedacb74d0ab9 Mon Sep 17 00:00:00 2001 From: Julian Knodt Date: Thu, 31 Dec 2020 01:58:27 +0100 Subject: [PATCH 27/33] first pass at default values for const generics - Adds optional default values to const generic parameters in the AST and HIR - Parses these optional default values - Adds a `const_generics_defaults` feature gate --- clippy_lints/src/utils/ast_utils.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/utils/ast_utils.rs b/clippy_lints/src/utils/ast_utils.rs index f0267e4c79289..940573e4caa69 100644 --- a/clippy_lints/src/utils/ast_utils.rs +++ b/clippy_lints/src/utils/ast_utils.rs @@ -407,6 +407,10 @@ pub fn eq_use_tree_kind(l: &UseTreeKind, r: &UseTreeKind) -> bool { } } +pub fn eq_anon_const(l: &AnonConst, r: &AnonConst) -> bool { + eq_expr(&l.value, &r.value) +} + pub fn eq_defaultness(l: Defaultness, r: Defaultness) -> bool { matches!( (l, r), @@ -497,7 +501,8 @@ pub fn eq_generic_param(l: &GenericParam, r: &GenericParam) -> bool { && match (&l.kind, &r.kind) { (Lifetime, Lifetime) => true, (Type { default: l }, Type { default: r }) => both(l, r, |l, r| eq_ty(l, r)), - (Const { ty: l, kw_span: _ }, Const { ty: r, kw_span: _ }) => eq_ty(l, r), + (Const { ty: lt, kw_span: _ , default: ld}, Const { ty: rt, kw_span: _, default: rd }) => + eq_ty(lt, rt) && both(ld, rd, |ld, rd| eq_anon_const(ld, rd)), _ => false, } && over(&l.attrs, &r.attrs, |l, r| eq_attr(l, r)) From e5a1f22f486d70676620cc67cad45ecf359fb56e Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Fri, 1 Jan 2021 16:21:31 +0100 Subject: [PATCH 28/33] Initial support for Rust 2021. Clippy treated Rust 2021 as Rust 2015, because 2018 was checked with `==` instead of `>=`. This fixes that, such that 2018-specific things are also enabled for 2021. --- clippy_lints/src/macro_use.rs | 2 +- clippy_lints/src/single_component_path_imports.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/macro_use.rs b/clippy_lints/src/macro_use.rs index b4b4b3dc18d51..bb52888883af5 100644 --- a/clippy_lints/src/macro_use.rs +++ b/clippy_lints/src/macro_use.rs @@ -105,7 +105,7 @@ impl MacroUseImports { impl<'tcx> LateLintPass<'tcx> for MacroUseImports { fn check_item(&mut self, cx: &LateContext<'_>, item: &hir::Item<'_>) { if_chain! { - if cx.sess().opts.edition == Edition::Edition2018; + if cx.sess().opts.edition >= Edition::Edition2018; if let hir::ItemKind::Use(path, _kind) = &item.kind; if let Some(mac_attr) = item .attrs diff --git a/clippy_lints/src/single_component_path_imports.rs b/clippy_lints/src/single_component_path_imports.rs index 35b38eca14d1b..1fc4ff5c2e61f 100644 --- a/clippy_lints/src/single_component_path_imports.rs +++ b/clippy_lints/src/single_component_path_imports.rs @@ -40,7 +40,7 @@ impl EarlyLintPass for SingleComponentPathImports { fn check_item(&mut self, cx: &EarlyContext<'_>, item: &Item) { if_chain! { if !in_macro(item.span); - if cx.sess.opts.edition == Edition::Edition2018; + if cx.sess.opts.edition >= Edition::Edition2018; if !item.vis.kind.is_pub(); if let ItemKind::Use(use_tree) = &item.kind; if let segments = &use_tree.prefix.segments; From db268f8ef8e8ba0b0f1fedeb82156b251c86395a Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 2 Jan 2021 16:05:54 +0100 Subject: [PATCH 29/33] Bump nightly version to 2021-01-02 --- rust-toolchain | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust-toolchain b/rust-toolchain index d2e84132f4ed9..c579beeae89be 100644 --- a/rust-toolchain +++ b/rust-toolchain @@ -1,3 +1,3 @@ [toolchain] -channel = "nightly-2020-12-20" +channel = "nightly-2021-01-02" components = ["llvm-tools-preview", "rustc-dev", "rust-src", "rustfmt"] From 106ca9665371b5738091533aca481ab0b230400b Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 2 Jan 2021 16:20:43 +0100 Subject: [PATCH 30/33] Use rustc --version in versioncheck --- tests/versioncheck.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tests/versioncheck.rs b/tests/versioncheck.rs index ec54e11dc06c5..589b19f68f7a0 100644 --- a/tests/versioncheck.rs +++ b/tests/versioncheck.rs @@ -28,17 +28,17 @@ fn check_that_clippy_has_the_same_major_version_as_rustc() { let clippy_minor = clippy_version.minor; let clippy_patch = clippy_version.patch; - // get the rustc version from cargo + // get the rustc version // this way the rust-toolchain file version is honored let rustc_version = String::from_utf8( - std::process::Command::new("cargo") + std::process::Command::new("rustc") .arg("--version") .output() - .expect("failed to run 'cargo --version'") + .expect("failed to run `rustc --version`") .stdout, ) .unwrap(); - // extract "1 50 0" from "cargo 1.50.0-nightly (a3c2627fb 2020-12-14)" + // extract "1 XX 0" from "rustc 1.XX.0-nightly ( )" let vsplit: Vec<&str> = rustc_version .split(' ') .nth(1) @@ -50,9 +50,7 @@ fn check_that_clippy_has_the_same_major_version_as_rustc() { .collect(); match vsplit.as_slice() { [rustc_major, rustc_minor, _rustc_patch] => { - // clippy 0.1.50 should correspond to rustc 1.50.0 - dbg!(&rustc_version); - dbg!(&clippy_version); + // clippy 0.1.XX should correspond to rustc 1.XX.0 assert_eq!(clippy_major, 0); // this will probably stay the same for a long time assert_eq!( clippy_minor.to_string(), @@ -68,8 +66,7 @@ fn check_that_clippy_has_the_same_major_version_as_rustc() { // we don't want our tests failing suddenly }, _ => { - dbg!(vsplit); - panic!("Failed to parse rustc version"); + panic!("Failed to parse rustc version: {:?}", vsplit); }, }; } From e4fbc5f4232b47962e9c79ffe10b84b339be2124 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 2 Jan 2021 16:26:10 +0100 Subject: [PATCH 31/33] Bump Clippy version to 0.1.51 --- Cargo.toml | 2 +- clippy_lints/Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 93a1e71ecabe1..e60aa472846cf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "clippy" -version = "0.1.50" +version = "0.1.51" authors = [ "Manish Goregaokar ", "Andre Bogus ", diff --git a/clippy_lints/Cargo.toml b/clippy_lints/Cargo.toml index 7e3eaf3ae7447..a9516560a6195 100644 --- a/clippy_lints/Cargo.toml +++ b/clippy_lints/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "clippy_lints" # begin automatic update -version = "0.1.50" +version = "0.1.51" # end automatic update authors = [ "Manish Goregaokar ", From d35d827bc8cac2992a8cd14e3418dadb202f4693 Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 2 Jan 2021 16:47:58 +0100 Subject: [PATCH 32/33] Update Cargo.lock --- Cargo.lock | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4676e4127e83c..2cf311a58f0b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -568,7 +568,7 @@ dependencies = [ [[package]] name = "clippy" -version = "0.0.212" +version = "0.1.51" dependencies = [ "cargo_metadata 0.12.0", "clippy-mini-macro-test", @@ -589,7 +589,7 @@ version = "0.2.0" [[package]] name = "clippy_lints" -version = "0.0.212" +version = "0.1.51" dependencies = [ "cargo_metadata 0.12.0", "if_chain", From 5dd64b3cd6280afce4786a467a24132c6e41fc8b Mon Sep 17 00:00:00 2001 From: flip1995 Date: Sat, 2 Jan 2021 18:01:42 +0100 Subject: [PATCH 33/33] Use bootstrap rustc for versioncheck in Clippy --- src/tools/clippy/tests/versioncheck.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/tools/clippy/tests/versioncheck.rs b/src/tools/clippy/tests/versioncheck.rs index 589b19f68f7a0..76b6126c76c6a 100644 --- a/src/tools/clippy/tests/versioncheck.rs +++ b/src/tools/clippy/tests/versioncheck.rs @@ -28,10 +28,11 @@ fn check_that_clippy_has_the_same_major_version_as_rustc() { let clippy_minor = clippy_version.minor; let clippy_patch = clippy_version.patch; - // get the rustc version - // this way the rust-toolchain file version is honored + // get the rustc version either from the rustc installed with the toolchain file or from + // `RUSTC_REAL` if Clippy is build in the Rust repo with `./x.py`. + let rustc = std::env::var("RUSTC_REAL").unwrap_or_else(|_| "rustc".to_string()); let rustc_version = String::from_utf8( - std::process::Command::new("rustc") + std::process::Command::new(&rustc) .arg("--version") .output() .expect("failed to run `rustc --version`")