Rust: Tracking issue for "implicit named arguments for formatting macros"

Created on 7 Jan 2020  路  28Comments  路  Source: rust-lang/rust

This is a tracking issue for the RFC 2795 (rust-lang/rfcs#2795).

Steps:

Checklist for the tracking issue:

Steps:

  • [ ] Implementation for format_args! behind format_implicit_args feature (name to be bikeshed?)
  • [ ] Implement alternative solution for format_args!(concat!(...)) case that doesn't risk spurious macro hygiene
  • [ ] Implementation for panic!. Behind feature gate and / or edition switch (see unresolved question)
  • [ ] Adjust documentation (see instructions on rustc-guide)
  • [ ] Stabilization PR (see instructions on rustc-guide)

Unresolved Questions:

  • [ ] Solution for format_args!(concat!(...)) - perhaps try out a new unstable macro as per this comment
  • [ ] Final design of the panic! solution - perhaps based off this comment
A-fmt B-RFC-approved C-tracking-issue F-format_implicit_args Libs-Tracked T-lang T-libs

Most helpful comment

Progress update from me: I've been busy but have started work on the implementation of this feature.

A bikeshed on the feature name: I'm leaning towards to the name format_args_capture over the original proposed name format_args_implicits from the RFC.

I think that format_args_capture reflects the mechanics of the feature better - it enables capturing of variables from the surrounding scope. It also avoids the word "implicit" which I find can raise peoples' blood temperature (sometimes for good reason).

The feature gate error message (on nightly) would then look like this:

error: there is no argument named `foo`
  --> C:\Users\david\dev\rust\src\test\ui\fmt\feature-gate-format-args-capture.rs:2:14
   |
LL |     format!("{foo}");                //~ ERROR: there is no argument named `foo`
   |              ^^^^^
   |
   = note: did you intend to capture a variable `foo` from the surrounding scope?
   = help: add `#![feature(format_args_capture)]` to the crate attributes to enable

error: aborting due to previous error

Is it too late to use a different feature name?

All 28 comments

Progress update from me: I've been busy but have started work on the implementation of this feature.

A bikeshed on the feature name: I'm leaning towards to the name format_args_capture over the original proposed name format_args_implicits from the RFC.

I think that format_args_capture reflects the mechanics of the feature better - it enables capturing of variables from the surrounding scope. It also avoids the word "implicit" which I find can raise peoples' blood temperature (sometimes for good reason).

The feature gate error message (on nightly) would then look like this:

error: there is no argument named `foo`
  --> C:\Users\david\dev\rust\src\test\ui\fmt\feature-gate-format-args-capture.rs:2:14
   |
LL |     format!("{foo}");                //~ ERROR: there is no argument named `foo`
   |              ^^^^^
   |
   = note: did you intend to capture a variable `foo` from the surrounding scope?
   = help: add `#![feature(format_args_capture)]` to the crate attributes to enable

error: aborting due to previous error

Is it too late to use a different feature name?

maybe format_args_capture_shorthand? "shorthand" is used in the docs for struct field init shorthand.

@davidhewitt format_args_capture seems reasonable, given the error message, and I agree with avoiding the word implicit.

FWIW at least in my mind capturing implies something closer to what e.g. closures do where the variable is stored within the returned Arguments struct. But that's not something that can be added to Arguments given our stability guarantees, I think, even though I've often wanted it -- so maybe that's not a major concern.

I will note though that the naming of the feature gate and diagnostics feel like something that we can adjust over time and in the implementation PR, definitely no need to get them right immediately :)

  • [ ] Final design of the panic! solution - perhaps based off this comment

Some notes about this problem:


There's several macros that currently that handle their no-formatting-arguments case specially, mostly for efficiency:

  • std::panic!()
  • core::panic!()
  • assert!(), which forwards to either std::panic!() or core::panic!()
  • all kind of log/panic/etc. macros in the rust ecosystem, see this comment.

In all cases, this results in these two problems:

  1. x!("{}") doesn't give an error, but just prints/uses literal "{}".
  2. x!("{var}") will not work with this RFC.

In the case of std::panic!(), this happens because it accepts objects of any type. E.g. panic!(5) will 'throw' a Box<Any> containing 5. So a panic!("hello") will trigger that same case.

core::panic!() doesn't throw boxes around, but still has a special case for efficiency. core::panic!("hello") will call core::panicking::panic (instead of panic_fmt), which takes a &str and constructs an optimal fmt::Arguments using that &str to avoid pulling in any formatting code.

Most of the crates from the community using a special no-arguments case are doing this for efficiency as well. Some will use args.to_string() to format the message into a buffer first, to reduce the number of write/log/etc. calls. This would be an unnecessary copy of a &str in the trivial case, which they try to avoid.


In the case of core::panic and the macros found in various crates, this could've been solved if Arguments::as_str() -> Option<&str> had existed, which would've meant that none of these macros needed a special case for efficiency. The trivial case could've been handled by the printing code instead, and the compiler would've easily eliminated the allocating case completely from the resulting binary. Unfortunately, changing that now will be a breaking change.

Not only will x!("{}") stop working, but most of these macros (including core::panic!()) also accept full expressions that evaluate to a &str right now, because they simply forward their one argument to a function that takes a &str. So:

  • core::panic!(&String::from("hello")) is accepted today.

Unless breaking these examples is acceptable, this would need to be changed over a Rust edition. For both std's and core's panic!() (and assert!()). What exact behaviour makes most sense (also in terms for warnings or clippy lints) in edition 2018 at that point, is still unclear. What behaviour makes sense in edition 2021 is also not clear:

  • What do do with std::panic!(&String::from("hello {}"))? If it accepts arbitrary objects, this would still compile, but act differently than std::panic!("hello {}"). Also, special casing on a string literal is currently not possible with macro_rules!(). $x:literal is possible, but would break panic!(5). Perhaps a separate panic function or macro for throwing boxes is necessary. (In that case, using panic!() for arbitraty objects could already get deprecated in edition 2018.)
  • Same question for core::panic!(). The most trivial solution would mean that core::panic!({ let x = "abc"; x }) no longer compiles. Special casing on $x:literal would work fine here, but it might be better to keep things simple.
  • (assert!() follows the behaviour of either core::panic!() or std::panic!() depending on whether it is used in no_std.)

For the macros in the community crates, there are two options w.r.t. an edition change:

  1. Expose some macro like format_args_or_literal!() to allow them to have the same behaviour over editions as the core panic macro. This macro would either return an enum, or expect users to use .as_str() as described above to handle the trivial case. All these crates will need to be updated, but their behaviour will not change for users staying on edition 2018.
  2. Expect crates to update to using .as_str() to handle the trivial case, and not tie their behaviour to the Rust edition. These crates will have to release a breaking change and bump their version. (Resulting in a bit more breakage but less complexity.)

My proposal:

  • Update all macros to always use format_args!(), without the special case, but the core/std macros only in edition 2021. This means dropping support for panic!("{}"), panic!({ let x = "abc"; x}), panic!(5), panic!(bla) etc.
  • Use Arguments::as_str() for optimizing the trivial case, instead. Expect community crates to start using that in all editions.
  • Add a separate panic() function in std to throw Box<Any>s.

Update: The core/std part of this is now an RFC: https://github.com/rust-lang/rfcs/pull/3007

Thanks @m-ou-se for the detailed write-up. I'm nominating this for @rust-lang/lang discussion since it has Edition ramifications, although I guess that the design of these macros is in many ways a @rust-lang/libs decision.

I agree with you that an edition is the appropriate way to manage this, and I think we should shoot for panic!("{var}") working consistently. We'd have to look carefully at how to implement this, I don't think it would work using the mechanisms we have, but I think it could be made to work. I'd probably say that "non-string-constants" should just be deprecated, but I don't have a strong opinion about it.

The automated migration seems easy enough, in any case, since panic!(...) just becomes panic!("{}", ...), right?

The automated migration seems easy enough, in any case, since panic!(...) just becomes panic!("{}", ...), right?

For core::panic!(), yes. But for std's, no. std::panic!("hello") throws a &str, not a fmt::Arguments or String. The standard panic handler would print the same, but custom panic hooks can tell the difference. Also, std::panic!() can throw things that don't implement Display/Debug right now.

I can see the panic! change working by exporting a different macro in the new 2021 prelude.

I'm generally in favor of removing Box<Any> support from the panic! macro: it's very rarely used and as @m-ou-se said, this functionality can still be provided via a normal function in std.

Something missing from my notes/proposal above is making the behaviour consistent or at least not too unexpected on 2018/2015, after format_implicit_args becomes stable.

If the changes to core/std's panic!() are gated behind edition 2021, then in 2018/2015 we get:

let a = 5;

println!("{a}"); // prints "5"
panic!("{a}"); // panics with "{a}"

println!("{a} {}", 5); // prints "5 5"
panic!("{a} {}", 5); // panics with "5 5"

Possible ways to make this better:

  1. Gate all of format_implicit_args behind edition 2021. No println!("{a}") in 2018. :(
  2. Gate format_implicit_args for panics behind edition 2021. No panic!("{a} {}", 5) in 2018.
  3. Warn for 1-argument panics containing implicit arguments. panic!("{a}") warns, but still prints "{a}". (But what should the warning suggest? panic!("{}", a)? v2::panic!("{a}")? "Switch to 2021"? ..?)
  4. Break things and don't gate anything behind an edition, and hope that literal braces (and non string literals) in single argument panics are uncommon right now. Possibly by starting giving deprecation warnings first for a while. (Though what should those warnings suggest for panic!("{")? panic!("{}", "{")? panic_str("{")? ..?)

None of these seem great. :(

  1. Break things and don't gate anything behind an edition, and hope that literal braces (and non string literals) in single argument panics are uncommon right now. Possibly by starting giving deprecation warnings first for a while. (Though what should those warnings suggest for panic!("{")? panic!("{}", "{")? panic_str("{")? ..?)

This could also be panic!({ "{" }) / panic!(("{")), right? Still not great of course.

This could also be panic!({ "{" }) / panic!(("{")), right? Still not great of course.

Maybe, but if panic!("{") would be deprecated, I suppose using expressions like that should be deprecated as well.

We discussed this in today's @rust-lang/lang meeting. The sense of the meeting was:

  • We would be in favor of completing this RFC and using an edition transition to make println!, panic!, etc all behave in a uniform fashion.
  • We would like to do so in a way however that preserves all existing capabilities (e.g., ability to panic with arbitrary values, and with string literals without allocation), though that may require more explicit syntax. We did not discuss in detail the comments I see here, such as whether panic!({"{"}) ought to be deprecated, however.
  • We would like to see a concrete proposal with a migration plan.

We did not discuss in detail the comments I see here, such as whether panic!({"{"}) ought to be deprecated, however.

My personal opinion is that panic!({expression}) would be quite reasonable as a way to 'panic with some explicit value', as opposed to panic!("...", ...) as the "with arguments" form.

One interesting question that I don't know the answer to is whether panic!("foo") ought to panic with a &'static str or a String -- it introduces an obvious "non-uniformity" in the type, depending on whether it contains "format notation". I do think that panic!("foo{{") ought to be required in order to name a { (i.e., we should interpret a string literal as a format string). I think I would tend towards panic!("foo") raising a String, and requiring panic!({"foo"}) to panic specifically with a string literal, but that's a lightly held opinion.

My preference would be to deprecate panic! with arbitrary expressions and only support format strings. I have never seen a use of non-string panics in the wild, so I think this functionality is better provided by a function in std::panicking that takes a Box<Any>.

Supporting Anicdote: I used a non-string panic once on accident and it took me well over 30 seconds to even understand what had gone wrong (because the compiler accepted it, so "clearly" i must have typed the right thing).

I too have only only ever experienced non-string panics in error. Especially the first time I did this it took quite a while to figure out where the Box<dyn Any> was originating from.

@nikomatsakis when you say the lang team intends to keep all existing functionality, do you mean that all of the functionality remains on the macros? Or is moving the "panic with expression" case to a new function potentially on the table?

My reservation with the proposed panic!({foo}) syntax for "panic with box any containing foo" is that it's dangerously close to panic!("{foo}"). I think especially beginner Rust users won't easily spot the difference and will also be the ones most confused when they find their panic message is broken.

As for a migration strategy, I prefer @m-ou-se 's option 3. I would like to propose that we do the following things at the same time without waiting for an editon change:

  • we introduce this RFC for all cases except single-argument panic!, as this RFC is non-breaking for these cases.
  • we introduce the new syntax for "panic with a value", whether function or new syntax in the panic! macro
  • for the single-argument panic, we start to emit a warning for string literals which contain format syntax, and otherwise continue to let it function as-is. The warning could suggest two options of resolution which would not change after migration to the next edition. E.g. for panic!("{foo}") - this would be panic!("{}", format_args!("{foo}")) if formatting was intended, and maybe panic!({"{foo}"}) if a literal was intended. (Though this nested set of sigils is quite heavy, which again makes me think migrating the "panic with a value" case to a function would be desirable).

After the edition change the single-argument panic case changes to exclusively expect a format string, and we're done!

I think this can be implemented by changing the single-argument panic arm to forward to a new compiler-builtin macro. This macro would be the only code that needs to change behaviour with an edition change.

I need to think further about the exact design of this new built-in macro. I think some downstream libraries which behave similarly to panic! may also want to use this macro to support cross-edition behaviour. I'll try to come back with a design hopefully at the weekend if y'all haven't already figured it out by then! 馃槃

I agree that panic!({"foo"}) is perhaps too close to panic!("{foo}"). Another option is to have a panic_with! macro or something like that (though this name is perhaps non-ideal, since it suggests a closure).

I would like to propose that we do the following things at the same time without waiting for an editon change:

In general, I think the way to "phase in" new syntax should fit this mold (and your suggestion does):

  • Permit the new syntax when we can do so backwards compatibly.
  • Have an "uglier" form that you can use until the next edition (panic!("{foo}"), in your proposal).
  • Introduce the "cleaner" form after new edition.

I guess my one concern with the plan as described is that it seems like it's a bit surprising that panic!("{x} {}", "foo") will print out x but panic!("{x}") will not. It might be cleaner to have folks "opt in" to the new semantics completely, perhaps by an explicit use std::prelude::v2021::panic or something like that? Have to think about that a bit.

Another option is to have a panic_with! macro or something like that (though this name is perhaps non-ideal, since it suggests a closure).

panic_with! is actually something I was wondering about yesterday too, though I hadn't considered the point you make about the closure.

One idea that crossed my mind was simply making this panic-with-a-value case a function called start_unwind. It fits well with the existing catch_unwind / resume_unwind. OTOH if it needs to stay a macro, I might avoid start_unwind! because the other two are functions.

I kind of like throw! or panic_throwing! (because it pairs with "catch" in catch_unwind). Maybe of the two, panic_throwing! is better because it keeps the panic terminology rather than suggesting this is some alternate error handling mechanism.

I guess my one concern with the plan as described is that it seems like it's a bit surprising that panic!("{x} {}", "foo") will print out x but panic!("{x}") will not. It might be cleaner to have folks "opt in" to the new semantics completely, perhaps by an explicit use std::prelude::v2021::panic or something like that? Have to think about that a bit.

馃憤 I don't have a strong opinion on whether it should be opt-in so happy to defer to core team members on this point.

One thing to consider is that perhaps it's already surprising for a lot of people that panic!("{x}") is not a compile error - so by starting to emit a warning for this case we might rescue a few people from hidden bugs!

panic! is a macro because it does formatting.

Presumably if it's taking a single value the start_unwind operation could just be a generic function like fn start_unwind<T>(t: T) -> !

Panicking doesn't necessarily unwind, with the panic=abort setting, or with no-std and a custom panic_handler.

Panicking doesn't necessarily unwind, with the panic=abort setting, or with no-std and a custom panic_handler.

Great point - in which case I think start_unwind is not a good name for this any more!

Opened #74622 specifically for this arbitrary-payload-throwing panic function, to have a place to bikeshed over the name etc.

My preference would be to deprecate panic! with arbitrary expressions and only support format strings. I have never seen a use of non-string panics in the wild, so I think this functionality is better provided by a function in std::panicking that takes a Box.

When the ability to use arbitrary expressions was originally proposed in 2012, the only rationale given was "in the future it will probably need to accept any sendable type argument, not just a string", so in retrospect it was probably just a mistaken prediction of how the idioms governing its use would evolve over time. I second the idea that panic should just support format strings (the only possible use case I can imagine is as a minor aid for printline debugging, but we have dbg! these days for that).

Regarding the panic! changes, even the already performed changes to core::panic!(foo) (as part of https://github.com/rust-lang/rust/pull/74056) are potentially problematic:

  • they break some cases of panics in consts for no_std (https://github.com/rust-lang/rust/issues/51999#issuecomment-687590082, a nightly-only regression)
  • they lead to increased code size by circumventing the trick explained here.

So whatever we end up doing should be done uniformly for both of these macros -- and maybe there should be some discussion if the above mentioned regressions and increased inconsistency between #[no_std] and regular code are acceptable given that there is not even a clear transition plan yet.

I forgot to say, both issues could be avoided if Arguments::as_str returned &str instead of &'static str -- but that might cause other problems elsewhere. This was mentioned at https://github.com/rust-lang/rust/pull/74056#issuecomment-653896585 but I do not understand how the static lifetime makes such a big difference here -- Cow<'_, str> also works very well. On the one hand I can see the argument that format strings are static anyway, on the other hand it does seem potentially useful for Arguments to be able to support allocation-free non-static strings as well.

I do not understand how the static lifetime makes such a big difference here

as_str() was made to avoid the need for special-casing macros (like bail!() here, or like anyhow::anyhow!(), etc.). They can only use it if it is 'static, as they keep the string around in an object, instead of printing it directly.

That makes sense. And this then implies that core::panic!(foo) cannot use core::panicking::panic any more as foo might not be static, which directly leads to the issues I mentioned above.

I don't see a good way out of this, but I am satisfied with having understood the tradeoff here.

Relevant discussion regarding panic has recently been proposed in https://github.com/rust-lang/rfcs/pull/3007

Was this page helpful?
0 / 5 - 0 ratings