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

RFC: Stabilize std::fmt #380

Merged
merged 4 commits into from
Nov 12, 2014
Merged

RFC: Stabilize std::fmt #380

merged 4 commits into from
Nov 12, 2014

Conversation

alexcrichton
Copy link
Member

High-level summary:

  • Leave the format syntax as-is.
  • Remove a number of superfluous formatting traits (renaming a few in the process).

Rendered

@nrc
Copy link
Member

nrc commented Oct 9, 2014

I think some of the mechanisms for formatting (including some proposed to be stable here) are too specific and could be made more general purpose. In particular, the concepts of annotated arguments and of printing are too closely coupled. I started some refactoring ages ago to try and make the arguments stuff more general purpose. I'll dig out the branch and see if I can come up with some specific recommendations.

@SiegeLord
Copy link

I think disallowing a=b as an expression across the board is a bit too heavy-handed if the only purpose is to future-proof keyword arguments. In my keyword argument library, which allows the user to define format!-like 'function macros', I disallow a=b expressions from appearing when calling such a 'function macro'. I.e. let's say you have a function f(x: ()). The following hold (in my library):

f!(x = ()); // basic syntax
f!(()); // implicitly set x to ()
let mut a;
f!(a = 1); // illegal, no such argument
f!({a = 1}); // implicitly set x to the result of a = 1
f!(x = a = 1); // explicitly set x to the result of a = 1

I imagine the same can be done for Rust's grammar in general.

@sinistersnare
Copy link

-1 for marking (or 'recommending') anything #[stable] how can you have stable libraries in an unstable language?

There has been at least one case where we had to change stable code due to language changes (rust-lang/rust/pull/17745), so how can we guarantee this will not happen again?

@alexcrichton
Copy link
Member Author

@nick29581, I would be curious to here some more detail about your thoughts, I'm not sure I quite know what you're alluding to.

@SiegeLord, that is indeed one possibility! I personally would rather have a consistent grammar than keyword arguments, however, as opposed to special-casing the grammar for expressions in function arguments for a hypothetical use case.

@sinistersnare, if we took that route, then nothing would be stable by the time we hit 1.0, and even if we didn't mark anything as #[stable] there are quite a large number of de-facto stable apis which we would like to track. Also note that we are indeed following semver. Pre-1.0 any minor release change is considered a breaking change. The current stability annotations imply what is stable for the next release. Once we hit 1.0, that means that everything marked #[stable] is guaranteed to be stable for 1.1, but it just so happens that pre-1.0 the stability markers don't convey as much as one may expect.

The usage of stability markers is to help track our progress through the standard library as we review APIs, and if we didn't use #[stable] for "stable apis", then we'd just use something else which would have precisely the same connotation that #[stable] has today.

@SiegeLord
Copy link

@SiegeLord, that is indeed one possibility! I personally would rather have a consistent grammar than keyword arguments, however, as opposed to special-casing the grammar for expressions in function arguments for a hypothetical use case.

I don't find that convincing. You are already changing the grammar is a negative way for that hypthetical use case, so it's just a matter of choosing the way of doing it.

Rust's grammar is already inconsistent. For example, take struct literals and if expressions... this is exactly the same sort of special case: two language constructs don't interact well in one place, and in that one place the grammar is altered to allow for them to coexist.

@SiegeLord
Copy link

One other thing to note is that the named argument syntax is basically unused (I couldn't find a single usage of it glancing through the Rust repository), and its usefulness seems questionable to begin with (it was a lot more useful in the days when formatting syntax was to be used for internationalization). I, for one, would rather support this kind of syntax:

let some_var = 1;
format!("{some_var}");

Or:

let some_var = 1;
format!("some_var = "some_var" and that's all I have to say about it");

I.e. similarly to how print works in Python (I've ommitted commas as they seem superfluous here, but that's not a critical design decision). This doesn't handle formatting specifiers well, although in my opinion that's not a big issue (i.e. you'd implement them via wrappers).

@aturon
Copy link
Member

aturon commented Oct 16, 2014

Thanks so much for this effort! We'll definitely have to sort out the named argument syntax stuff, but I'm going to focus on the rest of the API for now.

Here are some questions I had when reading the RFC and looking at the public API:

  • The RFC doesn't mention the rt submodule, which is current public. As I understand it, this is needed as a hook for the macros (since we don't have visibility hygiene). I think other parts of the module needed for macros are hidden from the docs; probably this should be too? But also worth thinking about stability attributes. (Or maybe we have a general policy that items hidden from the docs are considered experimental/private?)
  • It's unfortunate that we have FormatWriter in addition to std::io::Writer and duplicate implementations here. I understand why this is done, but I would like to try to move io::Writer into core if possible, and thus eliminate FormatWriter.
  • Can you add some rationale about FormatError? Can we do better here (in particular if we move io::Writer and use it?)
  • Is there any way we can remove the inherent methods on Formatter? Particularly if we do refactor away FormatWriter?
  • The format and write functions presumably exist just as targets for macros, right? Can we mark that better somehow? In general, it would be nice to move/hide all parts of this API that are not meant to be used directly but are only used by the provided macros. (Probably most of the API here...)

I suppose in the end my basic concern here is that a fair amount of implementation detail is exposed in this module. The most worrisome part of that is the stuff that shows up in Show, since that's a widely implemented trait. If we can make that API simpler and more consistent with e.g. Writer elsewhere, I think that'd be a win.

@alexcrichton
Copy link
Member Author

The RFC doesn't mention the rt submodule, which is current public

Aha, thanks! I've listed the remaining pieces and suggested #[experimental] for all of them as we further develop our macro story.

It's unfortunate that we have FormatWriter in addition to std::io::Writer and duplicate implementations here

I agree! I've switched the recommendation to #[experimental] pending movement to libcore with associated types.

Can you add some rationale about FormatError?

Can do, I've added a few words here. Do they look ok to you?

Is there any way we can remove the inherent methods on Formatter?

Do you think that we should only expose Formatter as "an instance of Writer"? Currently there are only two methods, both of which are used as helper methods for respecting the formatting flags requested as part of a format. I would personally think that these are ok, but I'm curious what your motivation is for moving them away.

The format and write functions presumably exist just as targets for macros, right?

That's a good point! I've switched the recommendation to #[experimental] pending the outcome of exported macros across crates.

I suppose in the end my basic concern here is that a fair amount of implementation detail is exposed in this module.

That is a good point that I hadn't though much about before. The original intention for this was to actually expose all the implementation details to create formatting strings at runtime, but this was never truly manifested and we should probably optimize for removal of as many of these bits as possible. For now I think #[experimental] + #[doc(hidden)] are the best we can do, but we may be able to finagle it such that the compiler's expansion into these details is allowed, but manually writing them down is disallowed.

@brendanzab
Copy link
Member

We were using named format arguments in gl-rs before we moved to a syntax extension, and found them helpful (to reduce argument repeats, and clarity). That may or may not be a useful data point though...

@alexcrichton
Copy link
Member Author

I've updated the RFC to allow expr = expr in Rust's grammar and just accept the possibility that the macro's default argument syntax may be different than Rust's in the future. It's likely that the macro could adopt the new syntax as well as supporting the ident = expr syntax.

@aturon
Copy link
Member

aturon commented Oct 27, 2014

@alexcrichton

Can you add some rationale about FormatError?

Can do, I've added a few words here. Do they look ok to you?

It's better, but I guess the whole thing is just a bit confusing because in the end you expect the write! macro to hook into io::Writer instead (and hence give you genuine IO errors). I suppose if we manage to move the reader/writer traits into core, this would be improved?

Is there any way we can remove the inherent methods on Formatter?

Do you think that we should only expose Formatter as "an instance of Writer"? Currently there are only two methods, both of which are used as helper methods for respecting the formatting flags requested as part of a format. I would personally think that these are ok, but I'm curious what your motivation is for moving them away.

Ah, I think I just meant the write/write_fmt methods that are meant to shadow the trait methods. In general, there just seems to be a fair amount of sneakiness/complexity around different notions of writing and errors, and I'm wondering how much of this is due to the facade and might be cleaned up if we're able to move traits around?

In any case, I agree in general that by marking most of this as experimental and not showing it in the docs, we leave plenty of room for improving later; I'm fine with the macros as the primary public API, for sure. We just have to make sure that the various lints work out properly in this case (i.e., so that you can use these macros without getting spurious warnings, etc.) I think it's fine now, but as we fix up gaps in the lints wrt macros this might become a problem later.

@alexcrichton
Copy link
Member Author

I suppose if we manage to move the reader/writer traits into core, this would be improved?

The only way I can think of to currently manage this is to have the error type of a writer be an associated type, in which case this is an obvious step to take (I think).

Specifically about FormatError though, I don't think it will change regardless of whether reader/writer is in core or not as I'm not sure it would be too beneficial to parameterize the entire formatting module on the error type returned (we'd want to lock to one and force everyone to use it). This is largely because a Formatter contains a trait object.

I'm wondering how much of this is due to the facade and might be cleaned up if we're able to move traits around?

Yeah this was introduced with the facade, and moving the traits to core would help a lot in this case. This was one of the largest pain points in creating the facade and I think that std::fmt suffered the most sadly :(.

In any case, I agree in general that by marking most of this as experimental

Yeah the precise feature gating story will play out over time, but my goal is to hide as much of this as possible as it doesn't feel right from a stability standpoint (but format! on the other hand does feel right to me).

@brson
Copy link
Contributor

brson commented Nov 12, 2014

Accepted. Discussion. Tracking.

@brson brson merged commit 07a9739 into rust-lang:master Nov 12, 2014

This RFC proposes removing the following traits:

* `Signed`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example in the liblog that currently uses the ability to format as a String or a Signed: https://github.com/rust-lang/rust/blob/master/src/liblog/lib.rs#L234-L249

This allows someone to show the log level as the string INFO or integer 3.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's definitely true that you can use {} and {:d} to differentiate ways to format the same structure today, but from what I've seen it's quite rare. I've also started seeing a number of precedents for functions like path.display() where some anonymous struct is returned that only implements one of the formatting traits so you can basically always use {} and not have to worry much about it.

@Centril Centril added the A-fmt Proposals relating to std::fmt and formatting macros. label Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-fmt Proposals relating to std::fmt and formatting macros.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants