Merge pull request github#4571 from MathiasVP/better-syntax-for-false-positives-and-negatives-inline-expectation

C++/Python: Better syntax for false positives and negatives in inline expectations

Author: jbj

cpp/ql/test/TestUtilities/InlineExpectationsTest.qll

@@ -43,15 +43,15 @@
  * There is no need to write a `select` clause or query predicate. All of the differences between
  * expected results and actual results will be reported in the `failures()` query predicate.
  *
- * To annotate the test source code with an expected result, place a comment on the
+ * To annotate the test source code with an expected result, place a comment starting with a `$` on the
  * same line as the expected result, with text of the following format as the body of the comment:
  *
- * `$tag=expected-value`
+ * `tag=expected-value`
  *
  * Where `tag` is the value of the `tag` parameter from `hasActualResult()`, and `expected-value` is
  * the value of the `value` parameter from `hasActualResult()`. The `=expected-value` portion may be
  * omitted, in which case `expected-value` is treated as the empty string. Multiple expectations may
- * be placed in the same comment, as long as each is prefixed by a `$`. Any actual result that
+ * be placed in the same comment. Any actual result that
  * appears on a line that does not contain a matching expected result comment will be reported with
  * a message of the form "Unexpected result: tag=value". Any expected result comment for which there
  * is no matching actual result will be reported with a message of the form
@@ -60,30 +60,33 @@
  * Example:
  * ```cpp
  * int i = x + 5;  // $const=5
- * int j = y + (7 - 3)  // $const=7 $const=3 $const=4  // The result of the subtraction is a constant.
+ * int j = y + (7 - 3)  // $const=7 const=3 const=4  // The result of the subtraction is a constant.
  * ```
  *
- * For tests that contain known false positives and false negatives, it is possible to further
- * annotate that a particular expected result is known to be a false positive, or that a particular
- * missing result is known to be a false negative:
+ * For tests that contain known missing and spurious results, it is possible to further
+ * annotate that a particular expected result is known to be spurious, or that a particular
+ * missing result is known to be missing:
  *
- * `$f+:tag=expected-value`  // False positive
- * `$f-:tag=expected-value`  // False negative
+ * `$ SPURIOUS: tag=expected-value`  // Spurious result
+ * `$ MISSING: tag=expected-value`  // Missing result
  *
- * A false positive expectation is treated as any other expected result, except that if there is no
- * matching actual result, the message will be of the form "Fixed false positive: tag=value". A
- * false negative expectation is treated as if there were no expected result, except that if a
+ * A spurious expectation is treated as any other expected result, except that if there is no
+ * matching actual result, the message will be of the form "Fixed spurious result: tag=value". A
+ * missing expectation is treated as if there were no expected result, except that if a
  * matching expected result is found, the message will be of the form
- * "Fixed false negative: tag=value".
+ * "Fixed missing result: tag=value".
+ *
+ * A single line can contain all the expected, spurious and missing results of that line. For instance:
+ * `$ tag1=value1 SPURIOUS: tag2=value2 MISSING: tag3=value3`.
  *
  * If the same result value is expected for two or more tags on the same line, there is a shorthand
  * notation available:
  *
- * `$tag1,tag2=expected-value`
+ * `tag1,tag2=expected-value`
  *
  * is equivalent to:
  *
- * `$tag1=expected-value $tag2=expected-value`
+ * `tag1=expected-value tag2=expected-value`
  */
 
 private import InlineExpectationsTestPrivate
@@ -126,7 +129,7 @@ abstract class InlineExpectationsTest extends string {
       (
         exists(FalseNegativeExpectation falseNegative |
           falseNegative.matchesActualResult(actualResult) and
-          message = "Fixed false negative:" + falseNegative.getExpectationText()
+          message = "Fixed missing result:" + falseNegative.getExpectationText()
         )
         or
         not exists(ValidExpectation expectation | expectation.matchesActualResult(actualResult)) and
@@ -143,7 +146,7 @@ abstract class InlineExpectationsTest extends string {
         message = "Missing result:" + expectation.getExpectationText()
         or
         expectation instanceof FalsePositiveExpectation and
-        message = "Fixed false positive:" + expectation.getExpectationText()
+        message = "Fixed spurious result:" + expectation.getExpectationText()
       )
     )
     or
@@ -160,20 +163,79 @@ abstract class InlineExpectationsTest extends string {
  * is treated as part of the expected results, except that the comment may contain a `//` sequence
  * to treat the remainder of the line as a regular (non-interpreted) comment.
  */
-private string expectationCommentPattern() { result = "\\s*(\\$(?:[^/]|/[^/])*)(?://.*)?" }
+private string expectationCommentPattern() { result = "\\s*\\$((?:[^/]|/[^/])*)(?://.*)?" }
 
 /**
- * RegEx pattern to match a single expected result, not including the leading `$`. It starts with an
- * optional `f+:` or `f-:`, followed by one or more comma-separated tags containing only letters,
- * `-`, and `_`, optionally followed by `=` and the expected value.
+ * The possible columns in an expectation comment. The `TDefaultColumn` branch represents the first
+ * column in a comment. This column is not precedeeded by a name. `TNamedColumn(name)` represents a
+ * column containing expected results preceeded by the string `name:`.
  */
-private string expectationPattern() {
-  result = "(?:(f(?:\\+|-)):)?((?:[A-Za-z-_]+)(?:\\s*,\\s*[A-Za-z-_]+)*)(?:=(.*))?"
+private newtype TColumn =
+  TDefaultColumn() or
+  TNamedColumn(string name) { name = ["MISSING", "SPURIOUS"] }
+
+bindingset[start, content]
+private int getEndOfColumnPosition(int start, string content) {
+  result =
+    min(string name, int cand |
+      exists(TNamedColumn(name)) and
+      cand = content.indexOf(name + ":") and
+      cand > start
+    |
+      cand
+    )
+  or
+  not exists(string name |
+    exists(TNamedColumn(name)) and
+    content.indexOf(name + ":") > start
+  ) and
+  result = content.length()
+}
+
+private predicate getAnExpectation(
+  LineComment comment, TColumn column, string expectation, string tags, string value
+) {
+  exists(string content |
+    content = comment.getContents().regexpCapture(expectationCommentPattern(), 1) and
+    (
+      column = TDefaultColumn() and
+      exists(int end |
+        end = getEndOfColumnPosition(0, content) and
+        expectation = content.prefix(end).regexpFind(expectationPattern(), _, _).trim()
+      )
+      or
+      exists(string name, int start, int end |
+        column = TNamedColumn(name) and
+        start = content.indexOf(name + ":") + name.length() + 1 and
+        end = getEndOfColumnPosition(start, content) and
+        expectation = content.substring(start, end).regexpFind(expectationPattern(), _, _).trim()
+      )
+    )
+  ) and
+  tags = expectation.regexpCapture(expectationPattern(), 1) and
+  if exists(expectation.regexpCapture(expectationPattern(), 2))
+  then value = expectation.regexpCapture(expectationPattern(), 2)
+  else value = ""
 }
 
-private string getAnExpectation(LineComment comment) {
-  result = comment.getContents().regexpCapture(expectationCommentPattern(), 1).splitAt("$").trim() and
-  result != ""
+private string getColumnString(TColumn column) {
+  column = TDefaultColumn() and result = ""
+  or
+  column = TNamedColumn(result)
+}
+
+/**
+ * RegEx pattern to match a single expected result, not including the leading `$`. It consists of one or
+ * more comma-separated tags containing only letters, digits, `-` and `_` (note that the first character
+ * must not be a digit), optionally followed by `=` and the expected value.
+ */
+private string expectationPattern() {
+  exists(string tag, string tags, string value |
+    tag = "[A-Za-z-_][A-Za-z-_0-9]*" and
+    tags = "((?:" + tag + ")(?:\\s*,\\s*" + tag + ")*)" and
+    value = "((?:\"[^\"]*\"|'[^']*'|\\S+)*)" and
+    result = tags + "(?:=" + value + ")?"
+  )
 }
 
 private newtype TFailureLocatable =
@@ -183,24 +245,14 @@ private newtype TFailureLocatable =
     test.hasActualResult(___location, element, tag, value)
   } or
   TValidExpectation(LineComment comment, string tag, string value, string knownFailure) {
-    exists(string expectation |
-      expectation = getAnExpectation(comment) and
-      expectation.regexpMatch(expectationPattern()) and
-      tag = expectation.regexpCapture(expectationPattern(), 2).splitAt(",").trim() and
-      (
-        if exists(expectation.regexpCapture(expectationPattern(), 3))
-        then value = expectation.regexpCapture(expectationPattern(), 3)
-        else value = ""
-      ) and
-      (
-        if exists(expectation.regexpCapture(expectationPattern(), 1))
-        then knownFailure = expectation.regexpCapture(expectationPattern(), 1)
-        else knownFailure = ""
-      )
+    exists(TColumn column, string tags |
+      getAnExpectation(comment, column, _, tags, value) and
+      tag = tags.splitAt(",") and
+      knownFailure = getColumnString(column)
     )
   } or
   TInvalidExpectation(LineComment comment, string expectation) {
-    expectation = getAnExpectation(comment) and
+    getAnExpectation(comment, _, expectation, _, _) and
     not expectation.regexpMatch(expectationPattern())
   }
 
@@ -265,16 +317,17 @@ private class ValidExpectation extends Expectation, TValidExpectation {
   }
 }
 
+/* Note: These next three classes correspond to all the possible values of type `TColumn`. */
 class GoodExpectation extends ValidExpectation {
   GoodExpectation() { getKnownFailure() = "" }
 }
 
 class FalsePositiveExpectation extends ValidExpectation {
-  FalsePositiveExpectation() { getKnownFailure() = "f+" }
+  FalsePositiveExpectation() { getKnownFailure() = "SPURIOUS" }
 }
 
 class FalseNegativeExpectation extends ValidExpectation {
-  FalseNegativeExpectation() { getKnownFailure() = "f-" }
+  FalseNegativeExpectation() { getKnownFailure() = "MISSING" }
 }
 
 class InvalidExpectation extends Expectation, TInvalidExpectation {

cpp/ql/test/library-tests/dataflow/fields/A.cpp

@@ -40,21 +40,21 @@ class A
     cc.insert(nullptr);
     ct.insert(new C());
     sink(&cc); // no flow
-    sink(&ct); // $ast $f-:ir
+    sink(&ct); // $ ast MISSING: ir
   }
   void f1()
   {
     C *c = new C();
     B *b = B::make(c);
-    sink(b->c); // $ast $f-:ir
+    sink(b->c); // $ast MISSING: ir
   }
 
   void f2()
   {
     B *b = new B();
     b->set(new C1());
-    sink(b->get());                // $ast $ir=55:12
-    sink((new B(new C()))->get()); // $ast $ir
+    sink(b->get());                // $ ast ir=55:12
+    sink((new B(new C()))->get()); // $ ast ir
   }
 
   void f3()
@@ -63,7 +63,7 @@ class A
     B *b2;
     b2 = setOnB(b1, new C2());
     sink(b1->c); // no flow
-    sink(b2->c); // $ast $f-:ir
+    sink(b2->c); // $ ast MISSING: ir
   }
 
   void f4()
@@ -72,7 +72,7 @@ class A
     B *b2;
     b2 = setOnBWrap(b1, new C2());
     sink(b1->c); // no flow
-    sink(b2->c); // $ast $f-:ir
+    sink(b2->c); // $ ast MISSING: ir
   }
 
   B *setOnBWrap(B *b1, C *c)
@@ -104,7 +104,7 @@ class A
   {
     if (C1 *c1 = dynamic_cast<C1 *>(c))
     {
-      sink(c1->a); // $ast,ir
+      sink(c1->a); // $ ast,ir
     }
     C *cc;
     if (C2 *c2 = dynamic_cast<C2 *>(c))
@@ -117,7 +117,7 @@ class A
     }
     if (C1 *c1 = dynamic_cast<C1 *>(cc))
     {
-      sink(c1->a); //$f+:ast
+      sink(c1->a); // $ SPURIOUS: ast
     }
   }
 
@@ -129,7 +129,7 @@ class A
   {
     B *b = new B();
     f7(b);
-    sink(b->c); // $ast,ir
+    sink(b->c); // $ ast,ir
   }
 
   class D
@@ -149,9 +149,9 @@ class A
   {
     B *b = new B();
     D *d = new D(b, r());
-    sink(d->b);    // $ast,ir=143:25 $ast,ir=150:12
-    sink(d->b->c); // $ast $f-:ir
-    sink(b->c);    // $ast,ir
+    sink(d->b);    // $ ast,ir=143:25 ast,ir=150:12
+    sink(d->b->c); // $ ast MISSING: ir
+    sink(b->c);    // $ ast,ir
   }
 
   void f10()
@@ -162,11 +162,11 @@ class A
     MyList *l3 = new MyList(nullptr, l2);
     sink(l3->head);                   // no flow, b is nested beneath at least one ->next
     sink(l3->next->head);             // no flow
-    sink(l3->next->next->head);       // $ast $f-:ir
+    sink(l3->next->next->head);       // $ ast MISSING: ir
     sink(l3->next->next->next->head); // no flow
     for (MyList *l = l3; l != nullptr; l = l->next)
     {
-      sink(l->head); // $ast $f-:ir
+      sink(l->head); // $ ast MISSING: ir
     }
   }
 

