Skip to content

Don't throw away unused arguments of format_args #118659

New issue

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

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

Already on GitHub? # to your account

Closed

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Dec 5, 2023

Fixes #115423

format_args!("{}", 12345u8) was accidentally accepted, even though that literal is out of range. The literal didn't end up in the expansion of format_args!(), because it got simplified to format_args!("12345").

This change is simply to not throw away those 'inlined' arguments. This actually simplifies the code in several ways. :)

format_args!("{}", 12345u8) shouldn't disable the out-of-range check
of the argument.
@rustbot
Copy link
Collaborator

rustbot commented Dec 5, 2023

r? @WaffleLapkin

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 5, 2023
@m-ou-se
Copy link
Member Author

m-ou-se commented Dec 5, 2023

I just realized that this has a subtle bug, so don't merge this yet. :)

@WaffleLapkin
Copy link
Member

WaffleLapkin commented Dec 6, 2023

Have you seen #116633? Is this a replacement of that PR?

@m-ou-se
Copy link
Member Author

m-ou-se commented Dec 6, 2023

Ah, I didn't see that PR. Oops.

That PR fixes the issue too. (It seems to be stuck though?)

There's a subtle difference, and that is that with #[allow(overflowing_literals)], print!("{}", 1234u8) with this PR will still result in 1234, while that PR will result in 210, which would be more correct.

I really don't understand why overflowing literals is a lint rather than a hard error though.

@WaffleLapkin
Copy link
Member

Yeah, given the T-lang approval I really wish the result is 210. What do you think should be the process here? You change this PR? We ping the author of the other PR? Something else?

@WaffleLapkin
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 17, 2023
@bors
Copy link
Collaborator

bors commented Dec 26, 2023

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

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 13, 2024

Closing in favor of #116633.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

format_args! does not respect integer literals' type
4 participants