Derive Parser with runtime context

[c3potheds]

Motivating use case: I wanted to use clap_derive to parse option strings like "5 days" or "tomorrow" into chrono::DateTime<Utc> instances. I wrote a function that can take such a string and return a Result<DateTime<Utc>, Box<dyn Error>>, which would work as a value_parser for the fields in my struct deriving Parser:

#[derive(Debug, PartialEq, Eq, Parser, Default)]
struct Due {
    /// The due date to assign or query.
    ///
    /// This is a human-readable description of a date or time, like "1 day" or
    /// "5pm".
    #[arg(long, value_parser = parse_due_date)]
    pub due: Option<DateTime<Utc>>,
}

fn parse_due_date(
    s: &str,
) -> Result<DateTime<Utc>, Box<dyn Error + Send + Sync>> {
    todo!()
}

Fundamentally, the DateTime representation of an option string like "tomorrow" is context-dependent, and will be different for different values of the "current time". No worries, I can use Utc::now() in the body of parse_due_date() to get the current time.

The problem is that I want to write unit tests for parsing, but if I use e.g. Utc::now() to get the current time, the "current time" will be different every time I run the tests and I can't write assertions that I get a particular output without controlling the input. To write deterministic tests that call try_parse_from() on example argument vectors, I need to somehow find a way to hardcode the "current" time in the value parser for the due field.

Here are the options I've found:

• Don't unit test parsing
  • I can write tests for parse_due_date by extracting a parse_due_date_impl(now: DateTime<Utc>, s: &str) function that takes the current time
  • Even if I forego unit tests just for parsing, even integration tests/end-to-end tests that execute the full command will depend on the system time. Other components of the application use an injectable "current time" parameter that are unit-testable with hard-coded times which let me get deterministic tests, but because the Due::parse() method is static, I can't inject anything at parse time.
  • Bottom line: I would have to remove a lot of tests and rule out many kinds of tests, which is very unfortunate
• Use a mutable lazy_static of a Mutex<Option<DateTime<Utc>>> that defaults to None, and have a public static current_time() function that returns the result of Utc::now() when the static variable is None and return the wrapped value if it's Some. In cfg(test), there can be a test utility function like override_current_time_for_testing(now: DateTime<Utc>) that tests can call to inject a hard-coded time, and then any value parsers in my clap_derive structs that use my mockable current_time() function will be able to be deterministic as long as the override function is called before parsing.
  • I can probably also have the override_current_time_for_testing() function return a handle that resets the override on Drop so the state of tests don't leak into each other. But there's still potential issues around overriding twice to be careful of; for my purposes it might be enough to just panic on a double override and print a readable error message since it is only available in tests.
  • This works but I'm a little sad that I have to use code smells like global mutable variables and that there is a hidden contract that implies nondeterminism unless some other function has been called first. I much prefer to explicitly inject the context that I need rather than rely on impure static functions.
  • This is probably what I'll do in the meantime, biting the bullet that I need a lazy static mutable variable and doing what I can to
• Stop using clap_derive and manually create Command parsers.
  • I should be able to more easily insert TypedValueParser implementors that carry context like the current time into a function that uses the clap builder API.
  • But I would be very sad to lose the elegance of clap_derive and express the command line options so elegantly with Rust structs and enums
• Add some way to pass context at parse time to clap_derive.
  • The idea would be that instead of/in addition to from_arg_matches(), we could have a way to additionally pass some context object of a user-supplied type that can be forwarded to TypedValueParsers used by any args, from_arg_matches_with_context<C>(context: C, matches: &ArgMatches).
  • This seems like it would need a TypedValueParserWithContext<C> trait to live alongside TypedValueParser and CommandFactoryWithContext<C> trait to live alongside CommandFactory. Then we'd also need a ParserWithContext<C> trait to provide a try_parse_from_with_context<I, T>(context: C, itr: I) where I: IntoIterator<Item = T>, T: Into<OsString> + Clone and other equivalents from Parser's API.
  • This is a lot of API surface area overhead to add for such a small feature
  • As far as I can tell, there is no current way to do this, and adding it would be a nontrivial change to clap and clap_derive.
  • Since we can pass custom, non-static TypedValueParser instances without a problem in the clap builder API, this functionality would only be necessary in clap_derive.

With the last option, I'd be able to rewrite my example as something like:

#[derive(Debug, PartialEq, Eq, ParserWithContext<DateTime<Utc>>, Default)]
struct Due {
    /// The due date to assign or query.
    ///
    /// This is a human-readable description of a date or time, like "1 day" or
    /// "5pm".
    #[arg(long, value_parser_with_context = parse_due_date)]
    pub due: Option<DateTime<Utc>>,
}

fn parse_due_date(
    now: DateTime<Utc>,
    s: &str,
) -> Result<DateTime<Utc>, Box<dyn Error + Send + Sync>> {
    todo!()
}

(I actually don't know if it's syntactically valid to derive(ParserWithContext<C>) with the angle brackets and everything... if not, maybe something could be added to the #[command] attribute, like #[command(context = DateTime<Utc>)])

Right now, I'm simply parsing the due dates as strings and doing validation in a later step, but it would be nice to move errors to the help messages that are created by clap, so I'm contemplating using the lazy static overridable "current time" and biting the bullet on the brittleness that introduces between hidden requirements, potentially leaky global mutable state, test/prod behavior divergence, etc. If I could be convinced that using clap's builder API instead of clap_derive would be beneficial for other reasons, I may choose to do that instead, in which case I could easily supply a TypedValueParser that captures the runtime context I need to preserve.

But in the mean time, having done all this investigation, I wanted to solicit discussion on whether adding some kind of WithContext variants to clap, at least when the clap_derive feature is enabled, might be sufficiently useful to others. I can think of other use cases, like having values from config files or environment variables determine how command line arguments are parsed, or testing localization. What do you all think?

[epage]

Don't unit test parsing

I can write tests for parse_due_date by extracting a parse_due_date_impl(now: DateTime, s: &str) function that takes the current time

I wouldn't quite phrase that as "don't unit test parsing". You are separating the integration logic from the unit and testing the unit. This is the approach I would recommend.

Even if I forego unit tests just for parsing, even integration tests/end-to-end tests that execute the full command will depend on the system time. Other components of the application use an injectable "current time" parameter that are unit-testable with hard-coded times which let me get deterministic tests, but because the Due::parse() method is static, I can't inject anything at parse time.

If you are doing end-to-end tests, how can you inject the current time? If you are using an env variable, can't you have your field parser read that variable as well?

Add some way to pass context at parse time to clap_derive.

This is quite an invasive change with limited application. This is also adding a lot more "magic" in the derive, rather than delegating to the builder API, which we've been moving more towards.

Right now, I'm simply parsing the due dates as strings and doing validation in a later step, but it would be nice to move errors to the help messages that are created by clap,

I assume there isn't an intermediate where it gets parsed as a relative time and then gets turned into an absolute time later?