Skip to content

Commit b1da660

Browse files
hvitvedhvitved
committed
Unified: Improve static name binding involving shadowing
1 parent 3aa5538 commit b1da660

4 files changed

Lines changed: 184 additions & 39 deletions

File tree

‎unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
private import unified
22
private import codeql.util.Unit
3+
private import codeql.util.Option
4+
private import StaticNameBinding
35

46
private module Plugins {
57
private import codeql.unified.internal.NameBindingPluginSwift
@@ -43,6 +45,23 @@ class NameBindingPlugin extends Unit {
4345
bindingset[cls, member]
4446
predicate isInheritableMember(ClassLikeDeclaration cls, Member member) { none() }
4547

48+
/**
49+
* Holds if `member` is considered invalid within the namespace `n`.
50+
*/
51+
bindingset[n, member]
52+
predicate isInvalidMember(NamespaceNode n, Member member) { none() }
53+
54+
/**
55+
* Gets the key used to determine if `m` is shadowed by another declaration
56+
* with the same name and key.
57+
*
58+
* Nodes without shadowing keys will not shadow inherited declarations.
59+
*
60+
* This means that shadowing can be completely disabled by not implementing this
61+
* predicate, and completely enabled by assigning the same key to all members.
62+
*/
63+
string getShadowingKey(Member m) { none() }
64+
4665
/** Gets the name of the implicit receiver parameter in `callable`, if it has one. */
4766
string getImplicitReceiverParameterName(Callable callable) { none() }
4867

@@ -89,6 +108,28 @@ predicate isInheritableMember(Member member) {
89108
)
90109
}
91110

111+
bindingset[n, member]
112+
predicate isInvalidMember(NamespaceNode n, Member member) {
113+
any(NameBindingPlugin p).isInvalidMember(n, member)
114+
}
115+
116+
private string getShadowingKey0(NameBindingNode n) {
117+
result = any(NameBindingPlugin p).getShadowingKey(any(Member m | n.isMember(m)))
118+
}
119+
120+
private class ShadowingKey extends string {
121+
ShadowingKey() { this = getShadowingKey0(_) }
122+
}
123+
124+
class ShadowingKeyOpt = Option<ShadowingKey>::Option;
125+
126+
ShadowingKeyOpt getShadowingKey(NameBindingNode n) {
127+
result.asSome() = getShadowingKey0(n)
128+
or
129+
not exists(getShadowingKey0(n)) and
130+
result.isNone()
131+
}
132+
92133
/**
93134
* A representative for a module scope.
94135
*

‎unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,21 @@
33
*/
44

55
private import unified
6+
private import codeql.unified.internal.StaticNameBinding
67
private import codeql.unified.internal.NameBindingPlugin
78

9+
private class GeneratedConstructor extends ConstructorDeclaration {
10+
GeneratedConstructor() { this.hasModifier("generated") }
11+
}
12+
13+
private class ConvenienceConstructor extends ConstructorDeclaration {
14+
ConvenienceConstructor() { this.hasModifier("convenience") }
15+
}
16+
17+
private class DesignatedConstructor extends ConstructorDeclaration {
18+
DesignatedConstructor() { not this instanceof ConvenienceConstructor }
19+
}
20+
821
class NameBindingPluginSwift extends NameBindingPlugin {
922
bindingset[e]
1023
override predicate isNonPattern(Expr e) { isUnboundPattern(e.(Identifier)) }
@@ -36,7 +49,48 @@ class NameBindingPluginSwift extends NameBindingPlugin {
3649
bindingset[cls, member]
3750
override predicate isInheritableMember(ClassLikeDeclaration cls, Member member) {
3851
exists(cls) and
39-
not member.hasModifier("private")
52+
not member.hasModifier("private") and
53+
not (cls.hasModifier("protocol") and member instanceof ConstructorDeclaration)
54+
}
55+
56+
bindingset[n, member]
57+
override predicate isInvalidMember(NamespaceNode n, Member member) {
58+
exists(NamespaceNode parent |
59+
parent = n.getAnInheritanceParent() and
60+
not parent
61+
.isInstanceOrStaticMemberNamespace(any(ClassLikeDeclaration p | p.hasModifier("protocol")))
62+
|
63+
// Remove generated constructors when there are inherited constructors available
64+
// Note: If the base class only has private constructors, this class must have an
65+
// explicit constructor, in which case there is no generated constructor to begin
66+
// with
67+
member instanceof GeneratedConstructor and
68+
n.getOwnMember(_).isMember(member) and
69+
parent.getMemberFull(_, _).isMember(any(ConstructorDeclaration inherited))
70+
or
71+
// Remove inherited designated constructors (generated or not) when there are
72+
// explicit designated constructors available
73+
// Note: We always inherit convenience constructors, even though it may not actually
74+
// be the case in Swift; this should be OK, since there can then not exist any calls
75+
// that target those constructors
76+
parent.getMemberFull(_, _).isMember(member.(DesignatedConstructor)) and
77+
exists(DesignatedConstructor designated |
78+
n.getOwnMember(_).isMember(designated) and
79+
not designated instanceof GeneratedConstructor
80+
)
81+
)
82+
}
83+
84+
override string getShadowingKey(Member m) {
85+
not m instanceof Callable and result = ""
86+
or
87+
not m instanceof GeneratedConstructor and
88+
result =
89+
concat(int i, Parameter p |
90+
p = m.(Callable).getParameter(i)
91+
|
92+
p.getExternalNameNode().getValue(), "," order by i
93+
)
4094
}
4195

4296
override string getImplicitReceiverParameterName(Callable callable) {

‎unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll‎

Lines changed: 78 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ class NameBindingNode extends TNameBindingNode {
2929

3030
Identifier asIdentifier() { this.isIdentifier(result) }
3131

32+
predicate isMember(Member m) { this.asIdentifier() = m.getNameNode() }
33+
3234
predicate isBulkImport(BulkImportingPattern p) { this = TBulkImport(p) }
3335

3436
predicate isLocalName(LocalName local) { this = TLocalName(local) }
@@ -41,6 +43,10 @@ class NameBindingNode extends TNameBindingNode {
4143
this = TInstanceMemberNamespace(cls)
4244
}
4345

46+
predicate isInstanceOrStaticMemberNamespace(ClassLikeDeclaration cls) {
47+
this.isInstanceMemberNamespace(cls) or this.isStaticMemberNamespace(cls)
48+
}
49+
4450
/** Holds if this represents the set of members that can be accessed unqualified within the given scope. */
4551
predicate isLocalNamespace(AstNode n) { this = TLocalNamespace(n) }
4652

@@ -312,6 +318,10 @@ signature module TrackInputSig {
312318
/** Holds if the forward-flow of `node` should be tracked. */
313319
predicate shouldTrack(NameBindingNode node);
314320

321+
default NameBindingNode getMember(NamespaceNode namespace, string name) {
322+
result = namespace.getMember(name)
323+
}
324+
315325
default predicate additionalValueStep(NameBindingNode node1, NameBindingNode node2) { none() }
316326
}
317327

@@ -327,6 +337,25 @@ module Track<TrackInputSig Input> {
327337
exists(NameBindingNode prev | prev = track(node) | valueStepEx(prev, result))
328338
}
329339

340+
pragma[nomagic]
341+
private predicate derivedStoreReadStep0(
342+
NamespaceNode namespace, string name, NameBindingNode node2
343+
) {
344+
readStep(namespace.ref(), name, node2)
345+
}
346+
347+
/**
348+
* Holds if `node1 -> node2` is derived by combining a store and a read step, with zero or more value steps and inheritance steps in-between.
349+
*/
350+
pragma[nomagic]
351+
private predicate derivedStoreReadStep(NameBindingNode node1, NameBindingNode node2) {
352+
exists(NamespaceNode namespace, string name |
353+
node1 = getMember(namespace, name) and // getMember() combines a store step with subsequent inheritance steps
354+
derivedStoreReadStep0(namespace, name, node2) and
355+
node1 != node2
356+
)
357+
}
358+
330359
/** Holds if there is an effective value step `node1 -> node2`. */
331360
pragma[inline]
332361
private predicate valueStepEx(NameBindingNode node1, NameBindingNode node2) {
@@ -338,23 +367,6 @@ module Track<TrackInputSig Input> {
338367
}
339368
}
340369

341-
pragma[nomagic]
342-
private predicate derivedStoreReadStep0(NamespaceNode namespace, string name, NameBindingNode node2) {
343-
readStep(namespace.ref(), pragma[only_bind_into](name), node2)
344-
}
345-
346-
/**
347-
* Holds if `node1 -> node2` is derived by combining a store and a read step, with zero or more value steps and inheritance steps in-between.
348-
*/
349-
pragma[nomagic]
350-
private predicate derivedStoreReadStep(NameBindingNode node1, NameBindingNode node2) {
351-
exists(NamespaceNode namespace, string name |
352-
node1 = namespace.getMember(name) and // getMember() combines a store step with subsequent inheritance steps
353-
derivedStoreReadStep0(namespace, name, node2) and
354-
node1 != node2
355-
)
356-
}
357-
358370
/** Holds if the member represented by `node` can be inherited. */
359371
pragma[nomagic]
360372
private predicate isInheritableMemberNode(NameBindingNode node) {
@@ -382,10 +394,13 @@ class NamespaceNode extends NameBindingNode {
382394
NameBindingNode ref() { result = TrackNamespace::track(this) }
383395

384396
/** Gets an own (non-inherited) member of this namespace of the given name. */
385-
NameBindingNode getOwnMember(string name) { storeStep(result, name, this) }
397+
NameBindingNode getOwnMember(string name, ShadowingKeyOpt shadowingKey) {
398+
storeStep(result, name, this) and
399+
shadowingKey = getShadowingKey(result)
400+
}
386401

387-
/** Holds if this namespace has an own-member of the given name */
388-
predicate hasOwnMember(string name) { exists(this.getOwnMember(name)) }
402+
/** Gets an own (non-inherited) member of this namespace of the given name. */
403+
NameBindingNode getOwnMember(string name) { result = this.getOwnMember(name, _) }
389404

390405
/** If this is the static namespace for a class, gets the corresponding instance namespace. */
391406
NamespaceNode toInstanceNamespace() {
@@ -423,23 +438,58 @@ class NamespaceNode extends NameBindingNode {
423438
/** Gets a namespace that directly inherits from this one. */
424439
NamespaceNode getAnInheritanceChild() { result.getAnInheritanceParent() = this }
425440

426-
/** Gets a member of this namespace of the given name. */
427-
cached
428-
NameBindingNode getMember(string name) {
429-
CachedStage::ref() and
430-
result = this.getOwnMember(name)
441+
pragma[nomagic]
442+
private predicate hasOwnMemberWithShadowingKey(string name, string shadowingKey) {
443+
exists(this.getOwnMember(name, any(ShadowingKeyOpt opt | opt.asSome() = shadowingKey)))
444+
}
445+
446+
/**
447+
* Gets a potential member of this namespace of the given name.
448+
*
449+
* Unlike `getMember`, this predicate does not filter away invalid members.
450+
*/
451+
pragma[nomagic]
452+
NameBindingNode getMemberFull(string name, ShadowingKeyOpt shadowingKey) {
453+
result = this.getOwnMember(name) and
454+
shadowingKey = getShadowingKey(result)
431455
or
432-
not this.hasOwnMember(name) and
433-
result = this.getAnInheritanceParent().getMember(name) and
456+
not this.hasOwnMemberWithShadowingKey(name, shadowingKey.asSome()) and
457+
result = this.getAnInheritanceParent().getMemberFull(name, shadowingKey) and
434458
isInheritableMemberNode(result)
435459
or
436-
result = this.getAnExtension().getMember(name)
460+
result = this.getAnExtension().getMemberFull(name, shadowingKey)
461+
}
462+
463+
private NameBindingNode getMember(string name, ShadowingKeyOpt shadowingKey) {
464+
CachedStage::ref() and
465+
not exists(Member m | result.isMember(m) | isInvalidMember(this, m)) and
466+
(
467+
result = this.getOwnMember(name) and
468+
shadowingKey = getShadowingKey(result)
469+
or
470+
result = this.getAnExtension().getMember(name, shadowingKey)
471+
or
472+
exists(NamespaceNode parent |
473+
parent = this.getAnInheritanceParent() and
474+
not this.hasOwnMemberWithShadowingKey(name, shadowingKey.asSome()) and
475+
result = parent.getMember(name, shadowingKey) and
476+
isInheritableMemberNode(result)
477+
)
478+
)
437479
}
480+
481+
/** Gets a member of this namespace of the given name. */
482+
cached
483+
NameBindingNode getMember(string name) { result = this.getMember(name, _) }
438484
}
439485

440486
private module TrackNamespaceInput implements TrackInputSig {
441487
predicate shouldTrack(NameBindingNode node) { node instanceof NamespaceNode }
442488

489+
NameBindingNode getMember(NamespaceNode namespace, string name) {
490+
result = namespace.getMemberFull(name, _)
491+
}
492+
443493
predicate additionalValueStep(NameBindingNode node1, NameBindingNode node2) {
444494
// Namespace-tracking goes through aliases, but declaration-tracking does not
445495
exists(TypeAliasDeclaration decl |

‎unified/ql/test/library-tests/static-name-binding/inheritance.swift‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -82,33 +82,33 @@ func t1() {
8282
let x5: E.C = E.C() // $ access=E access=A.B.C
8383

8484
let x6 = A.init() // $ access=A access=A.init
85-
let x7 = D.init() // $ access=D $ MISSING: access=A.init $ SPURIOUS: access=D.init
85+
let x7 = D.init() // $ access=D access=A.init
8686

8787
let x8 = A.f() // $ access=A access=A.f
88-
let x9 = D.f() // $ access=D access=D.f -- type inference should filter away the `D.f` target $ MISSING: access=A.f
89-
let x10 = D.f(x: 0) // $ access=D access=D.f $ MISSING: access=A.f -- type inference should filter away the `A.f` target
88+
let x9 = D.f() // $ access=D access=A.f access=D.f -- type inference should filter away the `D.f` target
89+
let x10 = D.f(x: 0) // $ access=D access=A.f access=D.f -- type inference should filter away the `A.f` target
9090

9191
let x11 = E.init() // $ access=E access=E.init
9292

9393
let x12 = G.init() // $ access=G access=G.init
9494

9595
let x13 = H.init(x: "String") // $ access=H access=H.init
9696

97-
let x14 = I.init(x: "String") // $ access=I $ MISSING: access=H.init $ SPURIOUS: access=I.init
97+
let x14 = I.init(x: "String") // $ access=I access=H.init
9898

99-
let x15 = J.init() // $ access=J access=J.init $ MISSING: access=H.init -- type inference should filter away the `H.init` target
99+
let x15 = J.init() // $ access=J access=J.init access=H.init -- type inference should filter away the `H.init` target
100100

101101
let x16 = K.init() // $ access=K access=K.init
102102

103103
let x17 = S.init() // $ access=S access=S.init
104104

105-
let x18 = M.init(x: 0) // $ access=M access=M.init $ MISSING: access=L.init_conv -- type inference should filter away the `L.init_conv` target
105+
let x18 = M.init(x: 0) // $ access=M access=M.init access=L.init_conv -- type inference should filter away the `L.init_conv` target
106106

107-
let x19 = M.init() // $ access=M access=M.init $ MISSING: access=L.init_conv -- type inference should filter away the `M.init` target
107+
let x19 = M.init() // $ access=M access=M.init access=L.init_conv -- type inference should filter away the `M.init` target
108108

109-
let x20 = N.init(x: 0) // $ access=N access=N.init access=N.init_conv -- type inference should filter away the `N.init_conv` target
109+
let x20 = N.init(x: 0) // $ access=N access=N.init access=N.init_conv access=L.init_conv -- type inference should filter away the `N.init_conv` and `L.init_conv` targets
110110

111-
let x21 = O.init(x: 0) // $ access=O access=O.init $ MISSING: access=N.init_conv -- type inference should filter away the `N.init_conv` target
111+
let x21 = O.init(x: 0) // $ access=O access=O.init access=N.init_conv access=L.init_conv -- type inference should filter away the `N.init_conv` and `L.init_conv` targets
112112

113-
let x22 = O.init(a: 1, b: 2, c: 3) // $ access=O access=O.init $ MISSING: access=N.init_conv -- type inference should filter away the `O.init` target
113+
let x22 = O.init(a: 1, b: 2, c: 3) // $ access=O access=O.init access=N.init_conv access=L.init_conv -- type inference should filter away the `O.init` and `L.init_conv` targets
114114
}

0 commit comments

Comments
 (0)