Flag x: rake: maintain the error status of invalid syntax in edge cases

slevithan · Jul 22, 2024 · da122a4 · da122a4
1 parent a73131e
commit da122a4
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 6 deletions.
diff --git a/spec/flag-x-spec.js b/spec/flag-x-spec.js
@@ -30,9 +30,10 @@ describe('flag x', () => {
       expect('aaa').toMatch(regex({raw: ['^a#comment\n+$']}));
       expect('aaa').toMatch(regex({raw: ['^a  #comment\n +$']}));
       expect(() => regex`a | ?`).toThrow();
+      expect(() => regex` ?`).toThrow();
     });
 
-    it('should not allow quantifiers to follow other quantifiers', function() {
+    it('should not allow quantifiers to follow other complete quantifiers', function() {
       expect(() => regex`a?? ?`).toThrow();
       expect(() => regex`a*? ?`).toThrow();
       expect(() => regex`a+? ?`).toThrow();
@@ -103,8 +104,54 @@ describe('flag x', () => {
       expect(() => regex`(?<n>)\k< n >`).toThrow();
     });
 
-    it('should avoid adding token separators when it is safe to do so', () => {
-      expect(regex` ^ (?! a \s b . c | d [] e ) $ `.source).toBe('^(?!a\\sb.c|d[]e)$');
+    describe('token separator cleanup (rake)', () => {
+      it('should avoid adding token separators when it is safe to do so', () => {
+        expect(regex` ^ (?! a \s b . c | d [] e ) $ `.source).toBe('^(?!a\\sb.c|d[]e)$');
+      });
+
+      it('should not remove (?:) when followed by a quantifier', () => {
+        expect('').toMatch(regex`(?:)?`);
+        expect('').toMatch(regex`^(?:)?$`);
+        expect(':').toMatch(regex`^((?:)?:)$`);
+        expect('=').toMatch(regex`^((?:)?=)$`);
+        expect('!').toMatch(regex`^((?:)?!)$`);
+        expect('DEFINE').toMatch(regex`^((?:)?(DEFINE))$`);
+      });
+
+      it('should maintain the error status of invalid syntax', () => {
+        expect(() => regex`(?(?:):)`).toThrow();
+        expect(() => regex`(?(?:)=)`).toThrow();
+        expect(() => regex`(?(?:)!)`).toThrow();
+        expect(() => regex`(?(?:)i:)`).toThrow();
+        expect(() => regex`(?i(?:):)`).toThrow();
+        expect(() => regex`(?i-(?:):)`).toThrow();
+        expect(() => regex`(?i-m(?:):)`).toThrow();
+        expect(() => regex`(?(?:)-i:)`).toThrow();
+        expect(() => regex`(?-(?:)i:)`).toThrow();
+        expect(() => regex`(?-i(?:):)`).toThrow();
+        expect(() => regex`(?(?:)(DEFINE))`).toThrow();
+        expect(() => regex`(?((?:)DEFINE))`).toThrow();
+        expect(() => regex`(?(DEFINE(?:)))`).toThrow();
+        expect(() => regex`\c(?:)A`).toThrow();
+        expect(() => regex`\x(?:)00`).toThrow();
+        expect(() => regex`\u(?:)0000`).toThrow();
+        expect(() => regex`\p(?:){Letter}`).toThrow();
+        expect(() => regex`\p{(?:)Letter}`).toThrow();
+        expect(() => regex`\p{Letter(?:)}`).toThrow();
+        expect(() => regex`.{(?:)1}`).toThrow();
+        expect(() => regex`.{1(?:)}`).toThrow();
+        expect(() => regex`.{1(?:),2}`).toThrow();
+        expect(() => regex`.{1,(?:)2}`).toThrow();
+        expect(() => regex`(?(?:)<a>)`).toThrow();
+        expect(() => regex`(?<(?:)a>)`).toThrow();
+        expect(() => regex`(?<a(?:)>)`).toThrow();
+        expect(() => regex`(?<a>)\k(?:)<a>`).toThrow();
+        expect(() => regex`(?<a>)\k<(?:)a>`).toThrow();
+        expect(() => regex`(?<a>)\k<a(?:)>`).toThrow();
+        expect(() => regex`(?<a>)\g(?:)<a>`).toThrow();
+        expect(() => regex`(?<a>)\g<(?:)a>`).toThrow();
+        expect(() => regex`(?<a>)\g<a(?:)>`).toThrow();
+      });
     });
   });
 

diff --git a/src/flag-x.js b/src/flag-x.js
@@ -139,11 +139,20 @@ export function rakePostprocessor(expression) {
   // No need for separators at:
   // - The beginning, if not followed by a quantifier.
   // - The end.
-  // - Before one of `()|.[$\`.
-  // - After one of `()|.]^>`, `\[bBdDfnrsStvwW]`, `(?:`, or a lookaround opening.
+  // - Outside of character classes:
+  //   - If followed by one of `)|.[$\`, or `(` if that's not followed by `DEFINE)`.
+  //     - Technically we shouldn't rake if followed by `)` and preceded by `(?(DEFINE`, but in
+  //       this case flag x injects `(?:)` between the preceding invalid `(?` to sandbox the `?`.
+  //   - If preceded by one of `()|.]^>`, `\[bBdDfnrsStvwW]`, `(?:`, or a lookaround opening.
+  //     - So long as the separator is not followed by a quantifier.
+  // Examples of things that are not safe to rake at the boundaries of:
+  // - Anywhere: Letters, numbers, or any of `-=_,<?*+{}`.
+  // - If followed by any of `:!>`.
+  // - If preceded by any of `\[cgkpPux]`.
+  // - Anything inside character classes.
   expression = replaceUnescaped(
     expression,
-    String.raw`^${sep}(?![?*+{])|${sep}$|${sep}(?=[()|.[$\\])|(?<=[()|.\]^>]|\\[bBdDfnrsStvwW]|\(\?(?:[:=!]|<[=!]))${sep}`,
+    String.raw`${sep}(?=[)|.[$\\]|\((?!DEFINE)|$)|(?<=[()|.\]^>]|\\[bBdDfnrsStvwW]|\(\?(?:[:=!]|<[=!])|^)${sep}(?![?*+{])`,
     '',
     Context.DEFAULT
   );