Implement rule 28-6-1, only move non-const lvalues

cpp/common/src/codingstandards/cpp/ast/ValueCategory.qll

@@ -0,0 +1,128 @@
+import cpp
+
+/**
+ * Get an expression's value category as a ValueCategory object.
+ *
+ * Note that the standard cpp library exposes `is{_}ValueCategory` predicates, but they do not
+ * necessarily work as expected due to how CodeQL handles reference adjustment and binding, which
+ * this predicate attempts to handle. There are additional unhandled cases involving
+ * lvalue-to-rvalue conversions as well.
+ *
+ * In C++17, an expression of type "reference to T" is adjusted to type T as stated in [expr]/5.
+ * This is not a conversion, but in CodeQL this is handled by `ReferenceDereferenceExpr`, a type of
+ * `Conversion`. Similarly, the binding of references to values is described in [dcl.init.ref],
+ * which is not a conversion, but in CodeQL this is handled by `ReferenceToExpr`, another type of
+ * `Conversion`.
+ *
+ * Furthermore, the `Expr` predicate `hasLValueToRValueConversion()` uses a different dbscheme table
+ * than the `Conversion` table, and it is possible to have expressions such that
+ * `hasLValueToRValueConversion()` holds, but there is no corresponding `Conversion` entry.
+ *
+ * Therefore, the value categories of expressions before `ReferenceDereferenceExpr` and after
+ * `ReferenceToExpr` conversions are therefore essentially unspecified, and the `is{_}ValueCategory`
+ * predicate results should not be relied upon. And types that are missing a
+ * `LvalueToRValueConversion` will also return incorrect value categories.
+ *
+ * For example, in CodeQL 2.21.4:
+ *
+ * ```cpp
+ * int &i = ...;
+ * auto r = std::move(i); // 1.) i is a `prvalue(load)` in CodeQL, not an lvalue.
+ *                        // 2.) i has a `ReferenceDereferenceExpr` conversion of lvalue category
+ *                        // 3.) the conversion from 2 has a `ReferenceToExpr` conversion of prvalue
+ *                        //     category.
+ * int i2 = ...;
+ * f(i2); // 1.) i2 is an lvalue.
+ *        // 2.) i2 undergoes lvalue-to-rvalue conversion, but there is no corresponding `Conversion`.
+ *        // 3.) i2 is treated at a prvalue by CodeQL, but `hasLValueToRValueConversion()` holds.
+ *
+ * int get_int();
+ * auto r2 = std::move(get_int()); // 1.) get_int() itself is treated as a prvalue by CodeQL.
+ *                                 // 2.) get_int() has a `TemporaryObjectExpr` conversion of lvalue
+ *                                 //     category.
+ *                                 // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
+ *                                 //     of prvalue category.
+ * std::string get_str();
+ * auto r3 = std::move(get_str()); // 1.) get_str() is treated as a prvalue by CodeQL.
+ *                                 // 2.) get_str() has a `TemporaryObjectExpr` conversion of xvalue
+ *                                 // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
+ *                                 //     of prvalue category.
+ * std::string &str_ref();
+ * auto r3 = std::move(str_ref()); // 1.) str_ref() is treated as a prvalue by CodeQL.
+ *                                 // 2.) str_ref() has a `ReferenceDereferenceExpr` conversion of
+ *                                 //     lvalue.
+ *                                 // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
+ *                                 //     of prvalue category.
+ * ```
+ *
+ * As can be seen above, the value categories of expressions are correct between the
+ * `ReferenceDereferenceExpr` and `ReferenceToExpr`, but not necessarily before or after.
+ *
+ * We must also check for `hasLValueToRValueConversion()` and handle that appropriately.
+ */
+ValueCategory getValueCategory(Expr e) {
+  // If `e` is adjusted from a reference to a value (C++17 [expr]/5) then we want the value category
+  // of the expression after `ReferenceDereferenceExpr`.
+  if e.getConversion() instanceof ReferenceDereferenceExpr
+  then result = getValueCategory(e.getConversion())
+  else (
+    // If `hasLValueToRValueConversion()` holds, then ensure we have an lvalue category.
+    if e.hasLValueToRValueConversion()
+    then result.isLValue()
+    else
+      // Otherwise, get the value category from `is{_}ValueCategory` predicates as normal.
+      result = getDirectValueCategory(e)
+  )
+}
+
+/**
+ * Gets the value category of an expression using `is{_}ValueCategory` predicates, without looking
+ * through conversions.
+ */
+private ValueCategory getDirectValueCategory(Expr e) {
+  if e.isLValueCategory()
+  then result = LValue(e.getValueCategoryString())
+  else
+    if e.isPRValueCategory()
+    then result = PRValue(e.getValueCategoryString())
+    else
+      if e.isXValueCategory()
+      then result = XValue(e.getValueCategoryString())
+      else none()
+}
+
+newtype TValueCategory =
+  LValue(string descr) {
+    exists(Expr e | e.isLValueCategory() and descr = e.getValueCategoryString())
+  } or
+  PRValue(string descr) {
+    exists(Expr e | e.isPRValueCategory() and descr = e.getValueCategoryString())
+  } or
+  XValue(string descr) {
+    exists(Expr e | e.isXValueCategory() and descr = e.getValueCategoryString())
+  }
+
+/**
+ * A value category, which can be an lvalue, prvalue, or xvalue.
+ *
+ * Note that prvalue has two possible forms: `prvalue` and `prvalue(load)`.
+ */
+class ValueCategory extends TValueCategory {
+  string description;
+
+  ValueCategory() {
+    this = LValue(description) or this = PRValue(description) or this = XValue(description)
+  }
+
+  predicate isLValue() { this instanceof LValue }
+
+  predicate isPRValue() { this instanceof PRValue }
+
+  predicate isXValue() { this instanceof XValue }
+
+  predicate isRValue() { this instanceof PRValue or this instanceof XValue }
+
+  predicate isGLValue() { this instanceof LValue or this instanceof XValue }
+
+  string toString() { result = description }
+}

