[generic_assert] Constify methods used by the formatting system

[c410-f3r]

cc #44838

Starts the "constification" of all the elements required to allow the execution of the formatting system in constant environments.

const _: () = { panic!("{:?}", 1i32); };

Further stuff is blocked by #133999.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[jhpratt]

Looks like this is strictly internal so far, so this shouldn't need team input.

@bors r+

[bors]

📌 Commit f5f72c1 has been approved by jhpratt

It is now in the queue for this repository.

[jhpratt]

@bors r-

[c410-f3r]

@rustbot review

Should be good now

[jhpratt]

Thanks!

@bors r+ rollup

[bors]

📌 Commit db17be8 has been approved by jhpratt

It is now in the queue for this repository.

[m-ou-se]

@c410-f3r @jhpratt This PR caused #139136 and #139621

There was a good reason that fmt::Arguments had only one constructor that was const (named new_const), which is only used if the format_args macro determines that using it as a constant is allowed:

rust/compiler/rustc_ast_lowering/src/format.rs

Lines 467 to 469 in 6bc57c6

	if allow_const && arguments.is_empty() && argmap.is_empty() {
	// Generate:
	// <core::fmt::Arguments>::new_const(lit_pieces)

The other methods must not be const!

Looks like this is strictly internal so far, so this shouldn't need team input.

The effect was not internal. This allowed more format_args!() invocations in const which has now shipped on stable. We're trying to revert it, but unfortunately that is now technically a breaking change.

[m-ou-se]

Next time you change/review anything about fmt::Arguments or format_args!(), please ping me.

Edit: Triagebot should do this automatically in the future: #140173

[jhpratt]

Sorry, of course! Certainly not intentional.

My question is, how could/should I have recognized that this was not internal? The only pub methods are doc-hidden or not directly exposed and therefore not part of the public API. The only tests affected still errored out, so there was no reason to believe that it permitted anything new.

Next time you change/review anything about fmt::Arguments or format_args!(), please ping me.

👍

[m-ou-se]

My question is, how could/should I have recognized that this was not internal?

The methods this PR changed are all methods on types that are #[lang] items. The interfaces of lang items are tightly coupled to code in compiler/ (in this case, rustc_ast_lowering/src/format.rs).

That this PR had effects on stable code can be seen from the few errors that disappeared in one of the tests, in the diff of this PR. E.g.

-  --> $DIR/format.rs:2:5
-    |
- LL |     panic!("{:?}", 0);
-    |     ^^^^^^^^^^^^^^^^^
-    |

That test doesn't use any unstable features, so that's a clear sign that this affects stable code.

In those cases, it might be a good idea to ping wg-const-eval to check if you're not accidentally changing the set of things that is allowed in const.

[RalfJung]

To be fair, it's not clear that I would have caught this, since all involved functions are unstable. If something relies on an unstable function not being const, that really warrants a comment at that function and a dedicated test (as is done in #139624), otherwise this is bound to go wrong. "Just be more careful" is generally not the attitude we are using in Rust, after all.

That test doesn't use any unstable features, so that's a clear sign that this affects stable code.

Every line that used to have an error still has an error. It's fairly normal for a PR to change what exact errors are emitted. So I would not call this a very clear sign.

The main issue is that we didn't have a test ensuring that panic!("a {}", "a") cannot be used in const.

[m-ou-se]

Every line that used to have an error still has an error. It's fairly normal for a PR to change what exact errors are emitted. So I would not call this a very clear sign.

FWIW, there were two error annotations: The first one is basically for "no Debug formatting in const" and the second is basically "no format_args!() in const". This PR removes the second one.

What makes it extra confusing is that the compiler is replacing the first error message with a generic "no formatting in const" message, but not the second one. That should have been the other way around.

The main issue is that we didn't have a test ensuring that panic!("a {}", "a") cannot be used in const.

I agree.

[RalfJung]

FWIW, there were two error annotations: The first one is basically for "no Debug formatting in const" and the second is basically "no format_args!() in const". This PR removes the second one.

Yeah, but it's quite common that a PR changes output from 2 annotations to 1, at least in the area of const stability checks -- and generally we consider that an improvement. So it does not stand out to me in the diff.

[RalfJung]

What makes it extra confusing is that the compiler is replacing the first error message with a generic "no formatting in const" message, but not the second one. That should have been the other way around.

That indicates that the logic around here uses the wrong key to trigger NonConstFmtMacroCall. I don't understand format_args! macro expansion well enough to suggest what else it should key on. It's using a diagnostic item so it may just be a better of moving the diagnostic item attribute elsewhere?

[m-ou-se]

That indicates that the logic around here uses the wrong key to trigger NonConstFmtMacroCall. I don't understand format_args! macro expansion well enough to suggest what else it should key on. It's using a diagnostic item so it may just be a better of moving the diagnostic item attribute elsewhere?

Yeah I already have that changed in my local copy. Will send it as a PR as soon as #139624 is merged.