#9231 suggest TryFrom when truncation possible #9664

navh · 2022-10-17T02:41:32Z

changelog: Sugg: [cast_possible_truncation]: Now suggests try_from

rust-highfive · 2022-10-17T02:41:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information.

kraktus · 2022-10-17T13:13:26Z

The truncation can be intentional and then suggesting tryFrom would change the behaviour of the code, I don't think the suggestion can be machine applicable.

kraktus · 2022-10-17T13:23:57Z

clippy_lints/src/casts/cast_possible_truncation.rs

+        expr.span,
+        &msg,
+        "avoid silent truncation by using",
+        format!("{cast_to}::try_from({name_of_cast_from}).unwrap()"),


I think using using expect here and hinting developers to explain why it shouldn't truncate would be even better

Suggested change

format!("{cast_to}::try_from({name_of_cast_from}).unwrap()"),

format!("{cast_to}::try_from({name_of_cast_from}).expect("truncation should be impossible because ...)"),

I've made this change and changed MachineApplicable to HasPlaceholders, is this the best way to classify this?

I tried to come up with some sort of {cast_to}::try_from({} & {cast_to}::MAX as {cast_from}).unwrap() machine applicable suggestion, but they all bury the intention of the lint.

HasPlaceholders is fine here 👍 Another option now that I think about it would be to make the whole expect reason a placeholder so that the user wouldn't be able to copy-paste the offered suggestion without having to think about the issue before getting a compilable version of their program. What do you think @xFrednet?

format!("{cast_to}::try_from({name_of_cast_from}).expect(..)

Thank you for the review and feedback @kraktus, it's highly appreciated!

Using try_from() changes the semantics of the code. In most cases, users would most likely prefer the value to be truncated over the code panicking in production. This also misses cases, where this behavior might be intentional. Though, looking at the lint description, it's not ideal to emit a lint without any suggestion.

The idea of using try_from is good and should be included in the lint message. The applicability of HasPlaceholders seamed a bit high to me at first, but good at a second thought.

That being said, I would suggest a different lint message. Something like this:

error: casting `f64` to `isize` may truncate the value --> $DIR/cast.rs:29:5 | LL | 1f64 as isize; | help: if this is intensional allow the lint with `#[allow(clippy::cast_precision_loss)]` ... help: ... or use `try_from` and handle the error accordingly | LL | isize::try_from(1f64) |

Note that this changes the semantics even more, as we don't instruct the user to use .expect("..") or .unwrap(). For this, I would make the applicability unspecified.

What are your thoughts about this? 🙃

You can create more complex messages with: span_lint_and_then.

Here is a mockup how the code could look:

span_lint_and_then( cx, <LINT>, <span>, <msg>, |diag| { diag.help( "if this is intensional allow the lint with `#[allow(clippy::cast_precision_loss)]` ...", ); diag.span_suggestion_with_style( <span>, "... or use `try_from` and handle the error accordingly", suggestion, Applicability::Unspecified, // always show the suggestion in a separate line SuggestionStyle::ShowAlways, ); }, );

I'm having trouble getting the two "help" messages to align to the left.

This makes it awkward to read. Note appearing in the middle for some lints also breaks it up in an awkward way.

Where can I go to learn more about formatting this output? Or is this correct?

error: casting `f32` to `i32` may truncate the value --> $DIR/cast.rs:24:5 | LL | 1f32 as i32; | ^^^^^^^^^^^ | = help: if this is intentional allow the lint with `#[allow(clippy::cast_precision_loss)]` ... = note: `-D clippy::cast-possible-truncation` implied by `-D warnings` help: ... or use `try_from` and handle the error accordingly | LL | i32::try_from(1f32); | ~~~~~~~~~~~~~~~~~~~

This is correct, i mixed up the formatting. You could try add the #[allow] attribute as another code suggestion, but I think this is better. Only thing is the note in the middle which we sadly can influence directly.

There is sadly no detailed documentation for the formatting.

Could you also update the lint description to mention these two solutions?

Yes, I put 'why is this bad' back to how it was, and appended a bit to the 'what it does' to reflect both the ignore and try_from suggestions.

xFrednet · 2022-10-21T19:28:09Z

Hey, I'll try to take a look at this soon, by mid next week at the latest.

A general FYI, Clippy has a no merge commit policy (with a few exceptions), PR branches should be rebased instead. Here is an info link on how to resolve merge conflicts instead. Sometimes it can be simpler to squash all commits into one. I'm mentioning this, as your branch tries to add a bunch of external and merge commits. We can also take a look at this after the next review. You're welcome to reach out if you need help with that :)

navh · 2022-10-21T21:17:39Z

Sometimes it can be simpler to squash all commits into one.

I should have taken this advice, instead I've force pushed a ton.... Documentation changes got lost in the crossfire, so I've re-done it, hopefully this works.

bors · 2022-10-23T22:33:22Z

☔ The latest upstream changes (presumably #9541) made this pull request unmergeable. Please resolve the merge conflicts.

xFrednet

This version already looks good to me. Two small adjustments and then we can have this merged :). Thank you for the quick updates.

xFrednet · 2022-10-25T22:07:02Z

clippy_lints/src/casts/mod.rs

@@ -80,7 +80,8 @@ declare_clippy_lint! {
    /// ### What it does
    /// Checks for casts between numerical types that may
    /// truncate large values. This is expected behavior, so the cast is `Allow` by
-    /// default.
+    /// default. It suggests user either explicitly ignore the lint,
+    /// or use `try_from()` and handle the truncation, default, or panic explicitly.


Could you also add a Use instead: block below, like this:

Use instead: ``` fn as_u8(x: u64) -> u8 { if let Ok(x) = u8::try_from(x) { x } else { todo!(); } } // Or fn as_u16(x: u64) -> u16 { #[allow(clippy::cast_possible_truncation)] x as u16 } ```

xFrednet · 2022-10-25T22:10:14Z

clippy_lints/src/casts/cast_possible_truncation.rs

+    let snippet = snippet(cx, expr.span, "x");
+    let name_of_cast_from = snippet.split(" as").next().unwrap_or("x");


Clippy usually uses .. as placeholders. Could you adjust this?

Suggested change

let snippet = snippet(cx, expr.span, "x");

let name_of_cast_from = snippet.split(" as").next().unwrap_or("x");

let snippet = snippet(cx, expr.span, "..");

let name_of_cast_from = snippet.split(" as").next().unwrap_or("..");

This should hopefully never come up either way :)

xFrednet · 2022-11-05T10:14:00Z

Hey @navh, this is a ping from triage. Are you stuck on anything, that I can help with?

@rustbot author

xFrednet · 2022-12-06T18:08:19Z

I'm closing this in favor of #10038 I tried to keep the original commiter from this PR :)

`cast_possible_truncation` Suggest TryFrom when truncation possible This fixes the last issues from #9664 as the author seems to be inactive. The PR author was sadly kept during the rebase, due to the conflict resolution. IDK if it's worth it do to a full review, I only added the last commit, everything else remained the same, besides a rebase. --- changelog: Sugg: [`cast_possible_truncation`]: Now suggests using `try_from` or allowing the lint [#10038](#10038)  closes: #9231

rust-highfive assigned xFrednet Oct 17, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 17, 2022

kraktus reviewed Oct 17, 2022

View reviewed changes

navh force-pushed the usize_to_u64 branch 2 times, most recently from d054509 to be27fa7 Compare October 21, 2022 21:04

rust-lang#9231 suggest TryFrom when truncation possible

3f1becb

navh force-pushed the usize_to_u64 branch from 1ce5ccd to 3f1becb Compare October 21, 2022 21:07

update possible_truncation documentation

c0bbb41

xFrednet reviewed Oct 25, 2022

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Nov 5, 2022

xFrednet mentioned this pull request Dec 6, 2022

cast_possible_truncation Suggest TryFrom when truncation possible #10038

Merged

xFrednet closed this Dec 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#9231 suggest TryFrom when truncation possible #9664

#9231 suggest TryFrom when truncation possible #9664

navh commented Oct 17, 2022 •

edited by xFrednet

Loading

rust-highfive commented Oct 17, 2022

kraktus commented Oct 17, 2022

kraktus Oct 17, 2022

navh Oct 17, 2022 •

edited

Loading

kraktus Oct 19, 2022

xFrednet Oct 19, 2022 •

edited

Loading

navh Oct 19, 2022

xFrednet Oct 20, 2022

navh Oct 20, 2022

xFrednet commented Oct 21, 2022

navh commented Oct 21, 2022

bors commented Oct 23, 2022

xFrednet left a comment

xFrednet Oct 25, 2022

xFrednet Oct 25, 2022

xFrednet commented Nov 5, 2022

xFrednet commented Dec 6, 2022

	format!("{cast_to}::try_from({name_of_cast_from}).unwrap()"),
	format!("{cast_to}::try_from({name_of_cast_from}).expect("truncation should be impossible because ...)"),

		let snippet = snippet(cx, expr.span, "x");
		let name_of_cast_from = snippet.split(" as").next().unwrap_or("x");

#9231 suggest TryFrom when truncation possible #9664

#9231 suggest TryFrom when truncation possible #9664

Conversation

navh commented Oct 17, 2022 • edited by xFrednet Loading

rust-highfive commented Oct 17, 2022

kraktus commented Oct 17, 2022

kraktus Oct 17, 2022

Choose a reason for hiding this comment

navh Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

kraktus Oct 19, 2022

Choose a reason for hiding this comment

xFrednet Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

navh Oct 19, 2022

Choose a reason for hiding this comment

xFrednet Oct 20, 2022

Choose a reason for hiding this comment

navh Oct 20, 2022

Choose a reason for hiding this comment

xFrednet commented Oct 21, 2022

navh commented Oct 21, 2022

bors commented Oct 23, 2022

xFrednet left a comment

Choose a reason for hiding this comment

xFrednet Oct 25, 2022

Choose a reason for hiding this comment

xFrednet Oct 25, 2022

Choose a reason for hiding this comment

xFrednet commented Nov 5, 2022

xFrednet commented Dec 6, 2022

navh commented Oct 17, 2022 •

edited by xFrednet

Loading

navh Oct 17, 2022 •

edited

Loading

xFrednet Oct 19, 2022 •

edited

Loading