-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: make MacroStmts expressions #19335
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
Conversation
faa65eb
to
b981967
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request refactors the handling of MacroStmts by updating its annotation to inherit dataflow behavior from ExprBlock and adjusts related test queries accordingly.
- Updated the MacroStmts annotation to include replace_bases and cfg parameters.
- Modified the formatting of the doc string in annotations.py.
- Revised the unsafe_query_4 definition in the MySQL test to use a conditional expression for query generation.
Reviewed Changes
Copilot reviewed 13 out of 25 changed files in this pull request and generated no comments.
File | Description |
---|---|
rust/schema/annotations.py | Updated MacroStmts annotation and adjusted doc string formatting. |
rust/ql/test/query-tests/security/CWE-089/sqlx.rs | Revised unsafe_query_4 to conditionally generate the query. |
Files not reviewed (12)
- rust/downgrades/48a13c5eec3663c32ae89bb0aad8a03f52ad68c3/upgrade.properties: Language not supported
- rust/ql/.generated.list: Language not supported
- rust/ql/lib/codeql/rust/controlflow/internal/ControlFlowGraphImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll: Language not supported
- rust/ql/lib/codeql/rust/elements/MacroStmts.qll: Language not supported
- rust/ql/lib/rust.dbscheme: Language not supported
- rust/ql/lib/upgrades/e8707b675dc574aca9863eabcc09ac76f15bb9c2/upgrade.properties: Language not supported
- rust/ql/test/library-tests/controlflow/BasicBlocks.expected: Language not supported
- rust/ql/test/library-tests/controlflow/Cfg.expected: Language not supported
- rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected: Language not supported
- rust/ql/test/library-tests/variables/Cfg.expected: Language not supported
- rust/ql/test/query-tests/security/CWE-089/SqlInjection.expected: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Seems like an important thing to get fixed as macros are often used as expressions.
I think we should also rename accordingly for clarity and consistency. MacroStmts
doesn't sound like an expression (which might be why it wasn't made one in the first place?) and with this change is the only thing in @expr
that doesn't have expr
in the name.
If I understand correctly MacroStmts
is very much like a block expression. So what about naming it MacroBlockExpr
? In addition we could rename its getExpr
predicate to getTailExpr
to make that clearer as well?
f5884bd
to
8101853
Compare
8101853
to
fb0269b
Compare
fb0269b
to
6bf50d8
Compare
@paldepind I addressed your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks great 👍
This pull request adds a dataflow step for the expression of a
MacroStmts
block.MacroStmts
nodes are inserted when a macro is expanded inside a block: for example:This pull request changes the control and data flow of
MacroStmts
to match that ofBlockExpr
.Thanks to @coadaflorin for spotting this case.