Swift: Simplify the API for Decl members #11046
Conversation
Arguably the old behaviour, of having them only be accessible via their parent `VarDecl` or `SubscriptDecl`, was a bug.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Add the new user-facing DeclWithMembers class. - Add Decl as a superclass, as per its name. - Leave IterableDeclContext in place, because it gets auto-generated :/
d10c
force-pushed
the
swift/iterabledeclcontext-cleanup
branch
from
20f1ee3 to
b693b3d
Compare
Conservatively chose to make `cached` the more basic of the two, but I'm not sure about that.
| @@ -9,5 +9,5 @@ class VarDecl extends Generated::VarDecl { | |||
| } | |||
|
|
|||
| class FieldDecl extends VarDecl { | |||
There was a problem hiding this comment.
Maybe this should be PropertyDecl, to keep it consistent with the language's preferred name.
| * Gets the class, struct, enum, protocol, or extension that declared | ||
| * this `Decl` as a member. | ||
| */ | ||
| cached |
There was a problem hiding this comment.
Is there a rule of thumb for when I should cache a predicate?
There was a problem hiding this comment.
It's always a bit difficult to judge when (and how) something should be cached. Generally, if a predicate is (1) expensive to compute, and (2) used in multiple queries, then it should most likely be cached. In this case, I don't think that the predicate should be cached as I don't think getDeclaringDecl is expensive to compute.
There was a problem hiding this comment.
The simplest rule of thumb is: if in doubt, don't mark predicates cached when you write them (or use pragma[only_bind_into] and other hints). The optimizer doesn't usually need these tags to do a good job, and you can make things worse by using them. In the case of cached, I believe it prevents the optimizer from using the context in which the predicate is called to reduce its size, which can sometimes cost more than any gains.
The time to use these tools IMO is when you've identified a measurable performance problem and you're trying to fix it. I guess the other situation is when you're implementing or copying a familiar pattern where cached has worked out well before.
| * Gets the `index`th member (0-based) of this nominal type or extension `Decl`, | ||
| * including `AccessorDecl`s of immediate members. | ||
| */ | ||
| override Decl getImmediateMember(int index) { |
There was a problem hiding this comment.
Not sure, but maybe this logic should be part of the extractor or codegen.
There was a problem hiding this comment.
We generally prefer to put logic in QL when we have the choice, and there are no good reasons for putting in the extractor.
There was a problem hiding this comment.
... I'd like to tell you why we prefer that. I think it has to do with QL being easier to see and easier to change.
| private import codeql.swift.elements.decl.SubscriptDecl | ||
| private import codeql.swift.elements.decl.Decl | ||
|
|
||
| class DeclWithMembers extends Decl, IterableDeclContext { |
There was a problem hiding this comment.
I'm still of the opinion that there's not really a need to have a class like this as part of the public AST. Rather, I'd much rather see the importing of the IterableDeclContext class disappear from swift.qll, and its use purely as an implementation detail in order to get getImmediateMember/getMember predicates on the classes that currently extend IterableDeclContext.
I think we should delay this change until we've settled on a decision here.
There was a problem hiding this comment.
And I'm not very happy with the name DeclWithMembers. I think we're talking about a declaration of a kind that can have members, not one that necessarily does. Consider:
struct MyStruct {}
| e.getAMember() = this | ||
| ) | ||
| ) | ||
| this.getDeclaringTypeDecl().getFullName() = typeName |
There was a problem hiding this comment.
I do like this, having getDeclaringDecl and getDeclaringTypeDecl both widely available and clearly defined is a big plus.
| class IterableDeclContext extends Generated::IterableDeclContext { | ||
| /** | ||
| * Gets the `index`th member of this iterable declaration context (0-based), | ||
| * including `AccessorDecl`s of immediate members. | ||
| */ | ||
| override Decl getImmediateMember(int index) { | ||
| result = | ||
| rank[1 + index](Decl member, Decl immMember, int immIndex, int accIndex | | ||
| immMember = super.getImmediateMember(immIndex) and | ||
| ( | ||
| member = immMember and accIndex = -1 | ||
| or | ||
| member = immMember.(VarDecl).getImmediateAccessorDecl(accIndex) | ||
| or | ||
| member = immMember.(SubscriptDecl).getImmediateAccessorDecl(accIndex) | ||
| ) | ||
| | | ||
| member order by immIndex, accIndex | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not really sold on this change, as I do think accessors are logically and visually under a VarDecl or SubscriptDecl (and by visually I mean they are declared in subblocks of variable/subscript declarations). I would rather keep them that way.
Even if we do go with this, this requires taking them out of the children of VarDecl/SubscriptDecl, as the ast/no_double_parents.ql test failure shows.
VarDecl/SubscriptDecl.MethodDeclrelies on the above behaviour, not a special-casedgetAMember()predicate.DeclWithMembers.getAMember()(etc) methods to the Decl class so as to be more generally useful. But this would involve interfering with the autogenerated code inGenerated::IterableDeclContext. [Update: blocked on an improved codegen script that allows us to customize generated class names independently of the swift extractor's names]getDeclaringDeclas an inverse ofgetAMember().getDeclaringTypeDeclthat also resolves Extensions to theNominalTypeDeclthat they extend. Use this to simplifyMethodDecl.hasQualifiedName/2.I would appreciate a critical eye on these changes, to see which ones are worth keeping or, as the case may be, redesigning. Judging from the failing tests, this is probably violating some preexisting assumptions.