Skip to content
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

format-dynamic-fill #3394

Closed
wants to merge 5 commits into from
Closed

Conversation

ModProg
Copy link

@ModProg ModProg commented Feb 23, 2023

@joshtriplett joshtriplett added T-libs-api Relevant to the library API team, which will review and decide on the RFC. I-libs-api-nominated Indicates that an issue has been nominated for prioritizing at the next libs-api team meeting. labels Feb 23, 2023
Co-authored-by: Josh Triplett <josh@joshtriplett.org>
@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2023

This seems relatively straightfoward. It adds a little bit overhead to std::fmt::write to be able to support this feature, but that can probably be kept to a minimum.


For context, there are currently eight formatting options: width, precision, fill, alignment, sign, alternate, zero padding and debug hex. Only the first two can be picked dynamically. With the proposed change, the first three become dynamic:

format options type dynamic (now) dynamic (proposed)
width usize ✔️ ✔️
precision usize ✔️ ✔️
fill char ✔️
alignment enum (none, <, >, ^)
sign enum (none, +, -)
alternate bool (none, #)
zero pad bool (none, 0)
debug hex bool (?, x?)

There is some overhead involved in allowing an option to be dynamic, so I think it makes sense to only do this for the usize and char options, and leave the boolean/enum flags as they are, since it's easy to turn that into a simple if if one needs to e.g. select the sign or alignment dynamically.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2023

Due to not being able to create or modify Formatter, this is the only way to allow this.

Perhaps we should (also) add a way to create a Formatter.

@m-ou-se m-ou-se removed the I-libs-api-nominated Indicates that an issue has been nominated for prioritizing at the next libs-api team meeting. label Mar 21, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2023

@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 21, 2023

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

Concerns:

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!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Mar 21, 2023
@ModProg
Copy link
Author

ModProg commented Mar 21, 2023

Perhaps we should (also) add a way to create a Formatter.

I asked that on zulip as well: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Formatter.20construction.20without.20need.20for.20hardcoded.20string/near/338148530

@ModProg
Copy link
Author

ModProg commented Mar 21, 2023

since it's easy to turn that into a simple if if one needs to e.g. select the sign or alignment dynamically.

Yes, but turns out, doing that for all combinations will still create a bunch of code (and actually make this otherwise very small crate, slower to compile than I had hoped) https://github.com/ModProg/interpolator/blob/577d7e4365818466203ad94a4228bfd4b1aff512/src/fmt/display.rs

@dtolnay
Copy link
Member

dtolnay commented Mar 22, 2023

Are there examples of a use case in which this would be used for real?

The only example invocation in the RFC is with non-dynamic fill characters ('+' and '-') which is presumably not how anybody would want to use this, since that is supported without this feature.

@rfcbot concern needs more compelling motivation

@ModProg
Copy link
Author

ModProg commented Mar 22, 2023

Are there examples of a use case in which this would be used for real?

The only example invocation in the RFC is with non-dynamic fill characters ('+' and '-') which is presumably not how anybody would want to use this, since that is supported without this feature.

My use case is implementing a runtime version of format with https://docs.rs/interpolator/. A use case I could imagine is something like having a setting that switches a program's output between aligning with a visible placeholder . and an invisible .

For my use case this isn't the ideal solution (I'd prefer being able to create a Formatter directly, e.g. with a builder), but it is the only one I can currently foresee being feasibly implemented.

@dtolnay
Copy link
Member

dtolnay commented May 25, 2023

I feel that the interpolator use case of "runtime version of format" is worthwhile for the standard library to make possible in some form. But beyond the exact use case of interpolator, it is not clear to me that building dynamic fill into the formatting syntax has any demand.

As I see it:

  • Even if dynamic fill were supported in formatting syntax, interpolator use case still would require individually listing all 2112 possible format configurations (e.g. {value:>-#0x?}), so it would still strongly prefer custom Formatter construction over this RFC. Custom Formatter construction would dramatically simplify the interpolator implementation and eliminate compile time cost of compiling that many format args.

  • As soon as Formatters become constructible, it seems likely that literally nobody would ever use dynamic fill ever again, other than maybe by accident while they are trying to express something else in the convoluted formatting syntax.

So I would say let's skip this feature and directly go to Formatters that can be populated with a dynamic set of configuration without using format_args syntax.

@rfcbot concern expose Formatter construction instead of this

Directly adding &mut methods to Formatter is probably unwise:

impl<'a> Formatter<'a> {
    pub fn set_fill(&mut self, fill: char) {
        self.fill = fill;
    }
}

That is how C++ iostream / iomanip work (https://en.cppreference.com/w/cpp/io/manip). For example std::cout << std::setfill('*') changes the fill character for all subsequent writes to the same stream.

Instead maybe this kind of API:

// core::fmt

impl<'a> Formatter<'a> {
    pub fn with_fill<'b>(&'b mut self, fill: char) -> Formatter<'b>
    where
        'a: 'b,
    {
        Formatter {
            fill,
            flags: self.flags,
            ...
            buf: &mut *self.buf, // reborrow with shorter lifetime
        }
    }
}
// user code

struct DynamicFill<D> {
    fill: char,
    delegate: D,
}

impl<D: Display> Display for DynamicFill<D> {
    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
        Display::fmt(&self.delegate, &mut formatter.with_fill(self.fill))
    }
}

or a more general builder:

impl Formatter<'_> {
    pub fn builder() -> FormatterBuilder;
}

impl FormatterBuilder {
    pub fn fill(&mut self, fill: char) -> &mut Self;
    pub fn build(&mut self, /*a `&'a mut dyn Write` arg goes either here or builder()*/) -> Formatter<'a>;
}
let mut buf = String::new();
let formatter = fmt::Formatter::builder()
    .fill(ch)
    .build(&mut buf);
thing.fmt(&mut formatter)?;

@thomcc
Copy link
Member

thomcc commented May 25, 2023

One case where I've wanted dynamic fill is implementing a C printf by mapping as much as possible to rust fmt, but that's pretty niche (I also don't have the code on hand). It would also be well-supported by @dtolnay's suggestion.

@ModProg
Copy link
Author

ModProg commented May 25, 2023

Yeah, this RFC arose more or less of the feeling that getting access to formatter construction/modification is something that will probably not happen in the near future, and this seamed more achievable.

I'm very much in favor of having access to the formatter (although this would mean I wasted a lot of time implementing interpolator the way I did 😄 )

@m-ou-se
Copy link
Member

m-ou-se commented May 25, 2023

In that case, it seems Formatter construction is the preferred route. So let's go in that direction instead. :)

@rfcbot cancel

@rfcbot
Copy link
Collaborator

rfcbot commented May 25, 2023

@m-ou-se proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels May 25, 2023
@ModProg
Copy link
Author

ModProg commented May 25, 2023

In that case, it seems Formatter construction is the preferred route. So let's go in that direction instead. :)

If there is a path forward for that, I'll happily agree.

@Nemo157
Copy link
Member

Nemo157 commented May 25, 2023

I also would love runtime setting of Formatter parameters, I currently have ~10kLoC of match converting args into format_args! calls, and that can't even handle the fill argument currently (though I guess I could have implemented support if this RFC was implemented by just doubling that match up).

@dtolnay
Copy link
Member

dtolnay commented May 25, 2023

@rfcbot fcp close

Next step: we can equate #3394 (comment) to an approved ACP, so next step would be prototype it in a PR.

Maybe RFC if we need to argue about whether a builder is needed. My preference is no separate builder type if possible.

@rfcbot
Copy link
Collaborator

rfcbot commented May 25, 2023

Team member @dtolnay has proposed to close 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!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-close This RFC is in PFCP or FCP with a disposition to close it. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels May 25, 2023
@rfcbot
Copy link
Collaborator

rfcbot commented May 26, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label May 26, 2023
@dtolnay dtolnay closed this May 26, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This RFC is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this RFC. T-libs-api Relevant to the library API team, which will review and decide on the RFC. to-announce
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants