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

format_args! does not respect integer literals' type #115423

Closed
endorpersand opened this issue Sep 1, 2023 · 7 comments · Fixed by #123935
Closed

format_args! does not respect integer literals' type #115423

endorpersand opened this issue Sep 1, 2023 · 7 comments · Fixed by #123935
Assignees
Labels
A-fmt Area: `std::fmt` C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@endorpersand
Copy link

endorpersand commented Sep 1, 2023

When using the 1234i8 syntax in the Display format in formatting macros, the i8 is ignored and treated as a u128.

I tried this code:

fn main() {
    println!("{}", 0x8Fi8);
}

I expected to see this happen: Getting an error that 0x8F is out of the bounds for type i8.

Instead, this happened: This function compiles and prints 143.

Meta

rustc --version --verbose:

rustc 1.71.1 (eb26296b5 2023-08-03)
binary: rustc
commit-hash: eb26296b556cef10fb713a38f3d16b9886080f26
commit-date: 2023-08-03
host: x86_64-apple-darwin
release: 1.71.1
LLVM version: 16.0.5

@endorpersand endorpersand added the C-bug Category: This is a bug. label Sep 1, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 1, 2023
@konnorandrews
Copy link

I went and looked through the format_args!() implementation and found where the issue comes from. The check at https://github.com/rust-lang/rust/blob/master/compiler/rustc_ast_lowering/src/format.rs#L131 ignores the integer type, and it blindly calls n.string() on the u128 value of the literal.

@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 1, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 1, 2023
@gurry
Copy link
Contributor

gurry commented Sep 3, 2023

@rustbot claim

@cuviper cuviper added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Sep 5, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 5, 2023
@apiraino
Copy link
Contributor

apiraino commented Sep 7, 2023

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@apiraino
Copy link
Contributor

apiraino commented Oct 11, 2023

Did run a bisection on the reproducible provided (and duplicated #116631)

Regression in nightly-2023-04-22, specifically I think commit 8bdcc62 or (as pointed out in PR #116633) #106824

found 11 bors merge commits in the specified range
commit[0] 2023-04-20: Auto merge of #110616 - m-ou-se:fmt-lang-items, r=jyn514
commit[1] 2023-04-20: Auto merge of #109999 - m-ou-se:flatten-format-args, r=oli-obk
commit[2] 2023-04-21: Auto merge of #110370 - c410-f3r:dqewdas, r=petrochenkov
commit[3] 2023-04-21: Auto merge of #110431 - jsoref:spelling-src-etc, r=Mark-Simulacrum
commit[4] 2023-04-21: Auto merge of #110636 - matthiaskrgr:rollup-faa33c6, r=matthiaskrgr
commit[5] 2023-04-21: Auto merge of #96840 - cjgillot:query-feed, r=oli-obk
commit[6] 2023-04-21: Auto merge of #109002 - michaelvanstraten:master, r=petrochenkov
commit[7] 2023-04-21: Auto merge of #110542 - petrochenkov:qcstore4, r=cjgillot
commit[8] 2023-04-21: Auto merge of #110569 - saethlin:mir-pass-cooperation, r=cjgillot
commit[9] 2023-04-21: Auto merge of #110107 - cjgillot:const-prop-lint-junk, r=oli-obk
commit[10] 2023-04-21: Auto merge of #110648 - Dylan-DPC:rollup-em3ovcq, r=Dylan-DPC

cc @m-ou-se

@m-ou-se m-ou-se self-assigned this Oct 11, 2023
@fmease fmease added the A-fmt Area: `std::fmt` label Oct 11, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Dec 5, 2023

Looks like it's not easy invoke the same integer-literal-range-checking code from the place where format_args!() is expanded. So the easiest solution is probably to expand to something like { let _ = literal; let _ = literal; fmt::Arguments::new(..) } when literals are removed, such that the literals are still be checked later.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 5, 2023

Actually, looks like there is a much easier fix that mostly just involves removing code. ^^'

Will send a PR soon, probably tomorrow.

@m-ou-se
Copy link
Member

m-ou-se commented Dec 5, 2023

Fix: #118659

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Apr 18, 2024
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 :)
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Apr 18, 2024
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 :)
@bors bors closed this as completed in f174c31 Apr 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 19, 2024
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 :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-fmt Area: `std::fmt` C-bug Category: This is a bug. P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants