Skip to content

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

Merged
merged 5 commits into from
May 3, 2025
Merged

Rust: make MacroStmts expressions #19335

merged 5 commits into from
May 3, 2025

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Apr 17, 2025

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:

    let unsafe_query_4 = if remote_string == "*" {
        "SELECT * FROM people".to_string()
    } else {
        format!("SELECT * FROM people WHERE firstname='{remote_string}'")
    };

This pull request changes the control and data flow of MacroStmts to match that of BlockExpr.

Thanks to @coadaflorin for spotting this case.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Apr 17, 2025
@aibaars aibaars force-pushed the aibaars/macro-stmts branch from faa65eb to b981967 Compare April 17, 2025 21:45
@aibaars aibaars marked this pull request as ready for review April 18, 2025 07:13
@Copilot Copilot AI review requested due to automatic review settings April 18, 2025 07:13
@aibaars aibaars requested a review from a team as a code owner April 18, 2025 07:13
Copy link
Contributor

@Copilot Copilot AI left a 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

Copy link
Contributor

@paldepind paldepind left a 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?

@aibaars aibaars force-pushed the aibaars/macro-stmts branch from f5884bd to 8101853 Compare May 2, 2025 13:09
@aibaars aibaars force-pushed the aibaars/macro-stmts branch from 8101853 to fb0269b Compare May 2, 2025 14:23
@aibaars aibaars force-pushed the aibaars/macro-stmts branch from fb0269b to 6bf50d8 Compare May 2, 2025 14:37
@aibaars
Copy link
Contributor Author

aibaars commented May 2, 2025

@paldepind I addressed your comments.

Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great 👍

@aibaars aibaars merged commit 9b0854e into main May 3, 2025
18 checks passed
@aibaars aibaars deleted the aibaars/macro-stmts branch May 3, 2025 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants