Skip to content

fix(ebnf): emit token references for parseable/custom productions so railroad diagrams render - #465

Open
Pastalikek65 wants to merge 1 commit into
alecthomas:masterfrom
Pastalikek65:fix-ebnf-parseable
Open

fix(ebnf): emit token references for parseable/custom productions so railroad diagrams render#465
Pastalikek65 wants to merge 1 commit into
alecthomas:masterfrom
Pastalikek65:fix-ebnf-parseable

Conversation

@Pastalikek65

Copy link
Copy Markdown
Contributor

Fixes #444.

Problem

For grammars using the Parseable interface (e.g. langx's operator-precedence approach), parser.String() produced EBNF referencing undeclared uppercase productions, and cmd/railroad crashed with a nil pointer dereference on the missing production entry:

panic: runtime error: invalid memory address or nil pointer dereference
main.countProductions ... (productions[n.Name].refs++ on a missing key)

Fix

  • ebnf.go: *parseable and *custom nodes 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 of Parseable (e.g. func (Modifier) Parse(...)) panicked with reflect: Elem of invalid type in parseType because t.Elem() was unconditionally called on a non-pointer type. Now handled uniformly.
  • cmd/railroad/main.go: nil-guarded productions[n.Name] lookups in both countProductions and generate, 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:

Root = <modifier> Body .
Body = <ident> <custom> .

One test assertion updated (TestParserWithCustomProduction), and one EBNF test added using a value-receiver Parseable (TestEBNF_Parseable) covering both bugs.

Note this is a breaking change to parser.String() output for grammars with parseable/custom productions.

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.
@alecthomas

Copy link
Copy Markdown
Owner

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread cmd/railroad/main.go
Comment on lines +170 to +173
if p, ok := productions[n.Name]; ok {
p.refs++
size++
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

EBNF for Parsable interface

2 participants