Skip to content

Commit 4e963a8

Browse files
hvitvedhvitved
authored
Merge pull request #4165 from hvitved/csharp/foreach-guard
C#: Fix bug in guards logic for `foreach` loops
2 parents dc9cc20 + b205702 commit 4e963a8

File tree

6 files changed

+68
-42
lines changed

6 files changed

+68
-42
lines changed

‎csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,12 @@ module AbstractValues {
212212
c.isValidFor(cfe) and
213213
foreachEmptiness(fs, cfe) and
214214
e = fs.getIterableExpr()
215-
)
215+
) and
216+
// Only when taking the non-empty successor do we know that the original iterator
217+
// expression was non-empty. When taking the empty successor, we may have already
218+
// iterated through the `foreach` loop zero or more times, hence the iterator
219+
// expression can be both empty and non-empty
220+
this.isNonEmpty()
216221
}
217222

218223
override EmptyCollectionValue getDualValue() {

‎csharp/ql/test/library-tests/controlflow/guards/AbstractValue.expected‎

Lines changed: 29 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ abstractValue
1818
| 0 | Collections.cs:78:36:78:36 | 0 |
1919
| 0 | Collections.cs:80:35:80:35 | 0 |
2020
| 0 | Collections.cs:81:36:81:36 | 0 |
21-
| 0 | Collections.cs:87:17:87:31 | 0 |
21+
| 0 | Collections.cs:87:17:87:32 | 0 |
2222
| 0 | Guards.cs:12:24:12:24 | 0 |
2323
| 0 | Guards.cs:78:26:78:26 | 0 |
2424
| 0 | Guards.cs:80:25:80:25 | 0 |
@@ -59,8 +59,8 @@ abstractValue
5959
| 1 | Guards.cs:331:17:331:19 | access to constant B |
6060
| 1 | Guards.cs:334:13:334:15 | access to constant B |
6161
| 1 | Guards.cs:335:18:335:18 | 1 |
62-
| 3 | Collections.cs:55:13:55:41 | 3 |
63-
| 3 | Collections.cs:63:17:63:45 | 3 |
62+
| 3 | Collections.cs:55:13:55:42 | 3 |
63+
| 3 | Collections.cs:63:17:63:46 | 3 |
6464
| 10 | Guards.cs:84:25:84:26 | 10 |
6565
| 10 | Guards.cs:86:26:86:27 | 10 |
6666
| empty | Collections.cs:54:13:54:16 | access to parameter args |
@@ -69,22 +69,22 @@ abstractValue
6969
| empty | Collections.cs:58:9:58:13 | ... = ... |
7070
| empty | Collections.cs:58:13:58:13 | access to local variable x |
7171
| empty | Collections.cs:65:13:65:13 | access to local variable x |
72-
| empty | Collections.cs:87:17:87:31 | array creation of type String[] |
73-
| empty | Collections.cs:87:30:87:31 | { ..., ... } |
74-
| empty | Collections.cs:88:22:88:23 | { ..., ... } |
72+
| empty | Collections.cs:87:17:87:32 | array creation of type String[] |
73+
| empty | Collections.cs:87:30:87:32 | { ..., ... } |
74+
| empty | Collections.cs:88:22:88:24 | { ..., ... } |
7575
| false | Guards.cs:178:16:178:20 | false |
7676
| false | Guards.cs:181:53:181:57 | false |
7777
| false | Guards.cs:217:18:217:22 | false |
7878
| false | Guards.cs:228:18:228:22 | false |
7979
| false | Guards.cs:295:18:295:22 | false |
8080
| false | Guards.cs:305:18:305:22 | false |
81-
| non-empty | Collections.cs:55:9:55:41 | ... = ... |
82-
| non-empty | Collections.cs:55:13:55:41 | array creation of type String[] |
83-
| non-empty | Collections.cs:55:25:55:41 | { ..., ... } |
81+
| non-empty | Collections.cs:55:9:55:42 | ... = ... |
82+
| non-empty | Collections.cs:55:13:55:42 | array creation of type String[] |
83+
| non-empty | Collections.cs:55:26:55:42 | { ..., ... } |
8484
| non-empty | Collections.cs:56:9:56:13 | ... = ... |
8585
| non-empty | Collections.cs:56:13:56:13 | access to local variable x |
86-
| non-empty | Collections.cs:63:17:63:45 | array creation of type String[] |
87-
| non-empty | Collections.cs:63:29:63:45 | { ..., ... } |
86+
| non-empty | Collections.cs:63:17:63:46 | array creation of type String[] |
87+
| non-empty | Collections.cs:63:30:63:46 | { ..., ... } |
8888
| non-empty | Collections.cs:68:13:68:13 | access to local variable x |
8989
| non-empty | Collections.cs:89:9:89:32 | ... = ... |
9090
| non-empty | Collections.cs:89:13:89:32 | array creation of type String[] |
@@ -155,11 +155,11 @@ abstractValue
155155
| non-null | Collections.cs:54:13:54:16 | access to parameter args |
156156
| non-null | Collections.cs:54:13:54:26 | call to method ToArray |
157157
| non-null | Collections.cs:55:9:55:9 | access to local variable x |
158-
| non-null | Collections.cs:55:9:55:41 | ... = ... |
159-
| non-null | Collections.cs:55:13:55:41 | array creation of type String[] |
160-
| non-null | Collections.cs:55:27:55:29 | "a" |
161-
| non-null | Collections.cs:55:32:55:34 | "b" |
162-
| non-null | Collections.cs:55:37:55:39 | "c" |
158+
| non-null | Collections.cs:55:9:55:42 | ... = ... |
159+
| non-null | Collections.cs:55:13:55:42 | array creation of type String[] |
160+
| non-null | Collections.cs:55:28:55:30 | "a" |
161+
| non-null | Collections.cs:55:33:55:35 | "b" |
162+
| non-null | Collections.cs:55:38:55:40 | "c" |
163163
| non-null | Collections.cs:56:9:56:9 | access to local variable x |
164164
| non-null | Collections.cs:56:9:56:13 | ... = ... |
165165
| non-null | Collections.cs:56:13:56:13 | access to local variable x |
@@ -169,11 +169,11 @@ abstractValue
169169
| non-null | Collections.cs:58:9:58:9 | access to local variable x |
170170
| non-null | Collections.cs:58:9:58:13 | ... = ... |
171171
| non-null | Collections.cs:58:13:58:13 | access to local variable x |
172-
| non-null | Collections.cs:63:17:63:45 | array creation of type String[] |
173-
| non-null | Collections.cs:63:17:63:54 | call to method ToList |
174-
| non-null | Collections.cs:63:31:63:33 | "a" |
175-
| non-null | Collections.cs:63:36:63:38 | "b" |
176-
| non-null | Collections.cs:63:41:63:43 | "c" |
172+
| non-null | Collections.cs:63:17:63:46 | array creation of type String[] |
173+
| non-null | Collections.cs:63:17:63:55 | call to method ToList |
174+
| non-null | Collections.cs:63:32:63:34 | "a" |
175+
| non-null | Collections.cs:63:37:63:39 | "b" |
176+
| non-null | Collections.cs:63:42:63:44 | "c" |
177177
| non-null | Collections.cs:64:9:64:9 | access to local variable x |
178178
| non-null | Collections.cs:65:13:65:13 | access to local variable x |
179179
| non-null | Collections.cs:67:13:67:13 | access to local variable x |
@@ -214,14 +214,20 @@ abstractValue
214214
| non-null | Collections.cs:82:24:82:30 | access to local function IsEmpty |
215215
| non-null | Collections.cs:82:24:82:30 | delegate creation of type Func<String,Boolean> |
216216
| non-null | Collections.cs:82:24:82:30 | this access |
217-
| non-null | Collections.cs:87:17:87:31 | array creation of type String[] |
218-
| non-null | Collections.cs:88:22:88:23 | array creation of type String[] |
217+
| non-null | Collections.cs:87:17:87:32 | array creation of type String[] |
218+
| non-null | Collections.cs:88:22:88:24 | array creation of type String[] |
219219
| non-null | Collections.cs:89:9:89:9 | access to local variable x |
220220
| non-null | Collections.cs:89:9:89:32 | ... = ... |
221221
| non-null | Collections.cs:89:13:89:32 | array creation of type String[] |
222222
| non-null | Collections.cs:89:28:89:30 | "a" |
223223
| non-null | Collections.cs:90:22:90:28 | array creation of type String[] |
224224
| non-null | Collections.cs:90:24:90:26 | "a" |
225+
| non-null | Collections.cs:95:29:95:32 | access to parameter args |
226+
| non-null | Collections.cs:96:13:96:19 | access to type Console |
227+
| non-null | Collections.cs:96:31:96:34 | access to parameter args |
228+
| non-null | Collections.cs:101:29:101:32 | access to parameter args |
229+
| non-null | Collections.cs:103:9:103:15 | access to type Console |
230+
| non-null | Collections.cs:103:27:103:30 | access to parameter args |
225231
| non-null | Guards.cs:12:13:12:13 | access to parameter s |
226232
| non-null | Guards.cs:14:13:14:19 | access to type Console |
227233
| non-null | Guards.cs:14:31:14:31 | access to parameter s |

‎csharp/ql/test/library-tests/controlflow/guards/Collections.cs‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,15 @@ void M5(List<string> args)
5252
var x = args.ToArray();
5353
args.Clear();
5454
x = args.ToArray();
55-
x = new string[]{ "a", "b", "c" };
55+
x = new string[] { "a", "b", "c" };
5656
x = x;
5757
x = new string[0];
5858
x = x;
5959
}
6060

6161
void M6()
6262
{
63-
var x = new string[]{ "a", "b", "c" }.ToList();
63+
var x = new string[] { "a", "b", "c" }.ToList();
6464
x.Clear();
6565
if (x.Count == 0)
6666
{
@@ -84,10 +84,23 @@ void M7(string[] args)
8484

8585
void M8()
8686
{
87-
var x = new string[] {};
88-
string[] y = {};
87+
var x = new string[] { };
88+
string[] y = { };
8989
x = new string[] { "a" };
9090
string[] z = { "a" };
9191
}
92+
93+
void M9(string[] args)
94+
{
95+
foreach (var arg in args)
96+
Console.WriteLine(args); // guarded by `args` being non-empty
97+
}
98+
99+
void M10(string[] args)
100+
{
101+
foreach (var arg in args)
102+
;
103+
Console.WriteLine(args);
104+
}
92105
}
93106

‎csharp/ql/test/library-tests/controlflow/guards/GuardedControlFlowNode.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
| Collections.cs:67:13:67:13 | access to local variable x | Collections.cs:65:13:65:13 | access to local variable x | Collections.cs:65:13:65:13 | access to local variable x | empty |
3838
| Collections.cs:67:13:67:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
3939
| Collections.cs:68:13:68:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
40+
| Collections.cs:96:31:96:34 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | non-empty |
4041
| Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |
4142
| Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:24 | ... == ... | Guards.cs:10:16:10:16 | access to parameter s | false |
4243
| Guards.cs:14:31:14:31 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |

‎csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
| Collections.cs:67:13:67:13 | access to local variable x | Collections.cs:65:13:65:13 | access to local variable x | Collections.cs:65:13:65:13 | access to local variable x | empty |
3838
| Collections.cs:67:13:67:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
3939
| Collections.cs:68:13:68:13 | access to local variable x | Collections.cs:65:13:65:24 | ... == ... | Collections.cs:65:13:65:13 | access to local variable x | true |
40+
| Collections.cs:96:31:96:34 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | Collections.cs:95:29:95:32 | access to parameter args | non-empty |
4041
| Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |
4142
| Guards.cs:12:13:12:13 | access to parameter s | Guards.cs:10:16:10:24 | ... == ... | Guards.cs:10:16:10:16 | access to parameter s | false |
4243
| Guards.cs:14:31:14:31 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | Guards.cs:10:16:10:16 | access to parameter s | non-null |

‎csharp/ql/test/library-tests/controlflow/guards/Implications.expected‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -180,26 +180,26 @@
180180
| Collections.cs:45:17:45:26 | call to method Any | true | Collections.cs:45:17:45:20 | access to parameter args | non-empty |
181181
| Collections.cs:50:13:50:27 | ... == ... | false | Collections.cs:50:13:50:16 | access to parameter args | non-empty |
182182
| Collections.cs:50:13:50:27 | ... == ... | true | Collections.cs:50:13:50:16 | access to parameter args | empty |
183-
| Collections.cs:56:13:56:13 | access to local variable x | empty | Collections.cs:55:13:55:41 | array creation of type String[] | empty |
184-
| Collections.cs:56:13:56:13 | access to local variable x | non-empty | Collections.cs:55:13:55:41 | array creation of type String[] | non-empty |
185-
| Collections.cs:56:13:56:13 | access to local variable x | non-null | Collections.cs:55:13:55:41 | array creation of type String[] | non-null |
186-
| Collections.cs:56:13:56:13 | access to local variable x | null | Collections.cs:55:13:55:41 | array creation of type String[] | null |
183+
| Collections.cs:56:13:56:13 | access to local variable x | empty | Collections.cs:55:13:55:42 | array creation of type String[] | empty |
184+
| Collections.cs:56:13:56:13 | access to local variable x | non-empty | Collections.cs:55:13:55:42 | array creation of type String[] | non-empty |
185+
| Collections.cs:56:13:56:13 | access to local variable x | non-null | Collections.cs:55:13:55:42 | array creation of type String[] | non-null |
186+
| Collections.cs:56:13:56:13 | access to local variable x | null | Collections.cs:55:13:55:42 | array creation of type String[] | null |
187187
| Collections.cs:58:13:58:13 | access to local variable x | empty | Collections.cs:57:13:57:25 | array creation of type String[] | empty |
188188
| Collections.cs:58:13:58:13 | access to local variable x | non-empty | Collections.cs:57:13:57:25 | array creation of type String[] | non-empty |
189189
| Collections.cs:58:13:58:13 | access to local variable x | non-null | Collections.cs:57:13:57:25 | array creation of type String[] | non-null |
190190
| Collections.cs:58:13:58:13 | access to local variable x | null | Collections.cs:57:13:57:25 | array creation of type String[] | null |
191-
| Collections.cs:64:9:64:9 | access to local variable x | empty | Collections.cs:63:17:63:54 | call to method ToList | empty |
192-
| Collections.cs:64:9:64:9 | access to local variable x | non-empty | Collections.cs:63:17:63:54 | call to method ToList | non-empty |
193-
| Collections.cs:64:9:64:9 | access to local variable x | non-null | Collections.cs:63:17:63:54 | call to method ToList | non-null |
194-
| Collections.cs:64:9:64:9 | access to local variable x | null | Collections.cs:63:17:63:54 | call to method ToList | null |
195-
| Collections.cs:65:13:65:13 | access to local variable x | non-null | Collections.cs:63:17:63:54 | call to method ToList | non-null |
196-
| Collections.cs:65:13:65:13 | access to local variable x | null | Collections.cs:63:17:63:54 | call to method ToList | null |
191+
| Collections.cs:64:9:64:9 | access to local variable x | empty | Collections.cs:63:17:63:55 | call to method ToList | empty |
192+
| Collections.cs:64:9:64:9 | access to local variable x | non-empty | Collections.cs:63:17:63:55 | call to method ToList | non-empty |
193+
| Collections.cs:64:9:64:9 | access to local variable x | non-null | Collections.cs:63:17:63:55 | call to method ToList | non-null |
194+
| Collections.cs:64:9:64:9 | access to local variable x | null | Collections.cs:63:17:63:55 | call to method ToList | null |
195+
| Collections.cs:65:13:65:13 | access to local variable x | non-null | Collections.cs:63:17:63:55 | call to method ToList | non-null |
196+
| Collections.cs:65:13:65:13 | access to local variable x | null | Collections.cs:63:17:63:55 | call to method ToList | null |
197197
| Collections.cs:65:13:65:24 | ... == ... | false | Collections.cs:65:13:65:13 | access to local variable x | non-empty |
198198
| Collections.cs:65:13:65:24 | ... == ... | true | Collections.cs:65:13:65:13 | access to local variable x | empty |
199-
| Collections.cs:67:13:67:13 | access to local variable x | non-null | Collections.cs:63:17:63:54 | call to method ToList | non-null |
200-
| Collections.cs:67:13:67:13 | access to local variable x | null | Collections.cs:63:17:63:54 | call to method ToList | null |
201-
| Collections.cs:68:13:68:13 | access to local variable x | non-null | Collections.cs:63:17:63:54 | call to method ToList | non-null |
202-
| Collections.cs:68:13:68:13 | access to local variable x | null | Collections.cs:63:17:63:54 | call to method ToList | null |
199+
| Collections.cs:67:13:67:13 | access to local variable x | non-null | Collections.cs:63:17:63:55 | call to method ToList | non-null |
200+
| Collections.cs:67:13:67:13 | access to local variable x | null | Collections.cs:63:17:63:55 | call to method ToList | null |
201+
| Collections.cs:68:13:68:13 | access to local variable x | non-null | Collections.cs:63:17:63:55 | call to method ToList | non-null |
202+
| Collections.cs:68:13:68:13 | access to local variable x | null | Collections.cs:63:17:63:55 | call to method ToList | null |
203203
| Collections.cs:74:35:74:41 | ... == ... | true | Collections.cs:74:35:74:35 | access to parameter s | non-null |
204204
| Collections.cs:74:35:74:41 | ... == ... | true | Collections.cs:74:40:74:41 | "" | non-null |
205205
| Collections.cs:75:17:75:33 | call to method Any | true | Collections.cs:75:17:75:20 | access to parameter args | non-empty |

0 commit comments

Comments
 (0)