Arg borrowing from Parser is inconvenient

[blyxxyz]

When creating new abstractions on top of lexopt (like subcommands) it's annoying that Arg borrows from Parser. It means you can't call .value() or pass the parser to another function until you've unpacked the Arg.

The borrowing is necessary to be able to match on Arg::Long in an ergonomic way. You can match a &str against a string literal but not a String. So the Parser owns the string and the Arg just borrows it.

If it ever becomes possible to match a String against a string literal then it might be worth releasing a new backward-incompatible version of lexopt to take advantage of that.

In the meantime there's an ugly trick you can use to decouple the Arg from the Parser:

fn unleash_arg<'a, 'b>(
    storage: &'a mut Option<String>,
    arg: lexopt::Arg<'b>,
) -> lexopt::Arg<'a> {
    use lexopt::Arg::*;
    match arg {
        Long(opt) => {
            *storage = Some(opt.to_owned());
            Long(storage.as_deref().unwrap())
        }
        Short(opt) => Short(opt),
        Value(val) => Value(val),
    }
}

let mut storage = None;
let arg = unleash_arg(&mut storage, arg);

unleash_arg could be made into a method on Arg, but there might be a better solution.

(See also: #3)

[BurntSushi]

Just brainstorming, could you turn Arg into a type that doesn't borrow from the parser? And then for matching, you could define a method on Arg that returns an ArgRef which gives you the &str you want to match. It's an extra type and an extra method call, which is kind of a bummer, but it does get rid of the pesky lifetime attachment to Parser.

[blyxxyz]

One downside would be that you can't take ownership of the OsString in the Arg::Value case, at least without doing something very weird.

That could mean a lot of unnecessary copies. But arguments are already arguably-unnecessarily copied at least once by std::env::args_os, so maybe one more time isn't prohibitive.

It could also remove access to OsString::into_string, and OsStr::to_str returns an Option that we can't simply convert to a lexopt::Error. But then that conversion is questionable to begin with because into_string doesn't use a proper error type, and we already have ValueExt which could offer a new method to replace it. (Come to think of it, maybe ValueExt should be extending OsStr instead of OsString, since both of its methods take &self.)

---

A similar change could be to do that but make Arg still borrow from Parser by containing a full &mut Parser. It could offer its own .value() method and/or a method to extract the reference, which means passing just the Arg around could be enough for abstract programming.

If this is taken far enough then perhaps all error messages could take full advantage of what Parser knows, like the name of the command.

I tried this at some point and ran into borrow checker issues in user code. I reproduced it (or something like it) by giving ArgRef a non-trivial drop:

struct Parser;

struct Arg<'a>(&'a mut Parser);

struct ArgRef<'a>(&'a str);

impl Drop for ArgRef<'_> {
    fn drop(&mut self) {}
}

impl Parser {
    fn next(&mut self) -> Arg {
        Arg(self)
    }
}

impl Arg<'_> {
    fn getref(&self) -> ArgRef {
        ArgRef("foo")
    }

    fn value(&mut self) -> String {
        "bar".to_owned()
    }
}

fn main() {
    let mut parser = Parser;
    let mut arg = parser.next();
    match arg.getref() {
        _ => println!("{}", arg.value()),
    }
}

This code compiles only if you remove the Drop impl.

I don't know if this would come up, and maybe jiggling who owns what could fix it, but it's something to watch out for.

---

I think a lot of it depends on how commonly the lifetime is an issue. If 95% of code has no problems with the current system then it's ok if the solution for the other 5% is ugly, as long as it exists.

[BurntSushi]

I forgot about this issue, ran into this exact problem, and came up with my own work-around that looks pretty similar to what you have in the OP, but isn't its own function:

        // We do this little dance to disentangle the lifetime of 'p' from the
        // lifetime on 'arg'. The cost is that we have to clone all long flag
        // names to give it a place to live that isn't tied to 'p'. Annoying,
        // but not the end of the world.
        let long_flag: Option<String> = match arg {
            Arg::Long(name) => Some(name.to_string()),
            _ => None,
        };
        let mut arg = match long_flag {
            Some(ref flag) => Arg::Long(flag),
            None => match arg {
                Arg::Short(c) => Arg::Short(c),
                Arg::Long(_) => unreachable!(),
                Arg::Value(value) => Arg::Value(value),
            },
        };

And indeed, I ran into exactly this issue where I have a bunch of different sub-command where I try to compose a bunch of different flags.

(p in the comment above is a variable of type lexopt::Parser.)

[blyxxyz]

Here's another solution:

enum OwnedArg {
    Short(char),
    Long(String),
    Value(OsString),
}

impl From<Arg<'_>> for OwnedArg {
    fn from(arg: Arg<'_>) -> OwnedArg {
        match arg {
            Arg::Short(opt) => OwnedArg::Short(opt),
            Arg::Long(opt) => OwnedArg::Long(opt.to_owned()),
            Arg::Value(val) => OwnedArg::Value(val),
        }
    }
}

Matching becomes problematic. You can use OwnedArg::Long(o) if o == "option", but not if there's a short form:

error[E0408]: variable `o` is not bound in all patterns
   --> src/lib.rs:155:9
    |
155 |         OwnedArg::Short('v') | OwnedArg::Long(o) if o == "verbose" => (),
    |         ^^^^^^^^^^^^^^^^^^^^                  - variable not in all patterns
    |         |
    |         pattern doesn't bind `o`

Maybe this:

impl OwnedArg {
    fn is_short(&self, option: char) -> bool {
        match self {
            OwnedArg::Short(opt) => *opt == option,
            _ => false,
        }
    }

    fn is_long(&self, option: &str) -> bool {
        match self {
            OwnedArg::Long(opt) => opt == option,
            _ => false,
        }
    }
}

if arg.is_short('v') || arg.is_long("verbose") {
    // ...
}

But that makes it tedious to adapt existing code that operates on an Arg.

---

I notice there's been movement on deref patterns. This now compiles on nightly!

#![feature(string_deref_patterns)]

match arg {
    OwnedArg::Short('v') | OwnedArg::Long("verbose") => (),
    _ => (),
}

It doesn't look ready for stabilization, but hopefully at some point Arg::Long can contain a String and sidestep the problem.

[BurntSushi]

Oh wow, the deref patterns looks great. I think that would indeed be a very nice solution here.