Skip to content

Swift: add new TypeValueExpr to CFG #19490

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 2 commits into from
May 14, 2025
Merged

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented May 14, 2025

We do see errors about missing types from type reprs, which is not ideal. However I propose to postpone looking at that when type value expressions get out from being experimental.

Also, changed qltest.sh to be more liberal about the location of special option comments in the test source code.

@Copilot Copilot AI review requested due to automatic review settings May 14, 2025 09:26
@redsun82 redsun82 requested a review from a team as a code owner May 14, 2025 09:26
@github-actions github-actions bot added the Swift label May 14, 2025
@redsun82 redsun82 requested a review from jketema May 14, 2025 09:26
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 PR extends CFG support for the new experimental TypeValueExpr, adds ValueGenerics tests in both AST and CFG, and relaxes how extractor options/env markers are picked up in qltest.sh.

  • Relax sed usage in qltest.sh to capture extractor options and env from anywhere in the source file
  • Update control‐flow and AST tests (and their expected outputs) to account for TypeValueExpr, next(isolation:), and ValueGenerics support
  • Introduce a TypeValueTree class stub in ControlFlowGraphImpl.qll for the new expression kind

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
swift/tools/qltest.sh Allow //codeql-extractor-*- markers on any line (not just line 1)
swift/ql/test/library-tests/controlflow/graph/cfg.swift Reorder member‐ref tests and add ValueGenerics snippet
swift/ql/test/library-tests/controlflow/graph/Cfg.expected Update expected CFG output for TypeValueExpr, next(isolation:), and ValueGenerics
swift/ql/test/library-tests/ast/cfg.swift Mirror CFG changes in the AST test and add ValueGenerics snippet
swift/ql/test/library-tests/ast/PrintAst.expected Adjust variable indices and add missing‐type entries for ValueGenerics
swift/ql/test/library-tests/ast/Missing.expected Add missing-type entries for TypeValueExpr in ValueGenerics
swift/ql/test/library-tests/ast/Errors.expected Add corresponding UnspecifiedElement errors
swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll Add TypeValueTree stub to handle TypeValueExpr in the CFG builder
Comments suppressed due to low confidence (1)

swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll:1887

  • [nitpick] The new TypeValueTree stub is declared but has no integration logic or registered use. Ensure it’s hooked into the tree‐builder dispatch so TypeValueExpr nodes actually produce this tree in the CFG.
private class TypeValueTree extends AstLeafTree {

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM. I assume we generally want to do this for every new expression and/or statement that is introduced?

@redsun82
Copy link
Contributor Author

LGTM. I assume we generally want to do this for every new expression and/or statement that is introduced?

yes, I entirely forgot to mention it on your PR, and it only occurred again to me when rereading the ticket about it that was mentioning CFG 😅

@redsun82 redsun82 merged commit c2f2522 into main May 14, 2025
20 checks passed
@redsun82 redsun82 deleted the redsun82/swift-type-value-expr-cfg branch May 14, 2025 11:24
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.

2 participants