Swift: Add EnumDecl.getEnumElement(_)

[geoffw0]

Add EnumDecl tests and a method EnumDecl.getEnumElement(_). This is a shortcut from an EnumDecl to the EnumElementDecls without having to deal with the EnumCaseDecls in between (that are rarely what you care about).

enum MyColours {   // EnumDecl
  case red         // EnumCaseDecl with one EnumElementDecl
  case green, blue // EnumCaseDecl with two EnumElementDecls
}

In the diff below, I highly recommend revealing EnumDecl.qll. The diff has mistakenly hidden it, presumably because it was (but no longer is) a generated file.

[MathiasVP]

A couple of comments and suggestions, but otherwise this LGTM

[MathiasVP]

Two questions:

1. For an EnumDecl d we now have both d.getAMember() and d.getAnEnumElement(). Should we override getAMember() in this class to add QLDoc that mention the difference between this new predicate and getAMember?

2. Now that we're making it more difficult for users to accidentally get hold of an EnumCaseDecl, how about we completely get rid of them from the AST by replacing any() with none() in https://github.com/github/codeql/blob/main/swift/ql/lib/codeql/swift/elements/decl/EnumCaseDeclConstructor.qll#L4?

[geoffw0]

1. Good idea, clearer documentation is always a good thing.
2. I'm a little reluctant to get rid of the ability to reason about EnumCaseDecls entirely. The fashion at the moment is very much for security queries where EnumCaseDecl is a useless detail. But some users may want to write style queries - for example "only put one enum value in each enum case declaration" would be a valid style query.

[MathiasVP]

Good point about 2. Let's leave in those EnumCaseDecls for now.

[geoffw0]

Back on 1., unfortunately I can't override the generated getMember and getAMember methods as they are marked final. Let me know if you can think of a workaround that will work for documentation purposes. I'm tempted to put a general warning on Decl.getMember itself (the ordered version) to prefer not to rely on the order of elements via this predicate when alternative methods are available?

[MathiasVP]

Ah, I wasn't aware that they're marked as final. Maybe they should't be for exactly this reason 🤔. I can't think of any good workaround for this either.

I'm tempted to put a general warning on Decl.getMember itself (the ordered version) to prefer not to rely on the order of elements via this predicate when alternative methods are available?

Sounds good to me.

cc @redsun82 maybe we should remove the final annotation on these predicates (so that we can override them with custom QLDoc).

[geoffw0]

I've added the warning to Decl.getMember. I'm open to suggestions to improve the wording.

[d10c]

FYI, Decl.getMember/0 is final because it's a generated accessor; the current extension point would be Decl.getImmediateMember/0.

Another alternative is to make the EnumCaseDecls hidden, i.e. they resolve to their child EnumElementDecls.

[geoffw0]

It's Decl.getMember/1 that's problematic, because the indexes aren't what you might expect (if you didn't realize the cases are children interleaves with the actual elements). It works fine, it's just potentially confusing.

the current extension point would be Decl.getImmediateMember/0.

Yeah, you're right, thanks. Though I'm really just looking to update the QLDoc / help text on getMember/1 for EnumDecl, which I've achieved by just rewording the root version.

[redsun82]

Another alternative is to make the EnumCaseDecls hidden, i.e. they resolve to their child EnumElementDecls.

This would only work if each EnumCaseDecl would have exactly one EnumElementDecl, which is not the case.

Btw, with #13249 the customization point would be Decl.getMember, as Decls would cease to be hideable.

[MathiasVP]

A couple of comments and suggestions, but otherwise this LGTM

[d10c]

LGTM! Let's merge this as soon as DCA is positive (also, .generated needs to be rebuilt it seems).

[geoffw0]

Let's merge this as soon as DCA is positive

I don't think DCA will be very revealing here, but you're the second person to ask, so I'll start one...

(also, .generated needs to be rebuilt it seems).

Fixed.

[geoffw0]

DCA LGTM.