Skip to content

Commit 7b90ba6

Browse files
d10cd10c
authored
Merge pull request #10550 from d10c/cpp/comma-before-misleading-indentation
2 parents 83464d4 + 949d3e1 commit 7b90ba6

File tree

7 files changed

+342
-0
lines changed

7 files changed

+342
-0
lines changed

‎cpp/ql/src/Best Practices/Likely Errors/CommaBeforeMisleadingIndentation.cpp

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* In this example, the developer intended to use a semicolon but accidentally used a comma:
3+
*/
4+
5+
enum privileges entitlements = NONE;
6+
7+
if (is_admin)
8+
entitlements = FULL, // BAD
9+
10+
restrict_privileges(entitlements);
11+
12+
/*
13+
* The use of a comma means that the first example is equivalent to this second example:
14+
*/
15+
16+
enum privileges entitlements = NONE;
17+
18+
if (is_admin) {
19+
entitlements = FULL;
20+
restrict_privileges(entitlements);
21+
}
22+
23+
/*
24+
* The indentation of the first example suggests that the developer probably intended the following code:
25+
*/
26+
27+
enum privileges entitlements = NONE;
28+
29+
if (is_admin)
30+
entitlements = FULL; // GOOD
31+
32+
restrict_privileges(entitlements);

‎cpp/ql/src/Best Practices/Likely Errors/CommaBeforeMisleadingIndentation.qhelp

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
6+
<overview>
7+
<p>
8+
If the expression after the comma operator starts at an earlier column than the expression before the comma, then
9+
this suspicious indentation possibly indicates a logic error, caused by a typo that may escape visual inspection.
10+
</p>
11+
<warning>
12+
This query has medium precision because CodeQL currently does not distinguish between tabs and spaces in whitespace.
13+
If a file contains mixed tabs and spaces, alerts may highlight code that is correctly indented for one value of tab size but not for other tab sizes.
14+
</warning>
15+
</overview>
16+
17+
<recommendation>
18+
<p>
19+
To ensure that your code is easy to read and review, use standard indentation around the comma operator. Always begin the right-hand-side operand at the same level of
20+
indentation (column number) as the left-hand-side operand. This makes it easier for other developers to see the intended behavior of your code.
21+
</p>
22+
<p>
23+
Use whitespace consistently to communicate your coding intentions. Where possible, avoid mixing tabs and spaces within a file. If you need to mix them, use them consistently.
24+
</p>
25+
</recommendation>
26+
27+
<example>
28+
<p>
29+
This example shows three different ways of writing the same code. The first example contains a comma instead of a semicolon which means that the final line is part of the <code>if</code> statement, even though the indentation suggests that it is intended to be separate. The second example looks different but is functionally the same as the first example. It is more likely that the developer intended to write the third example.
30+
</p>
31+
<sample src="CommaBeforeMisleadingIndentation.cpp" />
32+
</example>
33+
34+
<references>
35+
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Comma_operator">Comma operator</a></li>
36+
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Indentation_style#Tabs,_spaces,_and_size_of_indentations">Indentation style &mdash; Tabs, spaces, and size of indentations</a></li>
37+
</references>
38+
39+
</qhelp>

‎cpp/ql/src/Best Practices/Likely Errors/CommaBeforeMisleadingIndentation.ql

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/**
2+
* @name Comma before misleading indentation
3+
* @description If expressions before and after a comma operator use different indentation, it is easy to misread the purpose of the code.
4+
* @kind problem
5+
* @id cpp/comma-before-misleading-indentation
6+
* @problem.severity warning
7+
* @security-severity 7.8
8+
* @precision medium
9+
* @tags maintainability
10+
* readability
11+
* security
12+
* external/cwe/cwe-1078
13+
* external/cwe/cwe-670
14+
*/
15+
16+
import cpp
17+
import semmle.code.cpp.commons.Exclusions
18+
19+
/** Gets the sub-expression of 'e' with the earliest-starting Location */
20+
Expr normalizeExpr(Expr e) {
21+
result =
22+
min(Expr child |
23+
child.getParentWithConversions*() = e.getFullyConverted() and
24+
not child.getParentWithConversions*() = any(Call c).getAnArgument()
25+
|
26+
child order by child.getLocation().getStartColumn(), count(child.getParentWithConversions*())
27+
)
28+
}
29+
30+
predicate isParenthesized(CommaExpr ce) {
31+
ce.getParent*().(Expr).isParenthesised()
32+
or
33+
ce.isUnevaluated() // sizeof(), decltype(), alignof(), noexcept(), typeid()
34+
or
35+
ce.getParent*() = [any(IfStmt i).getCondition(), any(SwitchStmt s).getExpr()]
36+
or
37+
ce.getParent*() = [any(Loop l).getCondition(), any(ForStmt f).getUpdate()]
38+
or
39+
ce.getEnclosingStmt() = any(ForStmt f).getInitialization()
40+
}
41+
42+
from CommaExpr ce, Expr left, Expr right, Location leftLoc, Location rightLoc
43+
where
44+
ce.fromSource() and
45+
not isFromMacroDefinition(ce) and
46+
left = normalizeExpr(ce.getLeftOperand()) and
47+
right = normalizeExpr(ce.getRightOperand()) and
48+
leftLoc = left.getLocation() and
49+
rightLoc = right.getLocation() and
50+
not isParenthesized(ce) and
51+
leftLoc.getEndLine() < rightLoc.getStartLine() and
52+
leftLoc.getStartColumn() > rightLoc.getStartColumn()
53+
select right, "The indentation level may be misleading for some tab sizes."

‎cpp/ql/src/change-notes/2022-09-30-comma-before-missing-indentation.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new medium-precision query, `cpp/comma-before-misleading-indentation`, which detects instances of whitespace that have readability issues.

‎cpp/ql/test/query-tests/Best Practices/Likely Errors/CommaBeforeMisleadingIndentation/CommaBeforeMisleadingIndentation.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
| test.cpp:49:2:49:8 | (void)... | The indentation level may be misleading for some tab sizes. |
2+
| test.cpp:52:2:52:15 | (void)... | The indentation level may be misleading for some tab sizes. |
3+
| test.cpp:160:3:160:9 | (void)... | The indentation level may be misleading for some tab sizes. |
4+
| test.cpp:166:5:166:7 | ... ++ | The indentation level may be misleading for some tab sizes. |
5+
| test.cpp:176:6:178:6 | ... ? ... : ... | The indentation level may be misleading for some tab sizes. |

‎cpp/ql/test/query-tests/Best Practices/Likely Errors/CommaBeforeMisleadingIndentation/CommaBeforeMisleadingIndentation.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Best Practices/Likely Errors/CommaBeforeMisleadingIndentation.ql

‎cpp/ql/test/query-tests/Best Practices/Likely Errors/CommaBeforeMisleadingIndentation/test.cpp

Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
// clang-format off
2+
3+
typedef unsigned size_t;
4+
5+
struct X {
6+
int foo(int y) { return y; }
7+
} x;
8+
9+
#define FOO(x) ( \
10+
(x), \
11+
(x) \
12+
)
13+
14+
#define BAR(x, y) ((x), (y))
15+
16+
#define BAZ //printf
17+
18+
struct Foo {
19+
int i, i_array[3];
20+
int j;
21+
virtual int foo(int) = 0;
22+
virtual int bar(int, int) = 0;
23+
int test(int (*baz)(int));
24+
25+
struct Tata {
26+
struct Titi {
27+
void tutu() {}
28+
long toto() { return 42; }
29+
} titi;
30+
31+
Titi *operator->() { return &titi; }
32+
} *tata;
33+
};
34+
35+
int Foo::test(int (*baz)(int))
36+
{
37+
// Comma in simple if statement (prototypical example):
38+
39+
if (i)
40+
(void)i, // GOOD
41+
j++;
42+
43+
if (i)
44+
this->foo(i), // GOOD
45+
foo(i);
46+
47+
if (i)
48+
(void)i, // BAD
49+
(void)j;
50+
51+
if (1) FOO(i),
52+
(void)x.foo(j); // BAD
53+
54+
// Parenthesized comma (borderline example):
55+
56+
foo(i++), j++; // GOOD
57+
(foo(i++), j++); // GOOD
58+
(foo(i++), // GOOD
59+
j++);
60+
(foo(i++),
61+
foo(i++),
62+
j++, // GOOD (?) -- Currently explicitly excluded
63+
j++);
64+
65+
x.foo(i++), j++; // GOOD
66+
(x.foo(i++), j++); // GOOD
67+
(x.foo(i++), // GOOD
68+
j++);
69+
(x.foo(i++),
70+
x.foo(i++),
71+
j++, // GOOD (?) -- Currently explicitly excluded
72+
j++);
73+
74+
FOO(i++), j++; // GOOD
75+
(FOO(i++), j++); // GOOD
76+
(FOO(i++), // GOOD
77+
j++);
78+
(FOO(i++),
79+
FOO(i++),
80+
j++, // GOOD (?) -- Currently explicitly excluded
81+
j++);
82+
83+
(void)(i++), j++; // GOOD
84+
((void)(i++), j++); // GOOD
85+
((void)(i++), // GOOD
86+
j++);
87+
((void)(i++),
88+
(void)(i++),
89+
j++, // GOOD (?) -- Currently explicitly excluded
90+
j++);
91+
92+
// Comma in argument list doesn't count:
93+
94+
bar(i++, j++); // GOOD
95+
bar(i++,
96+
j++); // GOOD
97+
bar(i++
98+
, j++); // GOOD
99+
bar(i++,
100+
j++); // GOOD: common pattern and unlikely to be misread.
101+
102+
BAR(i++, j++); // GOOD
103+
BAR(i++,
104+
j++); // GOOD
105+
BAR(i++
106+
, j++); // GOOD
107+
BAR(i++,
108+
j++); // GOOD: common pattern and unlikely to be misread.
109+
110+
using T = decltype(x.foo(i++), // GOOD
111+
j++);
112+
(void)sizeof(x.foo(i++), // GOOD
113+
j++);
114+
using U = decltype(x.foo(i++), // GOOD? Unlikely to be misread
115+
j++);
116+
(void)sizeof(x.foo(i++), // GOOD? Unlikely to be misread
117+
j++);
118+
119+
BAZ("%d %d\n", i,
120+
j); // GOOD -- Currently explicitly excluded
121+
122+
// Comma in loops
123+
124+
while (i = foo(j++), // GOOD
125+
i != j && i != 42 &&
126+
!foo(j)) {
127+
i = j = i + j;
128+
}
129+
130+
while (i = foo(j++), // GOOD??? Currently ignoring loop heads
131+
i != j && i != 42 && !foo(j)) {
132+
i = j = i + j;
133+
}
134+
135+
for (i = 0, // GOOD? Currently ignoring loop heads.
136+
j = 1;
137+
i + j < 10;
138+
i++, j++);
139+
140+
for (i = 0,
141+
j = 1; i < 10; i += 2, // GOOD? Currently ignoring loop heads.
142+
j++) {}
143+
144+
// Comma in if-conditions:
145+
146+
if (i = foo(j++),
147+
i == j) // GOOD(?) -- Currently ignoring if-conditions for the same reason as other parenthesized commas.
148+
i = 0;
149+
150+
// Mixed tabs and spaces (ugly case):
151+
152+
for (i = 0, // GOOD if tab >= 4 spaces else BAD -- Currently ignoring loop heads.
153+
j = 0;
154+
i + j < 10;
155+
i++, // GOOD if tab >= 4 spaces else BAD -- Currently ignoring loop heads.
156+
j++);
157+
158+
if (i)
159+
(void)i, // GOOD if tab >= 4 spaces else BAD -- can't exclude w/o source code text :/
160+
(void)j;
161+
162+
// LHS ends on same line RHS begins on:
163+
164+
if (1) foo(
165+
i++
166+
), j++; // GOOD? [FALSE POSITIVE]
167+
168+
if (1) baz(
169+
i++
170+
), j++; // GOOD... when calling a function pointer..!?
171+
172+
// Weird cases:
173+
174+
if (foo(j))
175+
return i++
176+
, i++ // GOOD(?) [FALSE POSITIVE] -- can't exclude w/o source code text :/
177+
? 1
178+
: 2;
179+
180+
int quux =
181+
(tata->titi.tutu(),
182+
foo(tata->titi.toto())); // GOOD
183+
184+
(*tata)->toto(), // GOOD
185+
i_array[i] += (int)(*tata)->toto();
186+
187+
return quux;
188+
}
189+
190+
// Comma in variadic template splice:
191+
192+
namespace std {
193+
template <size_t... Is>
194+
struct index_sequence {};
195+
}
196+
197+
template <size_t I>
198+
struct zip_index {};
199+
200+
template <size_t I>
201+
int& at(zip_index<I>) { throw 1; }
202+
203+
template <class Fn, class At, size_t... Is>
204+
void for_each_input(Fn&& fn, std::index_sequence<Is...>) {
205+
(fn(zip_index<Is>{}, at(zip_index<Is>{})), ...); // GOOD
206+
}
207+
208+
// clang-format on

0 commit comments

Comments
 (0)