Skip to content

Commit 8b465e8

Browse files
hvitvedhvitved
authored
Merge pull request github#5820 from hvitved/csharp/cfg/constructor-same-compilation
C#: Improve CFG for constructors when there are multiple implementations
2 parents 65ac5b8 + 182b2d0 commit 8b465e8

File tree

12 files changed

+246
-181
lines changed

12 files changed

+246
-181
lines changed

‎csharp/ql/src/semmle/code/csharp/controlflow/internal/ControlFlowGraphImpl.qll

Lines changed: 44 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,27 @@ private import SuccessorType
4949
private import SuccessorTypes
5050
private import Splitting
5151
private import semmle.code.csharp.ExprOrStmtParent
52+
private import semmle.code.csharp.commons.Compilation
53+
54+
/**
55+
* A compilation.
56+
*
57+
* Unlike the standard `Compilation` class, this class also supports buildless
58+
* extraction.
59+
*/
60+
newtype CompilationExt =
61+
TCompilation(Compilation c) { not extractionIsStandalone() } or
62+
TBuildless() { extractionIsStandalone() }
63+
64+
/** Gets the compilation that source file `f` belongs to. */
65+
CompilationExt getCompilation(SourceFile f) {
66+
exists(Compilation c |
67+
f = c.getAFileCompiled() and
68+
result = TCompilation(c)
69+
)
70+
or
71+
result = TBuildless()
72+
}
5273

