refactor(commands): clearer token match typing, make tree.possible_commads return iterator instead of traversing the whole tree immediately

crates/commands/src/commands.rs

@@ -18,7 +18,7 @@ pub mod server_config;
 pub mod switch;
 pub mod system;
 
-use std::fmt::Display;
+use std::fmt::{Debug, Display};
 
 use smol_str::SmolStr;
 
@@ -27,7 +27,7 @@ use crate::{
     token::{ToToken, Token},
 };
 
-#[derive(Clone, Debug)]
+#[derive(Debug, Clone)]
 pub struct Command {
     // TODO: fix hygiene
     pub tokens: Vec<Token>,

crates/commands/src/lib.rs

@@ -1,4 +1,5 @@
 #![feature(let_chains)]
+#![feature(anonymous_lifetime_in_impl_trait)]
 
 pub mod commands;
 mod string;
@@ -10,8 +11,9 @@ uniffi::include_scaffolding!("commands");
 use core::panic;
 use std::collections::HashMap;
 use std::fmt::Write;
+use std::ops::Not;
 
-use smol_str::{format_smolstr, SmolStr};
+use smol_str::SmolStr;
 use tree::TreeBranch;
 
 pub use commands::Command;
@@ -69,10 +71,10 @@ pub fn parse_command(prefix: String, input: String) -> CommandResult {
     loop {
         let possible_tokens = local_tree.possible_tokens().cloned().collect::<Vec<_>>();
         println!("possible: {:?}", possible_tokens);
-        let next = next_token(possible_tokens.clone(), &prefix, input.clone(), current_pos);
+        let next = next_token(possible_tokens.clone(), input.clone(), current_pos);
         println!("next: {:?}", next);
         match next {
-            Ok((found_token, arg, new_pos)) => {
+            Some(Ok((found_token, arg, new_pos))) => {
                 current_pos = new_pos;
                 if let Token::Flag = found_token {
                     flags.insert(arg.unwrap().raw.into(), None);
@@ -94,7 +96,15 @@ pub fn parse_command(prefix: String, input: String) -> CommandResult {
                     panic!("found token could not match tree, at {input}");
                 }
             }
-            Err(None) => {
+            Some(Err((token, err))) => {
+                let error_msg = match err {
+                    TokenMatchError::MissingParameter { name } => {
+                        format!("Expected parameter `{name}` in command `{prefix}{input} {token}`.")
+                    }
+                };
+                return CommandResult::Err { error: error_msg };
+            }
+            None => {
                 if let Some(command) = local_tree.command() {
                     println!("{} {params:?}", command.cb);
                     return CommandResult::Ok {
@@ -109,33 +119,19 @@ pub fn parse_command(prefix: String, input: String) -> CommandResult {
 
                 let mut error = format!("Unknown command `{prefix}{input}`.");
 
-                let possible_commands = local_tree.possible_commands(2);
+                if fmt_possible_commands(&mut error, &prefix, local_tree.possible_commands(2)).not()
-                if !possible_commands.is_empty() {
+                {
-                    error.push_str(" Perhaps you meant to use one of the commands below:\n");
+                    error.push_str(" ");
-                    for command in possible_commands.iter().take(MAX_SUGGESTIONS) {
-                        if !command.show_in_suggestions {
-                            continue;
-                        }
-                        writeln!(&mut error, "- **{prefix}{command}** - *{}*", command.help)
-                            .expect("oom");
-                    }
-                } else {
-                    error.push_str("\n");
                 }
 
                 error.push_str(
-                    "For a list of possible commands, see <https://pluralkit.me/commands>.",
+                    "For a list of all possible commands, see <https://pluralkit.me/commands>.",
                 );
 
                 // todo: check if last token is a common incorrect unquote (multi-member names etc)
                 // todo: check if this is a system name in pk;s command
                 return CommandResult::Err { error };
             }
-            Err(Some(short_circuit)) => {
-                return CommandResult::Err {
-                    error: short_circuit.into(),
-                };
-            }
         }
     }
 }
@@ -143,16 +139,16 @@ pub fn parse_command(prefix: String, input: String) -> CommandResult {
 /// Find the next token from an either raw or partially parsed command string
 ///
 /// Returns:
+/// - nothing (none matched)
 /// - matched token, to move deeper into the tree
 /// - matched value (if this command matched an user-provided value such as a member name)
 /// - end position of matched token
-/// - optionally a short-circuit error
+/// - error when matching
 fn next_token(
     possible_tokens: Vec<Token>,
-    prefix: &str,
     input: SmolStr,
     current_pos: usize,
-) -> Result<(Token, Option<TokenMatchedValue>, usize), Option<SmolStr>> {
+) -> Option<Result<(Token, Option<TokenMatchedValue>, usize), (Token, TokenMatchError)>> {
     // get next parameter, matching quotes
     let param = crate::string::next_param(input.clone(), current_pos);
     println!("matched: {param:?}\n---");
@@ -163,14 +159,14 @@ fn next_token(
     if let Some((value, new_pos)) = param.clone()
         && value.starts_with('-')
     {
-        return Ok((
+        return Some(Ok((
             Token::Flag,
             Some(TokenMatchedValue {
                 raw: value,
                 param: None,
             }),
             new_pos,
-        ));
+        )));
     }
 
     // iterate over tokens and run try_match
@@ -178,17 +174,39 @@ fn next_token(
         // for FullString just send the whole string
         let input_to_match = param.clone().map(|v| v.0);
         match token.try_match(input_to_match) {
-            TokenMatchResult::Match(value) => {
+            Some(Ok(value)) => {
-                return Ok((token, value, param.map(|v| v.1).unwrap_or(current_pos)))
+                return Some(Ok((
-            }
+                    token,
-            TokenMatchResult::MissingParameter { name } => {
+                    value,
-                return Err(Some(format_smolstr!(
+                    param.map(|v| v.1).unwrap_or(current_pos),
-                    "Missing parameter `{name}` in command `{prefix}{input} {token}`."
                 )))
             }
-            TokenMatchResult::NoMatch => {}
+            Some(Err(err)) => {
+                return Some(Err((token, err)));
+            }
+            None => {} // continue matching until we exhaust all tokens
         }
     }
 
-    Err(None)
+    None
+}
+
+// todo: should probably move this somewhere else
+/// returns true if wrote possible commands, false if not
+fn fmt_possible_commands(
+    f: &mut String,
+    prefix: &str,
+    mut possible_commands: impl Iterator<Item = &Command>,
+) -> bool {
+    if let Some(first) = possible_commands.next() {
+        f.push_str(" Perhaps you meant to use one of the commands below:\n");
+        for command in std::iter::once(first).chain(possible_commands.take(MAX_SUGGESTIONS - 1)) {
+            if !command.show_in_suggestions {
+                continue;
+            }
+            writeln!(f, "- **{prefix}{command}** - *{}*", command.help).expect("oom");
+        }
+        return true;
+    }
+    return false;
 }

crates/commands/src/token.rs

@@ -45,20 +45,8 @@ pub enum Token {
     Flag,
 }
 
-// #[macro_export]
-// macro_rules! any {
-//     ($($token:expr),+) => {
-//         Token::Any(vec![$($token.to_token()),+])
-//     };
-// }
-
 #[derive(Debug)]
-pub enum TokenMatchResult {
+pub enum TokenMatchError {
-    /// Token did not match.
-    NoMatch,
-    /// Token matched, optionally with a value.
-    Match(Option<TokenMatchedValue>),
-    /// A required parameter was missing.
     MissingParameter { name: ParamName },
 }
 
@@ -68,33 +56,45 @@ pub struct TokenMatchedValue {
     pub param: Option<(ParamName, Parameter)>,
 }
 
-impl TokenMatchResult {
+impl TokenMatchedValue {
-    fn new_match(raw: impl Into<SmolStr>) -> Self {
+    fn new_match(raw: impl Into<SmolStr>) -> TryMatchResult {
-        Self::Match(Some(TokenMatchedValue {
+        Some(Ok(Some(Self {
             raw: raw.into(),
             param: None,
-        }))
+        })))
     }
 
-    fn new_match_param(raw: impl Into<SmolStr>, param_name: ParamName, param: Parameter) -> Self {
+    fn new_match_param(
-        Self::Match(Some(TokenMatchedValue {
+        raw: impl Into<SmolStr>,
+        param_name: ParamName,
+        param: Parameter,
+    ) -> TryMatchResult {
+        Some(Ok(Some(Self {
             raw: raw.into(),
             param: Some((param_name, param)),
-        }))
+        })))
     }
 }
 
-impl Token {
+/// None -> no match
-    pub fn try_match(&self, input: Option<SmolStr>) -> TokenMatchResult {
+/// Some(Ok(None)) -> match, no value
-        use TokenMatchResult::*;
+/// Some(Ok(Some(_))) -> match, with value
+/// Some(Err(_)) -> error while matching
+// q: why do this while we could have a NoMatch in TokenMatchError?
+// a: because we want to differentiate between no match and match failure (it matched with an error)
+//    "no match" has a different charecteristic because we want to continue matching other tokens...
+//    ...while "match failure" means we should stop matching and return the error
+type TryMatchResult = Option<Result<Option<TokenMatchedValue>, TokenMatchError>>;
 
+impl Token {
+    pub fn try_match(&self, input: Option<SmolStr>) -> TryMatchResult {
         let input = match input {
             Some(input) => input,
             None => {
                 // short circuit on:
                 return match self {
                     // empty token
-                    Self::Empty => Match(None),
+                    Self::Empty => Some(Ok(None)),
                     // missing paramaters
                     Self::FullString(param_name)
                     | Self::MemberRef(param_name)
@@ -104,16 +104,16 @@ impl Token {
                     | Self::Toggle(param_name)
                     | Self::Enable(param_name)
                     | Self::Disable(param_name)
-                    | Self::Reset(param_name) => MissingParameter { name: param_name },
+                    | Self::Reset(param_name) => {
-                    Self::Any(tokens) => {
+                        Some(Err(TokenMatchError::MissingParameter { name: param_name }))
-                        tokens.is_empty().then_some(NoMatch).unwrap_or_else(|| {
-                            let mut results = tokens.iter().map(|t| t.try_match(None));
-                            results.find(|r| !matches!(r, NoMatch)).unwrap_or(NoMatch)
-                        })
                     }
+                    Self::Any(tokens) => tokens.is_empty().then_some(None).unwrap_or_else(|| {
+                        let mut results = tokens.iter().map(|t| t.try_match(None));
+                        results.find(|r| !matches!(r, None)).unwrap_or(None)
+                    }),
                     // everything else doesnt match if no input anyway
-                    Token::Value(_) => NoMatch,
+                    Token::Value(_) => None,
-                    Token::Flag => NoMatch,
+                    Token::Flag => None,
                     // don't add a _ match here!
                 };
             }
@@ -122,31 +122,31 @@ impl Token {
 
         // try actually matching stuff
         match self {
-            Self::Empty => NoMatch,
+            Self::Empty => None,
             Self::Flag => unreachable!(), // matched upstream (dusk: i don't really like this tbh)
             Self::Any(tokens) => tokens
                 .iter()
                 .map(|t| t.try_match(Some(input.into())))
-                .find(|r| !matches!(r, NoMatch))
+                .find(|r| !matches!(r, None))
-                .unwrap_or(NoMatch),
+                .unwrap_or(None),
             Self::Value(values) => values
                 .iter()
                 .any(|v| v.eq(input))
-                .then(|| TokenMatchResult::new_match(input))
+                .then(|| TokenMatchedValue::new_match(input))
-                .unwrap_or(NoMatch),
+                .unwrap_or(None),
-            Self::FullString(param_name) => TokenMatchResult::new_match_param(
+            Self::FullString(param_name) => TokenMatchedValue::new_match_param(
                 input,
                 param_name,
                 Parameter::OpaqueString { raw: input.into() },
             ),
-            Self::SystemRef(param_name) => TokenMatchResult::new_match_param(
+            Self::SystemRef(param_name) => TokenMatchedValue::new_match_param(
                 input,
                 param_name,
                 Parameter::SystemRef {
                     system: input.into(),
                 },
             ),
-            Self::MemberRef(param_name) => TokenMatchResult::new_match_param(
+            Self::MemberRef(param_name) => TokenMatchedValue::new_match_param(
                 input,
                 param_name,
                 Parameter::MemberRef {
@@ -154,55 +154,55 @@ impl Token {
                 },
             ),
             Self::MemberPrivacyTarget(param_name) => match MemberPrivacyTarget::from_str(input) {
-                Ok(target) => TokenMatchResult::new_match_param(
+                Ok(target) => TokenMatchedValue::new_match_param(
                     input,
                     param_name,
                     Parameter::MemberPrivacyTarget {
                         target: target.as_ref().into(),
                     },
                 ),
-                Err(_) => NoMatch,
+                Err(_) => None,
             },
             Self::PrivacyLevel(param_name) => match PrivacyLevel::from_str(input) {
-                Ok(level) => TokenMatchResult::new_match_param(
+                Ok(level) => TokenMatchedValue::new_match_param(
                     input,
                     param_name,
                     Parameter::PrivacyLevel {
                         level: level.as_ref().into(),
                     },
                 ),
-                Err(_) => NoMatch,
+                Err(_) => None,
             },
             Self::Toggle(param_name) => match Enable::from_str(input)
                 .map(Into::<bool>::into)
                 .or_else(|_| Disable::from_str(input).map(Into::<bool>::into))
             {
-                Ok(toggle) => TokenMatchResult::new_match_param(
+                Ok(toggle) => TokenMatchedValue::new_match_param(
                     input,
                     param_name,
                     Parameter::Toggle { toggle },
                 ),
-                Err(_) => NoMatch,
+                Err(_) => None,
             },
             Self::Enable(param_name) => match Enable::from_str(input) {
-                Ok(t) => TokenMatchResult::new_match_param(
+                Ok(t) => TokenMatchedValue::new_match_param(
                     input,
                     param_name,
                     Parameter::Toggle { toggle: t.into() },
                 ),
-                Err(_) => NoMatch,
+                Err(_) => None,
             },
             Self::Disable(param_name) => match Disable::from_str(input) {
-                Ok(t) => TokenMatchResult::new_match_param(
+                Ok(t) => TokenMatchedValue::new_match_param(
                     input,
                     param_name,
                     Parameter::Toggle { toggle: t.into() },
                 ),
-                Err(_) => NoMatch,
+                Err(_) => None,
             },
             Self::Reset(param_name) => match Reset::from_str(input) {
-                Ok(_) => TokenMatchResult::new_match_param(input, param_name, Parameter::Reset),
+                Ok(_) => TokenMatchedValue::new_match_param(input, param_name, Parameter::Reset),
-                Err(_) => NoMatch,
+                Err(_) => None,
             },
             // don't add a _ match here!
         }

crates/commands/src/tree.rs

@@ -24,7 +24,7 @@ impl TreeBranch {
             current_branch = current_branch.branches.entry(token).or_insert(TreeBranch {
                 current_command: None,
                 branches: OrderMap::new(),
-            })
+            });
         }
         // when we're out of tokens, add an Empty branch with the callback and no sub-branches
         current_branch.branches.insert(
@@ -44,20 +44,25 @@ impl TreeBranch {
         self.branches.keys()
     }
 
-    pub fn possible_commands(&self, max_depth: usize) -> Vec<Command> {
+    pub fn possible_commands(&self, max_depth: usize) -> impl Iterator<Item = &Command> {
-        if max_depth == 0 {
+        // dusk: i am too lazy to write an iterator for this without using recursion so we box everything
-            return Vec::new();
+        fn box_iter<'a>(
+            iter: impl Iterator<Item = &'a Command> + 'a,
+        ) -> Box<dyn Iterator<Item = &'a Command> + 'a> {
+            Box::new(iter)
         }
-        let mut commands = Vec::new();
+
-        for token in self.possible_tokens() {
+        if max_depth == 0 {
-            if let Some(tree) = self.get_branch(token) {
+            return box_iter(std::iter::empty());
-                if let Some(command) = tree.command() {
+        }
-                    commands.push(command);
+        let mut commands = box_iter(std::iter::empty());
-                    // we dont need to look further if we found a command
+        for branch in self.branches.values() {
-                    continue;
+            if let Some(command) = branch.current_command.as_ref() {
-                }
+                commands = box_iter(commands.chain(std::iter::once(command)));
-                commands.append(&mut tree.possible_commands(max_depth - 1));
+                // we dont need to look further if we found a command (only Empty tokens have commands)
+                continue;
             }
+            commands = box_iter(commands.chain(branch.possible_commands(max_depth - 1)));
         }
         commands
     }