Repository navigation
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
force-pushed
the
unified/conditional-compilation
branch
from
November 3, 2026 10:46
bc24bfb to
b3bde8e
Compare
paldepind
changed the title
paldepind
marked this pull request as ready for review
November 3, 2026 11:26
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Conditional-compilation handling still drops valid synthesized initializers, mishandles name binding for flags and branch declarations, and causes false unused-variable reports.
4 open issues
What changed in this PR
This PR adds Swift conditional-compilation blocks to the Unified extractor and extends local name binding to follow declarations across compilation alternatives. Full static name-binding support remains follow-up work.
Changes:
- Represent
#if/#elseif/#elseblocks as nested conditional expressions. - Separate direct class members from compilation-expanded members and expose semantic top-level statements.
- Extend local scoping and add extraction, constructor, and control-flow coverage.
| File | Description |
|---|---|
| unified/ql/test/library-tests/local-name-binding/conditional-compilation.swift | Tests conditional bindings, shadowing, and captures. |
| unified/ql/test/library-tests/controlflow/cfg.swift | Adds conditional-compilation flow assertions. |
| unified/ql/test/library-tests/controlflow/cfg.expected | Records new control-flow results. |
| unified/ql/test/library-tests/controlflow/basicblock-slices.expected | Records conditional basic blocks. |
| unified/ql/test/library-tests/constructors/test.expected | Records conditional constructor and field results. |
| unified/ql/test/library-tests/constructors/constructors.swift | Adds conditional constructors and fields. |
| unified/ql/lib/unified.dbscheme | Updates member relations and conditional node schema. |
| unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll | Uses compilation-expanded top-level statements. |
| unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll | Recognizes conditional top-level private declarations. |
| unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll | Expands top-level static-member classification. |
| unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll | Marks compilation conditionals as unscoped. |
| unified/ql/lib/codeql/unified/internal/FacadeAst.qll | Exposes semantic statements and expanded members. |
| unified/ql/lib/codeql/unified/internal/AstExtra.qll | Expands top-level statement classification. |
| unified/ql/lib/codeql/unified/internal/Ast.qll | Updates generated AST accessors and types. |
| unified/extractor/tests/corpus/swift/variables/property-with-willset-and-didset-observers.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/struct.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/static-function.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/qualified-type.output | Updates nested direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/protocol-with-read-only-and-read-write-property-requirements.output | Updates protocol direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/protocol-declaration.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/property-with-getter-and-setter.output | Updates accessor direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/noncopyable-type.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/generic-class-parameters-and-constraints.output | Updates generated constructor placement. |
| unified/extractor/tests/corpus/swift/types/extension.output | Updates extension direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/enum-with-comma-separated-cases-chained-declaration.output | Updates enum direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/enum-with-cases.output | Updates enum direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/enum-with-associated-values.output | Updates enum-case constructor placement. |
| unified/extractor/tests/corpus/swift/types/empty-class.output | Updates generated constructor placement. |
| unified/extractor/tests/corpus/swift/types/constructor-with-parameters.output | Updates constructor direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/conditional-compilation-in-class-body.swift | Removes the old unsupported-block fixture. |
| unified/extractor/tests/corpus/swift/types/computed-property.output | Updates property direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/class-with-stored-properties.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/class-with-multiple-base-types.output | Updates generated constructor placement. |
| unified/extractor/tests/corpus/swift/types/class-with-method.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/class-with-initializer.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/types/class-inheritance.output | Updates generated constructor placement. |
| unified/extractor/tests/corpus/swift/types/class-function.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/functions/constructor-call-with-type-arguments.output | Updates generated constructor placement. |
| unified/extractor/tests/corpus/swift/expressions/super-expression.output | Updates class direct-member expectations. |
| unified/extractor/tests/corpus/swift/control-flow/discard-statement.output | Updates direct-member expectations. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-with-operators.swift | Adds compound compilation conditions. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-with-operators.output | Records condition operator extraction. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-with-elseif.swift | Adds an #elseif chain. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-with-elseif.output | Records nested conditional expressions. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-with-elseif-in-class-body.swift | Adds conditional initializer alternatives. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-with-elseif-in-class-body.output | Records conditional class members. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-postfix-expression.swift | Adds a postfix conditional fixture. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-postfix-expression.output | Records unsupported postfix extraction. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-nested.swift | Adds nested and empty compilation branches. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-nested.output | Records nested branch extraction. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-in-switch-cases.swift | Adds conditional switch cases. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-in-switch-cases.output | Records unsupported switch-case extraction. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-in-class-body.swift | Adds the relocated class-body fixture. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-in-class-body.output | Records extracted initializers and destructors. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-in-attributes.swift | Adds conditional attribute fixtures. |
| unified/extractor/tests/corpus/swift/conditional-compilation/conditional-compilation-in-attributes.output | Records current attribute extraction limitations. |
| unified/extractor/src/languages/swift/swift.rs | Lowers compilation blocks and searches conditional initializers. |
| unified/extractor/ast_types.yml | Defines conditional modifiers and direct members. |
| shared/namebinding/codeql/namebinding/LocalNameBinding.qll | Propagates shadowing declarations across unscoped alternatives. |
Review effort: Balanced (auto)
Comment on lines
+260
to
+263
| private predicate lastDeclAfter(AstNode node, SiblingShadowingDecl decl) { | ||
| node = decl | ||
| or | ||
| node = any(Conditional cond | isUnscopedConditional(cond) and lastDeclInBranch(cond, decl)) |
Comment on lines
+90
to
+92
| let has_explicit_initializer = members | ||
| .iter() | ||
| .any(|member| find_initializer(ctx.output, *member).is_some()); |
| (if_expr | ||
| // Mark this `if_expr` as coming from conditional compilation. | ||
| modifier: (modifier "conditional_compilation") | ||
| condition: {condition} |
| /** A `Stmt` at the top-level. */ | ||
| final class TopLevelStmt extends Stmt { | ||
| TopLevelStmt() { this = any(TopLevel t).getBody().getAStmt() } | ||
| TopLevelStmt() { this = any(TopLevel t).getBody().getASemanticStmt() } |
asgerf
reviewed
Nov 3, 2026
asgerf
left a comment
asgerf
left a comment
Contributor
There was a problem hiding this comment.
Some partial comments so far. I still need to review the local scoping changes.
| /** A block statement. */ | ||
| class Block extends G::Block { | ||
| /** Gets a statement, looking through conditional-compilation branches. */ | ||
| Stmt getASemanticStmt() { result = compilationExpandedStmt(this.getAStmt()) } |
Contributor
There was a problem hiding this comment.
The API we get after these changes:
- Classes:
getADirectMember()-- raw ASTgetAMember()-- include members in conditionalsgetAMemberIncludingExtensions()-- include members in extensions and conditionals
- Blocks:
getAStmt()-- raw ASTgetASemanticStmt()-- include members in conditionals
This is not a very consistent naming scheme. We can do better.
I'd suggest the following API. Boring but consistent.
- Classes:
getAMember()-- raw ASTgetAMemberIncludingConditionals()-- include members in conditionals (but not extensions)getAMemberIncludingExtensions()-- include members in extensions and conditionals
- Blocks:
getAStmt()-- raw ASTgetAStmtIncludingConditionals()-- renamed version ofgetASemanticStmt()
getAStmtIncludingConditionals() could possibly be removed and rolled into TopLevelStmt as it's the only current use-case (the other call sites can just use TopLevelStmt) but I admit it's hard to say if more use-cases will emerge.
Contributor
Author
There was a problem hiding this comment.
Some thoughts:
-
I think it's a downside that the "wrong" predicate (the raw member version for classes, which is rarely what you want) gets the shorter nicer name. That's why I renamed
getAMembertogetADirectMember, to make the probably-not-what-you-want predicate to have a longer name.For blocks it's different. Often get
getASemanticStmtisn't what you want, so I was ok withgetAStmthaving the short name. -
From the name it's not clear that
getAMemberIncludingExtensionsalso includes members in conditional compilation blocks. -
The word "semantic" is more vague, but also open to other things getting included in the future.
I don't have a strong preference though, so the suggested change (or other variations) are fine by me.
Contributor
Author
There was a problem hiding this comment.
For inspiration, this is what Copilot suggests. Longer names, but even more consistent and boring:
| Scope | Proposed accessor | Meaning |
|---|---|---|
| Class | getAMember() / getMember(i) |
Immediate AST members |
| Class | getAMemberIncludingConditionalCompilation() |
Flatten compilation branches; exclude extensions |
| Class | getAMemberIncludingExtensionsAndConditionalCompilation() |
Flatten compilation branches in the class and its extensions |
| Block | getAStmt() / getStmt(i) |
Immediate AST statements |
| Block | getAStmtIncludingConditionalCompilation() |
Flatten compilation branches |
Contributor
Author
There was a problem hiding this comment.
I've renamed things following Copilot's suggestion.
| * Gets the `i`th member in source order across all conditional-compilation alternatives, | ||
| * indexed from zero. | ||
| */ | ||
| Member getMember(int i) { |
Contributor
There was a problem hiding this comment.
I don't think we should provide indexed-access for "flattening accessors" like this. There should just be the plural version.
Contributor
Author
There was a problem hiding this comment.
I've removed this accessor and inlined it at the spot where it's used. Note that the commit also change how we create parameters for synthesized constructors for structs.
Extract conditional compilation blocks as if expressions. Members can now be nested inside conditional compilation blocks. Rename `member` to `direct_member` as this is now usually the wrong thing to access. A new predicate on the QL side finds all members, both direct and indirect ones through conditional compilation blocks.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f3fac01-33b2-423c-b823-11035cf44b84
paldepind
force-pushed
the
unified/conditional-compilation
branch
from
November 3, 2026 14:20
901c3b7 to
ec3a837
Compare
Cover unequal branch lengths, nested conditionals, and conditional fields without an else branch. Record the existing constructor parameter behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9225dea-8342-448b-88ec-ba9587d93816
When constructing parameters of synthesized constructors always assume that the `then` branch of conditional compilation blocks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9225dea-8342-448b-88ec-ba9587d93816
paldepind
force-pushed
the
unified/conditional-compilation
branch
from
November 6, 2026 12:05
0477b49 to
e4818cb
Compare
Preserve conditional compilation handling while adopting main's explicit empty content for unsupported Swift syntax. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a9225dea-8342-448b-88ec-ba9587d93816
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extract Swift conditional compilation blocks and handle them in local name binding.
Some of the changes:
Extract
#if/#elseif/#elseas nestedifexpressions.Separate direct class members from compilation-expanded members, exposed through
getAMember()/getMember(i).Extend the shared local name binding library to handle conditional-compilation blocks. The comments in QL should explain the changes for this.
NB: This PR does not adapt static name bindings to handle conditional compilation. I thought the PR was big enough as-is, so I'll do that in follow up work.
I suggest per-commit review.