Fix TypeError on unclosed [, ( and trailing | - #330
Open
theRizwan wants to merge 1 commit into
Open
Conversation
Selectors that ran out of tokens before a bracket closed threw
`TypeError: Cannot read properties of undefined` instead of the parser's
own `Expected a closing ...` error. The error path already existed in each
case; it just could not be reached, because building the message
dereferenced the token that was missing.
parser().astSync('a]') // Expected an opening square bracket. (ok)
parser().astSync('a[href') // TypeError: ...reading '0'
Closing delimiters with no opener were already handled properly, so this
brings the two directions into line.
- attribute(): the while loop exits on either a closing bracket or end of
input, and the check after it assumed the former. Errors now point at the
opening bracket.
- namespace(): a trailing `|` with nothing after it now reaches the
existing unexpectedPipe(), which reports against currToken.
- parentheses(): the unbalanced branch falls back to the opening token.
No behaviour change for input that already parsed, and no existing error
message changes.
Tests assert the message rather than the type. exceptions.mjs already
covered this input shape via `throws("unclosed attribute selector", ...)`,
which passed throughout: `throws` falls back to `{instanceOf: Error}` when
no message is given, and TypeError satisfies that.
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.
Fixes #329.
Selectors that ran out of tokens before a bracket closed threw
TypeError: Cannot read properties of undefinedinstead of the parser's ownExpected a closing …error. The error path already existed in each case — it just couldn't bereached, because constructing the message dereferenced the token that was missing.
a[hrefTypeError: …reading '0'Expected a closing square bracket.a[href=xTypeError: …reading '0'Expected a closing square bracket.a(TypeError: …reading '5'Expected a closing parenthesis..foo|TypeError: …reading '0'Unexpected '|'.Closing delimiters with no opener (
a],a)) already produced clean errors, so this brings thetwo directions into line.
Changes
Three guards in
src/parser.js:attribute()— thewhileloop exits on either a closing bracket or end of input, and thecheck after it assumed the former. Errors now point at the opening bracket, which is where the
author needs to look.
namespace()— a trailing|with nothing after it now goes to the existingunexpectedPipe(), which reports againstcurrToken(the pipe itself, always present).parentheses()— theunbalancedbranch falls back to the opening parenthesis when thestream ended.
No behaviour change for input that already parsed, and no change to any existing error message.
Tests
Four cases added to
src/__tests__/exceptions.mjs.They assert the message rather than the type, deliberately.
src/__tests__/exceptions.mjs:8already covers this input shape:
That test passes today, because
throwsfalls back to{ instanceOf: Error }when no message isgiven (
util/helpers.mjs:33) andTypeErrorsatisfies it. Asserting the message is what makesthe new tests able to fail.
Confirmed they do: reverting only
src/parser.jsand re-running givesVerification
npm test— 781/781 pass (777 before),oxlintclean, coverage thresholds metnpm run format:checkcleanpostcss-selector-parser@7.1.4from npm and againstmainat4a7e4e3Scope
I've limited this to the three paths I could reproduce. There are other unguarded token reads in
the file that may be unreachable in practice — I'd rather not add speculative guards to a parser
this widely used without a failing case to justify each one.