fix(ebnf): emit token references for parseable/custom productions so railroad diagrams render - #465
Conversation
Parseable and custom productions have no declarable structure, but EBNF was emitting an undeclared uppercase production name for them (e.g. 'Custom'), and the railroad diagram generator then crashed with a nil pointer dereference on the missing production entry. - Emit '<lowercasedname>' token references for *parseable and *custom nodes, consistent with the EBNF invariant of lowercase = lexer token. - Fix value-receiver implementations of the Parseable interface, which previously panicked with 'reflect: Elem of invalid type' in parseType. - Guard against nil production lookups in cmd/railroad's countProductions and generate so third-party EBNF with unresolved references renders instead of panicking. Fixes alecthomas#444.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c50ad3ac2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if p, ok := productions[n.Name]; ok { | ||
| p.refs++ | ||
| size++ | ||
| } |
There was a problem hiding this comment.
Count unresolved references toward production size
When a production consists solely of an unresolved reference, such as Root = Missing ., this branch leaves its computed size at zero. Because mergeSizeThreshold is zero, the post-pass then sets Root.refs to -1, and generate skips the entire production, producing HTML with no diagram instead of the intended unresolved NonTerminal. Increment size for every named term while only guarding the referenced production's refs update.
Useful? React with 👍 / 👎.
Fixes #444.
Problem
For grammars using the
Parseableinterface (e.g. langx's operator-precedence approach),parser.String()produced EBNF referencing undeclared uppercase productions, andcmd/railroadcrashed with a nil pointer dereference on the missing production entry:Fix
ebnf.go:*parseableand*customnodes now emit token references (<custom>) instead of a bare undeclared type name (Custom), consistent with the EBNF invariant (uppercase = production, lowercase = lexer token). These are not real lexer tokens at runtime, but they are the only renderable representation of an externally-parsed production.grammar.go: value-receiver implementations ofParseable(e.g.func (Modifier) Parse(...)) panicked withreflect: Elem of invalid typeinparseTypebecauset.Elem()was unconditionally called on a non-pointer type. Now handled uniformly.cmd/railroad/main.go: nil-guardedproductions[n.Name]lookups in bothcountProductionsandgenerate, so third-party EBNF with unresolved references renders instead of panicking.Example
Before this PR these EBNF lines would crash
cmd/railroad; now they render:One test assertion updated (
TestParserWithCustomProduction), and one EBNF test added using a value-receiverParseable(TestEBNF_Parseable) covering both bugs.Note this is a breaking change to
parser.String()output for grammars with parseable/custom productions.