Negative number arguments parsed as short options

[slilly-0xffff]

When a command argument is a signed integer type (i16, i32, etc.) and the user passes a negative value, the leading '-' is parsed as a short option flag rather than part of the number.

Example desktop app:

use embedded_cli::cli::CliBuilder;
use embedded_cli::codes;
use embedded_cli::Command;
use embedded_io::{ErrorType, Write};
use std::convert::Infallible;
use std::io::{stdin, stdout, Stdout, Write as _};
use termion::event::{Event, Key};
use termion::input::TermRead;
use termion::raw::{IntoRawMode, RawTerminal};

use ufmt::uwrite;

/// Simple demo to reproduce the negative number parsing bug in arguments.rs.
/// Try: `add 3 -2`  or  `add -4 5`

#[derive(Debug, Command)]
enum Base {
    /// Add two integers
    Add {
        a: i32, // First operand
        b: i32, // Second operand
    },
}

pub struct Writer {
    stdout: RawTerminal<Stdout>,
}

impl ErrorType for Writer {
    type Error = Infallible;
}

impl Write for Writer {
    fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Error> {
        self.stdout.write_all(buf).unwrap();
        Ok(buf.len())
    }

    fn flush(&mut self) -> Result<(), Self::Error> {
        self.stdout.flush().unwrap();
        Ok(())
    }
}

fn main() {
    let stdout = stdout().into_raw_mode().unwrap();

    let writer = Writer { stdout };

    const CMD_BUFFER_SIZE: usize = 256;
    const HISTORY_BUFFER_SIZE: usize = 512;

    let (command_buffer, history_buffer) = unsafe {
        static mut COMMAND_BUFFER: [u8; CMD_BUFFER_SIZE] = [0; CMD_BUFFER_SIZE];
        static mut HISTORY_BUFFER: [u8; HISTORY_BUFFER_SIZE] = [0; HISTORY_BUFFER_SIZE];
        #[allow(static_mut_refs)]
        (COMMAND_BUFFER.as_mut(), HISTORY_BUFFER.as_mut())
    };

    let mut cli = CliBuilder::default()
        .writer(writer)
        .command_buffer(command_buffer)
        .history_buffer(history_buffer)
        .build()
        .expect("Failed to build CLI");

    let _ = cli.set_prompt("calc> ");

    let stdin = stdin();
    for c in stdin.events() {
        let evt = c.unwrap();
        let bytes = match evt {
            Event::Key(Key::Esc) | Event::Key(Key::Ctrl('c')) => break,
            Event::Key(Key::Up) => vec![codes::ESCAPE, b'[', b'A'],
            Event::Key(Key::Down) => vec![codes::ESCAPE, b'[', b'B'],
            Event::Key(Key::Right) => vec![codes::ESCAPE, b'[', b'C'],
            Event::Key(Key::Left) => vec![codes::ESCAPE, b'[', b'D'],
            Event::Key(Key::BackTab) => vec![codes::TABULATION],
            Event::Key(Key::Backspace) => vec![codes::BACKSPACE],
            Event::Key(Key::Char(c)) => {
                let mut buf = [0; 4];
                c.encode_utf8(&mut buf).as_bytes().to_vec()
            }
            _ => continue,
        };
        for byte in bytes {
            cli.process_byte::<Base, _>(
                byte,
                &mut Base::processor(|cli, command| match command {
                    Base::Add { a, b } => {
                        uwrite!(cli.writer(), "{} + {} = {}", a, b, a + b)
                    }
                }),
            )
            .unwrap();
        }
    }
}

Example session:

calc> add 1 2
1 + 2 = 3
calc> add 2 -3
error: unexpected option: -3
calc> add -4 7
error: unexpected option: -4
calc>

The proposed fix:

diff --git a/embedded-cli/src/arguments.rs b/embedded-cli/src/arguments.rs
index 3c275fe..85be5be 100644
--- a/embedded-cli/src/arguments.rs
+++ b/embedded-cli/src/arguments.rs
@@ -105,6 +105,8 @@ impl<'a> Iterator for ArgsIter<'a> {
                 } else {
                     Arg::LongOption(unsafe { raw.get_unchecked(2..) })
                 }
+            } else if bytes[1].is_ascii_digit() {
+                Arg::Value(raw)
             } else {
                 let (opt, leftover) =
                     unsafe { utils::char_pop_front(raw.get_unchecked(1..)).unwrap_unchecked() };

Example session containing the proposed fix:

calc> add 1 2
1 + 2 = 3
calc> add 2 -3
2 + -3 = -1
calc> add -4 7
-4 + 7 = 3

I can submit a PR with the fix if that would be convenient for you.

[funbiscuit]

This is intended behaviour, look at #19

[slilly-0xffff]

Thanks for the tip. It sounds like you'd prefer to keep the current "greedy" flag detection. My proposed solution would prevent arguments such as "-1" "-2" "-3" etc. from being used as flags. I'll look for a workaround in my application, because I have no need for number flags like this.

I see the "--" is intended to ignore subsequent flags and start parsing positional arguments. To prevent others from stumbling over this behavior, it would be helpful to document it in the project README.md.

[funbiscuit]

Well, it does seem appropriate to not treat -<digits> as flags or options. So if "token" starts with "-" and is followed by digit then we treat it as normal positional arg, not flag or option. If you can submit a PR I'll review it.