Skip to content

Commit 6263dc9

Browse files
hvitvedhvitved
committed
Address review comments
1 parent 5b468f2 commit 6263dc9

5 files changed

Lines changed: 58 additions & 33 deletions

File tree

‎shared/typeinference/codeql/typeinference/internal/TypeInference.qll‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3683,6 +3683,16 @@ module Make1<LocationSig Location, InputSig1<Location> Input1> {
36833683
pragma[nomagic]
36843684
Type inferType(AstNode n) { result = inferType(n, TypePath::nil()) }
36853685

3686+
/** Gets the unique non-pseudo type inferred at `path` for `n`, if any. */
3687+
Type inferTypeUnique(AstNode n, TypePath path) {
3688+
result = unique(Type t | t = inferType(n, path) and not t instanceof PseudoType | t)
3689+
}
3690+
3691+
/**
3692+
* Gets the unique non-pseudo type inferred at the root for `n`, if any.
3693+
*/
3694+
Type inferTypeUnique(AstNode n) { result = inferTypeUnique(n, TypePath::nil()) }
3695+
36863696
/**
36873697
* The cached stage of this module.
36883698
*

‎unified/ql/lib/codeql/unified/internal/dataflow/DataFlowPluginSwift.qll‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ private import AllDataFlow
77
private import codeql.unified.internal.ExprPositions
88
private import codeql.unified.internal.typeinference.TypeInference as T
99

10+
private predicate isReferenceType(T::Type t) {
11+
// todo: type parameters with type constraints
12+
t.asClass().hasModifier(["actor", "class"])
13+
}
14+
1015
private class SwiftDataFlowPlugin extends DataFlowPlugin {
1116
// Note: For now we assume all code is Swift, but in the future we must restrict these rules to Swift-files
1217
override predicate bypassContentSet(ContentSet contents) {
@@ -16,9 +21,9 @@ private class SwiftDataFlowPlugin extends DataFlowPlugin {
1621
// Swift writes back mutated values of computed value-type properties via their setter
1722
bindingset[expr]
1823
override predicate mayWriteBackViaSetterCall(Expr expr) {
19-
isInWriteBackPosition(expr) and
20-
// conservative value-type check
21-
not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier("class"))
24+
exists(boolean direct | isInWriteBackPosition(expr, direct) |
25+
if direct = false then not isReferenceType(T::inferTypeUnique(expr)) else any()
26+
)
2227
}
2328

2429
override predicate exprStep(Expr expr1, Expr expr2, boolean mustFlow) {
@@ -101,25 +106,29 @@ private class SwiftDataFlowPlugin extends DataFlowPlugin {
101106
* Holds if `expr` is target of an assignment or 'inout' argument,
102107
* or the base of such an expression, e.g. `x` in `x.f1.f2 = foo()`
103108
*/
104-
private predicate isInWriteBackPosition(Expr expr) {
105-
isInAssignmentContext(expr, _)
109+
private predicate isInWriteBackPosition(Expr expr, boolean direct) {
110+
isInAssignmentContext(expr, _) and
111+
direct = false
106112
or
107113
// Arguments to an 'inout' parameter must be prefixed with '&'
108114
exists(UnaryExpr unary |
109115
unary.getOperator().(PrefixOperator).getValue() = "&" and
110116
unary.getOperand() = expr
111117
// TODO: support custom operators with an 'inout' parameter, which lack the '&' indicator
112-
)
118+
) and
119+
direct = true
113120
or
114121
// The receiver of a call to a 'mutating' function counts as an 'inout' argument
115122
exists(CallExpr call, Callable target |
116123
target = T::resolveCallTarget(call) and
117124
target.hasModifier("mutating") and
118125
expr = call.getCallee().(MemberAccessExpr).getBase()
119-
)
126+
) and
127+
direct = true
120128
or
121129
exists(MemberAccessExpr member |
122-
isInWriteBackPosition(member) and
123-
expr = member.getBase()
130+
isInWriteBackPosition(member, _) and
131+
expr = member.getBase() and
132+
direct = false
124133
)
125134
}

‎unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -856,6 +856,10 @@ predicate inferType = M3::inferType/1;
856856

857857
predicate inferType = M3::inferType/2;
858858

859+
predicate inferTypeUnique = M3::inferTypeUnique/1;
860+
861+
predicate inferTypeUnique = M3::inferTypeUnique/2;
862+
859863
predicate inferTypeCertain = M3::inferTypeCertain/2;
860864

861865
module Consistency = M3::Consistency;

‎unified/ql/test/library-tests/dataflow/accessors_instance.swift‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,9 @@ func t11() {
139139
}
140140
}
141141
let c = C()
142+
// `p` has value type, so this calls `p`'s setter
142143
c.p.0 = source("t11.1")
144+
// `q` has reference type, so this calls `q`'s getter followed by `p`'s setter
143145
c.q.p = (source("t11.2"), source("t11.3"))
144146
}
145147

