Skip to content

Don't allow flattened format_args in const. #139624

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

Merged
merged 2 commits into from
Apr 30, 2025

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Apr 10, 2025

Fixes #139136

Fixes #139621

We allow format_args!("a") in const, but don't allow any format_args with arguments in const, such as format_args!("{}", arg).

However, we accidentally allow format_args!("hello {}", "world") in const, as it gets flattened to format_args!("hello world").

This also applies to panic in const.

This wasn't supposed to happen. I added protection against this in the format args flattening code, but I accidentally marked a function as const that shouldn't have been const but this was removed in #135139.

This is a breaking change. The crater found no breakage, however.

This breaks things like:

const _: () = if false { panic!("a {}", "a") };

and

const F: std::fmt::Arguments<'static> = format_args!("a {}", "a");

@rustbot

This comment was marked as outdated.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 10, 2025
@m-ou-se m-ou-se assigned m-ou-se and unassigned Mark-Simulacrum Apr 10, 2025
@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 10, 2025

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 10, 2025
Don't allow flattened format_args in const.

Fixes rust-lang#139136 and rust-lang#139621 by breaking the 'flattened format_args' cases.

This is a breaking change.

Let's try a crater run.
@bors
Copy link
Collaborator

bors commented Apr 10, 2025

⌛ Trying commit cc791eb with merge 6619027...

@m-ou-se m-ou-se added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2025
@m-ou-se m-ou-se mentioned this pull request Apr 7, 2025
29 tasks
@bors
Copy link
Collaborator

bors commented Apr 10, 2025

☀️ Try build successful - checks-actions
Build commit: 6619027 (6619027b13a87a3597115f2c8231f107bf3a7a5f)

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 10, 2025

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-139624 created and queued.
🤖 Automatically detected try build 6619027
🔍 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 craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Apr 10, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-139624 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-139624 is completed!
📊 3 regressed and 5 fixed (612545 total)
📰 Open the full report.

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

@craterbot 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 Apr 11, 2025
@RalfJung
Copy link
Member

These all seem spurious.

Landing this requires FCP, but I'm not sure which team -- lang and/or libs-api?

@m-ou-se m-ou-se added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 11, 2025
@m-ou-se m-ou-se marked this pull request as ready for review April 11, 2025 12:05
@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 11, 2025

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 11, 2025

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 22, 2025

but I accidentally marked a function as const that shouldn't have been const

This wasn't actually the case. I did implement this correctly. The problem was caused by #135139, which added const to a bunch of functions without properly considering the effect on the format_args!() macro.

This means that this problem has only been around since 1.86.0, which explains why we didn't find any actual breakage.

It might make sense to backport this change to land it in 1.87.0.

@m-ou-se m-ou-se added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 22, 2025
@m-ou-se m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 23, 2025
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Apr 26, 2025
@rfcbot
Copy link
Collaborator

rfcbot commented Apr 26, 2025

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@jhpratt
Copy link
Member

jhpratt commented Apr 26, 2025

r=me with a test added per Ralf's suggestion.

@m-ou-se
Copy link
Member Author

m-ou-se commented Apr 30, 2025

@bors r=jhpratt

@bors
Copy link
Collaborator

bors commented Apr 30, 2025

📌 Commit 56426db has been approved by jhpratt

It is now in the queue for this repository.

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 30, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2025
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#134232 (Share the naked asm impl between cg_ssa and cg_clif)
 - rust-lang#139624 (Don't allow flattened format_args in const.)
 - rust-lang#140090 (Check bare function idents for non snake-case name)
 - rust-lang#140203 (Issue an error when using `no_mangle` on language items)
 - rust-lang#140450 (ast: Remove token visiting from AST visitor)
 - rust-lang#140498 (Misc tweaks to HIR typeck (mostly w.r.t. checking calls))
 - rust-lang#140504 (transmutability: ensure_sufficient_stack when answering query)
 - rust-lang#140506 (unstable-book: fix capitalization)
 - rust-lang#140516 (Replace use of rustc_type_ir by rustc_middle)

Failed merges:

 - rust-lang#140374 (Resolve instance for SymFn in global/naked asm)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bc99a04 into rust-lang:master Apr 30, 2025
6 checks passed
@rustbot rustbot added this to the 1.88.0 milestone Apr 30, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2025
Rollup merge of rust-lang#139624 - m-ou-se:unconst-format-args, r=jhpratt

Don't allow flattened format_args in const.

Fixes rust-lang#139136

Fixes rust-lang#139621

We allow `format_args!("a")` in const, but don't allow any format_args with arguments in const, such as `format_args!("{}", arg)`.

However, we accidentally allow `format_args!("hello {}", "world")` in const, as it gets flattened to `format_args!("hello world")`.

This also applies to panic in const.

This wasn't supposed to happen. I added protection against this in the format args flattening code, ~~but I accidentally marked a function as const that shouldn't have been const~~ but this was removed in rust-lang#135139.

This is a breaking change. The crater found no breakage, however.

This breaks things like:

```rust
const _: () = if false { panic!("a {}", "a") };
```

and

```rust
const F: std::fmt::Arguments<'static> = format_args!("a {}", "a");
```
@m-ou-se m-ou-se deleted the unconst-format-args branch May 1, 2025 09:18
@cuviper cuviper mentioned this pull request May 2, 2025
@cuviper cuviper modified the milestones: 1.88.0, 1.87.0 May 2, 2025
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 2, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2025
[beta] backports

- Don't allow flattened format_args in const. rust-lang#139624
- set subsections_via_symbols for ld64 helper sections rust-lang#139752
- Fix detection of `main` function if there are expressions around it rust-lang#140220
- rustdoc: Fix doctest heuristic for main fn wrapping rust-lang#140420
- extend the list of registered dylibs on `test::prepare_cargo_test` rust-lang#140563

r? cuviper
@jieyouxu jieyouxu removed needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination labels May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. to-announce Announce this issue on triage meeting
Projects
None yet