Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added from_over_into lint #6476

Merged
merged 5 commits into from
Dec 22, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
83 changes: 83 additions & 0 deletions clippy_lints/src/from_over_into.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
use crate::utils::paths::INTO;
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, 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.
///
/// **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<StringWrapper> for String {
/// fn into(self) -> StringWrapper {
/// StringWrapper(self)
/// }
/// }
/// ```
/// Use instead:
/// ```rust
/// struct StringWrapper(String);
///
/// impl From<String> for StringWrapper {
/// fn from(s: String) -> StringWrapper {
/// StringWrapper(s)
/// }
/// }
/// ```
pub FROM_OVER_INTO,
style,
"Warns on implementations of `Into<..>` to use `From<..>`"
}

pub struct FromOverInto {
msrv: Option<RustcVersion>,
}

impl FromOverInto {
#[must_use]
pub fn new(msrv: Option<RustcVersion>) -> 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;
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That item might be quite large. We may want to reduce the span to the impl header.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How exactly would I extract the impl header? Didn't find a proper span while looking through documentation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK, we can do this as a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oke. If it's fine with you I could work on this, I might just need a hint on extracting the imp header Span.

"An implementation of From is preferred since it gives you Into<..> for free where the reverse isn't true.",
1c3t3a marked this conversation as resolved.
Show resolved Hide resolved
None,
"consider to implement From instead",
1c3t3a marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
}

extract_msrv_attr!(LateContext);
}
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1014,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));

Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
21 changes: 21 additions & 0 deletions tests/ui/from_over_into.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#![warn(clippy::from_over_into)]

// this should throw an error
struct StringWrapper(String);

impl Into<StringWrapper> for String {
fn into(self) -> StringWrapper {
StringWrapper(self)
}
}

// this is fine
struct A(String);

impl From<String> for A {
fn from(s: String) -> A {
A(s)
}
}

fn main() {}
15 changes: 15 additions & 0 deletions tests/ui/from_over_into.stderr
Original file line number Diff line number Diff line change
@@ -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<StringWrapper> for String {
LL | | fn into(self) -> StringWrapper {
LL | | StringWrapper(self)
LL | | }
LL | | }
| |_^
|
= note: `-D clippy::from-over-into` implied by `-D warnings`
= help: consider to implement From instead

error: aborting due to previous error

8 changes: 8 additions & 0 deletions tests/ui/min_rust_version_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ pub fn checked_conversion() {
let _ = value <= (u32::MAX as i64) && value >= 0;
}

pub struct FromOverInto(String);

impl Into<FromOverInto> for String {
fn into(self) -> FromOverInto {
FromOverInto(self)
}
}

pub fn filter_map_next() {
let a = ["1", "lol", "3", "NaN", "5"];

Expand Down
8 changes: 4 additions & 4 deletions tests/ui/min_rust_version_attr.stderr
Original file line number Diff line number Diff line change
@@ -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, ") {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand All @@ -17,13 +17,13 @@ LL | assert_eq!(<stripped>.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, ") {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
1 change: 1 addition & 0 deletions tests/ui/unused_unit.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#![deny(clippy::unused_unit)]
#![allow(dead_code)]
#![allow(clippy::from_over_into)]

struct Unitter;
impl Unitter {
Expand Down
1 change: 1 addition & 0 deletions tests/ui/unused_unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#![deny(clippy::unused_unit)]
#![allow(dead_code)]
#![allow(clippy::from_over_into)]

struct Unitter;
impl Unitter {
Expand Down
38 changes: 19 additions & 19 deletions tests/ui/unused_unit.stderr
Original file line number Diff line number Diff line change
@@ -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<F: Fn() -> (), G>(&self, f: F, _g: G) -> ()
| ^^^^^^ help: remove the `-> ()`
Expand All @@ -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<F: Fn() -> (), 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<F: FnOnce() -> (), 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<F: FnOnce() -> (), 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 `-> ()`
Expand Down