‎unified/ql/test/library-tests/dataflow/test.expected‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,16 @@ edges
3232
| accessors_instance.swift:119:13:119:17 | field | accessors_instance.swift:125:18:125:22 | field | provenance | |
3333
| accessors_instance.swift:119:21:119:35 | source(...) | accessors_instance.swift:119:13:119:17 | field | provenance | |
3434
| accessors_instance.swift:134:24:134:31 | newValue [0] | accessors_instance.swift:134:24:134:33 | ... .0 | provenance | |
35-
| accessors_instance.swift:142:5:142:7 | [post] ... .p [0] | accessors_instance.swift:134:24:134:31 | newValue [0] | provenance | |
36-
| accessors_instance.swift:142:5:142:9 | ... .0 | accessors_instance.swift:142:5:142:7 | [post] ... .p [0] | provenance | |
37-
| accessors_instance.swift:142:13:142:27 | source(...) | accessors_instance.swift:142:5:142:9 | ... .0 | provenance | |
38-
| accessors_instance.swift:143:5:143:9 | ... .p [0] | accessors_instance.swift:134:24:134:31 | newValue [0] | provenance | |
39-
| accessors_instance.swift:143:13:143:46 | TupleExpr [0] | accessors_instance.swift:143:5:143:9 | ... .p [0] | provenance | |
40-
| accessors_instance.swift:143:14:143:28 | source(...) | accessors_instance.swift:143:13:143:46 | TupleExpr [0] | provenance | |
41-
| accessors_instance.swift:149:34:149:38 | field | accessors_instance.swift:158:5:158:7 | [post] ... .s [field] | provenance | |
42-
| accessors_instance.swift:149:42:149:56 | source(...) | accessors_instance.swift:149:34:149:38 | field | provenance | |
43-
| accessors_instance.swift:154:24:154:31 | newValue [field] | accessors_instance.swift:154:24:154:37 | ... .field | provenance | |
44-
| accessors_instance.swift:158:5:158:7 | [post] ... .s [field] | accessors_instance.swift:154:24:154:31 | newValue [field] | provenance | |
35+
| accessors_instance.swift:143:5:143:7 | [post] ... .p [0] | accessors_instance.swift:134:24:134:31 | newValue [0] | provenance | |
36+
| accessors_instance.swift:143:5:143:9 | ... .0 | accessors_instance.swift:143:5:143:7 | [post] ... .p [0] | provenance | |
37+
| accessors_instance.swift:143:13:143:27 | source(...) | accessors_instance.swift:143:5:143:9 | ... .0 | provenance | |
38+
| accessors_instance.swift:145:5:145:9 | ... .p [0] | accessors_instance.swift:134:24:134:31 | newValue [0] | provenance | |
39+
| accessors_instance.swift:145:13:145:46 | TupleExpr [0] | accessors_instance.swift:145:5:145:9 | ... .p [0] | provenance | |
40+
| accessors_instance.swift:145:14:145:28 | source(...) | accessors_instance.swift:145:13:145:46 | TupleExpr [0] | provenance | |
41+
| accessors_instance.swift:151:34:151:38 | field | accessors_instance.swift:160:5:160:7 | [post] ... .s [field] | provenance | |
42+
| accessors_instance.swift:151:42:151:56 | source(...) | accessors_instance.swift:151:34:151:38 | field | provenance | |
43+
| accessors_instance.swift:156:24:156:31 | newValue [field] | accessors_instance.swift:156:24:156:37 | ... .field | provenance | |
44+
| accessors_instance.swift:160:5:160:7 | [post] ... .s [field] | accessors_instance.swift:156:24:156:31 | newValue [field] | provenance | |
4545
| accessors_local.swift:7:21:7:34 | source(...) | accessors_local.swift:8:10:8:10 | x | provenance | |
4646
| accessors_local.swift:15:15:15:28 | source(...) | accessors_local.swift:17:10:17:10 | x | provenance | |
4747
| accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | provenance | |
@@ -569,17 +569,17 @@ nodes
569569
| accessors_instance.swift:125:18:125:22 | field | semmle.label | field |
570570
| accessors_instance.swift:134:24:134:31 | newValue [0] | semmle.label | newValue [0] |
571571
| accessors_instance.swift:134:24:134:33 | ... .0 | semmle.label | ... .0 |
572-
| accessors_instance.swift:142:5:142:7 | [post] ... .p [0] | semmle.label | [post] ... .p [0] |
573-
| accessors_instance.swift:142:5:142:9 | ... .0 | semmle.label | ... .0 |
574-
| accessors_instance.swift:142:13:142:27 | source(...) | semmle.label | source(...) |
575-
| accessors_instance.swift:143:5:143:9 | ... .p [0] | semmle.label | ... .p [0] |
576-
| accessors_instance.swift:143:13:143:46 | TupleExpr [0] | semmle.label | TupleExpr [0] |
577-
| accessors_instance.swift:143:14:143:28 | source(...) | semmle.label | source(...) |
578-
| accessors_instance.swift:149:34:149:38 | field | semmle.label | field |
579-
| accessors_instance.swift:149:42:149:56 | source(...) | semmle.label | source(...) |
580-
| accessors_instance.swift:154:24:154:31 | newValue [field] | semmle.label | newValue [field] |
581-
| accessors_instance.swift:154:24:154:37 | ... .field | semmle.label | ... .field |
582-
| accessors_instance.swift:158:5:158:7 | [post] ... .s [field] | semmle.label | [post] ... .s [field] |
572+
| accessors_instance.swift:143:5:143:7 | [post] ... .p [0] | semmle.label | [post] ... .p [0] |
573+
| accessors_instance.swift:143:5:143:9 | ... .0 | semmle.label | ... .0 |
574+
| accessors_instance.swift:143:13:143:27 | source(...) | semmle.label | source(...) |
575+
| accessors_instance.swift:145:5:145:9 | ... .p [0] | semmle.label | ... .p [0] |
576+
| accessors_instance.swift:145:13:145:46 | TupleExpr [0] | semmle.label | TupleExpr [0] |
577+
| accessors_instance.swift:145:14:145:28 | source(...) | semmle.label | source(...) |
578+
| accessors_instance.swift:151:34:151:38 | field | semmle.label | field |
579+
| accessors_instance.swift:151:42:151:56 | source(...) | semmle.label | source(...) |
580+
| accessors_instance.swift:156:24:156:31 | newValue [field] | semmle.label | newValue [field] |
581+
| accessors_instance.swift:156:24:156:37 | ... .field | semmle.label | ... .field |
582+
| accessors_instance.swift:160:5:160:7 | [post] ... .s [field] | semmle.label | [post] ... .s [field] |
583583
| accessors_local.swift:7:21:7:34 | source(...) | semmle.label | source(...) |
584584
| accessors_local.swift:8:10:8:10 | x | semmle.label | x |
585585
| accessors_local.swift:15:15:15:28 | source(...) | semmle.label | source(...) |
@@ -1215,9 +1215,9 @@ testFailures
12151215
| accessors_instance.swift:99:10:99:21 | ... .otherField | accessors_instance.swift:97:15:97:28 | source(...) | accessors_instance.swift:99:10:99:21 | ... .otherField | $@ | accessors_instance.swift:97:15:97:28 | source(...) | source(...) |
12161216
| accessors_instance.swift:112:10:112:16 | ... .field | accessors_instance.swift:106:21:106:34 | source(...) | accessors_instance.swift:112:10:112:16 | ... .field | $@ | accessors_instance.swift:106:21:106:34 | source(...) | source(...) |
12171217
| accessors_instance.swift:125:18:125:22 | field | accessors_instance.swift:119:21:119:35 | source(...) | accessors_instance.swift:125:18:125:22 | field | $@ | accessors_instance.swift:119:21:119:35 | source(...) | source(...) |
1218-
| accessors_instance.swift:134:24:134:33 | ... .0 | accessors_instance.swift:142:13:142:27 | source(...) | accessors_instance.swift:134:24:134:33 | ... .0 | $@ | accessors_instance.swift:142:13:142:27 | source(...) | source(...) |
1219-
| accessors_instance.swift:134:24:134:33 | ... .0 | accessors_instance.swift:143:14:143:28 | source(...) | accessors_instance.swift:134:24:134:33 | ... .0 | $@ | accessors_instance.swift:143:14:143:28 | source(...) | source(...) |
1220-
| accessors_instance.swift:154:24:154:37 | ... .field | accessors_instance.swift:149:42:149:56 | source(...) | accessors_instance.swift:154:24:154:37 | ... .field | $@ | accessors_instance.swift:149:42:149:56 | source(...) | source(...) |
1218+
| accessors_instance.swift:134:24:134:33 | ... .0 | accessors_instance.swift:143:13:143:27 | source(...) | accessors_instance.swift:134:24:134:33 | ... .0 | $@ | accessors_instance.swift:143:13:143:27 | source(...) | source(...) |
1219+
| accessors_instance.swift:134:24:134:33 | ... .0 | accessors_instance.swift:145:14:145:28 | source(...) | accessors_instance.swift:134:24:134:33 | ... .0 | $@ | accessors_instance.swift:145:14:145:28 | source(...) | source(...) |
1220+
| accessors_instance.swift:156:24:156:37 | ... .field | accessors_instance.swift:151:42:151:56 | source(...) | accessors_instance.swift:156:24:156:37 | ... .field | $@ | accessors_instance.swift:151:42:151:56 | source(...) | source(...) |
12211221
| accessors_local.swift:8:10:8:10 | x | accessors_local.swift:7:21:7:34 | source(...) | accessors_local.swift:8:10:8:10 | x | $@ | accessors_local.swift:7:21:7:34 | source(...) | source(...) |
12221222
| accessors_local.swift:17:10:17:10 | x | accessors_local.swift:15:15:15:28 | source(...) | accessors_local.swift:17:10:17:10 | x | $@ | accessors_local.swift:15:15:15:28 | source(...) | source(...) |
12231223
| accessors_local.swift:25:10:25:10 | x | accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | $@ | accessors_local.swift:22:15:22:28 | source(...) | source(...) |

0 commit comments

Comments
 (0)