Skip to content

Commit 4e83559

Browse files
Merge pull request #992 from github/michaelrfairhurst/preconditions5-only-std-move-lvalues
Implement rule 28-6-1, only move non-const lvalues
2 parents 8e66563 + 78a2397 commit 4e83559

12 files changed

Lines changed: 650 additions & 1 deletion

File tree

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import cpp
2+
3+
/**
4+
* Get an expression's value category as a ValueCategory object.
5+
*
6+
* Note that the standard cpp library exposes `is{_}ValueCategory` predicates, but they do not
7+
* necessarily work as expected due to how CodeQL handles reference adjustment and binding, which
8+
* this predicate attempts to handle. There are additional unhandled cases involving
9+
* lvalue-to-rvalue conversions as well.
10+
*
11+
* In C++17, an expression of type "reference to T" is adjusted to type T as stated in [expr]/5.
12+
* This is not a conversion, but in CodeQL this is handled by `ReferenceDereferenceExpr`, a type of
13+
* `Conversion`. Similarly, the binding of references to values is described in [dcl.init.ref],
14+
* which is not a conversion, but in CodeQL this is handled by `ReferenceToExpr`, another type of
15+
* `Conversion`.
16+
*
17+
* Furthermore, the `Expr` predicate `hasLValueToRValueConversion()` uses a different dbscheme table
18+
* than the `Conversion` table, and it is possible to have expressions such that
19+
* `hasLValueToRValueConversion()` holds, but there is no corresponding `Conversion` entry.
20+
*
21+
* Therefore, the value categories of expressions before `ReferenceDereferenceExpr` and after
22+
* `ReferenceToExpr` conversions are therefore essentially unspecified, and the `is{_}ValueCategory`
23+
* predicate results should not be relied upon. And types that are missing a
24+
* `LvalueToRValueConversion` will also return incorrect value categories.
25+
*
26+
* For example, in CodeQL 2.21.4:
27+
*
28+
* ```cpp
29+
* int &i = ...;
30+
* auto r = std::move(i); // 1.) i is a `prvalue(load)` in CodeQL, not an lvalue.
31+
* // 2.) i has a `ReferenceDereferenceExpr` conversion of lvalue category
32+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion of prvalue
33+
* // category.
34+
* int i2 = ...;
35+
* f(i2); // 1.) i2 is an lvalue.
36+
* // 2.) i2 undergoes lvalue-to-rvalue conversion, but there is no corresponding `Conversion`.
37+
* // 3.) i2 is treated at a prvalue by CodeQL, but `hasLValueToRValueConversion()` holds.
38+
*
39+
* int get_int();
40+
* auto r2 = std::move(get_int()); // 1.) get_int() itself is treated as a prvalue by CodeQL.
41+
* // 2.) get_int() has a `TemporaryObjectExpr` conversion of lvalue
42+
* // category.
43+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
44+
* // of prvalue category.
45+
* std::string get_str();
46+
* auto r3 = std::move(get_str()); // 1.) get_str() is treated as a prvalue by CodeQL.
47+
* // 2.) get_str() has a `TemporaryObjectExpr` conversion of xvalue
48+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
49+
* // of prvalue category.
50+
* std::string &str_ref();
51+
* auto r3 = std::move(str_ref()); // 1.) str_ref() is treated as a prvalue by CodeQL.
52+
* // 2.) str_ref() has a `ReferenceDereferenceExpr` conversion of
53+
* // lvalue.
54+
* // 3.) the conversion from 2 has a `ReferenceToExpr` conversion
55+
* // of prvalue category.
56+
* ```
57+
*
58+
* As can be seen above, the value categories of expressions are correct between the
59+
* `ReferenceDereferenceExpr` and `ReferenceToExpr`, but not necessarily before or after.
60+
*
61+
* We must also check for `hasLValueToRValueConversion()` and handle that appropriately.
62+
*/
63+
ValueCategory getValueCategory(Expr e) {
64+
// If `e` is adjusted from a reference to a value (C++17 [expr]/5) then we want the value category
65+
// of the expression after `ReferenceDereferenceExpr`.
66+
if e.getConversion() instanceof ReferenceDereferenceExpr
67+
then result = getValueCategory(e.getConversion())
68+
else (
69+
// If `hasLValueToRValueConversion()` holds, then ensure we have an lvalue category.
70+
if e.hasLValueToRValueConversion()
71+
then result.isLValue()
72+
else
73+
// Otherwise, get the value category from `is{_}ValueCategory` predicates as normal.
74+
result = getDirectValueCategory(e)
75+
)
76+
}
77+
78+
/**
79+
* Gets the value category of an expression using `is{_}ValueCategory` predicates, without looking
80+
* through conversions.
81+
*/
82+
private ValueCategory getDirectValueCategory(Expr e) {
83+
if e.isLValueCategory()
84+
then result = LValue(e.getValueCategoryString())
85+
else
86+
if e.isPRValueCategory()
87+
then result = PRValue(e.getValueCategoryString())
88+
else
89+
if e.isXValueCategory()
90+
then result = XValue(e.getValueCategoryString())
91+
else none()
92+
}
93+
94+
newtype TValueCategory =
95+
LValue(string descr) {
96+
exists(Expr e | e.isLValueCategory() and descr = e.getValueCategoryString())
97+
} or
98+
PRValue(string descr) {
99+
exists(Expr e | e.isPRValueCategory() and descr = e.getValueCategoryString())
100+
} or
101+
XValue(string descr) {
102+
exists(Expr e | e.isXValueCategory() and descr = e.getValueCategoryString())
103+
}
104+
105+
/**
106+
* A value category, which can be an lvalue, prvalue, or xvalue.
107+
*
108+
* Note that prvalue has two possible forms: `prvalue` and `prvalue(load)`.
109+
*/
110+
class ValueCategory extends TValueCategory {
111+
string description;
112+
113+
ValueCategory() {
114+
this = LValue(description) or this = PRValue(description) or this = XValue(description)
115+
}
116+
117+
predicate isLValue() { this instanceof LValue }
118+
119+
predicate isPRValue() { this instanceof PRValue }
120+
121+
predicate isXValue() { this instanceof XValue }
122+
123+
predicate isRValue() { this instanceof PRValue or this instanceof XValue }
124+
125+
predicate isGLValue() { this instanceof LValue or this instanceof XValue }
126+
127+
string toString() { result = description }
128+
}
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
//** THIS FILE IS AUTOGENERATED, DO NOT MODIFY DIRECTLY. **/
2+
import cpp
3+
import RuleMetadata
4+
import codingstandards.cpp.exclusions.RuleMetadata
5+
6+
newtype Preconditions5Query = TStdMoveWithNonConstLvalueQuery()
7+
8+
predicate isPreconditions5QueryMetadata(Query query, string queryId, string ruleId, string category) {
9+
query =
10+
// `Query` instance for the `stdMoveWithNonConstLvalue` query
11+
Preconditions5Package::stdMoveWithNonConstLvalueQuery() and
12+
queryId =
13+
// `@id` for the `stdMoveWithNonConstLvalue` query
14+
"cpp/misra/std-move-with-non-const-lvalue" and
15+
ruleId = "RULE-28-6-1" and
16+
category = "required"
17+
}
18+
19+
module Preconditions5Package {
20+
Query stdMoveWithNonConstLvalueQuery() {
21+
//autogenerate `Query` type
22+
result =
23+
// `Query` type for `stdMoveWithNonConstLvalue` query
24+
TQueryCPP(TPreconditions5PackageQuery(TStdMoveWithNonConstLvalueQuery()))
25+
}
26+
}

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ import Pointers
8282
import Preconditions1
8383
import Preconditions3
8484
import Preconditions4
85+
import Preconditions5
8586
import Preprocessor
8687
import Preprocessor2
8788
import Representation
@@ -187,6 +188,7 @@ newtype TCPPQuery =
187188
TPreconditions1PackageQuery(Preconditions1Query q) or
188189
TPreconditions3PackageQuery(Preconditions3Query q) or
189190
TPreconditions4PackageQuery(Preconditions4Query q) or
191+
TPreconditions5PackageQuery(Preconditions5Query q) or
190192
TPreprocessorPackageQuery(PreprocessorQuery q) or
191193
TPreprocessor2PackageQuery(Preprocessor2Query q) or
192194
TRepresentationPackageQuery(RepresentationQuery q) or
@@ -292,6 +294,7 @@ predicate isQueryMetadata(Query query, string queryId, string ruleId, string cat
292294
isPreconditions1QueryMetadata(query, queryId, ruleId, category) or
293295
isPreconditions3QueryMetadata(query, queryId, ruleId, category) or
294296
isPreconditions4QueryMetadata(query, queryId, ruleId, category) or
297+
isPreconditions5QueryMetadata(query, queryId, ruleId, category) or
295298
isPreprocessorQueryMetadata(query, queryId, ruleId, category) or
296299
isPreprocessor2QueryMetadata(query, queryId, ruleId, category) or
297300
isRepresentationQueryMetadata(query, queryId, ruleId, category) or
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
| test.cpp:14:3:14:9 | call to get_val | prvalue |
2+
| test.cpp:15:3:15:9 | call to get_ref | lvalue |
3+
| test.cpp:16:3:16:10 | call to get_rref | xvalue |
4+
| test.cpp:19:12:19:14 | val | lvalue |
5+
| test.cpp:20:12:20:14 | val | lvalue |
6+
| test.cpp:21:13:21:16 | call to move | xvalue |
7+
| test.cpp:21:18:21:20 | val | lvalue |
8+
| test.cpp:22:12:22:14 | val | lvalue |
9+
| test.cpp:23:12:23:15 | call to move | xvalue |
10+
| test.cpp:23:17:23:19 | val | lvalue |
11+
| test.cpp:25:12:25:18 | call to get_val | prvalue |
12+
| test.cpp:26:13:26:19 | call to get_val | prvalue |
13+
| test.cpp:27:13:27:16 | call to move | xvalue |
14+
| test.cpp:27:18:27:24 | call to get_val | prvalue |
15+
| test.cpp:28:12:28:18 | call to get_val | prvalue |
16+
| test.cpp:29:12:29:15 | call to move | xvalue |
17+
| test.cpp:29:17:29:23 | call to get_val | prvalue |
18+
| test.cpp:31:14:31:16 | val | lvalue |
19+
| test.cpp:32:12:32:14 | ref | lvalue |
20+
| test.cpp:33:12:33:14 | ref | lvalue |
21+
| test.cpp:34:13:34:16 | call to move | xvalue |
22+
| test.cpp:34:18:34:20 | ref | lvalue |
23+
| test.cpp:35:12:35:14 | ref | lvalue |
24+
| test.cpp:36:12:36:15 | call to move | xvalue |
25+
| test.cpp:36:17:36:19 | ref | lvalue |
26+
| test.cpp:38:12:38:18 | call to get_ref | lvalue |
27+
| test.cpp:39:12:39:18 | call to get_ref | lvalue |
28+
| test.cpp:40:13:40:16 | call to move | xvalue |
29+
| test.cpp:40:18:40:24 | call to get_ref | lvalue |
30+
| test.cpp:41:12:41:18 | call to get_ref | lvalue |
31+
| test.cpp:42:12:42:15 | call to move | xvalue |
32+
| test.cpp:42:17:42:23 | call to get_ref | lvalue |
33+
| test.cpp:44:16:44:19 | call to move | xvalue |
34+
| test.cpp:44:21:44:23 | val | lvalue |
35+
| test.cpp:45:12:45:15 | rref | lvalue |
36+
| test.cpp:46:12:46:15 | rref | lvalue |
37+
| test.cpp:47:13:47:16 | call to move | xvalue |
38+
| test.cpp:47:18:47:21 | rref | lvalue |
39+
| test.cpp:48:12:48:15 | rref | lvalue |
40+
| test.cpp:49:12:49:15 | call to move | xvalue |
41+
| test.cpp:49:17:49:20 | rref | lvalue |
42+
| test.cpp:51:12:51:19 | call to get_rref | lvalue |
43+
| test.cpp:52:13:52:20 | call to get_rref | xvalue |
44+
| test.cpp:53:13:53:16 | call to move | xvalue |
45+
| test.cpp:53:18:53:25 | call to get_rref | xvalue |
46+
| test.cpp:54:12:54:19 | call to get_rref | xvalue |
47+
| test.cpp:55:12:55:15 | call to move | xvalue |
48+
| test.cpp:55:17:55:24 | call to get_rref | xvalue |
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import cpp
2+
import codingstandards.cpp.ast.ValueCategory
3+
4+
predicate isRelevant(Expr e) {
5+
e.(VariableAccess).getTarget().hasName(["val", "ref", "rref"])
6+
or
7+
e.(FunctionCall).getTarget().hasName(["get_val", "get_ref", "get_rref", "move"])
8+
}
9+
10+
from Expr e, ValueCategory cat
11+
where
12+
e.getEnclosingFunction().hasName("main") and
13+
isRelevant(e) and
14+
cat = getValueCategory(e)
15+
select e, cat
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
void take_val(int) {}
2+
void take_ref(int &) {}
3+
void take_rref(int &&) {}
4+
template <typename T> void take_uni(T &&) {}
5+
6+
int get_val();
7+
int &get_ref();
8+
int &&get_rref();
9+
10+
template <typename T> T &&move(T &t) { return static_cast<T &&>(t); }
11+
template <typename T> T &&move(T &&t) { return static_cast<T &&>(t); }
12+
13+
int main() {
14+
get_val();
15+
get_ref();
16+
get_rref();
17+
18+
int val = 42;
19+
take_val(val);
20+
take_ref(val);
21+
take_rref(move(val));
22+
take_uni(val);
23+
take_uni(move(val));
24+
25+
take_val(get_val());
26+
take_rref(get_val());
27+
take_rref(move(get_val()));
28+
take_uni(get_val());
29+
take_uni(move(get_val()));
30+
31+
int &ref = val;
32+
take_val(ref);
33+
take_ref(ref);
34+
take_rref(move(ref));
35+
take_uni(ref);
36+
take_uni(move(ref));
37+
38+
take_val(get_ref());
39+
take_ref(get_ref());
40+
take_rref(move(get_ref()));
41+
take_uni(get_ref());
42+
take_uni(move(get_ref()));
43+
44+
int &&rref = move(val);
45+
take_val(rref);
46+
take_ref(rref);
47+
take_rref(move(rref));
48+
take_uni(rref);
49+
take_uni(move(rref));
50+
51+
take_val(get_rref());
52+
take_rref(get_rref());
53+
take_rref(move(get_rref()));
54+
take_uni(get_rref());
55+
take_uni(move(get_rref()));
56+
57+
return 0;
58+
}
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/**
2+
* @id cpp/misra/std-move-with-non-const-lvalue
3+
* @name RULE-28-6-1: The argument to std::move shall be a non-const lvalue
4+
* @description Calling std::move on a const lvalue will not result in a move, and calling std::move
5+
* on an rvalue is redundant.
6+
* @kind problem
7+
* @precision very-high
8+
* @problem.severity error
9+
* @tags external/misra/id/rule-28-6-1
10+
* scope/single-translation-unit
11+
* correctness
12+
* maintainability
13+
* external/misra/enforcement/decidable
14+
* external/misra/obligation/required
15+
*/
16+
17+
import cpp
18+
import codingstandards.cpp.standardlibrary.Utility
19+
import codingstandards.cpp.ast.ValueCategory
20+
21+
predicate resolvesToConstOrConstRef(Type t) {
22+
t.isConst()
23+
or
24+
resolvesToConstOrConstRef(t.(ReferenceType).getBaseType())
25+
or
26+
resolvesToConstOrConstRef(t.(TypedefType).getBaseType())
27+
or
28+
resolvesToConstOrConstRef(t.(Decltype).getBaseType())
29+
}
30+
31+
predicate isConstLvalue(Expr arg) {
32+
getValueCategory(arg).isLValue() and
33+
resolvesToConstOrConstRef(arg.getType())
34+
}
35+
36+
Type typeOfArgument(Expr e) {
37+
// An xvalue may be a constructor, which has no return type. However, these xvalues act as values
38+
// of the constructed type.
39+
if e instanceof ConstructorCall
40+
then result = e.(ConstructorCall).getTargetType()
41+
else result = e.getType()
42+
}
43+
44+
from StdMoveCall call, Expr arg, string description
45+
where
46+
arg = call.getArgument(0) and
47+
(
48+
isConstLvalue(arg) and
49+
description = "const " + getValueCategory(arg).toString()
50+
or
51+
getValueCategory(arg).isRValue() and
52+
description = getValueCategory(arg).toString()
53+
)
54+
select call,
55+
"Call to 'std::move' with " + description + " argument of type '" + typeOfArgument(arg).toString()
56+
+ "'."

0 commit comments

Comments
 (0)