Don't allow {} to refer to implicit captures in format_args. #93394

m-ou-se · 2022-01-27T22:52:14Z

Fixes #93378

rust-highfive · 2022-01-27T22:52:17Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

CAD97 · 2022-01-28T10:36:07Z

This should imho probably also add an explicit test for the println!("{a} {}", a=a) case, to document it at least.

CAD97 · 2022-01-28T10:53:17Z

compiler/rustc_builtin_macros/src/format.rs

@@ -369,7 +377,7 @@ impl<'a, 'b> Context<'a, 'b> {

        let count = self.pieces.len()
            + self.arg_with_formatting.iter().filter(|fmt| fmt.precision_span.is_some()).count();
-        if self.names.is_empty() && !numbered_position_args && count != self.args.len() {
+        if self.names.is_empty() && !numbered_position_args && count != self.num_args() {


Normally this (positive) branch would result in a message 1 positional argument in format string, but no arguments were given, whereas now adding an implicit capture gets invalid reference to positional argument 0 (no arguments were given). Ideally this wording would be kept, even with implicit args; I think the proper check would then be self.names.len() == self.num_captured_args && !numbered_position_args && count != self.num_args()?

Then the let count = above would also have to be adjusted to exclude all placeholders that refer to captures, which is a bit more complicated.

I agree the diagnostics here can be improved when there's captured arguments involved, but I rather leave that to a separate change that can fix all those cases properly.

m-ou-se · 2022-02-02T20:06:09Z

Marked as beta-accepted per discussion in the libs api team just now.

estebank · 2022-02-02T22:55:26Z

r=me, but we really want a crater run of this to make sure we're not regressing here and am slightly uncomfortable with a beta backport (but it's not my call)

estebank · 2022-02-02T22:55:43Z

src/test/ui/fmt/format-args-capture-issue-93378.stderr

+error: invalid reference to positional argument 0 (no arguments were given)
+  --> $DIR/format-args-capture-issue-93378.rs:9:23
+   |
+LL |     println!("{a:.n$} {b:.*}");
+   |               ------- ^^^--^
+   |               |          |
+   |               |          this precision flag adds an extra required argument at position 0, which is why there are 3 arguments expected


This output needs some work to make it clearer :(

Yes, there's quite a few cases right now where this macro produces suboptimal diagnostics.

We have plans to change how fmt::Arguments is implemented/represented, which would require a lot of changes to the format_args builtin macro. That would probably be the best time to just rewrite the macro and the way it produces diagnostics.

m-ou-se · 2022-02-03T09:31:59Z

we really want a crater run of this to make sure we're not regressing here

We could run this PR through crater, but I suppose the beta crater run is good enough to catch any problems.

m-ou-se · 2022-02-03T09:36:58Z

Looks like the crater queue is almost empty, so we can quickly run this through crater to be sure:

@craterbot check

craterbot · 2022-02-03T09:36:59Z

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

m-ou-se · 2022-02-03T09:40:58Z

@bors try

bors · 2022-02-03T09:41:08Z

⌛ Trying commit cef9b47 with merge 1b0139f5bdae7328e6d232947a8112333bb44eeb...

m-ou-se · 2022-02-03T09:52:50Z

I was told crater needs some maintenance after the current experiment finishes, so let's hold off until that's done.

@pietroalbini can you start a check run here once you're done with that? Thanks.

bors · 2022-02-03T10:59:18Z

☀️ Try build successful - checks-actions
Build commit: 1b0139f5bdae7328e6d232947a8112333bb44eeb (1b0139f5bdae7328e6d232947a8112333bb44eeb)

apiraino · 2022-02-03T17:31:36Z

T-compiler discussion about beta-backport approval on Zulip

Also, stable backport nomination discussed and approved (Zulip), in particular that this backport should be included in a point release, but does not motivate one.

@rustbot label +stable-nominated +stable-approved

craterbot · 2022-02-04T02:23:40Z

🚧 Experiment pr-93394 is now running

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

craterbot · 2022-02-06T07:34:56Z

🎉 Experiment pr-93394 is completed!
📊 2 regressed and 4 fixed (210500 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

m-ou-se · 2022-02-07T12:04:00Z

Just two failures in the crater run. Both of them are unrelated: a build script failing to download some file.

@bors r=estebank

bors · 2022-02-07T12:04:02Z

📌 Commit cef9b47 has been approved by estebank

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

Rollup of 13 pull requests Successful merges: - rust-lang#88313 (Make the pre-commit script pre-push instead) - rust-lang#91530 (Suggest 1-tuple parentheses on exprs without existing parens) - rust-lang#92724 (Cleanup c_str.rs) - rust-lang#93208 (Impl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}Assign<$t> for Wrapping<$t> for rust 1.60.0) - rust-lang#93394 (Don't allow {} to refer to implicit captures in format_args.) - rust-lang#93416 (remove `allow_fail` test flag) - rust-lang#93487 (Fix linking stage1 toolchain in `./x.py setup`) - rust-lang#93673 (Linkify sidebar headings for sibling items) - rust-lang#93680 (Drop json::from_reader) - rust-lang#93682 (Update tracking issue for `const_fn_trait_bound`) - rust-lang#93722 (Use shallow clones for submodules managed by rustbuild, not just bootstrap.py) - rust-lang#93723 (Rerun bootstrap's build script when RUSTC changes) - rust-lang#93737 (bootstrap: prefer using '--config' over 'RUST_BOOTSTRAP_CONFIG') Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

[beta] Backports Backports of: * rust-lang#92611 — Add links to the reference and rust by example for asm! docs and lints * rust-lang#92983 — Update Linux runners to Ubuntu 20.04 * rust-lang#93081 — Update LLVM submodule * rust-lang#93394 — Don't allow {} to refer to implicit captures in format_args. * Cargo: * rust-lang/cargo#10377 — Remove strip = "off" (and undocumented strip = "n"/strip = "no")

rust-highfive assigned estebank Jan 27, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 27, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 27, 2022

m-ou-se mentioned this pull request Jan 27, 2022

Named format arguments introduce implicit positional arguments #93378

Closed

m-ou-se added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 27, 2022

This comment has been minimized.

# to view

m-ou-se added 3 commits January 28, 2022 00:20

Don't allow {} to refer to implicit captures in format_args.

9b8e4c6

Add test for format args capture bug.

fb2d530

Extend format-args capture test.

cef9b47

m-ou-se force-pushed the fix-93378 branch from 2de8557 to cef9b47 Compare January 27, 2022 23:20

CAD97 mentioned this pull request Jan 28, 2022

Named format arguments can be used as positional #93415

Closed

CAD97 reviewed Jan 28, 2022

View reviewed changes

ehuss mentioned this pull request Jan 30, 2022

String interpolation filling in value instead of compile time error #93478

Closed

m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 2, 2022

estebank reviewed Feb 2, 2022

View reviewed changes

apiraino added stable-accepted Accepted for backporting to the compiler in the stable channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Feb 3, 2022

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 Feb 4, 2022

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 Feb 6, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 7, 2022

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Feb 7, 2022

Rollup merge of rust-lang#93394 - m-ou-se:fix-93378, r=estebank

ad3309d

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

m-ou-se mentioned this pull request Feb 7, 2022

Rollup of 13 pull requests #93736

Closed

m-ou-se mentioned this pull request Feb 7, 2022

Rollup of 13 pull requests #93738

Merged

bors merged commit 4445a8f into rust-lang:master Feb 7, 2022

rustbot added this to the 1.60.0 milestone Feb 7, 2022

matthiaskrgr mentioned this pull request Feb 8, 2022

error in format string : unreachable!("foo {bar,baz}"); no longer compiles #93761

Closed

Urgau mentioned this pull request Feb 8, 2022

Fix invalid special casing of the unreachable! macro #93179

Merged

ehuss pushed a commit to ehuss/rust that referenced this pull request Feb 10, 2022

Rollup merge of rust-lang#93394 - m-ou-se:fix-93378, r=estebank

120e33f

Don't allow {} to refer to implicit captures in format_args. Fixes rust-lang#93378

ehuss mentioned this pull request Feb 10, 2022

[beta] Backports #93846

Merged

ehuss modified the milestones: 1.60.0, 1.59.0 Feb 10, 2022

ehuss removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 10, 2022

Mark-Simulacrum removed stable-nominated Nominated for backporting to the compiler in the stable channel. stable-accepted Accepted for backporting to the compiler in the stable channel. labels Jul 15, 2022

Don't allow {} to refer to implicit captures in format_args. #93394

Don't allow {} to refer to implicit captures in format_args. #93394

Uh oh!

Conversation

m-ou-se commented Jan 27, 2022

Uh oh!

rust-highfive commented Jan 27, 2022

Uh oh!

This comment has been minimized.

CAD97 commented Jan 28, 2022

Uh oh!

CAD97 Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

m-ou-se Jan 28, 2022

Choose a reason for hiding this comment

Uh oh!

m-ou-se commented Feb 2, 2022

Uh oh!

estebank commented Feb 2, 2022

Uh oh!

estebank Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

m-ou-se Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

m-ou-se commented Feb 3, 2022

Uh oh!

m-ou-se commented Feb 3, 2022

Uh oh!

craterbot commented Feb 3, 2022

Uh oh!

m-ou-se commented Feb 3, 2022

Uh oh!

bors commented Feb 3, 2022

Uh oh!

m-ou-se commented Feb 3, 2022

Uh oh!

bors commented Feb 3, 2022

Uh oh!

apiraino commented Feb 3, 2022

Uh oh!

craterbot commented Feb 4, 2022

Uh oh!

craterbot commented Feb 6, 2022

Uh oh!

m-ou-se commented Feb 7, 2022

Uh oh!

bors commented Feb 7, 2022

Uh oh!

Uh oh!