Repository navigation
fix(ebnf): round-trip negation and lookahead assertions - #481
Open
MaxFreedomPollard wants to merge 1 commit into
Open
MaxFreedomPollard wants to merge 1 commit into
MaxFreedomPollard wants to merge 1 commit into
Conversation
The ebnf package parses the EBNF that Parser.String() produces, and its
only test asserts that parsing then printing is the identity. Two forms
did not survive that round trip.
Term.Negation is captured by the `@("~")?` tag at ebnf/ebnf.go:31, but
Term.String() (ebnf/ebnf.go:43) never emitted it, so `~"a"` printed as
`"a"`. That inverts the meaning of the production: the README defines
`~<expr>` as matching any token that is not the start of the expression.
cmd/railroad/main.go:130 already reads Term.Negation, so Term.String()
was the one consumer of the field that dropped it.
SubExpression.String() (ebnf/ebnf.go:92) emitted `(?="a")`, while the
printer it is meant to mirror emits `(?= "a")` with a space after the
assertion (ebnf.go:147 and ebnf.go:149).
Neither showed up in TestEBNF because the ebnf package's own grammar uses
no negation and no lookahead group.
Term.String() now prefixes "~" when Negation is set, and
SubExpression.String() emits the space. The new table-driven test covers
both, including verbatim the Parser.String() output that TestEBNF_Other
asserts in the root package.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ebnf.ParseStringparses the EBNF thatParser.String()emits, and the package's only test asserts that parsing and reprinting is the identity. Two forms do not survive that round trip.Term.Negationis captured by the@("~")?tag onTerm(ebnf/ebnf.go:31), butTerm.String()(ebnf/ebnf.go:43) never emits it, so~"a"prints back as"a". That flips the meaning of the production: the README describes~<expr>as matching any token that is not the start of the expression.cmd/railroad/main.go:130already readsTerm.Negation, soTerm.String()was the one consumer of the field that dropped it.SubExpression.String()(ebnf/ebnf.go:92) prints(?="a"), but the printer it mirrors emits(?= "a")with a space after the assertion (ebnf.go:147 and ebnf.go:149), so lookahead groups do not round trip either.Neither shows up in
TestEBNFbecause the ebnf package's own grammar uses no negation and no lookahead group. Feeding back the string thatTestEBNF_Otherin the root package asserts makes both visible at once:Term.String()now prefixes~whenNegationis set, andSubExpression.String()emits the space. The new table-driven test round-trips negated literals, tokens, productions, groups and repetitions, both lookahead assertions, and thatTestEBNF_Otherstring verbatim. All 8 subtests fail on master and pass with the change.Verified on go1.26.4 darwin/arm64 and golangci-lint 2.12.2 with the repo's config: