False positive on \newenvironment

[paaguti]

This LaTeX fragment flags an errir

\newcounter{milestone}
\newenvironment{milestone}[1][]{%
  \refstepcounter{milestone}\par\medskip
  \setlength\parindent{-18pt}
  \textbf{\sffamily \mstext~\themilestone:}\label{ms:ms\themilestone} #1
}{\medskip\setlength\parindent{0pt}%
}

with a missing }' on \themilestone and an unexpected {`on the line afterwards.

[nolanking90]

@pfoerster when the parser hits a label it assumes there is either a key or a command and not both, so it calls the curly_group_word() method. I tested changing this behavior to check for a key, a command, or a key followed by a command.
If we use this method instead of calling curly_group_word() it no longer gives a false positive diagnostic (and it passes all the unit tests).

    fn label_definition(&mut self) {
        self.builder.start_node(LABEL_DEFINITION.into());
        self.eat();
        self.trivia();

        if self.lexer.peek() == Some(Token::LBrack) {
            self.brack_group();
        }

        if self.lexer.peek() == Some(Token::LCurly) {
            self.builder.start_node(CURLY_GROUP_WORD.into());
            self.eat();
            self.trivia();

            if self.peek() == Some(Token::Word) || self.peek() == Some(Token::Pipe) {
                self.key();
            }

            match self.peek() {
                Some(Token::CommandName(_)) => {
                    self.content(ParserContext::default());
                }
                Some(_) | None => {}
            }

            self.expect(Token::RCurly);
            self.builder.finish_node();
        }

        self.builder.finish_node();
    }

I'm basically just inlining the curly_group_word() method, and pulling the case of a key out of the match. This could have been two if statements instead of an if and a match, but I wasn't sure how to rewrite the CommandName case using an if. I'm open to suggestions here if you think there is a better way to do it.

This makes "ms:ms" a label key in this case and gives a warning about an unused label. At first, I thought this was "correct but annoying" and wondered if I could fix that too. After thinking some more about the use case I think getting an annoying warning here is a feature not a bug.

Labels and counters really shouldn't be used this way. If you change the number or order of these environments, the counters all change, causing the labels to change. So then you have to manually change all the references to fix it. This is exactly what labels and references are supposed to be automating for you. So, maybe using the method above, which eliminates the false syntax error but always produces a warning about the label, is the best solutions.

The more reasonable use case of passing in a label as an argument to the environment does not give this unused label warning for me.

\newcounter{milestone}
\newenvironment{milestone}[1]{%
  \refstepcounter{milestone}\par\medskip
  \setlength\parindent{-18pt}
  \textbf{\sffamily \mstext~\themilestone:}%
  \label{ms:ms#1}
}%
{\medskip\setlength\parindent{0pt}}

In either case, without some macro expansion, the actual instances of the labels aren't going to be reflected in the tree, so they will be missed by any diagnostics that involve checking the labels (e.g. a reference to one of these will throw a missing reference error/warning, I assume).

Let me know what you think. I can draft a PR if you think it's worth doing.

[pfoerster]

Let me know what you think. I can draft a PR if you think it's worth doing.

@nolanking90 Thank you for looking into this. Looks good to me, would definitely appreciate a PR :)

This could have been two if statements instead of an if and a match, but I wasn't sure how to rewrite the CommandName case using an if.

Something like this should work:

if let Some(Token::CommandName(_)) = self.peek() {
    self.content(ParserContext::default());
}