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

Fix rust-lang/rust#79255 - Incorrect try suggestion for float cast #6362

Merged

Conversation

nico-abram
Copy link
Contributor

changelog: Fix rust-lang/rust#79255 - Incorrect try suggestion for float literal cast ending in dot

@rust-highfive
Copy link

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 22, 2020
@nico-abram nico-abram force-pushed the unnecessary_cast_dot_float_literal branch from 698fd4c to 8dcfc54 Compare November 22, 2020 00:30
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

I think it would be simpler if we just remove the dot instead. What do you think?

@ebroto ebroto 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 24, 2020
@nico-abram
Copy link
Contributor Author

nico-abram commented Nov 25, 2020

Sounds good to me, simpler implementation and will keep the suggestions more uniform
I was just going off the issue saying The correct suggestion is 3.0_f32, will probably change it tomorrow

@iago-lito thoughts?

@iago-lito
Copy link

iago-lito commented Nov 25, 2020

@nico Hm, the truth is that I didn't even think that 3_f32 was also valid ^ ^"

So IIUC now the question is which formatting to suggest among:

  • 3.0_f32
  • 3_f32
  • 3.0f32
  • 3f32
    right?

Well, I'm not sure whether there are official recommendations regarding this.
My personal feeling is that Rust likes to favor explicitness and readability, so I think the first option, 3.0_f32, is the most consistent suggestion.


Alternately, picking the option that yields the formatting maybe-closest from what the user had in mind is also interesting, like:

  • 3. as f32 would suggest 3_f32
  • 3 as f32 would suggest 3f32
  • 3.0 as f32 would suggest 3.0_f32 or 3.0f32

but that feels messy. I'd rather expect Rust not to try to accomodate my thoughs and just spit out the most explicit + readable option.


Even more alternately, defer the question to rustfmt options. Once the user has defined the one they like best in their rustfmt.toml, either they apply the fix then run a fmt pass, or the fix is super-sophisticated and goes read these options to directly decide which formatting to suggest.


To be clear, I also think most of this is overkill. My preferred solution is to just
suggest 3.0_f32 and call it a day ;)

@nico-abram nico-abram force-pushed the unnecessary_cast_dot_float_literal branch from 8dcfc54 to 4e5ea62 Compare November 25, 2020 22:18
@nico-abram nico-abram requested a review from ebroto November 25, 2020 22:19
@nico-abram nico-abram force-pushed the unnecessary_cast_dot_float_literal branch from 4e5ea62 to 3b53de6 Compare November 25, 2020 22:42
Copy link
Member

@ebroto ebroto left a comment

Choose a reason for hiding this comment

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

@iago-lito I think this solution is good because it doesn't need to change a lot the current implementation, note that _ is always added.

Also, .0 is kind of redundant given that we are specifying the type explicitly.

@ebroto
Copy link
Member

ebroto commented Nov 26, 2020

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Nov 26, 2020

📌 Commit 3b53de6 has been approved by ebroto

@bors
Copy link
Contributor

bors commented Nov 26, 2020

⌛ Testing commit 3b53de6 with merge 403816f...

@bors
Copy link
Contributor

bors commented Nov 26, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: ebroto
Pushing 403816f to master...

@bors bors merged commit 403816f into rust-lang:master Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect try suggestion for casting float literal.
5 participants