chrono
chrono copied to clipboard
chronotope
fix format panic problem
Add some modification to format::inner_format(), which will prevent the program from panic when an inproper format string is provided. This is the problem mentioned in issue #575 .
The new strategy I'm using is to skip those inproper format substrings. You can find some of the examples in the test I add. I think this strategy is better than directly panic, especially when handling user input strings. Skipping the wrong parts will also indicate an invalid format string, and giving the programmer an opportunity to fix the problem while running, instead of panic and terminating the program.
This unfortunately causes errors to become silent.
A better solution would be to introduce a format_opt method that correctly returns a Result<DelayedFormat, FormatError>.
If invalid, I would imagine this would simply produce "%{whatever}", not "whatever}"
That sounds reasonable. I'll work on it.
By the way, I think the idea to introduce a new method that returns
Result<DelayedFormat, FormatError>, which is mentioned before, is also an
attractive solution to this problem. I can also give a try on it.
Maybe create another function that returns a Result?
I roughly created a try_format function for this purpose. However, I find that there's many format functions for each structure like DateTime, Date, etc. Adding such a function will get a lot of files involved, since we're adding it to every structure that implements format function. I think create a new PR for this task is better choice.
What's the path here? Is there some outer API here that's not propagating the error from the inner API, but panicking instead?
Is there some outer API here that's not propagating the error from the inner API, but panicking instead?
Yes, it's the panic problem. The original format function panics when passed in an inproper format string, making it hard to "format" a user-input string that's possibly inproper.
Yeah, I don't think this is an improvement. If we're going to stop panicking, we should explicitly signal an error some other way.
Just to add another option here, and I recognize this isn't super user friendly or discoverable, (we can potentially fix that with a few helper functions or more docs) but the potentially invalid user input problem can be solved in the currently released chrono, using StrftimeItems::new() and the write! macro depending on the task.
If you want to just check whether the format string is valid or not, using StrftimeItems suffices, however if you want to format without panicking, using write! instead of to_string or format! allows catching this (playground link).
use chrono::{Utc, format::{Item, strftime::StrftimeItems}}; // 0.4.19
use std::fmt::Write;
const BAD_FMT_STR: &str = "%Y-%m-%";
const GOOD_FMT_STR: &str = "%Y-%m-%d";
fn main() {
assert!(StrftimeItems::new(BAD_FMT_STR).any(|i| matches!(i, Item::Error)));
assert!(StrftimeItems::new(GOOD_FMT_STR).all(|i| !matches!(i, Item::Error)));
let mut out = String::new();
assert!(write!(&mut out, "{}", Utc::now().date().naive_local().format(GOOD_FMT_STR)).is_ok());
assert!(write!(&mut out, "{}", Utc::now().date().naive_local().format(BAD_FMT_STR)).is_err());
}
I've added a draft PR which shows some options here: https://github.com/chronotope/chrono/pull/735
@retrhelo Thank you for your work here! We are going in a different direction to fix this panic, see https://github.com/chronotope/chrono/issues/1127.