cpp/ql/test/library-tests/dataflow/fields/B.cpp

@@ -6,7 +6,7 @@ class B
     Elem *e = new Elem();
     Box1 *b1 = new Box1(e, nullptr);
     Box2 *b2 = new Box2(b1);
-    sink(b2->box1->elem1); // $ast $f-:ir
+    sink(b2->box1->elem1); // $ ast MISSING: ir
     sink(b2->box1->elem2); // no flow
   }
 
@@ -16,7 +16,7 @@ class B
     Box1 *b1 = new B::Box1(nullptr, e);
     Box2 *b2 = new Box2(b1);
     sink(b2->box1->elem1); // no flow
-    sink(b2->box1->elem2); // $ast $f-:ir
+    sink(b2->box1->elem2); // $ ast MISSING: ir
   }
 
   static void sink(void *o) {}

cpp/ql/test/library-tests/dataflow/fields/C.cpp

@@ -26,10 +26,10 @@ class C
 
   void func()
   {
-    sink(s1); // $ast $ir
-    sink(s2); // $f-:ast $f-:ir
-    sink(s3); // $ast $ir
-    sink(s4); // $f-:ast $f-:ir
+    sink(s1); // $ast ir
+    sink(s2); // $ MISSING: ast,ir
+    sink(s3); // $ast ir
+    sink(s4); // $ MISSING: ast,ir
   }
 
   static void sink(const void *o) {}

cpp/ql/test/library-tests/dataflow/fields/D.cpp

@@ -19,7 +19,7 @@ class D {
   };
 
   static void sinkWrap(Box2* b2) {
-    sink(b2->getBox1()->getElem()); // $ast=28:15 $ast=35:15 $ast=42:15 $ast=49:15 $f-:ir
+    sink(b2->getBox1()->getElem()); // $ast=28:15 ast=35:15 ast=42:15 ast=49:15 MISSING: ir
   }
 
   Box2* boxfield;
@@ -61,6 +61,6 @@ class D {
 
 private:
   void f5b() {
-    sink(boxfield->box->elem); // $ast $f-:ir
+    sink(boxfield->box->elem); // $ ast MISSING: ir
   }
 };

cpp/ql/test/library-tests/dataflow/fields/E.cpp

@@ -18,7 +18,7 @@ void sink(char *b);
 
 void handlePacket(packet *p)
 {
-    sink(p->data.buffer); // $ast $f-:ir
+    sink(p->data.buffer); // $ ast MISSING: ir
 }
 
 void f(buf* b)
@@ -28,7 +28,7 @@ void f(buf* b)
     argument_source(raw);
     argument_source(b->buffer);
     argument_source(p.data.buffer);
-    sink(raw); // $ast $f-:ir
-    sink(b->buffer); // $ast $f-:ir
+    sink(raw); // $ ast MISSING: ir
+    sink(b->buffer); // $ ast MISSING: ir
     handlePacket(&p);
 }