Skip to content

Swift: Extend AST classes and add control-flow library #9268

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.

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
merged 7 commits into from
May 23, 2022

Conversation

MathiasVP
Copy link
Contributor

Commit-by-commit review recommended:

Note that all code has already been reviewed during the proof-of-concept phase. A couple of changes were needed to fit the code onto Swift 5.6 (instead of version 5.5 for which the code was developed).

@MathiasVP MathiasVP requested a review from a team as a code owner May 23, 2022 12:01
@MathiasVP MathiasVP requested a review from rdmarsh2 May 23, 2022 12:01
@github-actions github-actions bot added the Swift label May 23, 2022
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label May 23, 2022
exists(ControlFlowElement pred | this.appliesTo(pred) | this.hasSuccessor(pred, cfe, _))
}

/** The `succ` relation restricted to predecessors `pred` that this split applies to. */

Check warning

Code scanning / CodeQL

Predicate QLDoc style.

The QLDoc for a predicate without a result should start with 'Holds'.
@MathiasVP MathiasVP force-pushed the swift-add-cfg-library branch from cd9e4f1 to 83bcb53 Compare May 23, 2022 12:06
@@ -1,5 +1,5 @@
| main.swift:3:8:3:13 | ConditionElement | main.swift:3:8:3:13 | BinaryExpr |
| main.swift:10:17:10:24 | ConditionElement | main.swift:10:17:10:24 | ParenExpr |
| main.swift:10:17:10:24 | ConditionElement | main.swift:10:18:10:22 | BinaryExpr |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change happens because of the implementation of convertsFrom in IdentityExpr. So now we skip past the parentheses.

private import codeql.swift.generated.expr.Expr

class Expr extends ExprBase { }
class Expr extends ExprBase {
final override Expr getResolveStep() { convertsFrom(result) }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand what's going on with the whole resolve mechanism - is it just the inverse of getConverted or is there more to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's pretty much it, yes. AST classes can override getResolveStep to not be included in the main AST (i.e., when navigating the parent/child nodes in the AST classes). This is done by autogenerating AST classes lik:

class FooBase {
  Element getBar() {
    exists(Bar bar |
      bar_of_foo(this, bar) and
      result = bar.resolve()
    )
  }
}

where resolve is then the TC of getResolveStep.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, resolve only does something in the case of IdentityExprs (which in particular include parenthesis expressions), ExplicitCasts and ImplicitConversions. For the Expr classes we provide aliases for result = getResolveStep() as convertsFrom(result) (the result became a parameter because we couldn't think of a good name), and getConversion(x) can then be implemented as "whatever expression is the result that converted something from x" (i.e., Expr getConversion() { result.convertsFrom(this) }).

Does that make sense?

@rdmarsh2
Copy link
Contributor

There's a bunch of implicit this warnings, but the code looks good to me otherwise

@MathiasVP
Copy link
Contributor Author

There's a bunch of implicit this warnings, but the code looks good to me otherwise

Most of them should be fixed in 4ba2984 now. There are a couple of ones left in the shared parts of the library, but I'd prefer that we fix those in a different PR.

@MathiasVP MathiasVP merged commit 9b0d84c into github:main May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants