Do not inline integer literals which are out of range in format_args! by agluszak · Pull Request #116633 · rust-lang/rust

agluszak · 2023-10-11T13:16:18Z

Inlining integers (even those out of range) was introduced in #106824.

Closes #116631.

Closes #115423.

rustbot · 2023-10-11T13:16:28Z

r? @compiler-errors

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

agluszak · 2023-10-11T13:20:45Z

Technically that's a breaking change...

compiler-errors · 2023-10-11T13:22:02Z

Hm yeah. Nominating for T-lang, impl looks ok but I didn't look at it too long -- will do after T-lang confirms we should re-break this.

In the mean time, I'll do a crater run.

@bors try

bors · 2023-10-11T13:23:13Z

⌛ Trying commit b0ae218 with merge b67bddc...

Do not inline integer linterals which are out of range in format_args! Inlining integers (even those out of range) was introduced in rust-lang#106824. Closes rust-lang#116631.

bors · 2023-10-11T14:36:32Z

☀️ Try build successful - checks-actions
Build commit: b67bddc (b67bddc71c8ce18bca43818e26d6290f72650a92)

compiler-errors · 2023-10-11T14:37:05Z

@craterbot check

craterbot · 2023-10-11T14:37:18Z

👌 Experiment pr-116633 created and queued.
🤖 Automatically detected try build b67bddc
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2023-10-11T14:37:32Z

🚧 Experiment pr-116633 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lukas-code · 2023-10-11T15:11:49Z

compiler/rustc_ast/src/ast.rs

+            LitIntType::Signed(IntTy::I32) => i32::MAX as u128,
+            LitIntType::Signed(IntTy::I64) => i64::MAX as u128,
+            LitIntType::Signed(IntTy::I128) => i128::MAX as u128,
+            LitIntType::Signed(IntTy::Isize) => isize::MAX as u128,


This uses the host's isize, but should be using use the target's isize. Same for usize below.

Fair. What should I do to obtain these values for the target?

You can get size of isize and usize from tcx.data_layout.pointer_size. That will return a Size, which has the signed_int_max and unsigned_int_max methods that seem useful. It looks like the nearest tcx is the one in the LoweringContext, which you get as self from here:

rust/compiler/rustc_ast_lowering/src/format.rs

Line 15 in b0ae218

pub(crate) fn lower_format_args(&mut self, sp: Span, fmt: &FormatArgs) -> hir::ExprKind<'hir> {

lukas-code · 2023-10-11T15:18:24Z

compiler/rustc_ast/src/ast.rs

+            LitIntType::Unsigned(UintTy::U64) => u64::MAX as u128,
+            LitIntType::Unsigned(UintTy::U128) => u128::MAX,
+            LitIntType::Unsigned(UintTy::Usize) => usize::MAX as u128,
+            LitIntType::Unsuffixed => u128::MAX,


Suggested change

LitIntType::Unsuffixed => u128::MAX,

LitIntType::Unsuffixed => i32::MAX,

Unsuffixed and unconstrained integer literals should have type i32, even if they overflow.

See the old behavior before inlining: https://rust.godbolt.org/z/PMG7ac4d4

asquared31415 · 2023-10-11T17:53:38Z

tests/ui/fmt/issue-116631.rs

@@ -0,0 +1,3 @@
+fn main() {
+    format_args!("{}\n", 0xffff_ffff_u8); //~ ERROR literal out of range for `u8`


Should this have a test for each of the different integer types?

At the very least it should have a test case that covers #115423 (which I don't interpet is about being out of range, but rather about not respecting the type of the literal).

asquared31415 · 2023-10-11T17:54:23Z

According to the linked PR, it's meant to be gated behind -Zflatten-format-args=yes, is that gating broken somehow?

Edit: ah that flag seems to have been stabilized to default to on, I didn't see that

craterbot · 2023-10-13T09:44:33Z

🎉 Experiment pr-116633 is completed!
📊 13 regressed and 0 fixed (376984 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

scottmcm · 2023-10-18T14:36:14Z

We discussed this in the @rust-lang/lang triage meeting today.

Everyone agreed that this was "just" a bug, and should be fixed. Printing a token that's explicitly marked an i8 and getting a result outside the range of an i8 is clearly wrong.

Ideally this would trigger the overflowing_literals lint same as using a token like this as an expression in other places.

traviscross · 2023-10-24T17:47:40Z

@rustbot labels -I-lang-nominated

compiler-errors · 2023-10-25T15:21:43Z

Awaiting feedback above

@rustbot author

agluszak · 2023-10-26T22:03:57Z

I'll fix it on Saturday or Sunday

JohnCSimon · 2024-02-11T20:26:02Z

@agluszak

ping from triage - can you post your status on this PR? This PR has not received an update in a few months. Thank you!

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Dylan-DPC · 2024-04-10T13:08:57Z

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

Don't inline integer literals when they overflow - new attempt Basically rust-lang#116633 but I implemented the suggested changes. Fixes rust-lang#115423. Fixes rust-lang#116631. This is my first contribution to this repo so please let me know if I'm supposed to change something :)

Rollup merge of rust-lang#123935 - tstsrt:fix-115423, r=oli-obk Don't inline integer literals when they overflow - new attempt Basically rust-lang#116633 but I implemented the suggested changes. Fixes rust-lang#115423. Fixes rust-lang#116631. This is my first contribution to this repo so please let me know if I'm supposed to change something :)

Fix rust-lang#116631

b0ae218

rustbot assigned compiler-errors Oct 11, 2023

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 Oct 11, 2023

compiler-errors added the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 11, 2023

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2023

lukas-code reviewed Oct 11, 2023

View reviewed changes

apiraino mentioned this pull request Oct 11, 2023

format_args! does not respect integer literals' type #115423

Closed

asquared31415 reviewed Oct 11, 2023

View reviewed changes

agluszak changed the title ~~Do not inline integer linterals which are out of range in format_args!~~ Do not inline integer literals which are out of range in format_args! Oct 11, 2023

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Oct 13, 2023

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 24, 2023

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 Oct 25, 2023

WaffleLapkin mentioned this pull request Dec 6, 2023

Don't throw away unused arguments of format_args #118659

Closed

Dylan-DPC closed this Apr 10, 2024

tstsrt mentioned this pull request Apr 14, 2024

Don't inline integer literals when they overflow - new attempt #123935

Merged

	LitIntType::Unsuffixed => u128::MAX,
	LitIntType::Unsuffixed => i32::MAX,

		@@ -0,0 +1,3 @@
		fn main() {
		format_args!("{}\n", 0xffff_ffff_u8); //~ ERROR literal out of range for `u8`

Uh oh!

Conversation

agluszak commented Oct 11, 2023 • edited by apiraino Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 11, 2023

Uh oh!

agluszak commented Oct 11, 2023

Uh oh!

compiler-errors commented Oct 11, 2023

Uh oh!

bors commented Oct 11, 2023

Uh oh!

bors commented Oct 11, 2023

Uh oh!

compiler-errors commented Oct 11, 2023

Uh oh!

craterbot commented Oct 11, 2023

Uh oh!

craterbot commented Oct 11, 2023

Uh oh!

lukas-code Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

agluszak Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

lukas-code Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukas-code Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

asquared31415 Oct 11, 2023

Choose a reason for hiding this comment

Uh oh!

pnkfelix Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

asquared31415 commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craterbot commented Oct 13, 2023

Uh oh!

scottmcm commented Oct 18, 2023

Uh oh!

traviscross commented Oct 24, 2023

Uh oh!

compiler-errors commented Oct 25, 2023

Uh oh!

agluszak commented Oct 26, 2023

Uh oh!

JohnCSimon commented Feb 11, 2024

Uh oh!

Dylan-DPC commented Apr 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

agluszak commented Oct 11, 2023 •

edited by apiraino

Loading

lukas-code Oct 11, 2023 •

edited

Loading

asquared31415 commented Oct 11, 2023 •

edited

Loading