Merge pull request #202 from jbj/resolveClass-conservative

C++: more conservative resolveClass

Author: ian-semmle

cpp/ql/src/semmle/code/cpp/internal/ResolveClass.qll

@@ -1,23 +1,31 @@
 import semmle.code.cpp.Type
 
-/** Holds if `d` is a complete class named `name`. */
+pragma[noinline]
+private string getTopLevelClassName(@usertype c) {
+  isClass(c) and
+  usertypes(c, result, _) and
+  not namespacembrs(_, c) and // not in a namespace
+  not member(_, _, c) and // not in some structure
+  not class_instantiation(c, _) // not a template instantiation
+}
+
+/** Holds if `d` is a unique complete class named `name`. */
 pragma[noinline]
 private predicate existsCompleteWithName(string name, @usertype d) {
-  isClass(d) and
   is_complete(d) and
-  usertypes(d, name, _)
+  name = getTopLevelClassName(d) and
+  strictcount(@usertype other | is_complete(other) and getTopLevelClassName(other) = name) = 1
 }
 
 /** Holds if `c` is an incomplete class named `name`. */
 pragma[noinline]
 private predicate existsIncompleteWithName(string name, @usertype c) {
-  isClass(c) and
   not is_complete(c) and
-  usertypes(c, name, _)
+  name = getTopLevelClassName(c)
 }
 
 /**
- * Holds if `c` is an imcomplete class, and there exists a complete class `d`
+ * Holds if `c` is an imcomplete class, and there exists a unique complete class `d`
  * with the same name.
  */
 private predicate hasCompleteTwin(@usertype c, @usertype d) {
@@ -30,10 +38,8 @@ private predicate hasCompleteTwin(@usertype c, @usertype d) {
 import Cached
 cached private module Cached {
   /**
-   * If `c` is incomplete, and there exists a complete class with the same name,
-   * then the result is that complete class. Otherwise, the result is `c`. If
-   * multiple complete classes have the same name, this predicate may have
-   * multiple results.
+   * If `c` is incomplete, and there exists a unique complete class with the same name,
+   * then the result is that complete class. Otherwise, the result is `c`.
    */
   cached @usertype resolveClass(@usertype c) {
     hasCompleteTwin(c, result)

cpp/ql/test/library-tests/structs/compatible_cpp/b1.cpp

@@ -24,3 +24,9 @@ class Damson {
   int damson_x;
   void foo();
 };
+
+namespace unrelated {
+  class AppleCompatible {
+    long apple_x;
+  };
+}

cpp/ql/test/library-tests/structs/compatible_cpp/compatible.expected

@@ -32,6 +32,9 @@
 | b1.cpp:23:7:23:12 | Damson | 5 members | 2 locations | 1 | foo |
 | b1.cpp:23:7:23:12 | Damson | 5 members | 2 locations | 2 | operator= |
 | b1.cpp:23:7:23:12 | Damson | 5 members | 2 locations | 3 | operator= |
+| b1.cpp:29:9:29:23 | AppleCompatible | 3 members | 1 locations | 0 | apple_x |
+| b1.cpp:29:9:29:23 | AppleCompatible | 3 members | 1 locations | 1 | operator= |
+| b1.cpp:29:9:29:23 | AppleCompatible | 3 members | 1 locations | 2 | operator= |
 | b2.cpp:2:7:2:21 | AppleCompatible | 3 members | 2 locations | 0 | apple_x |
 | b2.cpp:2:7:2:21 | AppleCompatible | 3 members | 2 locations | 1 | operator= |
 | b2.cpp:2:7:2:21 | AppleCompatible | 3 members | 2 locations | 2 | operator= |

cpp/ql/test/library-tests/structs/compatible_cpp/compatible_types.expected

@@ -10,6 +10,7 @@
 | b1.cpp:11:7:11:22 | BananaCompatible | 0 | file://:0:0:0:0 | int | 1 types |
 | b1.cpp:16:7:16:12 | Cherry | 0 | file://:0:0:0:0 | int | 1 types |
 | b1.cpp:23:7:23:12 | Damson | 0 | file://:0:0:0:0 | int | 1 types |
+| b1.cpp:29:9:29:23 | AppleCompatible | 0 | file://:0:0:0:0 | long | 1 types |
 | b2.cpp:2:7:2:21 | AppleCompatible | 0 | file://:0:0:0:0 | int | 1 types |
 | b2.cpp:9:7:9:22 | BananaCompatible | 0 | file://:0:0:0:0 | int | 1 types |
 | b2.cpp:14:7:14:12 | Cherry | 0 | file://:0:0:0:0 | short | 1 types |

cpp/ql/test/library-tests/structs/incomplete_definition/pointerbasetype.expected

@@ -1,2 +1,6 @@
 | a.h:5:8:5:13 | cheese | y.cpp:4:8:4:10 | Foo | 3 |
 | x.cpp:3:6:3:10 | bar_x | a.h:4:8:4:10 | Bar | 3 |
+| x.cpp:19:6:19:10 | foo_x | y.cpp:4:8:4:10 | Foo | 3 |
+| x.cpp:23:5:23:17 | templateField | x.cpp:6:10:6:12 | Foo | 3 |
+| x.cpp:23:5:23:17 | templateField | x.cpp:12:9:12:11 | Foo | 3 |
+| x.cpp:26:18:26:29 | template_foo | x.cpp:22:7:22:14 | Template<Foo *> | 0 |

cpp/ql/test/library-tests/structs/incomplete_definition/x.cpp

@@ -1,3 +1,31 @@
 #include "a.h"
 
 Bar *bar_x;
+
+namespace unrelated {
+  struct Foo {
+    short val;
+  };
+}
+
+struct ContainsAnotherFoo {
+  class Foo {
+    long val;
+  };
+};
+
+// The type of `foo_x` should not refer to any of the above classes, none of
+// which are named `Foo` in the global scope.
+Foo *foo_x;
+
+template<typename T>
+class Template {
+  T templateField;
+};
+
+Template<Foo *> *template_foo;
+
+// Instantiation of the template with unrelated classes named `Foo` should not
+// get mixed up with the instantiation above.
+template class Template<unrelated::Foo *>;
+template class Template<ContainsAnotherFoo::Foo *>;

cpp/ql/test/library-tests/structs/qualified_names/c1.cpp

@@ -0,0 +1,9 @@
+#include "header.h"
+
+struct MultipleDefsButSameHeader {
+  int i;
+};
+
+struct OneDefInDifferentFile {
+  int i;
+};

cpp/ql/test/library-tests/structs/qualified_names/c2.cpp

@@ -0,0 +1,6 @@
+#include "header.h"
+
+struct MultipleDefsButSameHeader {
+  char char1;
+  char char2;
+};

cpp/ql/test/library-tests/structs/qualified_names/decls.cpp

@@ -0,0 +1,5 @@
+namespace foo {
+  class C;
+
+  C *x;
+}

cpp/ql/test/library-tests/structs/qualified_names/defs.cpp

@@ -0,0 +1,29 @@
+namespace foo {
+  class C {
+  };
+}
+
+namespace bar {
+  class C {
+  };
+}
+
+class DefinedAndDeclared {
+};
+
+// Despite this declaration being present, the variable below is associated
+// with the definition above rather than this declaration.
+class DefinedAndDeclared;
+
+DefinedAndDeclared *definedAndDeclared;
+
+#include "header.h"
+
+// Because there are multiple definitions of `MultipleDefsButSameHeader`, the
+// type of this variable will refer to the declaration in `header.h` rather
+// than any of the definitions.
+MultipleDefsButSameHeader *mdbsh;
+
+// Because there is only one definition of `OneDefInDifferentFile`, the type of
+// this variable will refer to that definition.
+OneDefInDifferentFile *odidf;