JavaScript: Fix RegExpTerm.getPredecessor and getSuccessor.

These were originally meant to give you the term that is textually matched right before/right after the receiver. When I introduced support for lookbehinds, I changed the behaviour to give you the term that is _operationally_ matched before/after the receiver (remember that lookbehinds are implemented by reverse-matching).

However, I think that's rarely ever what you want, and is wrong for the only two uses of these predicates, where it's the textual matching order that we are after, not the operational order.

Consequently, I've changed the semantics back and updated the comments to hopefully clarify the intention.

Author: Max Schaefer

change-notes/1.24/analysis-javascript.md

@@ -17,3 +17,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/;