cpp/common/src/codingstandards/cpp/exclusions/cpp/Preconditions5.qll

@@ -0,0 +1,26 @@
+//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY.  **/
+import cpp
+import RuleMetadata
+import codingstandards.cpp.exclusions.RuleMetadata
+
+newtype Preconditions5Query = TStdMoveWithNonConstLvalueQuery()
+
+predicate isPreconditions5QueryMetadata(Query query, string queryId, string ruleId, string category) {
+  query =
+    // `Query` instance for the `stdMoveWithNonConstLvalue` query
+    Preconditions5Package::stdMoveWithNonConstLvalueQuery() and
+  queryId =
+    // `@id` for the `stdMoveWithNonConstLvalue` query
+    "cpp/misra/std-move-with-non-const-lvalue" and
+  ruleId = "RULE-28-6-1" and
+  category = "required"
+}
+
+module Preconditions5Package {
+  Query stdMoveWithNonConstLvalueQuery() {
+    //autogenerate `Query` type
+    result =
+      // `Query` type for `stdMoveWithNonConstLvalue` query
+      TQueryCPP(TPreconditions5PackageQuery(TStdMoveWithNonConstLvalueQuery()))
+  }
+}

cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll

@@ -80,6 +80,7 @@ import Pointers
 import Preconditions1
 import Preconditions3
 import Preconditions4
+import Preconditions5
 import Preprocessor
 import Preprocessor2
 import Representation
@@ -182,6 +183,7 @@ newtype TCPPQuery =
   TPreconditions1PackageQuery(Preconditions1Query q) or
   TPreconditions3PackageQuery(Preconditions3Query q) or
   TPreconditions4PackageQuery(Preconditions4Query q) or
+  TPreconditions5PackageQuery(Preconditions5Query q) or
   TPreprocessorPackageQuery(PreprocessorQuery q) or
   TPreprocessor2PackageQuery(Preprocessor2Query q) or
   TRepresentationPackageQuery(RepresentationQuery q) or
@@ -284,6 +286,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat
   isPreconditions1QueryMetadata(query, queryId, ruleId, category) or
   isPreconditions3QueryMetadata(query, queryId, ruleId, category) or
   isPreconditions4QueryMetadata(query, queryId, ruleId, category) or
