Feature request: Allow manually setting the context ("last_option") in error messages

[jinliu]

Consider the following example:

fn main() -> Result<(), Box<dyn std::error::Error>>{
    use lexopt::prelude::*;

    let mut parser = lexopt::Parser::from_env();
    while let Some(arg) = parser.next()? {
        match arg {
            Long("debug") => {
                println!("Debug mode enabled");
            },
            Value(command) => {
                let arg1 = parser.value()?;
                println!("Command: {}, arg1: {}", command.string()?, arg1.string()?)
            }
            _ => {
                eprintln!("Unknown argument: {:?}", arg);
            }
        }
    }
    Ok(())
}

Running it with a missing command argument:

$ lexopt-test --debug test
Debug mode enabled
Error: missing argument for option '--debug'

The error message is misleading. It should be Error: missing argument for command 'test'.

Perhaps it could be improved in the following ways:

1. Reset last_option on Parser::next().

If the last option requires an argument, the user would use parser.value(), not parser.next().
If the last option has an optional argument, then the user might use parser.next(), but missing that argument would not be an error about the last option, anyway.
So this change only affects the rare case like an option require 0 or 2 arguments, so after seeing the option, the user would first call parser.next(), and if it's a value, call parser.value() again, and the error message should still be about the option.

2. Provide Parser::set_error_message(message_template: &str) and Parser::reset_error_message() to override (and restore) the default error message.

[blyxxyz]

That error message is definitely wrong. Solution 1 might be a good idea.

But parser.value() isn't supposed to be used like this, it's really only for options. I'd like to know more about your use case to figure out if the docs or the API need improvement.

Right now you have this behavior:

$ lexopt-test test --debug
Command: test, arg1: --debug

Is that intentional? I'd expect it to either enable debug mode or complain about an unknown option.

If you do want to just take the next argument no matter what then the current preferred solution would look something like this:

                let arg1 = parser.raw_args()?.next().ok_or_else(|| {
                    format!(
                        "missing argument for subcommand '{}'",
                        command.to_string_lossy()
                    )
                })?;

But I think that for subcommands you'd usually want to call .next() again in a loop so you can gather options. Is that an option here?

[jinliu]

OK. Now I see that parser.raw_args()?.next() makes a lot of sense in my case. The document actually made it clear that parser.value() is "for an option", but my eyes were fixed at the "a value is collected even if it looks like an option" part and missed that.

What I'm trying to do is to replicate xdotool's cmdline syntax. It's like:
xdotool --debug --dry-run search --name konsole windowmove --relative -10 10
I.e., first some global options, then several subcommands (e.g. search and windowmove), each with its own options and arguments.

My parser code is here: kdotool.

It first parses global options in main(), in a loop like:

while let Some(arg) = parser.next()? {
    match (arg) {
        Short('d') | Long("debug") => {...},
        ...
        Value(val) => { break; }
    }
}

And then passes the parser to the subcommand handling code in generate_script(). The problem is I can't stuff the Value(val) back into the parser ("rewind the parser"), and it's a bit ugly passing it all the way into generate_script(). So I write a function that peeks into the raw_args and only consumes options, and replace parser.next() with it:

fn try_parse_option(parser: &mut Parser) -> Option<Arg> {
    let s = parser.try_raw_args()?.peek()?.to_str().unwrap().to_string();
    if s.starts_with('-') {
        let next_char = s.chars().nth(1)?;
        if next_char.is_ascii_alphabetic() || next_char == '-' {
            return parser.next().unwrap();
        }
    }
    None
}

Maybe something like Parser::try_option() or Parser::rewind()?

And the next problem is in windowmove --relative -10 10 subcommand, where -10 is parsed into an option. Fixed by the is_ascii_alphabetic() test in code above.

Now the only problem left is that error messages should have the subcommand name as context. E.g. "windowmove: missing argument". Now I see it can be done by wrapping Parser into a MyParser struct, and wrapping Parser::next() etc. into corresponding a method which adds my context to Error's. No change needed in lexopt.

[blyxxyz]

it's a bit ugly passing it all the way into generate_script()

Ah, I see now. The chained subcommand style is unusual, I've never thought about that one.

Your try_parse_option() has unfortunate side effects:

$ kdotool -dn
Error: invalid option '-n'

try_raw_args() returns None when it's inbetween d and n, so it thinks it ran out of options.