5374
/** An element that defines a new CFG scope. */
5475
class CfgScope extends Element, @top_level_exprorstmt_parent {
@@ -135,10 +156,7 @@ predicate scopeFirst(CfgScope scope, ControlFlowElement first) {
135156
then first(c.(Constructor).getInitializer(), first)
136157
else
137158
if InitializerSplitting::constructorInitializes(c, _)
138-
then
139-
first(any(InitializerSplitting::InitializedInstanceMember m |
140-
InitializerSplitting::constructorInitializeOrder(c, m, 0)
141-
).getInitializer(), first)
159+
then first(InitializerSplitting::constructorInitializeOrder(c, _, 0), first)
142160
else first(c.getBody(), first)
143161
)
144162
or
@@ -152,36 +170,36 @@ predicate scopeLast(Callable scope, ControlFlowElement last, Completion c) {
152170
last(scope.getBody(), last, c) and
153171
not c instanceof GotoCompletion
154172
or
155-
exists(InitializerSplitting::InitializedInstanceMember m |
156-
m = InitializerSplitting::lastConstructorInitializer(scope) and
157-
last(m.getInitializer(), last, c) and
158-
not scope.hasBody()
159-
)
173+
last(InitializerSplitting::lastConstructorInitializer(scope, _), last, c) and
174+
not scope.hasBody()
160175
}
161176

162-
private class CallableTree extends ControlFlowTree, Callable {
177+
private class ConstructorTree extends ControlFlowTree, Constructor {
163178
final override predicate propagatesAbnormal(ControlFlowElement child) { none() }
164179

165180
final override predicate first(ControlFlowElement first) { none() }
166181

167182
final override predicate last(ControlFlowElement last, Completion c) { none() }
168183

184+
/** Gets the body of this constructor belonging to compilation `comp`. */
185+
pragma[noinline]
186+
ControlFlowElement getBody(CompilationExt comp) {
187+
result = this.getBody() and
188+
comp = getCompilation(result.getFile())
189+
}
190+
169191
final override predicate succ(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
170-
exists(Constructor con, InitializerSplitting::InitializedInstanceMember m, int i |
171-
this = con and
172-
last(m.getInitializer(), pred, c) and
173-
c instanceof NormalCompletion and
174-
InitializerSplitting::constructorInitializeOrder(con, m, i)
192+
exists(CompilationExt comp, int i, AssignExpr ae |
193+
ae = InitializerSplitting::constructorInitializeOrder(this, comp, i) and
194+
last(ae, pred, c) and
195+
c instanceof NormalCompletion
175196
|
176197
// Flow from one member initializer to the next
177-
exists(InitializerSplitting::InitializedInstanceMember next |
178-
InitializerSplitting::constructorInitializeOrder(con, next, i + 1) and
179-
first(next.getInitializer(), succ)
180-
)
198+
first(InitializerSplitting::constructorInitializeOrder(this, comp, i + 1), succ)
181199
or
182200
// Flow from last member initializer to constructor body
183-
m = InitializerSplitting::lastConstructorInitializer(con) and
184-
first(con.getBody(), succ)
201+
ae = InitializerSplitting::lastConstructorInitializer(this, comp) and
202+
first(this.getBody(comp), succ)
185203
)
186204
}
187205
}
@@ -884,21 +902,18 @@ module Expressions {
884902
first(this.getChildElement(i + 1), succ)
885903
)
886904
or
887-
exists(Constructor con |
905+
exists(ConstructorTree con, CompilationExt comp |
888906
last(this, pred, c) and
889907
con = this.getConstructor() and
908+
comp = getCompilation(this.getFile()) and
890909
c instanceof NormalCompletion
891910
|
892911
// Flow from constructor initializer to first member initializer
893-
exists(InitializerSplitting::InitializedInstanceMember m |
894-
InitializerSplitting::constructorInitializeOrder(con, m, 0)
895-
|
896-
first(m.getInitializer(), succ)
897-
)
912+
first(InitializerSplitting::constructorInitializeOrder(con, comp, 0), succ)
898913
or
899914
// Flow from constructor initializer to first element of constructor body
900-
not InitializerSplitting::constructorInitializeOrder(con, _, _) and
901-
first(con.getBody(), succ)
915+
not exists(InitializerSplitting::constructorInitializeOrder(con, comp, _)) and
916+
first(con.getBody(comp), succ)
902917
)
903918
}
904919
}

‎csharp/ql/src/semmle/code/csharp/controlflow/internal/Splitting.qll

Lines changed: 43 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -218,59 +218,65 @@ module InitializerSplitting {
218218
* A non-static member with an initializer, for example a field `int Field = 0`.
219219
*/
220220
class InitializedInstanceMember extends Member {
221-
private AssignExpr ae;
222-
223221
InitializedInstanceMember() {
224-
not this.isStatic() and
225-
expr_parent_top_level_adjusted(ae, _, this) and
226-
not ae = any(Callable c).getExpressionBody()
222+
exists(AssignExpr ae |
223+
not this.isStatic() and
224+
expr_parent_top_level(ae, _, this) and
225+
not ae = any(Callable c).getExpressionBody()
226+
)
227227
}
228228

229229
/** Gets the initializer expression. */
230-
AssignExpr getInitializer() { result = ae }
230+
AssignExpr getInitializer() { expr_parent_top_level(result, _, this) }
231231

232232
/**
233233
* Gets a control flow element that is a syntactic descendant of the
234234
* initializer expression.
235235
*/
236236
ControlFlowElement getAnInitializerDescendant() {
237-
result = ae
237+
result = this.getInitializer()
238238
or
239239
result = this.getAnInitializerDescendant().getAChild()
240240
}
241241
}
242242

243243
/**
244244
* Holds if `c` is a non-static constructor that performs the initialization
245-
* of member `m`.
245+
* of a member via assignment `init`.
246246
*/
247-
predicate constructorInitializes(InstanceConstructor c, InitializedInstanceMember m) {
248-
c.isUnboundDeclaration() and
249-
c.getDeclaringType().getAMember() = m and
250-
not c.getInitializer().isThis()
247+
predicate constructorInitializes(InstanceConstructor c, AssignExpr init) {
248+
exists(InitializedInstanceMember m |
249+
c.isUnboundDeclaration() and
250+
c.getDeclaringType().getAMember() = m and
251+
not c.getInitializer().isThis() and
252+
init = m.getInitializer()
253+
)
251254
}
252255

253256
/**
254-
* Holds if `m` is the `i`th member initialized by non-static constructor `c`.
257+
* Gets the `i`th member initializer expression for non-static constructor `c`
258+
* in compilation `comp`.
255259
*/
256-
predicate constructorInitializeOrder(Constructor c, InitializedInstanceMember m, int i) {
257-
constructorInitializes(c, m) and
258-
m =
259-
rank[i + 1](InitializedInstanceMember m0 |
260-
constructorInitializes(c, m0)
260+
AssignExpr constructorInitializeOrder(Constructor c, CompilationExt comp, int i) {
261+
constructorInitializes(c, result) and
262+
result =
263+
rank[i + 1](AssignExpr ae0, Location l |
264+
constructorInitializes(c, ae0) and
265+
l = ae0.getLocation() and
266+
getCompilation(l.getFile()) = comp
261267
|
262-
m0
263-
order by
264-
m0.getLocation().getStartLine(), m0.getLocation().getStartColumn(),
265-
m0.getLocation().getFile().getAbsolutePath()
268+
ae0 order by l.getStartLine(), l.getStartColumn(), l.getFile().getAbsolutePath()
266269
)
267270
}
268271

269-
/** Gets the last member initialized by non-static constructor `c`. */
270-
InitializedInstanceMember lastConstructorInitializer(Constructor c) {
272+
/**
273+
* Gets the last member initializer expression for non-static constructor `c`
274+
* in compilation `comp`.
275+
*/
276+
AssignExpr lastConstructorInitializer(Constructor c, CompilationExt comp) {
271277
exists(int i |
272-
constructorInitializeOrder(c, result, i) and
273-
not constructorInitializeOrder(c, _, i + 1)
278+
result = constructorInitializeOrder(c, comp, i) and
279+
not exists(constructorInitializeOrder(c, comp, i + 1))
274280
)
275281
}
276282

@@ -379,8 +385,9 @@ module InitializerSplitting {
379385
this.appliesTo(pred) and
380386
succ(pred, succ, c) and
381387
succ =
382-
any(InitializedInstanceMember m | constructorInitializes(this.getConstructor(), m))
383-
.getAnInitializerDescendant()
388+
any(InitializedInstanceMember m |
389+
constructorInitializes(this.getConstructor(), m.getInitializer())
390+
).getAnInitializerDescendant()
384391
}
385392
}
386393
}
@@ -1204,14 +1211,12 @@ module BooleanSplitting {
12041211
exists(Callable c, int r | c = kind.getEnclosingCallable() |
12051212
result = r + ExceptionHandlerSplitting::getNextListOrder() - 1 and
12061213
kind =
1207-
rank[r](BooleanSplitSubKind kind0 |
1214+
rank[r](BooleanSplitSubKind kind0, Location l |
12081215
kind0.getEnclosingCallable() = c and
1209-
kind0.startsSplit(_)
1216+
kind0.startsSplit(_) and
1217+
l = kind0.getLocation()
12101218
|
1211-
kind0
1212-
order by
1213-
kind0.getLocation().getStartLine(), kind0.getLocation().getStartColumn(),
1214-
kind0.toString()
1219+
kind0 order by l.getStartLine(), l.getStartColumn(), kind0.toString()
12151220
)
12161221
)
12171222
}
@@ -1430,10 +1435,11 @@ module LoopSplitting {
14301435
exists(Callable c, int r | c = enclosingCallable(loop) |
14311436
result = r + BooleanSplitting::getNextListOrder() - 1 and
14321437
loop =
1433-
rank[r](AnalyzableLoopStmt loop0 |
1434-
enclosingCallable(loop0) = c
1438+
rank[r](AnalyzableLoopStmt loop0, Location l |
1439+
enclosingCallable(loop0) = c and
1440+
l = loop0.getLocation()
14351441
|
1436-
loop0 order by loop0.getLocation().getStartLine(), loop0.getLocation().getStartColumn()
1442+
loop0 order by l.getStartLine(), l.getStartColumn()
14371443
)
14381444
)
14391445
}

‎csharp/ql/test/library-tests/controlflow/graph/BasicBlock.expected

Lines changed: 8 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,6 @@
752752
| MultiImplementationA.cs:8:16:8:16 | exit M | MultiImplementationB.cs:5:16:5:16 | exit M | 1 |
753753
| MultiImplementationA.cs:8:29:8:32 | null | MultiImplementationA.cs:8:16:8:16 | exit M (abnormal) | 3 |
754754
| MultiImplementationA.cs:8:29:8:32 | null | MultiImplementationB.cs:5:16:5:16 | exit M (abnormal) | 3 |
755-
| MultiImplementationA.cs:13:16:13:16 | this access | MultiImplementationA.cs:13:16:13:20 | ... = ... | 3 |
756755
| MultiImplementationA.cs:14:31:14:31 | access to parameter i | MultiImplementationA.cs:14:31:14:31 | exit get_Item (normal) | 2 |
757756
| MultiImplementationA.cs:14:31:14:31 | access to parameter i | MultiImplementationB.cs:12:31:12:40 | exit get_Item (normal) | 2 |
758757
| MultiImplementationA.cs:14:31:14:31 | enter get_Item | MultiImplementationA.cs:14:31:14:31 | enter get_Item | 1 |
@@ -776,19 +775,17 @@
776775
| MultiImplementationA.cs:16:17:16:18 | exit M1 (normal) | MultiImplementationB.cs:14:17:14:18 | exit M1 | 2 |
777776
| MultiImplementationA.cs:17:5:19:5 | {...} | MultiImplementationA.cs:18:9:18:22 | M2(...) | 2 |
778777
| MultiImplementationA.cs:18:9:18:22 | enter M2 | MultiImplementationA.cs:18:9:18:22 | exit M2 | 4 |
779-
| MultiImplementationA.cs:20:12:20:13 | call to constructor Object | MultiImplementationA.cs:20:12:20:13 | call to constructor Object | 1 |
778+
| MultiImplementationA.cs:20:12:20:13 | call to constructor Object | MultiImplementationA.cs:20:12:20:13 | exit C2 (normal) | 14 |
779+
| MultiImplementationA.cs:20:12:20:13 | call to constructor Object | MultiImplementationB.cs:18:12:18:13 | exit C2 (normal) | 14 |
780780
| MultiImplementationA.cs:20:12:20:13 | enter C2 | MultiImplementationA.cs:20:12:20:13 | enter C2 | 1 |
781781
| MultiImplementationA.cs:20:12:20:13 | enter C2 | MultiImplementationB.cs:18:12:18:13 | enter C2 | 1 |
782782
| MultiImplementationA.cs:20:12:20:13 | exit C2 | MultiImplementationA.cs:20:12:20:13 | exit C2 | 1 |
783783
| MultiImplementationA.cs:20:12:20:13 | exit C2 | MultiImplementationB.cs:18:12:18:13 | exit C2 | 1 |
784-
| MultiImplementationA.cs:20:22:20:31 | {...} | MultiImplementationA.cs:20:12:20:13 | exit C2 (normal) | 6 |
785-
| MultiImplementationA.cs:20:22:20:31 | {...} | MultiImplementationB.cs:18:12:18:13 | exit C2 (normal) | 6 |
786784
| MultiImplementationA.cs:21:12:21:13 | enter C2 | MultiImplementationA.cs:21:12:21:13 | enter C2 | 1 |
787785
| MultiImplementationA.cs:21:12:21:13 | enter C2 | MultiImplementationB.cs:19:12:19:13 | enter C2 | 1 |
788786
| MultiImplementationA.cs:21:12:21:13 | exit C2 (normal) | MultiImplementationA.cs:21:12:21:13 | exit C2 | 2 |
789787
| MultiImplementationA.cs:21:12:21:13 | exit C2 (normal) | MultiImplementationB.cs:19:12:19:13 | exit C2 | 2 |
790-
| MultiImplementationA.cs:21:24:21:24 | 0 | MultiImplementationA.cs:21:19:21:22 | call to constructor C2 | 2 |
791-
| MultiImplementationA.cs:21:27:21:29 | {...} | MultiImplementationA.cs:21:27:21:29 | {...} | 1 |
788+
| MultiImplementationA.cs:21:24:21:24 | 0 | MultiImplementationA.cs:21:27:21:29 | {...} | 3 |
792789
| MultiImplementationA.cs:22:6:22:7 | enter ~C2 | MultiImplementationA.cs:22:6:22:7 | enter ~C2 | 1 |
793790
| MultiImplementationA.cs:22:6:22:7 | enter ~C2 | MultiImplementationB.cs:20:6:20:7 | enter ~C2 | 1 |
794791
| MultiImplementationA.cs:22:6:22:7 | exit ~C2 | MultiImplementationA.cs:22:6:22:7 | exit ~C2 | 1 |
@@ -801,7 +798,6 @@
801798
| MultiImplementationA.cs:23:28:23:35 | exit implicit conversion | MultiImplementationB.cs:21:28:21:35 | exit implicit conversion | 1 |
802799
| MultiImplementationA.cs:23:50:23:53 | null | MultiImplementationA.cs:23:28:23:35 | exit implicit conversion (normal) | 2 |
803800
| MultiImplementationA.cs:23:50:23:53 | null | MultiImplementationB.cs:21:28:21:35 | exit implicit conversion (normal) | 2 |
804-
| MultiImplementationA.cs:24:16:24:16 | this access | MultiImplementationA.cs:24:32:24:34 | ... = ... | 4 |
805801
| MultiImplementationA.cs:30:21:30:23 | enter get_P3 | MultiImplementationA.cs:30:21:30:23 | exit get_P3 | 5 |
806802
| MultiImplementationA.cs:30:21:30:23 | enter get_P3 | MultiImplementationB.cs:27:21:27:23 | exit get_P3 | 5 |
807803
| MultiImplementationA.cs:36:9:36:10 | enter M1 | MultiImplementationA.cs:36:9:36:10 | enter M1 | 1 |
@@ -835,7 +831,6 @@
835831
| MultiImplementationB.cs:5:16:5:16 | exit M | MultiImplementationB.cs:5:16:5:16 | exit M | 1 |
836832
| MultiImplementationB.cs:5:23:5:23 | 2 | MultiImplementationA.cs:8:16:8:16 | exit M (normal) | 2 |
837833
| MultiImplementationB.cs:5:23:5:23 | 2 | MultiImplementationB.cs:5:16:5:16 | exit M (normal) | 2 |
838-
| MultiImplementationB.cs:11:16:11:16 | this access | MultiImplementationB.cs:11:16:11:20 | ... = ... | 3 |
839834
| MultiImplementationB.cs:12:31:12:40 | enter get_Item | MultiImplementationA.cs:14:31:14:31 | enter get_Item | 1 |
840835
| MultiImplementationB.cs:12:31:12:40 | enter get_Item | MultiImplementationB.cs:12:31:12:40 | enter get_Item | 1 |
841836
| MultiImplementationB.cs:12:31:12:40 | exit get_Item | MultiImplementationA.cs:14:31:14:31 | exit get_Item | 1 |
@@ -859,19 +854,17 @@
859854
| MultiImplementationB.cs:14:17:14:18 | exit M1 (normal) | MultiImplementationB.cs:14:17:14:18 | exit M1 | 2 |
860855
| MultiImplementationB.cs:15:5:17:5 | {...} | MultiImplementationB.cs:16:9:16:31 | M2(...) | 2 |
861856
| MultiImplementationB.cs:16:9:16:31 | enter M2 | MultiImplementationB.cs:16:9:16:31 | exit M2 | 5 |
862-
| MultiImplementationB.cs:18:12:18:13 | call to constructor Object | MultiImplementationB.cs:18:12:18:13 | call to constructor Object | 1 |
857+
| MultiImplementationB.cs:18:12:18:13 | call to constructor Object | MultiImplementationA.cs:20:12:20:13 | exit C2 (abnormal) | 12 |
858+
| MultiImplementationB.cs:18:12:18:13 | call to constructor Object | MultiImplementationB.cs:18:12:18:13 | exit C2 (abnormal) | 12 |
863859
| MultiImplementationB.cs:18:12:18:13 | enter C2 | MultiImplementationA.cs:20:12:20:13 | enter C2 | 1 |
864860
| MultiImplementationB.cs:18:12:18:13 | enter C2 | MultiImplementationB.cs:18:12:18:13 | enter C2 | 1 |
865861
| MultiImplementationB.cs:18:12:18:13 | exit C2 | MultiImplementationA.cs:20:12:20:13 | exit C2 | 1 |
866862
| MultiImplementationB.cs:18:12:18:13 | exit C2 | MultiImplementationB.cs:18:12:18:13 | exit C2 | 1 |
867-
| MultiImplementationB.cs:18:22:18:36 | {...} | MultiImplementationA.cs:20:12:20:13 | exit C2 (abnormal) | 4 |
868-
| MultiImplementationB.cs:18:22:18:36 | {...} | MultiImplementationB.cs:18:12:18:13 | exit C2 (abnormal) | 4 |
869863
| MultiImplementationB.cs:19:12:19:13 | enter C2 | MultiImplementationA.cs:21:12:21:13 | enter C2 | 1 |
870864
| MultiImplementationB.cs:19:12:19:13 | enter C2 | MultiImplementationB.cs:19:12:19:13 | enter C2 | 1 |
871865
| MultiImplementationB.cs:19:12:19:13 | exit C2 (normal) | MultiImplementationA.cs:21:12:21:13 | exit C2 | 2 |
872866
| MultiImplementationB.cs:19:12:19:13 | exit C2 (normal) | MultiImplementationB.cs:19:12:19:13 | exit C2 | 2 |
873-
| MultiImplementationB.cs:19:24:19:24 | 1 | MultiImplementationB.cs:19:19:19:22 | call to constructor C2 | 2 |
874-
| MultiImplementationB.cs:19:27:19:29 | {...} | MultiImplementationB.cs:19:27:19:29 | {...} | 1 |
867+
| MultiImplementationB.cs:19:24:19:24 | 1 | MultiImplementationB.cs:19:27:19:29 | {...} | 3 |
875868
| MultiImplementationB.cs:20:6:20:7 | enter ~C2 | MultiImplementationA.cs:22:6:22:7 | enter ~C2 | 1 |
876869
| MultiImplementationB.cs:20:6:20:7 | enter ~C2 | MultiImplementationB.cs:20:6:20:7 | enter ~C2 | 1 |
877870
| MultiImplementationB.cs:20:6:20:7 | exit ~C2 | MultiImplementationA.cs:22:6:22:7 | exit ~C2 | 1 |
@@ -884,7 +877,6 @@
884877
| MultiImplementationB.cs:21:28:21:35 | exit implicit conversion | MultiImplementationB.cs:21:28:21:35 | exit implicit conversion | 1 |
885878
| MultiImplementationB.cs:21:56:21:59 | null | MultiImplementationA.cs:23:28:23:35 | exit implicit conversion (abnormal) | 3 |
886879
| MultiImplementationB.cs:21:56:21:59 | null | MultiImplementationB.cs:21:28:21:35 | exit implicit conversion (abnormal) | 3 |
887-
| MultiImplementationB.cs:22:16:22:16 | this access | MultiImplementationB.cs:22:32:22:34 | ... = ... | 4 |
888880
| MultiImplementationB.cs:27:21:27:23 | enter get_P3 | MultiImplementationA.cs:30:21:30:23 | exit get_P3 | 5 |
889881
| MultiImplementationB.cs:27:21:27:23 | enter get_P3 | MultiImplementationB.cs:27:21:27:23 | exit get_P3 | 5 |
890882
| MultiImplementationB.cs:32:9:32:10 | enter M1 | MultiImplementationA.cs:36:9:36:10 | enter M1 | 1 |
@@ -930,6 +922,8 @@
930922
| NullCoalescing.cs:15:31:15:31 | 0 | NullCoalescing.cs:16:17:16:18 | "" | 5 |
931923
| NullCoalescing.cs:16:17:16:25 | ... ?? ... | NullCoalescing.cs:17:13:17:19 | (...) ... | 5 |
932924
| NullCoalescing.cs:17:13:17:24 | ... ?? ... | NullCoalescing.cs:13:10:13:11 | exit M6 | 4 |
925+
| PartialImplementationA.cs:3:12:3:18 | enter Partial | PartialImplementationA.cs:3:12:3:18 | exit Partial | 12 |
926+
| PartialImplementationB.cs:4:12:4:18 | enter Partial | PartialImplementationB.cs:4:12:4:18 | exit Partial | 12 |
933927
| Patterns.cs:5:10:5:11 | enter M1 | Patterns.cs:8:18:8:23 | Int32 i1 | 8 |
934928
| Patterns.cs:8:13:8:23 | [false] ... is ... | Patterns.cs:8:13:8:23 | [false] ... is ... | 1 |
935929
| Patterns.cs:8:13:8:23 | [true] ... is ... | Patterns.cs:8:13:8:23 | [true] ... is ... | 1 |

0 commit comments

Comments
 (0)