+  isPreconditions5QueryMetadata(query, queryId, ruleId, category) or
   isPreprocessorQueryMetadata(query, queryId, ruleId, category) or
   isPreprocessor2QueryMetadata(query, queryId, ruleId, category) or
   isRepresentationQueryMetadata(query, queryId, ruleId, category) or

cpp/common/test/library/codingstandards/cpp/ast/ValueCategory/ValueCategoryTest.expected

@@ -0,0 +1,48 @@
+| test.cpp:14:3:14:9 | call to get_val | prvalue |
+| test.cpp:15:3:15:9 | call to get_ref | lvalue |
+| test.cpp:16:3:16:10 | call to get_rref | xvalue |
+| test.cpp:19:12:19:14 | val | lvalue |
+| test.cpp:20:12:20:14 | val | lvalue |
+| test.cpp:21:13:21:16 | call to move | xvalue |
+| test.cpp:21:18:21:20 | val | lvalue |
+| test.cpp:22:12:22:14 | val | lvalue |
+| test.cpp:23:12:23:15 | call to move | xvalue |
+| test.cpp:23:17:23:19 | val | lvalue |
+| test.cpp:25:12:25:18 | call to get_val | prvalue |
+| test.cpp:26:13:26:19 | call to get_val | prvalue |
+| test.cpp:27:13:27:16 | call to move | xvalue |
+| test.cpp:27:18:27:24 | call to get_val | prvalue |
+| test.cpp:28:12:28:18 | call to get_val | prvalue |
+| test.cpp:29:12:29:15 | call to move | xvalue |
+| test.cpp:29:17:29:23 | call to get_val | prvalue |
+| test.cpp:31:14:31:16 | val | lvalue |
+| test.cpp:32:12:32:14 | ref | lvalue |
+| test.cpp:33:12:33:14 | ref | lvalue |
+| test.cpp:34:13:34:16 | call to move | xvalue |
+| test.cpp:34:18:34:20 | ref | lvalue |
+| test.cpp:35:12:35:14 | ref | lvalue |
+| test.cpp:36:12:36:15 | call to move | xvalue |
+| test.cpp:36:17:36:19 | ref | lvalue |
+| test.cpp:38:12:38:18 | call to get_ref | lvalue |
+| test.cpp:39:12:39:18 | call to get_ref | lvalue |
+| test.cpp:40:13:40:16 | call to move | xvalue |
+| test.cpp:40:18:40:24 | call to get_ref | lvalue |
+| test.cpp:41:12:41:18 | call to get_ref | lvalue |
+| test.cpp:42:12:42:15 | call to move | xvalue |
+| test.cpp:42:17:42:23 | call to get_ref | lvalue |
+| test.cpp:44:16:44:19 | call to move | xvalue |
+| test.cpp:44:21:44:23 | val | lvalue |
+| test.cpp:45:12:45:15 | rref | lvalue |
+| test.cpp:46:12:46:15 | rref | lvalue |
+| test.cpp:47:13:47:16 | call to move | xvalue |
+| test.cpp:47:18:47:21 | rref | lvalue |
+| test.cpp:48:12:48:15 | rref | lvalue |
+| test.cpp:49:12:49:15 | call to move | xvalue |
+| test.cpp:49:17:49:20 | rref | lvalue |
+| test.cpp:51:12:51:19 | call to get_rref | lvalue |
+| test.cpp:52:13:52:20 | call to get_rref | xvalue |
+| test.cpp:53:13:53:16 | call to move | xvalue |
+| test.cpp:53:18:53:25 | call to get_rref | xvalue |
+| test.cpp:54:12:54:19 | call to get_rref | xvalue |
+| test.cpp:55:12:55:15 | call to move | xvalue |
+| test.cpp:55:17:55:24 | call to get_rref | xvalue |

cpp/common/test/library/codingstandards/cpp/ast/ValueCategory/ValueCategoryTest.ql

