Visitar URL original
Unified: Improve static name binding involving shadowing by hvitved · Pull Request #22782 · github/codeql · GitHub
Skip to content

Unified: Improve static name binding involving shadowing - #22782

Draft
hvitved wants to merge 4 commits into
github:mainfrom
hvitved:unified/static-name-binding-shadowing
Draft

Unified: Improve static name binding involving shadowing#22782
hvitved wants to merge 4 commits into
github:mainfrom
hvitved:unified/static-name-binding-shadowing

Conversation

@hvitved

@hvitved hvitved commented Oct 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR implements two aspects regarding shadowing:

  • Take signatures into account (for now, only names, not types).
  • Remove constructors generated in the extractor when there are in fact an inherited constructor available.

github-advanced-security[bot]
github-advanced-security Bot found potential problems Oct 8, 2026
View reviewed changes
Comment thread unified/ql/lib/codeql/unified/internal/Ast.qll
}

class Member extends @unified_member, F::AstNode { }
class Member extends @unified_member, F::AstNode {
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Oct 8, 2026
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 10:54
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

Signature keys omit type information, and inaccessible parent constructors can incorrectly suppress generated constructors.

1 open finding
What changed in this PR

Improves unified Swift static name binding for inherited member shadowing and synthesized constructors.

Changes:

  • Adds signature-aware inherited-member lookup.
  • Filters synthesized constructors when inheritable constructors exist.
  • Extends the unified member AST API with name nodes and adds coverage.
File Description
unified/​extractor/​ast_types.yml Adds member name-node support.
unified/​ql/​lib/​codeql/​unified/​internal/​Ast.qll Regenerates member accessor overrides.
unified/​ql/​lib/​codeql/​unified/​internal/​FacadeAst.qll Overrides variable name-node lookup.
unified/​ql/​lib/​codeql/​unified/​internal/​NameBindingPlugin.qll Adds shadowing and invalid-member extension points.
unified/​ql/​lib/​codeql/​unified/​internal/​NameBindingPluginSwift.qll Implements Swift shadowing and constructor filtering.
unified/​ql/​lib/​codeql/​unified/​internal/​StaticNameBinding.qll Applies keyed shadowing during inherited lookup.
unified/​ql/​test/​library-tests/​static-name-binding/​inheritance.swift Tests overload and inherited-constructor binding.

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll Outdated
@hvitved
hvitved force-pushed the unified/static-name-binding-shadowing branch from 07aaa47 to cb9d071 Compare October 8, 2026 11:21
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 11:35
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

Constructor filtering mishandles explicit, transitive generated, and convenience initializer inheritance.

2 open findings
1 resolved since last review

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll Outdated
Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll Outdated
@hvitved
hvitved force-pushed the unified/static-name-binding-shadowing branch from cb9d071 to 484ccf3 Compare October 8, 2026 12:40
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 12:41
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

The constructor filter incorrectly removes valid inherited initializers when convenience initializers are declared.

1 open finding
2 resolved since last review

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll Outdated
@hvitved
hvitved force-pushed the unified/static-name-binding-shadowing branch from 484ccf3 to cd47298 Compare October 8, 2026 13:18
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 13:18
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

Constructor filtering mishandles protocol initializer requirements and valid inherited convenience initializers.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll Outdated
@hvitved
hvitved force-pushed the unified/static-name-binding-shadowing branch from cd47298 to db279a5 Compare October 8, 2026 13:59
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 14:01
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

Constructor filtering incorrectly removes inherited convenience initializers that remain valid under Swift’s automatic inheritance rules.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll
@hvitved
hvitved force-pushed the unified/static-name-binding-shadowing branch from db279a5 to d0deee9 Compare October 8, 2026 16:04
@hvitved
hvitved force-pushed the unified/static-name-binding-shadowing branch from d0deee9 to 5796f97 Compare October 8, 2026 16:09
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 16:10
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

Constructor inheritance can incorrectly consider designated initializers already rejected from the parent’s effective initializer set.

1 open finding
1 resolved since last review

🧠 Review effort: Balanced

Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

Comment thread unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll
@hvitved
hvitved requested a balanced review from Copilot October 8, 2026 17:00
Copilot AI reviewed Oct 8, 2026
View reviewed changes

Copilot AI left a comment

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.

🔵 Needs a closer look

Recursive name resolution and Swift constructor inheritance retain documented corner cases requiring final human judgment.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
+1 more reviewer
@github-advanced-security github-advanced-security[bot] github-advanced-security[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

no-change-note-required This PR does not need a change note Unified

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hvitved @github-advanced-security