Skip to content

Swift: Add EnumDecl.getEnumElement(_) #13213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
redsun82 merged 12 commits into from
May 23, 2023
Merged

Swift: Add EnumDecl.getEnumElement(_) #13213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
66b13e2
Swift: Add a test of enum decls.
geoffw0 May 17, 2023
95caaec
Swift: Add EnumDecl.getEnumElement(_).
geoffw0 May 17, 2023
6c35bbf
Swift: Simplify / focus the test.
geoffw0 May 17, 2023
3539e55
Swift: Autoformat.
geoffw0 May 17, 2023
f0ce5b0
Swift: Address QL-for-QL warning.
geoffw0 May 18, 2023
d26a861
Swift: Codegen.
geoffw0 May 18, 2023
5ffde7a
Update swift/ql/lib/codeql/swift/elements/decl/EnumDecl.qll
geoffw0 May 19, 2023
881134a
Swift: Add warning note to Decl.getMember.
geoffw0 May 19, 2023
8306862
Merge branch 'main' into hideenumcasedecl
geoffw0 May 22, 2023
3bcaff6
Swift: re-run codegen.
geoffw0 May 22, 2023
6059d8c
Merge branch 'codeql-cli-2.13.3' into hideenumcasedecl
redsun82 May 23, 2023
67d4788
Merge branch 'codeql-cli-2.13.3' into hideenumcasedecl
redsun82 May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions swift/ql/.generated.list
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion swift/ql/.gitattributes
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 34 additions & 2 deletions swift/ql/lib/codeql/swift/elements/decl/EnumDecl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,36 @@
// generated by codegen/codegen.py, remove this comment if you wish to edit this file
private import codeql.swift.generated.decl.EnumDecl
private import codeql.swift.elements.decl.EnumCaseDecl
private import codeql.swift.elements.decl.EnumElementDecl
private import codeql.swift.elements.decl.Decl

class EnumDecl extends Generated::EnumDecl { }
/**
* An enumeration declaration, for example:
* ```
* enum MyColours {
* case red
* case green
* case blue
* }
* ```
*/
class EnumDecl extends Generated::EnumDecl {
/**
* Gets the `index`th enumeration element of this enumeration (0-based).
*/
final EnumElementDecl getEnumElement(int index) {
Comment thread
d10c marked this conversation as resolved.
result =
rank[index + 1](int memberIndex, Decl d |
d = this.getMember(memberIndex) and
d instanceof EnumElementDecl
Comment thread
d10c marked this conversation as resolved.
|
d order by memberIndex
)
}

/**
* Gets an enumeration element of this enumeration.
*/
final EnumElementDecl getAnEnumElement() {
result = this.getMember(_).(EnumCaseDecl).getElement(_)
}
Comment on lines +33 to +35
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 May 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@d10c d10c May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@redsun82 redsun82 May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
4 changes: 4 additions & 0 deletions swift/ql/lib/codeql/swift/generated/Raw.qll
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions swift/ql/lib/codeql/swift/generated/decl/Decl.qll
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions swift/ql/test/library-tests/elements/decl/enumdecl/enumdecl.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
| enumdecl.swift:2:1:6:1 | MyColours | (EnumDecl), .getEnumElement(0) = red, .getEnumElement(1) = green, .getEnumElement(2) = yellow, .getEnumElement(3) = blue, .getType = MyColours |
| enumdecl.swift:3:2:3:7 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyColours, .getElement(0) = red |
| enumdecl.swift:3:7:3:7 | red | (EnumElementDecl), .getDeclaringDecl = MyColours |
| enumdecl.swift:4:2:4:14 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyColours, .getElement(0) = green, .getElement(1) = yellow |
| enumdecl.swift:4:7:4:7 | green | (EnumElementDecl), .getDeclaringDecl = MyColours |
| enumdecl.swift:4:14:4:14 | yellow | (EnumElementDecl), .getDeclaringDecl = MyColours |
| enumdecl.swift:5:2:5:7 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyColours, .getElement(0) = blue |
| enumdecl.swift:5:7:5:7 | blue | (EnumElementDecl), .getDeclaringDecl = MyColours |
| enumdecl.swift:8:1:11:1 | MyContainer | (EnumDecl), .getEnumElement(0) = str, .getEnumElement(1) = pair, .getType = MyContainer |
| enumdecl.swift:9:2:9:17 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyContainer, .getElement(0) = str |
| enumdecl.swift:9:7:9:17 | str | (EnumElementDecl), .getDeclaringDecl = MyContainer, .getParam(0) = _ |
| enumdecl.swift:10:2:10:26 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyContainer, .getElement(0) = pair |
| enumdecl.swift:10:7:10:26 | pair | (EnumElementDecl), .getDeclaringDecl = MyContainer, .getParam(0) = x, .getParam(1) = y |
| enumdecl.swift:13:1:16:1 | MyNumbers | (EnumDecl), .getEnumElement(0) = one, .getEnumElement(1) = two, .getEnumElement(2) = three, .getEnumElement(3) = four, .getType = MyNumbers |
| enumdecl.swift:14:2:14:16 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyNumbers, .getElement(0) = one, .getElement(1) = two |
| enumdecl.swift:14:7:14:13 | one | (EnumElementDecl), .getDeclaringDecl = MyNumbers |
| enumdecl.swift:14:16:14:16 | two | (EnumElementDecl), .getDeclaringDecl = MyNumbers |
| enumdecl.swift:15:2:15:14 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyNumbers, .getElement(0) = three, .getElement(1) = four |
| enumdecl.swift:15:7:15:7 | three | (EnumElementDecl), .getDeclaringDecl = MyNumbers |
| enumdecl.swift:15:14:15:14 | four | (EnumElementDecl), .getDeclaringDecl = MyNumbers |
| enumdecl.swift:18:1:20:1 | MyGreek | (EnumDecl), .getEnumElement(0) = alpha, .getEnumElement(1) = beta, .getEnumElement(2) = gamma, .getEnumElement(3) = delta, .getEnumElement(4) = epsilon, .getType = MyGreek |
| enumdecl.swift:19:2:19:34 | case ... | (EnumCaseDecl), .getDeclaringDecl = MyGreek, .getElement(0) = alpha, .getElement(1) = beta, .getElement(2) = gamma, .getElement(3) = delta, .getElement(4) = epsilon |
| enumdecl.swift:19:7:19:7 | alpha | (EnumElementDecl), .getDeclaringDecl = MyGreek |
| enumdecl.swift:19:14:19:14 | beta | (EnumElementDecl), .getDeclaringDecl = MyGreek |
| enumdecl.swift:19:20:19:20 | gamma | (EnumElementDecl), .getDeclaringDecl = MyGreek |
| enumdecl.swift:19:27:19:27 | delta | (EnumElementDecl), .getDeclaringDecl = MyGreek |
| enumdecl.swift:19:34:19:34 | epsilon | (EnumElementDecl), .getDeclaringDecl = MyGreek |
25 changes: 25 additions & 0 deletions swift/ql/test/library-tests/elements/decl/enumdecl/enumdecl.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import swift

string describe(Decl d) {
d instanceof EnumDecl and result = "(EnumDecl)"
or
d instanceof EnumCaseDecl and result = "(EnumCaseDecl)"
or
d instanceof EnumElementDecl and result = "(EnumElementDecl)"
or
result = ".getType = " + d.(EnumDecl).getType().toString()
or
result = ".getDeclaringDecl = " + d.getDeclaringDecl().toString()
or
exists(int i |
result = ".getElement(" + i.toString() + ") = " + d.(EnumCaseDecl).getElement(i).toString()
or
result = ".getParam(" + i.toString() + ") = " + d.(EnumElementDecl).getParam(i).toString()
or
result = ".getEnumElement(" + i.toString() + ") = " + d.(EnumDecl).getEnumElement(i).toString()
)
}

from Decl d
where d.getLocation().getFile().getName() != ""
select d, strictconcat(describe(d), ", ")
20 changes: 20 additions & 0 deletions swift/ql/test/library-tests/elements/decl/enumdecl/enumdecl.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@

enum MyColours {
case red
case green, yellow
case blue
}

enum MyContainer {
case str(String)
case pair(x: Int, y: Int)
}

enum MyNumbers: Int {
case one = 1, two
case three, four
}

enum MyGreek {
case alpha, beta, gamma, delta, epsilon
}
6 changes: 5 additions & 1 deletion swift/schema.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ class Type(Element):
@group("decl")
class Decl(AstNode):
module: "ModuleDecl"
members: list["Decl"] | child
members: list["Decl"] | child | desc("""
Prefer to use more specific methods (such as `EnumDecl.getEnumElement`) rather than relying
on the order of members given by `getMember`. In some cases the order of members may not
align with expectations, and could change in future releases.
""")

@group("expr")
class Expr(AstNode):
Expand Down