Merge pull request #2468 from max-schaefer/js/regexp-predecessor

Approved by asgerf

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -25,3 +25,4 @@
 
 ## Changes to libraries
 
+* The predicates `RegExpTerm.getSuccessor` and `RegExpTerm.getPredecessor` have been changed to reflect textual, not operational, matching order. This only makes a difference in lookbehind assertions, which are operationally matched backwards. Previously, `getSuccessor` would mimick this, so in an assertion `(?<=ab)` the term `b` would be considered the predecessor, not the successor, of `a`. Textually, however, `a` is still matched before `b`, and this is the order we now follow.

javascript/ql/src/semmle/javascript/Regexp.qll

@@ -73,21 +73,21 @@ class RegExpTerm extends Locatable, @regexpterm {
   /** Holds if this regular expression term can match the empty string. */
   predicate isNullable() { none() } // Overridden in subclasses.
 
-  /** Gets the regular expression term that is matched before this one, if any. */
+  /** Gets the regular expression term that is matched (textually) before this one, if any. */
   RegExpTerm getPredecessor() {
     exists(RegExpSequence seq, int i |
       seq.getChild(i) = this and
-      seq.getChild(i - getDirection()) = result
+      seq.getChild(i - 1) = result
     )
     or
     result = getParent().(RegExpTerm).getPredecessor()
   }
 
-  /** Gets the regular expression term that is matched after this one, if any. */
+  /** Gets the regular expression term that is matched (textually) after this one, if any. */
   RegExpTerm getSuccessor() {
     exists(RegExpSequence seq, int i |
       seq.getChild(i) = this and
-      seq.getChild(i + getDirection()) = result
+      seq.getChild(i + 1) = result
     )
     or
     exists(RegExpTerm parent |
@@ -98,12 +98,6 @@ class RegExpTerm extends Locatable, @regexpterm {
     )
   }
 
-  /**
-   * Gets the matching direction of this term: `1` if it is in a forward-matching
-   * context, `-1` if it is in a backward-matching context.
-   */
-  private int getDirection() { if isInBackwardMatchingContext() then result = -1 else result = 1 }
-
   /**
    * Holds if this regular term is in a forward-matching context, that is,
    * it has no enclosing lookbehind assertions.

javascript/ql/test/query-tests/RegExp/UnmatchableCaret/tst.js

@@ -26,4 +26,7 @@
 /^(^y|^z)(u$|v$)$/;
 
 // OK
-/x*^y/;
+/x*^y/;
+
+// OK
+/(?<=(^|\/)(\.|\.\.))$/;

javascript/ql/test/query-tests/RegExp/UnmatchableDollar/UnmatchableDollar.expected

@@ -1,3 +1,4 @@
 | tst.js:2:10:2:10 | $ | This assertion can never match. |
 | tst.js:11:3:11:3 | $ | This assertion can never match. |
 | tst.js:20:3:20:3 | $ | This assertion can never match. |
+| tst.js:38:6:38:6 | $ | This assertion can never match. |

javascript/ql/test/query-tests/RegExp/UnmatchableDollar/tst.js

@@ -32,4 +32,7 @@
 /x(?!y+$).*y.*/;
 
 // OK
-/x(?=[yz]+$).*yz.*/;
+/x(?=[yz]+$).*yz.*/;
+
+// NOT OK
+/(?<=$x)yz/;