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.

Sign up for GitHub

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

Jump to bottom

Fix issue #12034: add autofixes for unnecessary_fallible_conversions #12070

Merged
merged 9 commits into from Feb 9, 2024
Merged

Fix issue #12034: add autofixes for unnecessary_fallible_conversions #12070

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
84e7974
Add autofix for functions in unnecessary_fallible_conversions
roife Jan 1, 2024
69cc983
Add tests for autofix in unnecessary_fallible_conversions
roife Jan 1, 2024
094ce3d
Add comments for unnecessary_fallible_conversions
roife Jan 1, 2024
1cce757
Add error-annotations in tests for unnecessary_fallible_conversions
roife Jan 1, 2024
eb05b07
Move qpath_spans into FunctionKind
roife Feb 2, 2024
e83c7d4
Refactor sugg builder in unnecessary_fallible_conversions
roife Feb 2, 2024
015ac10
Refactor machine_applicable_sugg in unnecessary_fallible_conversions
roife Feb 2, 2024
fb9fd51
Remove wildcard usage for patterns
roife Feb 3, 2024
3c76b2c
Merging the calculation of sugg and applicability
roife Feb 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
102 changes: 69 additions & 33 deletions clippy_lints/src/methods/unnecessary_fallible_conversions.rs
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::get_parent_expr;
use clippy_utils::ty::implements_trait;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_hir::{Expr, ExprKind, QPath};
use rustc_lint::LateContext;
use rustc_middle::ty;
use rustc_middle::ty::print::with_forced_trimmed_paths;
Expand All @@ -29,6 +29,7 @@ fn check<'tcx>(
node_args: ty::GenericArgsRef<'tcx>,
kind: FunctionKind,
primary_span: Span,
qpath: Option<&QPath<'_>>,
roife marked this conversation as resolved.
Show resolved Hide resolved
) {
if let &[self_ty, other_ty] = node_args.as_slice()
// useless_conversion already warns `T::try_from(T)`, so ignore it here
Expand All @@ -45,47 +46,80 @@ fn check<'tcx>(
&& implements_trait(cx, self_ty, from_into_trait, &[other_ty])
&& let Some(other_ty) = other_ty.as_type()
{
// Extend the span to include the unwrap/expect call:
// `foo.try_into().expect("..")`
// ^^^^^^^^^^^^^^^^^^^^^^^
//
// `try_into().unwrap()` specifically can be trivially replaced with just `into()`,
// so that can be machine-applicable
let parent_unwrap_call = get_parent_expr(cx, expr).and_then(|parent| {
if let ExprKind::MethodCall(path, .., span) = parent.kind
&& let sym::unwrap | sym::expect = path.ident.name
{
Some(span)
// include `.` before `unwrap`/`expect`
Some(span.with_lo(expr.span.hi()))
} else {
None
}
});
let (source_ty, target_ty, sugg, span, applicability) = match kind {
FunctionKind::TryIntoMethod if let Some(unwrap_span) = parent_unwrap_call => {
// Extend the span to include the unwrap/expect call:
// `foo.try_into().expect("..")`
// ^^^^^^^^^^^^^^^^^^^^^^^
//
// `try_into().unwrap()` specifically can be trivially replaced with just `into()`,
// so that can be machine-applicable

(
self_ty,
other_ty,
"into()",
primary_span.with_hi(unwrap_span.hi()),
Applicability::MachineApplicable,
)
// If there is an unwrap/expect call, extend the span to include the call
let span = if let Some(unwrap_call) = parent_unwrap_call {
primary_span.with_hi(unwrap_call.hi())
} else {
primary_span
};

let qpath_spans = qpath.and_then(|qpath| match qpath {
QPath::Resolved(_, path) => {
let segments = path.segments.iter().map(|seg| seg.ident).collect::<Vec<_>>();
(segments.len() == 2).then(|| vec![segments[0].span, segments[1].span])
roife marked this conversation as resolved.
Show resolved Hide resolved
},
QPath::TypeRelative(_, seg) => Some(vec![seg.ident.span]),
QPath::LangItem(_, _) => unreachable!("`TryFrom` and `TryInto` are not lang items"),
});
Copy link
Contributor

@m-rph m-rph Jan 15, 2024 •
edited

Choose a reason for hiding this comment

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

This is a bit of a nit, and may be unnecessary, but if you return Option<[&'_ span]>, I think the matching below could be cleaner as you will be matching something like

(FunctionKind::TryFromFunction, [from_method_span], Some(unwrap_span)) => todo!(),
(FunctionKind::TryFromFunction, [from_cls_span, from_method_span], Some(unwrap_span)) => todo!(),
_ => unreachable!()

which I think would make this a bit cleaner, other than that, I think it's very good and a great addition!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it took me so long to see this comment. I tried this but it introduced too many arms (multiple FunctionKind::TryFromFunctions and FunctionKind::TryIntoFunctions) and made the code more complicated

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I am good then, but I think we still need another reviewer with r perms. Maybe zulip?


let (source_ty, target_ty, sugg, applicability) = match (kind, &qpath_spans, parent_unwrap_call) {
(FunctionKind::TryIntoMethod, _, Some(unwrap_span)) => {
let sugg = vec![(primary_span, String::from("into")), (unwrap_span, String::new())];
(self_ty, other_ty, sugg, Applicability::MachineApplicable)
},
(FunctionKind::TryFromFunction, Some(spans), Some(unwrap_span)) => {
let sugg = match spans.len() {
1 => vec![(spans[0], String::from("from")), (unwrap_span, String::new())],
2 => vec![
(spans[0], String::from("From")),
(spans[1], String::from("from")),
(unwrap_span, String::new()),
],
_ => unreachable!(),
roife marked this conversation as resolved.
Show resolved Hide resolved
};
(other_ty, self_ty, sugg, Applicability::MachineApplicable)
},
(FunctionKind::TryIntoFunction, Some(spans), Some(unwrap_span)) => {
let sugg = match spans.len() {
1 => vec![(spans[0], String::from("into")), (unwrap_span, String::new())],
2 => vec![
(spans[0], String::from("Into")),
(spans[1], String::from("into")),
(unwrap_span, String::new()),
],
_ => unreachable!(),
};
(self_ty, other_ty, sugg, Applicability::MachineApplicable)
},
(FunctionKind::TryFromFunction, _, _) => {
let sugg = vec![(primary_span, String::from("From::from"))];
(other_ty, self_ty, sugg, Applicability::Unspecified)
},
(FunctionKind::TryIntoFunction, _, _) => {
let sugg = vec![(primary_span, String::from("Into::into"))];
(self_ty, other_ty, sugg, Applicability::Unspecified)
},
(FunctionKind::TryIntoMethod, _, _) => {
let sugg = vec![(primary_span, String::from("into"))];
(self_ty, other_ty, sugg, Applicability::Unspecified)
},
roife marked this conversation as resolved.
Show resolved Hide resolved
FunctionKind::TryFromFunction => (
other_ty,
self_ty,
"From::from",
primary_span,
Applicability::Unspecified,
),
FunctionKind::TryIntoFunction => (
self_ty,
other_ty,
"Into::into",
primary_span,
Applicability::Unspecified,
),
FunctionKind::TryIntoMethod => (self_ty, other_ty, "into", primary_span, Applicability::Unspecified),
};

span_lint_and_then(
Expand All @@ -97,7 +131,7 @@ fn check<'tcx>(
with_forced_trimmed_paths!({
diag.note(format!("converting `{source_ty}` to `{target_ty}` cannot fail"));
});
diag.span_suggestion(span, "use", sugg, applicability);
diag.multipart_suggestion("use", sugg, applicability);
},
);
}
Expand All @@ -113,6 +147,7 @@ pub(super) fn check_method(cx: &LateContext<'_>, expr: &Expr<'_>) {
cx.typeck_results().node_args(expr.hir_id),
FunctionKind::TryIntoMethod,
path.ident.span,
None,
);
}
}
Expand All @@ -135,6 +170,7 @@ pub(super) fn check_function(cx: &LateContext<'_>, expr: &Expr<'_>, callee: &Exp
_ => return,
},
callee.span,
Some(qpath),
);
}
}
37 changes: 37 additions & 0 deletions tests/ui/unnecessary_fallible_conversions.fixed
View file
Open in desktop
@@ -1,6 +1,43 @@
#![warn(clippy::unnecessary_fallible_conversions)]

fn main() {
// --- TryFromMethod `T::try_from(u)` ---

let _: i64 = 0i32.into();
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _: i64 = 0i32.into();
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryFromFunction `T::try_from(U)` ---

let _ = i64::from(0i32);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _ = i64::from(0i32);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryIntoFunction `U::try_into(t)` ---

let _: i64 = i32::into(0);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _: i64 = i32::into(0i32);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryFromFunction `<T as TryFrom<U>>::try_from(U)` ---

let _ = <i64 as From<i32>>::from(0);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _ = <i64 as From<i32>>::from(0);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryIntoFunction `<U as TryInto<_>>::try_into(U)` ---

let _: i64 = <i32 as Into<_>>::into(0);
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _: i64 = <i32 as Into<_>>::into(0);
//~^ ERROR: use of a fallible conversion when an infallible one could be used
}
37 changes: 37 additions & 0 deletions tests/ui/unnecessary_fallible_conversions.rs
View file
Open in desktop
@@ -1,6 +1,43 @@
#![warn(clippy::unnecessary_fallible_conversions)]

fn main() {
// --- TryFromMethod `T::try_from(u)` ---

let _: i64 = 0i32.try_into().unwrap();
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _: i64 = 0i32.try_into().expect("can't happen");
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryFromFunction `T::try_from(U)` ---

let _ = i64::try_from(0i32).unwrap();
roife marked this conversation as resolved.
Show resolved Hide resolved
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _ = i64::try_from(0i32).expect("can't happen");
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryIntoFunction `U::try_into(t)` ---

let _: i64 = i32::try_into(0).unwrap();
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _: i64 = i32::try_into(0i32).expect("can't happen");
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryFromFunction `<T as TryFrom<U>>::try_from(U)` ---

let _ = <i64 as TryFrom<i32>>::try_from(0).unwrap();
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _ = <i64 as TryFrom<i32>>::try_from(0).expect("can't happen");
//~^ ERROR: use of a fallible conversion when an infallible one could be used

// --- TryIntoFunction `<U as TryInto<_>>::try_into(U)` ---

let _: i64 = <i32 as TryInto<_>>::try_into(0).unwrap();
//~^ ERROR: use of a fallible conversion when an infallible one could be used

let _: i64 = <i32 as TryInto<_>>::try_into(0).expect("can't happen");
//~^ ERROR: use of a fallible conversion when an infallible one could be used
}
124 changes: 119 additions & 5 deletions tests/ui/unnecessary_fallible_conversions.stderr
View file
Open in desktop
@@ -1,20 +1,134 @@
error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:4:23
--> $DIR/unnecessary_fallible_conversions.rs:6:23
|
LL | let _: i64 = 0i32.try_into().unwrap();
| ^^^^^^^^^^^^^^^^^^^ help: use: `into()`
| ^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
= note: `-D clippy::unnecessary-fallible-conversions` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_fallible_conversions)]`
help: use
|
LL - let _: i64 = 0i32.try_into().unwrap();
LL + let _: i64 = 0i32.into();
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:5:23
--> $DIR/unnecessary_fallible_conversions.rs:9:23
|
LL | let _: i64 = 0i32.try_into().expect("can't happen");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `into()`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _: i64 = 0i32.try_into().expect("can't happen");
LL + let _: i64 = 0i32.into();
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:14:13
|
LL | let _ = i64::try_from(0i32).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _ = i64::try_from(0i32).unwrap();
LL + let _ = i64::from(0i32);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:17:13
|
LL | let _ = i64::try_from(0i32).expect("can't happen");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _ = i64::try_from(0i32).expect("can't happen");
LL + let _ = i64::from(0i32);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:22:18
|
LL | let _: i64 = i32::try_into(0).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _: i64 = i32::try_into(0).unwrap();
LL + let _: i64 = i32::into(0);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:25:18
|
LL | let _: i64 = i32::try_into(0i32).expect("can't happen");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _: i64 = i32::try_into(0i32).expect("can't happen");
LL + let _: i64 = i32::into(0i32);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:30:13
|
LL | let _ = <i64 as TryFrom<i32>>::try_from(0).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _ = <i64 as TryFrom<i32>>::try_from(0).unwrap();
LL + let _ = <i64 as From<i32>>::from(0);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:33:13
|
LL | let _ = <i64 as TryFrom<i32>>::try_from(0).expect("can't happen");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _ = <i64 as TryFrom<i32>>::try_from(0).expect("can't happen");
LL + let _ = <i64 as From<i32>>::from(0);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:38:18
|
LL | let _: i64 = <i32 as TryInto<_>>::try_into(0).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _: i64 = <i32 as TryInto<_>>::try_into(0).unwrap();
LL + let _: i64 = <i32 as Into<_>>::into(0);
|

error: use of a fallible conversion when an infallible one could be used
--> $DIR/unnecessary_fallible_conversions.rs:41:18
|
LL | let _: i64 = <i32 as TryInto<_>>::try_into(0).expect("can't happen");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: converting `i32` to `i64` cannot fail
help: use
|
LL - let _: i64 = <i32 as TryInto<_>>::try_into(0).expect("can't happen");
LL + let _: i64 = <i32 as Into<_>>::into(0);
|

error: aborting due to 2 previous errors
error: aborting due to 10 previous errors