fix(expr)!: reject regex constructs outside the Python/Rust intersection - #337
Conversation
721df6c to
8a84994
Compare
…in regex
Two more constructs outside the spec's Python/Rust intersection dialect
(2.2.5) slipped past the AST portability walk because they are erased by
AST->HIR translation:
- Inline flags: (?U) swap greed and (?R) CRLF mode are Rust-only (Python
rejects them as unknown flags), bare global negation (?-...) has no
Python equivalent, and negated Unicode mode -u silently changes
\w / \d / . semantics. The shared flags i, m, s, x (and positive u)
remain allowed, both bare and scoped, including scoped negation
(?-i:...).
- Word boundaries: \b{start}, \b{end}, \b{start-half}, \b{end-half},
\< and \> are Rust-only spellings that Python reads as \b plus literal
characters (or a bad escape), diverging silently.
Both are now rejected in check_ast_portability, and the HIR walker
rejects the WordStart*/WordEnd* Look variants as a belt-and-braces
backstop instead of allow-listing them.
Addresses review feedback on OpenJobDescription#337.
…in regex
Two more constructs outside the spec's Python/Rust intersection dialect
(2.2.5) slipped past the AST portability walk because they are erased by
AST->HIR translation:
- Inline flags: (?U) swap greed and (?R) CRLF mode are Rust-only (Python
rejects them as unknown flags), bare global negation (?-...) has no
Python equivalent, and negated Unicode mode -u silently changes
\w / \d / . semantics. The shared flags i, m, s, x (and positive u)
remain allowed, both bare and scoped, including scoped negation
(?-i:...).
- Word boundaries: \b{start}, \b{end}, \b{start-half}, \b{end-half},
\< and \> are Rust-only spellings that Python reads as \b plus literal
characters (or a bad escape), diverging silently.
Both are now rejected in check_ast_portability, and the HIR walker
rejects the WordStart*/WordEnd* Look variants as a belt-and-braces
backstop instead of allow-listing them.
Addresses review feedback on OpenJobDescription#337.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
6d2f7fb to
36b7371
Compare
…in regex
Two more constructs outside the spec's Python/Rust intersection dialect
(2.2.5) slipped past the AST portability walk because they are erased by
AST->HIR translation:
- Inline flags: (?U) swap greed and (?R) CRLF mode are Rust-only (Python
rejects them as unknown flags), bare global negation (?-...) has no
Python equivalent, and negated Unicode mode -u silently changes
\w / \d / . semantics. The shared flags i, m, s, x (and positive u)
remain allowed, both bare and scoped, including scoped negation
(?-i:...).
- Word boundaries: \b{start}, \b{end}, \b{start-half}, \b{end-half},
\< and \> are Rust-only spellings that Python reads as \b plus literal
characters (or a bad escape), diverging silently.
Both are now rejected in check_ast_portability, and the HIR walker
rejects the WordStart*/WordEnd* Look variants as a belt-and-braces
backstop instead of allow-listing them.
Addresses review feedback on OpenJobDescription#337.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Round 2 of review feedback on the Python/Rust intersection dialect guard (spec 2.2.5): - Verbose mode + character classes: Rust strips unescaped whitespace and # comments inside bracketed classes under (?x); Python's VERBOSE mode explicitly keeps them as literals, so (?x)[a b] matches differently with no error on either side. Rejected by scanning class source spans when any positive x flag is present (scope is not tracked, so this can over-reject a class outside an (?x:...) group, which is safe). - Bare inline flag position: Python 3.11+ rejects global flags that are not at the start of the pattern, and 3.6-3.10 applied them to the whole pattern where Rust applies them forward-only. Bare (?flags) groups must now form a contiguous run from offset 0; consecutive leading groups like (?i)(?s)ab stay allowed. - Capture group names: regex_syntax permits '.', '[', ']' in names (non-first position); Python raises 'bad character in group name'. Names must now be valid Python identifiers. Also documents the known accepted divergence for plain \$ (Python matches before a trailing newline, Rust is end-of-haystack only) in the check_hir_portability doc comment and the spec. Addresses review feedback on OpenJobDescription#337. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
36b7371 to
bb4a93f
Compare
leongdl
left a comment
There was a problem hiding this comment.
Reviewed at tip bb4a93f with the full suite green locally (3609 passed, 0 failed) and a mutation harness over the new rejections: 10/10 suppressed-rejection mutants were caught by named tests, including a combined mutant that disabled both the AST rejection and the HIR backstop for \b{start} — the defense-in-depth is pinned end to end. The AST-level approach is the right one, and the visitor held up under adversarial reading: the byte-offset span slicing in the verbose-class scan is char-boundary-safe, the escape-parity scan handles [\\ ] vs [\ ] correctly, and finish() runs post-traversal so uses_verbose can only over-reject, never under-reject.
One line flagged below where the validator and Python still draw different lines — low severity because the failure mode is loud, but it's the same defect class this PR fixes for ./[/] in group names.
Two nits, no action needed: the bare-flag contiguity check over-rejects (?x) (?i)ab (both engines accept it — verbose mode makes the separating space ignorable), which is the safe direction; and there's no test for # inside a verbose-mode class or for scoped (?R:...), though the flag families are otherwise covered.
One genuinely spec-level item, not this PR's to fix: \w/\d/\s set membership differs at the margins (Python's \w matches ² and ½; Rust's does not). The spelling is shared, so no AST check can see it — same family as the documented bare-$ divergence. Worth a line in the spec discussion alongside RFC 0006 §2.2.5 if there isn't one already.
…in regex
Two more constructs outside the spec's Python/Rust intersection dialect
(2.2.5) slipped past the AST portability walk because they are erased by
AST->HIR translation:
- Inline flags: (?U) swap greed and (?R) CRLF mode are Rust-only (Python
rejects them as unknown flags), bare global negation (?-...) has no
Python equivalent, and negated Unicode mode -u silently changes
\w / \d / . semantics. The shared flags i, m, s, x (and positive u)
remain allowed, both bare and scoped, including scoped negation
(?-i:...).
- Word boundaries: \b{start}, \b{end}, \b{start-half}, \b{end-half},
\< and \> are Rust-only spellings that Python reads as \b plus literal
characters (or a bad escape), diverging silently.
Both are now rejected in check_ast_portability, and the HIR walker
rejects the WordStart*/WordEnd* Look variants as a belt-and-braces
backstop instead of allow-listing them.
Addresses review feedback on OpenJobDescription#337.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Round 2 of review feedback on the Python/Rust intersection dialect guard (spec 2.2.5): - Verbose mode + character classes: Rust strips unescaped whitespace and # comments inside bracketed classes under (?x); Python's VERBOSE mode explicitly keeps them as literals, so (?x)[a b] matches differently with no error on either side. Rejected by scanning class source spans when any positive x flag is present (scope is not tracked, so this can over-reject a class outside an (?x:...) group, which is safe). - Bare inline flag position: Python 3.11+ rejects global flags that are not at the start of the pattern, and 3.6-3.10 applied them to the whole pattern where Rust applies them forward-only. Bare (?flags) groups must now form a contiguous run from offset 0; consecutive leading groups like (?i)(?s)ab stay allowed. - Capture group names: regex_syntax permits '.', '[', ']' in names (non-first position); Python raises 'bad character in group name'. Names must now be valid Python identifiers. Also documents the known accepted divergence for plain \$ (Python matches before a trailing newline, Rust is end-of-haystack only) in the check_hir_portability doc comment and the spec. Addresses review feedback on OpenJobDescription#337. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
bb4a93f to
d9f2f72
Compare
The regex dialect is specified as the intersection of Python's re module
and Rust's regex crate, but four Rust-only constructs were accepted:
Unicode property classes (\p{...}), the (?<name>...) capture group
spelling, POSIX character classes ([[:alpha:]]), and character class set
difference (--). The last two silently match differently in engines that
parse them as ordinary bracket expressions.
Pattern validation previously inspected only the HIR, which erases these
syntax distinctions. It now walks the pattern AST first (via
regex_syntax::ast::Visitor), rejecting the four reported constructs plus
the rest of the Rust-only class-set family: intersection (&&), symmetric
difference (~~), and nested character classes ([a[b]]). The AST is then
translated to HIR for the existing belt-and-braces walk, avoiding a
second parse.
Fixes OpenJobDescription#310
BREAKING CHANGE: regex patterns using \p{...}/\P{...}, (?<name>...),
POSIX character classes ([[:alpha:]]), character class set operators
(--, &&, ~~), or nested character classes ([a[b]]) are now rejected
with an 'Unsupported regex feature' error instead of being accepted.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
…in regex
Two more constructs outside the spec's Python/Rust intersection dialect
(2.2.5) slipped past the AST portability walk because they are erased by
AST->HIR translation:
- Inline flags: (?U) swap greed and (?R) CRLF mode are Rust-only (Python
rejects them as unknown flags), bare global negation (?-...) has no
Python equivalent, and negated Unicode mode -u silently changes
\w / \d / . semantics. The shared flags i, m, s, x (and positive u)
remain allowed, both bare and scoped, including scoped negation
(?-i:...).
- Word boundaries: \b{start}, \b{end}, \b{start-half}, \b{end-half},
\< and \> are Rust-only spellings that Python reads as \b plus literal
characters (or a bad escape), diverging silently.
Both are now rejected in check_ast_portability, and the HIR walker
rejects the WordStart*/WordEnd* Look variants as a belt-and-braces
backstop instead of allow-listing them.
Addresses review feedback on OpenJobDescription#337.
Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
Round 2 of review feedback on the Python/Rust intersection dialect guard (spec 2.2.5): - Verbose mode + character classes: Rust strips unescaped whitespace and # comments inside bracketed classes under (?x); Python's VERBOSE mode explicitly keeps them as literals, so (?x)[a b] matches differently with no error on either side. Rejected by scanning class source spans when any positive x flag is present (scope is not tracked, so this can over-reject a class outside an (?x:...) group, which is safe). - Bare inline flag position: Python 3.11+ rejects global flags that are not at the start of the pattern, and 3.6-3.10 applied them to the whole pattern where Rust applies them forward-only. Bare (?flags) groups must now form a contiguous run from offset 0; consecutive leading groups like (?i)(?s)ab stay allowed. - Capture group names: regex_syntax permits '.', '[', ']' in names (non-first position); Python raises 'bad character in group name'. Names must now be valid Python identifiers. Also documents the known accepted divergence for plain \$ (Python matches before a trailing newline, Rust is end-of-haystack only) in the check_hir_portability doc comment and the spec. Addresses review feedback on OpenJobDescription#337. Signed-off-by: Mark <399551+mwiebe@users.noreply.github.com>
d9f2f72 to
0cd3700
Compare
Fixes: #310
What was the problem/requirement? (What/Why)
Background: OpenJD job template expressions can use regular expressions to
match text — for example
re_search('hello123', r'\d+')finds the digits in astring. The same template might be run by this Rust implementation today and
the Python one tomorrow, so a regex has to mean exactly the same thing
everywhere. The trouble is that Python's regex engine and Rust's regex engine
each have their own special features the other doesn't have. RFC 0006 (and
§2.2.5 of the Expression Language spec) solves this by saying: only the
features that both engines share are allowed — the intersection. Anything
outside it must be an error, so a template author finds out immediately instead
of getting a pattern that silently behaves differently on another
implementation.
Issue #310 reported four Rust-only constructs that slipped through:
\p{Nd}— Unicode property classes (Python rejects these outright)(?<name>...)— Rust's named-group spelling (Python requires(?P<name>...))[[:alpha:]]— POSIX classes (Python reads this as an ordinary bracketexpression that matches different characters — no error, just wrong results)
[a-z--[aeiou]]— Rust's--set-difference operator (same silent-mismatchproblem)
The last two are the worst kind of bug: no error anywhere, just a template
that quietly matches different strings depending on which implementation runs it.
Why it slipped through: pattern validation parsed the regex into
regex_syntax's HIR (a normalized internal form). By that point thedistinctions are already erased —
(?<n>a)and(?P<n>a)look identical,and
[[:alpha:]]has already been expanded into a plain character class.What was the solution? (How)
Validation now walks the pattern's AST (the un-normalized parse tree,
via
regex_syntax::ast::Visitor), where all of these constructs are stillvisible, and rejects them with the existing
Unsupported regex feature: ...error style. The AST is then translated toHIR for the pre-existing checks, so the pattern is still parsed only once.
Beyond the four reported constructs, this also rejects the rest of Rust's
class-set extension family —
&&(intersection),~~(symmetricdifference), and nested classes like
[a[b]]— because they have theidentical silent-mismatch problem in Python and
--is just one member ofthat family.
What is the impact of this change?
Patterns using these Rust-only constructs now produce a clear
"Unsupported regex feature" error instead of being accepted, matching the
spec's dialect. Portable patterns are unaffected.
How was this change tested?
expression, caret), per the repo's test quality standard, plus an
acceptance test that
(?P<name>...)still works.openjd-exprsuite: 3,602 tests pass.cargo test --workspacepasses (two pre-existing Windows cross-userfailures reproduce identically on
main; environmental).cargo clippy -D warnings,cargo fmt, andcargo docall clean.Was this change documented?
Yes — the regex validation section of
specs/expr/function-library.mdnowdescribes the AST-level dialect enforcement and lists the rejected
constructs. New code has doc comments.
Is this a breaking change?
Yes — the commit is marked
fix(expr)!with aBREAKING CHANGEfooter.Regex patterns using
\p{...}/\P{...},(?<name>...), POSIX characterclasses (
[[:alpha:]]), character class set operators (--,&&,~~),or nested character classes (
[a[b]]) were previously accepted and are nowrejected with an "Unsupported regex feature" error.
To update: rewrite affected patterns using portable equivalents — e.g.
\dor explicit ranges instead of\p{Nd},(?P<name>...)instead of(?<name>...),[a-zA-Z]instead of[[:alpha:]], and an explicitclass like
[b-df-hj-np-tv-z]instead of[a-z--[aeiou]]. Theserewrites are also what the spec requires for the template to work on
other conformant implementations.
Does this change impact security?
No.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.