Skip to content

Swift: extract MacroDecl#14796

Merged
redsun82 merged 16 commits into
mainfrom
alexdenisov/macros
Nov 24, 2023
Merged

Swift: extract MacroDecl#14796
redsun82 merged 16 commits into
mainfrom
alexdenisov/macros

Conversation

@AlexDenisov
Copy link
Copy Markdown
Contributor

The other three elements are skipped (MacroExpansionExpr, MacroExpansionDecl, MissingDecl) as they only appear in cases when a macro is not expanded (due to an error).

They only appear in an intermediate AST and disappear as soon as the
macro is expanded.
The only way to get these in is to construct an "incorrect" AST, e.g.:

```
let x = #does_not_exist() // MacroExpansionExpr
struct S {
  #does_not_exist() // MacroExpansionDecl
}
```
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

A couple of suggestions, but otherwise this LGTM!


private import codeql.swift.generated.MacroRole

class MacroRole extends Generated::MacroRole {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you add some QLDoc to describe what a MacroRole is? 🙏

It looks like you added it to the autogenerated class via the schena.py file (which is awesome!), but I think we should port it to the user-facing file as well.

Comment on lines +1 to +10
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(expression)
#-----| [MacroRole] #freestanding(declaration)
#-----| [MacroRole] #freestanding(declaration)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like we should add MacroRole to the list of things that shouldn't be printed by PrintAST, no?

FWIW, that list of things is here: https://github.com/github/codeql/blob/main/swift/ql/lib/codeql/swift/printast/PrintAstNode.qll#L23

@AlexDenisov
Copy link
Copy Markdown
Contributor Author

Thank you @MathiasVP, these are great suggestions, I updated PR 🙌

Comment thread swift/schema.py
Copy link
Copy Markdown
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry again for the code generation bug.

@redsun82 redsun82 merged commit b514bd8 into main Nov 24, 2023
@redsun82 redsun82 deleted the alexdenisov/macros branch November 24, 2023 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants