Address copilot feedback

Author: MichaelRFairhurst

…ndards/cpp/dominance/SuccessorUnless.qll …standards/cpp/dominance/FeasiblePath.qllcpp/common/src/codingstandards/cpp/dominance/SuccessorUnless.qll renamed to cpp/common/src/codingstandards/cpp/dominance/FeasiblePath.qll cpp/common/src/codingstandards/cpp/dominance/SuccessorUnless.qll renamed to cpp/common/src/codingstandards/cpp/dominance/FeasiblePath.qll

@@ -1,12 +1,12 @@
 import cpp
 
 /**
- * The configuration signature for the SuccessorUnless module.
+ * The configuration signature for the FeasiblePath module.
  *
  * Typically, the `Node` type should be a `ControlFlowNode`, but it can be overridden to enable
  * other kinds of graphs.
  */
-signature module SuccessorUnlessConfigSig {
+signature module FeasiblePathConfigSig {
   /** The state type used to carry context through the CFG exploration. */
   class State;
 
@@ -17,14 +17,14 @@ signature module SuccessorUnlessConfigSig {
 
   predicate isStart(State state, Node node);
 
-  predicate isUnless(State state, Node node);
+  predicate isExcludedPath(State state, Node node);
 
   predicate isEnd(State state, Node node);
 }
 
 /**
- * A module that finds successor of a node -- unless there is an intermediate node that satisfies
- * a given condition.
+ * A module that finds whether a feasible path exists between two control flow nodes, and
+ * additionally support configuration that should not be traversed.
  *
  * Also accepts a state parameter to allow a context to flow through the CFG.
  *
@@ -33,21 +33,21 @@ signature module SuccessorUnlessConfigSig {
  * Implement the module `ConfigSig`, with some context type:
  *
  * ```ql
- * module MyConfig implements SuccessorUnless<SomeContext>::ConfigSig {
+ * module MyConfig implements FeasiblePathConfigSig {
  *   predicate isStart(SomeContext state, ControlFlowNode node) {
  *     node = state.someStartCondition()
  *   }
  *
- *   predicate isUnless(SomeContext state, ControlFlowNode node) {
- *    node = state.someUnlessCondition()
+ *   predicate isExcludedPath(SomeContext state, ControlFlowNode node) {
+ *    node = state.someExcludedPathCondition()
  *   }
  *
  *   predicate isEnd(SomeContext state, ControlFlowNode node) {
  *     node = state.someEndCondition()
  *   }
  * }
  *
- * import SuccessorUnless<SomeContext>::Make<MyConfig> as Successor
+ * import FeasiblePath<MyConfig> as MyFeasiblePath
  * ```
  *
  * ## Rationale
@@ -62,7 +62,7 @@ signature module SuccessorUnlessConfigSig {
  *   not exists(ControlFlowNode mid |
  *     mid = start.getASuccessor+() and
  *     end = mid.getASuccessor*() and
- *     isUnless(mid)
+ *     isExcludedPath(mid)
  *   )
  * }
  * ```
@@ -73,19 +73,19 @@ signature module SuccessorUnlessConfigSig {
  * while (cond) {
  *   start();
  *   end();
- *   unless();
+ *   excluded();
  * }
  * ```
  *
- * In the above code, `unless()` is a successor of `start()`, and `end()` is also a successor of
- * `unless()` (via the loop back edge). However, there is no path from `start()` to `end()` that
- * does not pass through `unless()`.
+ * In the above code, `excluded()` is a successor of `start()`, and `end()` is also a successor of
+ * `excluded()` (via the loop back edge). However, there is no path from `start()` to `end()` that
+ * does not pass through `excluded()`.
  *
  * This module will correctly handle this case. Forward exploration through the graph will stop
- * at the `unless()` nodes, such that only paths from `start()` to `end()` that do not pass through
- * `unless()` nodes will be found.
+ * at the `excluded()` nodes, such that only paths from `start()` to `end()` that do not pass
+ * through `excluded()` nodes will be found.
  */
-module SuccessorUnless<SuccessorUnlessConfigSig Config> {
+module FeasiblePath<FeasiblePathConfigSig Config> {
   predicate isSuccessor(Config::State state, Config::Node start, Config::Node end) {
     isMid(state, start, end) and
     Config::isEnd(state, end)
@@ -100,7 +100,7 @@ module SuccessorUnless<SuccessorUnlessConfigSig Config> {
       exists(Config::Node prevMid |
         isMid(state, start, prevMid) and
         mid = prevMid.getASuccessor() and
-        not Config::isUnless(state, mid) and
+        not Config::isExcludedPath(state, mid) and
         not Config::isEnd(state, prevMid)
       )
     )

cpp/common/src/codingstandards/cpp/standardlibrary/STLContainers.qll

@@ -41,11 +41,14 @@ class BoundedIndex extends TBoundedIndex {
   float getUpperBound() { result = ub }
 
   predicate mayOverlap(BoundedIndex other) {
-    // The lower bound is contained by the other index range.
-    lb <= other.getUpperBound() and lb >= other.getLowerBound()
-    or
-    // or the upper bound is contained by the other index range.
-    ub <= other.getUpperBound() and ub >= other.getLowerBound()
+    // It's easier to think about bounds that cannot overlap, and then negate that.
+    not (
+      // If our lower bound is greater than the other's upper bound, they can't overlap.
+      lb > other.getUpperBound()
+      or
+      // If our upper bound is less than the other's lower bound, they can't overlap.
+      ub < other.getLowerBound()
+    )
   }
 
   predicate forIndexExpr(Expr expr) {
@@ -120,7 +123,7 @@ class STLContainerUnknownElementModification extends FunctionCall {
               "clear", "push_back", "pop_back", "pop_front", "insert", "emplace", "emplace_back",
               "emplace_front", "emplace_hint", "erase", "resize", "assign", "assign_range", "swap",
               "push", "pop", "fill", "extract", "merge", "insert_or_assign", "try_emplace",
-              "operaator+=", "replace", "replace_with_range", "copy"
+              "operator+=", "replace", "replace_with_range", "copy"
             ])
   }
 }
@@ -164,7 +167,7 @@ class STLContainer extends Class {
   STLContainer() {
     getNamespace() instanceof StdNS and
     // TODO: add to change log that we added std::array
-    getSimpleName() in ["vector", "deque", "array", "valarray",] and
+    getSimpleName() in ["vector", "deque", "array", "valarray"] and
     containerKind = TIndexableContainer()
     or
     getSimpleName() in [
@@ -175,7 +178,8 @@ class STLContainer extends Class {
     getNamespace() instanceof StdNS and
     getSimpleName() in [
         "multiset", "map", "multimap", "unordered_multiset", "unordered_map", "unordered_multimap"
-      ]
+      ] and
+    containerKind = TAssociativeContainer()
     or
     // TODO: This intentionally doesn't check namespace::std, what are the implications?
     // TODO: add support for `std::string_view`?

cpp/common/test/includes/standard-library/utility.h

@@ -12,11 +12,11 @@ template <class T> typename add_rvalue_reference<T>::type declval() noexcept;
 template <class T> void swap(T &a, T &b) noexcept;
 
 template <class T1, class T2> struct pair {
-    T1 first;
-    T2 second;
-    pair(const T1& a, const T2& b);
+  T1 first;
+  T2 second;
+  pair(const T1 &a, const T2 &b);
 };
 
 } // namespace std
 
-#endif _GHLIBCPP_UTILITY
+#endif // _GHLIBCPP_UTILITY

cpp/misra/src/rules/RULE-0-1-1/UnnecessaryWriteToLocalObject.ql

@@ -17,16 +17,12 @@
 
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
-import semmle.code.cpp.dataflow.internal.TaintTrackingUtil as Taint
 import codingstandards.cpp.misra
 import codingstandards.cpp.lifetimes.CppObjects
 import codingstandards.cpp.lifetimes.CppSubObjects
-import codingstandards.cpp.dominance.BehavioralSet
-import codingstandards.cpp.dominance.SuccessorUnless
+import codingstandards.cpp.dominance.FeasiblePath
 import codingstandards.cpp.types.TrivialType
 import codingstandards.cpp.standardlibrary.STLContainers
-import semmle.code.cpp.ir.dataflow.internal.SsaInternalsCommon as Ssa
-import semmle.code.cpp.dataflow.new.DataFlow as NewDataFlow
 
 /**
  * A type that is relevant for analysis by this rule.
@@ -384,7 +380,7 @@ class CrementAwareNode extends TCrementAwareNode {
     or
     this = TPreCrementNode(_) and result = "pre crement node"
     or
-    this = TPostCrementNode(_) and result = "pre crement node"
+    this = TPostCrementNode(_) and result = "post crement node"
   }
 
   Location getLocation() { result = getControlFlowNode().getLocation() }
@@ -577,7 +573,7 @@ predicate isExcludedWrite(RelevantControlFlowNode node, RelevantObject obj) {
   node.getAPredecessor+() = [obj.getASubobjectAddressExpr(), obj.getASubobjectTakenAsReference()]
 }
 
-module WriteWithReadConfig implements SuccessorUnlessConfigSig {
+module WriteWithReadConfig implements FeasiblePathConfigSig {
   class State = TrackedObject;
 
   class Node = CrementAwareNode;
@@ -589,7 +585,7 @@ module WriteWithReadConfig implements SuccessorUnlessConfigSig {
     )
   }
 
-  predicate isUnless(TrackedObject state, CrementAwareNode node) {
+  predicate isExcludedPath(TrackedObject state, CrementAwareNode node) {
     exists(TrackedObject rewritten |
       rewritten = state.getParent*() and
       isWrite(_, rewritten, node) and
@@ -609,7 +605,7 @@ module WriteWithReadConfig implements SuccessorUnlessConfigSig {
   }
 }
 
-predicate isWriteWithRead = SuccessorUnless<WriteWithReadConfig>::isSuccessor/3;
+predicate isWriteWithRead = FeasiblePath<WriteWithReadConfig>::isSuccessor/3;
 
 predicate isWriteWithObserve(RelevantObject obj, CrementAwareNode node) {
   isWrite(obj, _, node) and

cpp/misra/test/rules/RULE-0-1-1/test.cpp

@@ -247,7 +247,7 @@ void f14() {
   // Test that dereferencing can observe the pointer base, but only when
   // assigned to.
   //
-  // Note that the cases involve assigment to dereferenced pointers could be
+  // Note that the cases involve assignment to dereferenced pointers could be
   // fully modeled to count as an assignment to the pointee object, but we don't
   // currently do that advanced of an analysis.
   int l0[] = {0, 1, 2}; // COMPLIANT