fix: include the backslash in [[:punct:]] - #203
Merged
Conversation
The punct table held 31 characters where POSIX defines 32; the backslash was missing, so [[:punct:]] failed to match it. Backslash is a legal character in a POSIX filename, and bash matches it: bash [[:punct:]] vs \ -> true picomatch [[:punct:]] vs \ -> false bash a[[:punct:]]b vs a\b -> true picomatch a[[:punct:]]b vs a\b -> false picomatch also contradicted itself: graph (\x21-\x7E) and print (\x20-\x7E) both match the backslash, and POSIX defines graph as alnum + punct, so a character in graph but in neither of its parts could not be right. The existing punct test exercises ! ? # & @ + * : = | and never a backslash, which is why this went unnoticed. Two assertions embed the class source as an expected regex string and are updated to match, as is the equivalent listed in the README.
The README is generated from .verb.md by verb, but the previous commit only updated the README, so regenerating the docs would have dropped the backslash from the documented [[:punct:]] equivalent again. Also add the negation counterpart of the new assertion: since [[:punct:]] now matches the backslash, [![:punct:]] must not.
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
2 participants
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.
The problem
[[:punct:]]does not match a backslash. The table atlib/constants.js:84holds 31 characters where POSIX defines 32 — the backslash is absent.Backslash is a legal character in a POSIX filename, and bash matches it:
[[:punct:]]\a[[:punct:]]ba\b*[[:punct:]]*x\y[[:punct:]]![[:punct:]]aThe last two rows are controls: the class works, only this one character is missing.
It is also inconsistent with picomatch's own tables
graphis\x21-\x7Eandprintis\x20-\x7E, so both already match the backslash:POSIX defines
graphasalnumpluspunct. A character that is ingraphbut in neither of its two parts cannot be right, so this holds independently of what bash does.Why it was not caught
The
[[:punct:]]test attest/posix-classes.js:265exercises! ? # & @ + * : = |and never a backslash. Nothing asserted the buggy behaviour, so this is an untested gap rather than a deliberate deviation.The change
One character added to the class in
lib/constants.js. Two assertions embed the class source as an expected regex string (test/posix-classes.js:24and:136) and are updated to match, as is the equivalent listed atREADME.md:585and the test's own description.assert(isMatch('\\', '[[:punct:]]'))is added to the existing punct test.Verification
Suite: 1996 passing before, 1996 passing after.
Reverting only
lib/constants.jsand keeping the tests fails 3 — the two expected-regex assertions and the new backslash one — so the added assertion exercises this bug rather than the class in general.Disclosure: this patch was prepared with AI assistance. The bash comparison, the table counts and the red/green check above were executed against this branch; happy to adjust anything on request.