@@ -0,0 +1,15 @@
+import cpp
+import codingstandards.cpp.ast.ValueCategory
+
+predicate isRelevant(Expr e) {
+  e.(VariableAccess).getTarget().hasName(["val", "ref", "rref"])
+  or
+  e.(FunctionCall).getTarget().hasName(["get_val", "get_ref", "get_rref", "move"])
+}
+
+from Expr e, ValueCategory cat
+where
+  e.getEnclosingFunction().hasName("main") and
+  isRelevant(e) and
+  cat = getValueCategory(e)
+select e, cat

cpp/common/test/library/codingstandards/cpp/ast/ValueCategory/test.cpp

@@ -0,0 +1,58 @@
+void take_val(int) {}
+void take_ref(int &) {}
+void take_rref(int &&) {}
+template <typename T> void take_uni(T &&) {}
+
+int get_val();
+int &get_ref();
+int &&get_rref();
+
+template <typename T> T &&move(T &t) { return static_cast<T &&>(t); }
+template <typename T> T &&move(T &&t) { return static_cast<T &&>(t); }
+
+int main() {
+  get_val();
+  get_ref();
+  get_rref();
+
+  int val = 42;
+  take_val(val);
+  take_ref(val);
+  take_rref(move(val));
+  take_uni(val);
+  take_uni(move(val));
+
+  take_val(get_val());
+  take_rref(get_val());
+  take_rref(move(get_val()));
+  take_uni(get_val());
+  take_uni(move(get_val()));
+
+  int &ref = val;
+  take_val(ref);
+  take_ref(ref);
+  take_rref(move(ref));
+  take_uni(ref);
+  take_uni(move(ref));
+
+  take_val(get_ref());
+  take_ref(get_ref());
+  take_rref(move(get_ref()));
+  take_uni(get_ref());
+  take_uni(move(get_ref()));
+
+  int &&rref = move(val);
+  take_val(rref);
+  take_ref(rref);
+  take_rref(move(rref));
+  take_uni(rref);
+  take_uni(move(rref));
+
+  take_val(get_rref());
+  take_rref(get_rref());
+  take_rref(move(get_rref()));
+  take_uni(get_rref());
+  take_uni(move(get_rref()));
+
+  return 0;
+}

cpp/misra/src/rules/RULE-28-6-1/StdMoveWithNonConstLvalue.ql

@@ -0,0 +1,56 @@
+/**
+ * @id cpp/misra/std-move-with-non-const-lvalue
+ * @name RULE-28-6-1: The argument to std::move shall be a non-const lvalue
+ * @description Calling std::move on a const lvalue will not result in a move, and calling std::move
+ *              on an rvalue is redundant.
+ * @kind problem
+ * @precision very-high
+ * @problem.severity error
+ * @tags external/misra/id/rule-28-6-1
+ *       scope/single-translation-unit
+ *       correctness
+ *       maintainability
+ *       external/misra/enforcement/decidable
+ *       external/misra/obligation/required
+ */
+
+import cpp
+import codingstandards.cpp.standardlibrary.Utility
+import codingstandards.cpp.ast.ValueCategory
+
+predicate resolvesToConstOrConstRef(Type t) {
+  t.isConst()
+  or
+  resolvesToConstOrConstRef(t.(ReferenceType).getBaseType())
+  or
+  resolvesToConstOrConstRef(t.(TypedefType).getBaseType())
+  or
+  resolvesToConstOrConstRef(t.(Decltype).getBaseType())
+}
+
+predicate isConstLvalue(Expr arg) {
+  getValueCategory(arg).isLValue() and
+  resolvesToConstOrConstRef(arg.getType())
+}
+
+Type typeOfArgument(Expr e) {
+  // An xvalue may be a constructor, which has no return type. However, these xvalues act as values
+  // of the constructed type.
+  if e instanceof ConstructorCall
+  then result = e.(ConstructorCall).getTargetType()
+  else result = e.getType()
+}
+
+from StdMoveCall call, Expr arg, string description
+where
+  arg = call.getArgument(0) and
+  (
+    isConstLvalue(arg) and
+    description = "const " + getValueCategory(arg).toString()
+    or
+    getValueCategory(arg).isRValue() and
+    description = getValueCategory(arg).toString()
+  )
+select call,
+  "Call to 'std::move' with " + description + " argument of type '" + typeOfArgument(arg).toString()
+    + "'."