fix(parser): return instead of throwing on an unterminated scope#55
Open
svyatov wants to merge 1 commit into
Open
fix(parser): return instead of throwing on an unterminated scope#55svyatov wants to merge 1 commit into
svyatov wants to merge 1 commit into
Conversation
A body line that begins with a word followed by "(" reaches scope() through
footer() and token(). A second "(" before the closing ")" is a syntax error
there, and scope() was the one sub-parser that threw rather than returning the
Error, so the throw escaped message() and the whole commit failed to parse.
Both callers already handle an Error return. summary() treats it as "no scope",
token() skips the node. Return the error so they get the chance, and rewind past
the consumed "(" as well, since abort() rewinds only to the scope node, which
starts one character later.
summary() re-raises this specific error, because the grammar excludes parens
from a scope, so an unclosed "(" in a summary really is a syntax error. Existing
error messages and positions are unchanged.
Closes conventional-commits#54
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
1 participant
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.
Put
z.array(z.boolean())at the start of a commit body line and the whole commit fails to parse. Downstream, release-please catches the throw, drops the commit from the changelog, and finishes green, which is how it found me: one of our fixes went missing from two consecutive release PRs before anyone noticed. @codyoss hit the same thing on google-cloud-go, reported in #54.What happens
A body line gets offered to
footer()first, by way ofpreFooter().token()readsz.arrayas a type, sees the(, and hands off toscope(), which consumesz.boolean, hits the second(, and finds no)where the grammar wants one.Every other sub-parser in
lib/parser.jsreturns anErrorthere and lets its caller decide.scope()is the only one that throws, so the throw escapesmessage()and takes the commit with it.Both callers were already written for a returned error.
summary()doesif (s instanceof Error) s = null, andtoken()doesif (!(s instanceof Error)) node.children.push(s). They never get asked.The change
scope()returns the error instead of throwing. That alone is not enough:abort()rewinds only as far as the scope node, which starts one character after the(was consumed, so the fix records the position before consuming the(and rewinds there too. Otherwise the caller resumes mid-token.With the error returned,
token()skips the scope,footer()fails at its separator,preFooter()backtracks, andbody()reads the line as plain text. Footers after such a line still parse.What deliberately did not change
A summary like
fix(a(b)): xstill throws, same message, same position. The grammar saysso an unclosed paren in a summary really is a syntax error, while the same characters in a body line are only text.
scope()tags this error andsummary()re-raises it, which keeps those two answers apart.The existing
throws error when closing ")" token is missingtest therefore passes untouched, along with the rest of the suite and every snapshot. Since #54 lists the summary case among its failures, to be clear about what this PR covers: the body half is a bug, the summary half is the grammar working as written.Tests
Five, added under
<body>, using plain assertions sotest/parser.js.snapstays untouched. Four of them fail without the parser change. The fifth pins the summary behavior above and passes either way, on purpose.One is the case from #48, open since March 2024 with a failing test and no fix. I lifted its assertions, credit to @Otard95, and this closes that one too.
Locally the suite goes from 34 passing to 39,
standardstays clean, andnpm run coverageclears the.nycrcthresholds (branch coverage 93.0 to 95.3). The patch is aconst, a method call, and a property assignment, so nothing in it needs a syntax floor above the Node 10 that CI still builds.Closes #54, closes #48.