Refactoring parser to operator kind

[kisaragi]

(First of all, I feel I couldn't give proper title with this. Please feel free to change to or purpose better title)

---

I'm refactoring my code; it gets operator kind from token-sequence, as below:

Before refactoring

pub(in self) trait FromParser: Sized {
    type Err;
    
    fn read(parser: &Parser) -> Result<Self, Self::Err>;
}

#[derive(Copy, Clone)]
enum RelationCheckExpressionOps {
    Less,
    LessEqual,
    More,
    MoreEqual,
    Spaceship,
}

impl FromParser for RelationCheckExpressionOps {
    type Err = anyhow::Error;

    fn read(parser: &Parser) -> Result<Self, Self::Err> {
        match parser.lexer.peek() {
            Token::SymLess => {
                parser.lexer.next();
                Ok(Self::Less)
            }
            Token::PartLessEq => {
                parser.lexer.next();
                Ok(Self::LessEqual)
            }
            Token::SymMore => {
                parser.lexer.next();
                Ok(Self::More)
            }
            Token::PartMoreEq => {
                parser.lexer.next();
                Ok(Self::MoreEqual)
            }
            Token::PartLessEqMore => {
                parser.lexer.next();
                Ok(Self::Spaceship)
            }
            other => anyhow::bail!("excess token: {token:?}", token = other)
        }
    }
}

// And so on, bunch of same operations for
// different operator precedence group (Additive, Multitive, etc.)

where parser.lexer.next() consumes current token and parser.lexer.peek() does not.

My current approach for refactoring is defining macros that match on match-clause-like arms, and generate whole match-clause on their count:

After refactoring

macro_rules! operator_from_parser {
    ($name:ty, $token1:ident => $variant1:ident) => {
        impl FromParser for $name {
            type Err = anyhow::Error;

            fn read(parser: &Parser) -> Result<Self, Self::Err> {
                match parser.lexer.peek(){
                    Token::$token1 => {
                        parser.lexer.next();
                        Ok(Self::$variant1)
                    },
                    other => excess_token!(other)
                }
            }
        }
    };
    ($name:ty, $token1:ident => $variant1:ident, $token2:ident => $variant2:ident) => {
        impl FromParser for $name {
            type Err = anyhow::Error;

            fn read(parser: &Parser) -> Result<Self, Self::Err> {
                match parser.lexer.peek() {
                    Token::$token1 => {
                        parser.lexer.next();
                        Ok(Self::$variant1)
                    },
                    Token::$token2 => {
                        parser.lexer.next();
                        Ok(Self::$variant2)
                    },
                    other => excess_token!(other)
                }
            }
        }
    };
    ($name:ty, $token1:ident => $variant1:ident, $token2:ident => $variant2:ident, $token3:ident => $variant3:ident) => {
        impl FromParser for $name {
            type Err = anyhow::Error;

            fn read(parser: &Parser) -> Result<Self, Self::Err> {
                match parser.lexer.peek() {
                    Token::$token1 => {
                        parser.lexer.next();
                        Ok(Self::$variant1)
                    },
                    Token::$token2 => {
                        parser.lexer.next();
                        Ok(Self::$variant2)
                    },
                    Token::$token3 => {
                        parser.lexer.next();
                        Ok(Self::$variant3)
                    },
                    other => excess_token!(other)
                }
            }
        }
    };
    ($name:ty, $token1:ident => $variant1:ident, $token2:ident => $variant2:ident, $token3:ident => $variant3:ident, $token4:ident => $variant4:ident) => {
        impl FromParser for $name {
            type Err = anyhow::Error;

            fn read(parser: &Parser) -> Result<Self, Self::Err> {
                match parser.lexer.peek(){
                    Token::$token1 => {
                        parser.lexer.next();
                        Ok(Self::$variant1)
                    },
                    Token::$token2 => {
                        parser.lexer.next();
                        Ok(Self::$variant2)
                    },
                    Token::$token3 => {
                        parser.lexer.next();
                        Ok(Self::$variant3)
                    },
                    Token::$token4 => {
                        parser.lexer.next();
                        Ok(Self::$variant4)
                    },
                    other => excess_token!(other)
                }
            }
        }
    };
    ($name:ty, $token1:ident => $variant1:ident, $token2:ident => $variant2:ident, $token3:ident => $variant3:ident, $token4:ident => $variant4:ident, $token5:ident => $variant5:ident) => {
        impl FromParser for $name {
            type Err = anyhow::Error;

            fn read(parser: &Parser) -> Result<Self, Self::Err> {
                match parser.lexer.peek() {
                    Token::$token1 => {
                        parser.lexer.next();
                        Ok(Self::$variant1)
                    },
                    Token::$token2 => {
                        parser.lexer.next();
                        Ok(Self::$variant2)
                    },
                    Token::$token3 => {
                        parser.lexer.next();
                        Ok(Self::$variant3)
                    },
                    Token::$token4 => {
                        parser.lexer.next();
                        Ok(Self::$variant4)
                    },
                    Token::$token5 => {
                        parser.lexer.next();
                        Ok(Self::$variant5)
                    },
                    other => excess_token!(other)
                }
            }
        }
    };
}

#[derive(Copy, Clone)]
enum RelationCheckExpressionOps {
    Less,
    LessEqual,
    More,
    MoreEqual,
    Spaceship,
}

operator_from_parser!(RelationCheckExpressionOps, SymLess => Less, PartLessEq => LessEqual, SymMore => More, PartMoreEq => MoreEqual, PartLessEqMore => Spaceship);

I'd like to ask: can they be more elegant? It's ugly to repeating arm-matching.
What I've tried:

• $($token:ident => $variant:ident,)+ in operator_from_parser, but macros cannot be expanded in match-clause arms. I feel it does not go well.

[H2CO3]

@kisaragi

but macros cannot be expanded in match-clause arms

They totally can; this seems to work just fine for me?

macro_rules! operator_from_parser {
    ($name:ty, $($token:ident => $variant:ident,)+) => {
        impl FromParser for $name {
            type Err = anyhow::Error;
        
            fn read(parser: &Parser) -> Result<Self, Self::Err> {
                let op = match parser.lexer.peek() {
                    $(Token::$token => Self::$variant,)+
                    other => anyhow::bail!("excess token: {token:?}", token = other)
                };
                
                parser.lexer.next();
                Ok(op)
            }
        }
    }
}

[kisaragi]

Thank you for excellent solution! I've marked for it.

this seems to work just fine for me?

Sure, I hadn't thought that far... though.

[Michael-F-Bryan]

My advice would be to a) ask yourself whether you could make things cleaner by restructuring your code^[1] to avoid the boilerplate, and b) ask yourself whether this code will be read/modified frequently?

Most of the time, macros will have a negative effect on your code's maintainability and it's better to copy/paste once than than add more cognitive load to whoever is troubleshooting dodgy FromParser implementations 2 months from now. While macro use-sites tend to be clean because you have control over the syntax, you first need to understand what the macro is doing, which often requires you to read an indecipherable macro definition.

---

1. e.g. by using a lookup table or some fn from_token(t: Token) -> Option<Self> constructor. ↩︎