Rewinding is sort of possible by cloning the parser, but that copies every individual remaining string and quickly gets you into O(n²) territory, so I wouldn't advise it.

There's one more wrinkle, which is --. That tells lexopt (and getopt) to treat all the upcoming arguments like positional arguments instead of options. It's actually the only way I can find to make xdotool accept negative horizontal offsets: windowmove --relative -- -100 -100. xdotool forgets about this state when it reaches the next subcommand (which is a good thing, otherwise you wouldn't be able to use options on any later subcommands). To replicate this with lexopt each subcommand needs to have a new Parser.

I've found a pattern that should be able to handle all of this rigorously. It's boilerplate-y, but not too bad I think. Every subcommand has to watch out for an unexpected positional argument, and when it sees it it stores it for later and breaks out of its parsing loop. (Maybe that's what you were doing before? It does seem necessary to do it this way.) We reset the parser by constructing a new one out of parser.raw_args(), which isn't free but shouldn't be overly expensive.

where -10 is parsed into an option

Negative numbers come up regularly, but they tend to come up in slightly varying ways, so it's hard to invent a one-size-fits-all solution. See #18.

For your case I think it's best to have a signature-identical wrapper around parser.next() that outputs Arg::Value for negative numbers, just like when there's no leading dash. See next_maybe_num in the upcoming example.

Putting it all together:

use std::ffi::OsString;

fn try_get_number(parser: &mut lexopt::Parser) -> Option<OsString> {
    let mut raw = parser.try_raw_args()?;
    let arg = raw.peek()?.to_str()?;
    if arg.starts_with('-') && arg[1..].starts_with(|c: char| c.is_ascii_digit()) {
        raw.next()
    } else {
        None
    }
}

fn next_maybe_num(parser: &mut lexopt::Parser) -> Result<Option<lexopt::Arg>, lexopt::Error> {
    if let Some(number) = try_get_number(parser) {
        Ok(Some(lexopt::Arg::Value(number)))
    } else {
        parser.next()
    }
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
    use lexopt::prelude::*;

    let mut subcommand = None;

    let mut parser = lexopt::Parser::from_env();
    while let Some(arg) = parser.next()? {
        match arg {
            Long("debug") => {
                println!("Debug mode enabled");
            }
            Value(value) if subcommand.is_none() => {
                subcommand = Some(value.string()?);
                break;
            }
            _ => return Err(arg.unexpected().into()),
        }
    }

    if subcommand.is_none() {
        return Err("missing subcommand".into());
    }
    generate_script(subcommand, parser)?;

    Ok(())
}

fn generate_script(
    mut next_subcommand: Option<String>,
    mut parser: lexopt::Parser,
) -> Result<(), Box<dyn std::error::Error>> {
    use lexopt::prelude::*;

    while let Some(subcommand) = next_subcommand.take() {
        parser = lexopt::Parser::from_args(parser.raw_args()?);

        match subcommand.as_str() {
            "search" => {
                #[derive(Debug)]
                enum SearchType {
                    Name,
                    Class,
                }
                let mut search_type = None;
                let mut pattern = None;

                while let Some(arg) = parser.next()? {
                    match arg {
                        Long("name") => search_type = Some(SearchType::Name),
                        Long("class") => search_type = Some(SearchType::Class),
                        Value(value) if pattern.is_none() => pattern = Some(value.string()?),
                        Value(value) => {
                            next_subcommand = Some(value.string()?);
                            break;
                        }
                        _ => return Err(arg.unexpected().into()),
                    }
                }

                let search_type = search_type.ok_or("missing search type")?;
                let pattern = pattern.ok_or("missing pattern")?;

                println!("search {search_type:?} {pattern:?}");
            }
            "windowmove" => {
                let mut relative = false;
                let mut x: Option<i64> = None;
                let mut y: Option<i64> = None;

                while let Some(arg) = next_maybe_num(&mut parser)? {
                    match arg {
                        Long("relative") => relative = true,
                        Value(value) if x.is_none() => x = Some(value.parse()?),
                        Value(value) if y.is_none() => y = Some(value.parse()?),
                        Value(value) => {
                            next_subcommand = Some(value.string()?);
                            break;
                        }
                        _ => return Err(arg.unexpected().into()),
                    }
                }

                let x = x.ok_or("missing argument x")?;
                let y = y.ok_or("missing argument y")?;

                println!("windowmove [relative={relative}] {x} {y}");
            }
            "selectwindow" => {
                if let Some(arg) = parser.next()? {
                    match arg {
                        Value(value) => next_subcommand = Some(value.string()?),
                        _ => return Err(arg.unexpected().into()),
                    }
                }

                println!("selectwindow");
            }
            _ => return Err(format!("unknown subcommand '{subcommand}'").into()),
        }
    }

    Ok(())
}

Is this flexible enough for all your cases?

[jinliu]

Thanks so much for the help! Yes it works.

It didn't occur to me that multiple optional positional args can be parsed in the Value(value) if x.is_none() => style. And compared to breaking out of the parser.next() loop and calling parser.raw_args()?.next() manually, it has the benefit that options and positional args can be mixed.

However, it still feels somewhat more natural to fetch these args procedurally, instead of in a loop, esp. for required args. So I added a wrapper for that, anyway. Combined with adding subcommand name to the error messages, I ended up with:

pub struct Error {
    inner: anyhow::Error,
    context: Option<String>,
}

impl Error {
    pub fn new<T: Into<anyhow::Error>>(err: T, context: &Option<String>) -> Error {
        Error {
            inner: err.into(),
            context: context.clone(),
        }
    }
}

impl std::fmt::Display for Error {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        match &self.context {
            Some(context) => write!(f, "{context}: {}", self.inner),
            None => write!(f, "{}", self.inner),
        }
    }
}

impl std::fmt::Debug for Error {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        std::fmt::Display::fmt(self, f)
    }
}

impl std::error::Error for Error {}

fn map_err<T, E: std::error::Error + Send + Sync + 'static>(
    e: Result<T, E>,
    context: &Option<String>,
) -> Result<T, Error> {
    e.map_err(|e| Error::new(e, context))
}

pub struct Parser {
    inner: lexopt::Parser,
    context: Option<String>,
}

impl Parser {
    pub fn from_env() -> Parser {
        Parser {
            inner: lexopt::Parser::from_env(),
            context: None,
        }
    }

    pub fn context(&self) -> Option<&str> {
        self.context.as_deref()
    }

    // Reset the parser at the current position, but with a new context.
    pub fn reset(&mut self, context: &str) -> Result<(), Error> {
        self.inner = lexopt::Parser::from_args(self.raw_args()?);
        self.context = Some(context.into());
        Ok(())
    }

    pub fn raw_args(&mut self) -> Result<lexopt::RawArgs, Error> {
        // self.inner
        //     .raw_args()
        //     .map_err(|e| Error::new(e, &self.context))
        map_err(self.inner.raw_args(), &self.context)
    }

    pub fn next(&mut self) -> Result<Option<lexopt::Arg>, Error> {
        map_err(self.inner.next(), &self.context)
    }

    pub fn value(&mut self) -> Result<String, Error> {
        match self.inner.value() {
            Ok(os_string) => Ok(os_string.to_string_lossy().into()),
            Err(e) => Err(Error::new(e, &self.context)),
        }
    }

    pub fn next_maybe_num(&mut self) -> Result<Option<lexopt::Arg>, Error> {
        if let Some(number) = self.try_get_number() {
            Ok(Some(lexopt::Arg::Value(number.into())))
        } else {
            self.next()
        }
    }

    pub fn try_get_number(&mut self) -> Option<String> {
        let mut raw = self.inner.try_raw_args()?;
        let arg = raw.peek()?.to_str()?;
        if arg.starts_with('-') && arg[1..].starts_with(|c: char| c.is_ascii_digit()) {
            raw.next()
                .map(|os_string| os_string.to_string_lossy().into())
        } else {
            None
        }
    }

    pub fn positional<T: std::str::FromStr>(&mut self, name: &str) -> Result<T, Error>
    where
        T::Err: std::error::Error + Send + Sync + 'static,
    {
        if let Some(os_string) = self.raw_args()?.next() {
            map_err(os_string.to_string_lossy().parse::<T>(), &self.context)
        } else {
            Err(Error::new(
                anyhow::Error::msg(format!("missing positional argument '{name}'")),
                &self.context,
            ))
        }
    }
}

Now I see that in the user program's cmdline parsing code, there are various errors not from the parser (e.g. parse() errors in the positional() above), so the adding context to error messages task should really be done in the user code, not in parser, to handle all these errors. So I'm closing this feature request. And thanks again for your kind help!

[blyxxyz]

Great! This is a really interesting case, thanks for showing me.