Unified: Resolve accessor accesses in type inference by hvitved · Pull Request #22909 · github/codeql

hvitved · 2026-10-27T20:10:41Z

No description provided.

Copilot

🟡 Changes recommended

Calls to function-valued computed properties incorrectly include accessor targets and add the property's Function type to the inferred call result.

5 open issues

What changed in this PR

Extends Unified’s type inference to resolve Swift property getters and setters alongside stored fields.

Changes:

Adds accessor-aware member lookup and read/write resolution.
Preserves backing-field bindings, unwraps modified types, and distinguishes getter/setter names.
Adds Swift accessor fixtures and updates type-inference expectations.

File	Description
`unified/ql/test/library-tests/type-inference/type-inference.ql`	Checks accessor-resolution annotations.
`unified/ql/test/library-tests/type-inference/type-inference.expected`	Updates expected inferred types.
`unified/ql/test/library-tests/type-inference/accessors.swift`	Covers computed, observed, generic, inherited, and protocol properties.
`unified/ql/lib/codeql/unified/internal/typeinference/TypeMention.qll`	Resolves modified types through their underlying type.
`unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll`	Models accessor fields and exposes accessor resolution.
`unified/ql/lib/codeql/unified/internal/NameBindingPlugin.qll`	Includes accessors in instance-member classification.
`unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll`	Prefers backing fields over associated accessors.
`unified/ql/lib/codeql/unified/internal/FacadeAst.qll`	Adds distinct getter and setter fallback names.

_{Review effort: Max}

Copilot

🟡 Changes recommended

Unresolved Swift accessor defects lose static/class modifiers, confuse static and instance names, and miss inout setters and top-level/local accessor references, breaking resolution and type inference.

3 open issues

5 resolved since last review

_{Review effort: Max}

Copilot

🟡 Changes recommended

Computed properties passed as Swift inout arguments omit writeback setter resolution, while property observers can bypass the intended backing-field exclusion.

2 open issues

3 resolved since last review

_{Review effort: Max}

hvitved · 2026-10-29T10:53:54Z

+class InheritedAccessors: GenericAccessors<Int> {
+  override var observed: Int {
+    didSet {
+      let current = observed  // $ accessor=InheritedAccessors.observed $ MISSING: type=current:Int field=GenericAccessors.observed -- static name binding is currently unable to resolve the access to the backing field


@asgerf : Is this perhaps something you can look into?

Copilot

🟡 Changes recommended

Class-valued properties passed directly as inout lose setter dataflow, while incompatible shadowing keys cause stored properties to resolve to spurious inherited setter targets.

2 open issues

2 resolved since last review

_{Review effort: Max}

asgerf · 2026-10-29T12:21:15Z

+          name = p.getExternalNameNode().getValue()
+          or
+          not exists(p.getExternalNameNode()) and
+          name = "_"


I think we're OK with a spurious setter call

asgerf · 2026-10-29T12:14:50Z

+    // conservative value-type check
+    not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier("class"))


This makes sense although I'd add a boolean parameter to isInWriteBackPosition to remember if it's in direct write-back position.

asgerf

Some minor comments otherwise LGTM 👍

asgerf · 2026-10-29T10:57:38Z

+        }
+        var q: C {
+            get { C() }
+            set { sink(newValue.p.0) }  // $ SPURIOUS: hasValueFlow=t11.2


Add a comment to explain why it should not flow in here?

asgerf · 2026-10-29T11:52:22Z

-    // TODO: Check that 'expr' has a value type. Reference types don't cause writebacks.
+    isInWriteBackPosition(expr) and
+    // conservative value-type check
+    not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier("class"))


Suggested change

not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier("class"))

not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier(["class", "actor"]))

I feel it would make sense to factor into a predicate isReferenceType(Type t). It should also hold for type parameters bounded by a reference-type but feel free to defer that for later. There should just be a single place that owns this.

asgerf · 2026-10-29T12:14:50Z

+    // conservative value-type check
+    not forex(T::Type t | t = T::inferType(expr) | t.asClass().hasModifier("class"))


This makes sense although I'd add a boolean parameter to isInWriteBackPosition to remember if it's in direct write-back position.

asgerf · 2026-10-29T12:21:15Z

+          name = p.getExternalNameNode().getValue()
+          or
+          not exists(p.getExternalNameNode()) and
+          name = "_"


I think we're OK with a spurious setter call

hvitved requested a max review from Copilot October 27, 2026 20:13

Copilot started reviewing on behalf of hvitved October 27, 2026 20:14

View session

github-actions Bot added the Unified label Oct 27, 2026

Copilot AI reviewed Oct 27, 2026

View reviewed changes

hvitved force-pushed the unified/type-inference-resolve-accessors branch from 06918fe to 16f5ce6 Compare October 28, 2026 08:28

hvitved requested a max review from Copilot October 28, 2026 08:28

Copilot started reviewing on behalf of hvitved October 28, 2026 08:29

View session

Copilot AI reviewed Oct 28, 2026

View reviewed changes

Comment thread unified/extractor/src/languages/swift/swift.rs Outdated

Comment thread unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll Outdated

Comment thread unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll Outdated

hvitved force-pushed the unified/type-inference-resolve-accessors branch 2 times, most recently from adec24a to 7ef8de1 Compare October 28, 2026 09:23

hvitved requested a max review from Copilot October 28, 2026 09:25

Copilot started reviewing on behalf of hvitved October 28, 2026 09:26

View session

Copilot AI reviewed Oct 28, 2026

View reviewed changes

Comment thread unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll Outdated

Comment thread unified/ql/lib/codeql/unified/internal/typeinference/TypeInference.qll Outdated

hvitved force-pushed the unified/type-inference-resolve-accessors branch from 7ef8de1 to 27ba20a Compare October 28, 2026 09:48

hvitved added the no-change-note-required This PR does not need a change note label Oct 28, 2026

hvitved marked this pull request as ready for review October 28, 2026 09:49

hvitved requested review from a team as code owners October 28, 2026 09:49

hvitved added 2 commits October 29, 2026 11:49

Unified: Add accessor FP/FN flow test

9fd04e6

Unified: Resolve accessor accesses in type inference

5b468f2

hvitved force-pushed the unified/type-inference-resolve-accessors branch from 27ba20a to 5b468f2 Compare October 29, 2026 10:52

hvitved requested a max review from Copilot October 29, 2026 10:52

Copilot started reviewing on behalf of hvitved October 29, 2026 10:53

View session

hvitved commented Oct 29, 2026

View reviewed changes

Copilot AI reviewed Oct 29, 2026

View reviewed changes

asgerf reviewed Oct 29, 2026

View reviewed changes

Address review comments

6263dc9

hvitved requested a review from a team as a code owner October 29, 2026 12:43

hvitved requested a review from asgerf October 29, 2026 12:44

asgerf approved these changes Oct 29, 2026

View reviewed changes

		// conservative value-type check
		not forex(T::Type t \| t = T::inferType(expr) \| t.asClass().hasModifier("class"))

Conversation

hvitved commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

hvitved Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

asgerf Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

asgerf Oct 29, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants