Replace Debug::{key_with, value_with} with DebugMap::entry_with

Proposal

Replace the (currently nightly) methods for printing the Debug repr of a map's key & value with one method for both.

Problem statement

To ensure that the methods key_with & value_with are called in the right order, DebugMap remembers whether key_with was called without an accompanying value_with call, and if the order is violated, it panics. This setup makes it easier to commit a logical error that will crash the app at runtime, and unnecessarily increases code size with a conditional panic

Solution sketch

impl DebugMap {
    fn entry_with<K, V>(&mut self, k: K, v: V) -> &mut Self
    where
        K: FnOnce(&mut Formatter<'_>) -> std::fmt::Result,
        V: FnOnce(&mut Formatter<'_>) -> std::fmt::Result; 
}

This approach:

Reduces the possibility of a logical error;
Reduces code size
Reduces API size (1 method instead of 2)

Links and related work

This sample of code on Github suggests that practically all of DebugMap::key_with invocations are immediately followed by an invocation of DebugMap::value_with, which raises a question of "do they really have to be 2 separate functions?"

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

We think this problem seems worth solving, and the standard library might be the right place to solve it.
We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.

If this is accepted, I'll immediately submit an implementation of this change

See the original ACP (#288) for additional context.

Adding a new entry_with method would be no problem IMO (and is mentioned as a possible additional thing to add in the ACP), but removing key_with / value_with is undesireable.

You'll note in the implementation PR for key_with / value_with that the behavior you dislike (functions that must be called in specific order) was already present long before the closure-style helpers were added. If you want to remove the possible panic!() then a slightly larger code restructuring might be needed, so that entry{,_with}() is a fully-fused combination of key{,_with}() and value{,_with}() rather than just calling them.

entry_with just calling to key_with & value_with would ruin the whole point of the change, I expect the proposed method to be implemented in a way that will combine the impl of the 2. It's true that the order assertion is already implemented in key & value methods, but I don't think this means that we should continue in the same direction, instead we have a chance to make the right decision towards a less error-prone API

How would you implement h.key("some_key").value_with(|f| { /* multi-line formatting closure */ }) if the API only contains key, value, and entry_with?

impl DebugMap {
    fn entry_with<K, V>(&mut self, k: K, v: V) -> &mut Self
    where
        K: FnOnce(&mut Formatter<'_>) -> std::fmt::Result,
        V: FnOnce(&mut Formatter<'_>) -> std::fmt::Result; 
}

Note that this requires the K and V callbacks to co-exist, so they can't have any overlapping mutable borrows. As a hypothetical example, maybe they need to access the same memoization storage, which they could do with sequenced key_with and value_with.

Ok, fair points, I agree I overlooked those cases, let's keep key_with & value_with, but could we get entry_with?

Regarding the issue of additional panic machinery: can you check whether the compiler is capable of optimizing away the assert? And if it isn't, possibly the functions could be modified to make it easier for the compiler to do so (e.g. small always-inline functions containing the state checks that then call larger functions).

Just checked, and the asserts are not removed when calling key_with & value_with right after each other.

Checked the following code using cargo asm --rust fmt_entry:

#![feature(debug_closure_helpers)]

#[no_mangle]
pub fn fmt_entry(f: &mut std::fmt::Formatter, k: &str, v: &str) -> std::fmt::Result {
    f.debug_map()
        .key_with(|f| f.write_str(k))
        .value_with(|f| f.write_str(v))
        .finish()
}

fn main() {
    println!("{}", std::fmt::from_fn(|f| fmt_entry(f, "key", "value")));
}

On Tue, Mar 25, 2025 at 11:50:03AM -0700, Tim Kurdov wrote: Just checked, and the asserts are not removed when calling `key_with` & `value_with` right after each other.

Thank you for checking! It would be worth experimenting with a few approaches to see if there's a way to get the compiler to be able to optimize out the asserts.

This sample of code on Github suggests that practically all of DebugMap::key_with invocations are immediately followed by an invocation of DebugMap::value_with, which raises a question of "do they really have to be 2 separate functions?"

This characterization does not reflect what is shown to me in the search. The search returns 14 files + "540 identical files found across repositories" (forks) which I will disregard. Of the 14, there is 1 that calls key_with followed by value_with. There are 2 that call key_with followed by value, 1 calls key_with only (the libcore implementation of key), 3 call something else called key_with that is not core::fmt::DebugMap, and 7 are a fork of one of the previous.

The useful sample size is small. In some sense 1/3 is practically all ("all but 2") but it's also not the evidence that the ACP makes it seem.

We discussed this ACP in the most recent standard library API team meeting. #563 (comment) and #563 (comment) were compelling reasons that removing key_with/value_with in favor of entry_with is undesirable. (We see you also changed your mind on this.)

As far as keeping key_with/value_with while adding a new entry_with, we did not find sufficient motivation for this.

We talked about there being 3 ways to supply formatting logic for a key and value:

key/value, implicitly using a type's Debug impl
key_with/value_with with a closure written inside the function call
key_with/value_with with something other than a closure

The most utility to be expected from entry_with is when both key and value are passed in way 3.

  let do_print_key = ...;
  let do_print_value = ...;
- f.key_with(do_print_key);
- f.value_with(do_print_value);
+ f.entry_with(do_print_key, do_print_value);

But as you can see, the amount of improvement here is still negligible. If one line is desired aesthetically, this is substantially identical to f.key_with(do_print_key).value_with(do_print_value).

If either key or value can be passed in way 1, entry_with provides negative utility.

- f.key(&k);
- f.value_with(|f| {
-     ...
- });
+ f.entry_with(
+     |f| k.fmt(f),
+     |f| {
+         ...
+     },
+ );

If one or both are passed in way 2, entry_with provides zero or negative utility.

- f.key_with(|f| {
-     ...
- });
- f.value_with(|f| {
-     ...
- });
+ f.entry_with(
+     |f| {
+         ...
+     },
+     |f| {
+         ...
+     },
+ );

For "Reduces the possibility of a logical error", we did not buy this as a rationale. In our estimation adding entry_with would not move the needle on correctness of such formatting code.

For "Reduces code size", assuming this refers to compiled binary size not source code, there is a possibility this can be turned into a rationale but the ACP doesn't have that data. There is precedent in work like rust-lang/rust#98190 (comment) that agglomerating common sequences of fmt function calls can shave bytes off of debug code. Currently we do not expect the effect to be significant when considering how niche this part of the API already is going to be.

Navigation Menu

Repository navigation

Replace `Debug::{key_with, value_with}` with `DebugMap::entry_with` #563

Proposal

Problem statement

Solution sketch

Links and related work

What happens now?

Possible responses

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Replace Debug::{key_with, value_with} with DebugMap::entry_with #563

Description

Proposal

Problem statement

Solution sketch

Links and related work

What happens now?

Possible responses

Activity

its-the-shrimp commented on Mar 17, 2025

jmillikin commented on Mar 17, 2025

its-the-shrimp commented on Mar 17, 2025

jmillikin commented on Mar 17, 2025

cuviper commented on Mar 17, 2025

its-the-shrimp commented on Mar 17, 2025

joshtriplett commented on Mar 25, 2025

its-the-shrimp commented on Mar 25, 2025

joshtriplett commented on Mar 26, 2025

dtolnay commented on Mar 28, 2025

dtolnay commented on Mar 28, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Replace `Debug::{key_with, value_with}` with `DebugMap::entry_with` #563