Swift: make only certain elements hideable in the AST by redsun82 · Pull Request #13249 · github/codeql

redsun82 · 2023-05-23T08:34:08Z

By tweaking code generation and adding an explicit @ql.hideable attribute in the schema, only certain specific types get the resolve infrastructure, decreasing the number of Immediate predicates being generated.

geoffw0 · 2023-05-23T09:19:29Z

-      this.resolvesFrom(e) and
-      result = e.getFullyUnresolved()
-    )
-  }


Is it possible to have a hideable parent of a non-hideable child?

you mean in AST parent/child terms, or in superclass/subclass terms? Both can mix, which just made me think of an issue with this change: hideability should actually travel both directions of inheritance. For example marking Expr explicitly as hideable, doesn't only mean that all Expr subclasses are hideable, but also superclasses such as AstNode. Failing to do so like I'm doing here means that something like BraceStmt.getElement/1 would fail to hide conversions which would be quite confusing.

The worry that prompted this question was the removal of getFullyUnresolved() calls from non-hideable elements. Which seems maybe wrong if a hideable element can resolve to it (I'm not really sure why we use getFullyUnresolved() though).

yeah, it was wrong, I ended up reverting that. This does mean one could implement getResolveStep on a non-hideable class, which would compile but not really work. Just something to keep in mind. We can add final overrides to the non-hideable AstNode classes to avoid such a problem if we see this happening at some point.

geoffw0

You've addressed my concerns, and I want this change, but I can't confidently say I have a full understanding of everything that is going on.

My understanding is:

the goal of this change is to reduce the amount of generated QL and potentially make things a bit speedier as well.
previously everything was hideable.
now 'just' Type, Expr and Pattern are hideable...
- plus everything above them in the QL class tree (e.g. Element), because when you have an Element x it could in fact be a Type / Expr / Pattern.
- plus by anything below them in the QL class tree (e.g. BitwiseOperation), because those things are a Type / Expr / Pattern.
but plenty of specific classes outside of that are not hideable any more.
for non-hideable classes, we can skip including the getImmediate* machinery and calls to .getResolveStep() and the like.
- but not the other direction, .getFullyUnresolved(), because the result could be hideable.

I would appreciate if you could correct any misconceptions in the above, and perhaps get a second pair of eyes on this?

redsun82 · 2023-05-23T12:34:42Z

You've addressed my concerns, and I want this change, but I can't confidently say I have a full understanding of everything that is going on.

My understanding is:

the goal of this change is to reduce the amount of generated QL and potentially make things a bit speedier as well.

previously everything was hideable.

now 'just' Type, Expr and Pattern are hideable...

plus everything above them in the QL class tree (e.g. Element), because when you have an Element x it could in fact be a Type / Expr / Pattern.

plus by anything below them in the QL class tree (e.g. BitwiseOperation), because those things are a Type / Expr / Pattern.

but plenty of specific classes outside of that are not hideable any more.

for non-hideable classes, we can skip including the getImmediate* machinery and calls to .getResolveStep() and the like.

but not the other direction, .getFullyUnresolved(), because the result could be hideable.

I would appreciate if you could correct any misconceptions in the above, and perhaps get a second pair of eyes on this?

I've added @MathiasVP as a reviewer too, as yes, two set of eyes is better than one.

Your analysis is correct, although it is to be noted that as far as the current actually hidden nodes go, it is also not really possible for unhideable nodes to have a .getFullyUnresolved() different from this, because no current getResolveStep() actually gives a non-hideable result. I also don't foresee it happening ever, but the possibility is there. So, theoretically, we could remove .getFullyUnresolved() from all non-Expr, non-Pattern or non-Type things. But I preferred to actually revert all changes outside those strictly required, so that the only hand-written changes to QL code in this PR are related to some getProperty becoming the customization points to override in place of getImmediateProperty.

geoffw0 · 2023-05-23T13:23:49Z

Happy with this PR pending @MathiasVP s opinion and a successful DCA run.

MathiasVP

I think the changes makes sense. A couple of open questions. I'd also like to run a few CFG consistency queries on this PR to make sure we didn't break CFG construction in some subtle way.

MathiasVP · 2023-05-23T15:59:05Z

  // constants that resolve to the same value.
  exists(ControlFlowElement parent |
-    parent.asAstNode() = n.asAstNode().getResolveStep() and
+    parent.asAstNode() = n.asAstNode().(HideableElement).getResolveStep() and


I'm not sure I like this change very much 🤔. So now, if you have a value of a very general class (like ... Locatable, for example) and you want to resolve it to skip hidden elements you have to explicitly consider two cases:

Either it's an instance of HideableElement and so you have to do an inline cast and call getResolveStep, or

It's not a HideableElement and you don't have to do anything.

this was indeed reverted in a later commit

MathiasVP · 2023-05-23T16:01:31Z

  n = any(ConditionElement condElem).getBoolean().getFullyUnresolved()
  or
-  n = any(ConditionElement condElem).getAvailability().getFullyUnresolved()
+  n = any(ConditionElement condElem).getAvailability()


An open question:

Looking through the changes to CFG construction, we've had bugs in the past because someone (most likely me 😅) forgot to call recursively on the fully unresolved / fully converted sub-element which led to inconsistent CFGs. And in those cases it was pretty easy to see syntactically that:

We were calling getFullyUnresolved (or getFullyConverted) in 99% of the cases, and

1% of the cases were missing a call to getFullyUnresolved (or getFullyConverted)

And so that 1% was most likely a bug. But now some places will need the to call getFullyUnresolved (when it's available), and other places won't need to do that (because there's no such method on the class).

Do you think this is a problem?

I backtracked on this on a later commit. As I understood that "hideability" travels both directions of a hierarchy, Element ended up hideable no matter what, so in the end I kept the resolve/getFullyUnresolved predicates on Element, and undid all these changes. I agree that that was a possible source of bugs.

Sorry for the mix up.

MathiasVP · 2023-05-23T21:41:03Z

@redsun82 it took me a few restarts to get a pair of semmle-code and codeql SHAs with compatible dbschemes, but that last experiment seems to have worked 😂.

redsun82 · 2023-05-24T06:19:16Z

DCA looks good to me, apart the known Thomvis/Construct wobbly number of extracted files.

redsun82 · 2023-05-24T06:27:21Z

@geoffw0: @MathiasVP's comments are addressed by later commits, and DCA shows nothing suspicious, so I think we can merge this.

redsun82 · 2023-05-24T06:53:28Z

Ah, wait, there is this comment by @MathiasVP:

I'd also like to run a few CFG consistency queries on this PR to make sure we didn't break CFG construction in some subtle way.

Do you know what it means @geoffw0? On the other hand, as far as I understand this was a comment based on the CFG library changes, that got completely reverted.

geoffw0 · 2023-05-24T08:26:10Z

Do you know what it means @geoffw0?

I guess it means he wants to run a few more checks locally and build confidence. When/if @MathiasVP gives the 👍, I'm happy for this to be merged.

aibaars · 2023-05-24T09:19:02Z

Ah, wait, there is this comment by @MathiasVP:

I'd also like to run a few CFG consistency queries on this PR to make sure we didn't break CFG construction in some subtle way.

I guess it means running the equivalent of https://github.com/github/codeql/blob/main/ruby/ql/consistency-queries/CfgConsistency.ql . For Ruby we run consistency checks automatically in CI as part of codeql test run --consistency-queries ...

 --consistency-queries=<dir>
                             [Advanced] A directory with consistency queries that will be run for each test database. These queries should not produce any output (except when they find a problem) unless the test directory includes
                               a CONSISTENCY subdirectory with a .expected file. This is mostly useful for testing extractors.

github-actions Bot added the Swift label May 23, 2023

redsun82 mentioned this pull request May 23, 2023

Swift: Add EnumDecl.getEnumElement(_) #13213

Merged

geoffw0 reviewed May 23, 2023

View reviewed changes

redsun82 changed the base branch from main to codeql-cli-2.13.3 May 23, 2023 10:41

redsun82 added 4 commits May 23, 2023 12:45

Swift: add @ql.hideable to schema loading

7dd18ff

Swift: implement @ql.hideable

a087fef

Swift: make only Expr, Pattern and Type hideable

b19194b

Swift: make hideability spread upward in the hierarchy

b4edc92

redsun82 force-pushed the redsun82/swift-hidden-ast branch from 1e4fc05 to b4edc92 Compare May 23, 2023 10:45

redsun82 marked this pull request as ready for review May 23, 2023 11:02

redsun82 requested a review from a team as a code owner May 23, 2023 11:02

geoffw0 reviewed May 23, 2023

View reviewed changes

redsun82 requested a review from MathiasVP May 23, 2023 12:25

geoffw0 mentioned this pull request May 23, 2023

Swift: reorganize VarDecl instances within BraceStmt #13240

Merged

redsun82 added 2 commits May 23, 2023 15:31

Merge branch 'codeql-cli-2.13.3' into redsun82/swift-hidden-ast

fce33d3

Merge branch 'codeql-cli-2.13.3' into redsun82/swift-hidden-ast

09ce29b

MathiasVP reviewed May 23, 2023

View reviewed changes

geoffw0 approved these changes May 24, 2023

View reviewed changes

redsun82 merged commit ff78ac9 into codeql-cli-2.13.3 May 24, 2023

redsun82 deleted the redsun82/swift-hidden-ast branch May 24, 2023 09:22

Conversation

redsun82 commented May 23, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

redsun82 commented May 23, 2023

Uh oh!

geoffw0 commented May 23, 2023

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented May 23, 2023

Uh oh!

redsun82 commented May 24, 2023

Uh oh!

redsun82 commented May 24, 2023

Uh oh!

redsun82 commented May 24, 2023

Uh oh!

geoffw0 commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aibaars commented May 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

geoffw0 commented May 24, 2023 •

edited

Loading