FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Type inference: Improve base type matching by hvitved · Pull Request #22761 · github/codeql · GitHub

  1. github
  2. codeql

Repository navigation

Type inference: Improve base type matching - #22761

Merged
hvitved merged 3 commits into
github:mainfrom
hvitved:type-inference-more-base-matching
Oct 9, 2026
ConversationCommits (3)ChecksFindingsFiles changed
Merged

Type inference: Improve base type matching#22761
hvitved merged 3 commits into
github:mainfrom
hvitved:type-inference-more-base-matching

Conversation

hvitved commented Oct 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Before this PR, type information could only propagate backwards through sub typing when using type constraints (because that is the only form supported in Rust):

class Base<B> {
  init(_ b : B) { }
}

class Derived<D> : Base<[D]> {  }

func foo<T1, T2: Base<T1>>(_ value1: T1, _ base: T2) {  }

func bar<T1>(_ value1: T1, _ base: Base<T1>) { }

foo([0], Derived([])) // works: can infer the element type of `[]` to be `Int`
bar([0], Derived([])) // doesn't work: cannot infer the element type of `[]` to be `Int`

This PR closes that gap, which is needed for proper handling of inherited constructors in Swift.

DCA is uneventful for both Rust and Swift.

github-actions Bot added Rust Pull requests that update Rust code Unified labels Oct 6, 2026
hvitved force-pushed the type-inference-more-base-matching branch 2 times, most recently from b16635a to 26a8bbc Compare October 7, 2026 12:07
hvitved force-pushed the type-inference-more-base-matching branch from 26a8bbc to 8cb1938 Compare October 7, 2026 13:22
github-actions Bot removed the Rust Pull requests that update Rust code label Oct 7, 2026
hvitved requested a balanced review from Copilot October 7, 2026 14:33
Copilot AI reviewed Oct 7, 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Copilot review overview

🔵 Needs a closer look

Shared type-inference semantics affect multiple frontends and warrant human validation despite focused Swift coverage.

Review effort: Balanced
Findings: 1

Open (1) What changed in this PR

Improves nested generic type inference through base-type relationships.

Changes:

  • Matches type parameters across nested base-type paths.
  • Supports contextual inference for empty array literals.
  • Adds Swift generic-inheritance tests and updated expectations.
File Description
unified/​ql/​test/​library-tests/​type-inference/​type-inference.expected Updates generated inference results.
unified/​ql/​test/​library-tests/​type-inference/​generics.swift Adds nested generic inference cases.
unified/​ql/​lib/​codeql/​unified/​internal/​typeinference/​TypeInference.qll Models unknown empty-array element types.
unified/​ql/​lib/​codeql/​unified/​internal/​FacadeAst.qll Exposes array element counts.
shared/​typeinference/​codeql/​typeinference/​internal/​TypeInference.qll Adds nested base-type parameter matching.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread shared/typeinference/codeql/typeinference/internal/TypeInference.qll Outdated
hvitved force-pushed the type-inference-more-base-matching branch from 8cb1938 to 468caa3 Compare October 7, 2026 14:45
hvitved added the no-change-note-required This PR does not need a change note label Oct 7, 2026
hvitved marked this pull request as ready for review October 7, 2026 18:42
hvitved requested review from a team as code owners October 7, 2026 18:42
hvitved requested a review from paldepind October 7, 2026 18:42
paldepind reviewed Oct 9, 2026
View reviewed changes

paldepind 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Two questions:

  • Shouldn't we take variance into account? For instance, this type propagation does not make sense for contravariant positions such as the return position? Might it makes sense to have variance encoded on access positions with a type that can be invariant/contravariant/covariant?

  • I wonder if it wouldn't be possible to reuse more of the existing contraint propagation? I haven't thought this through in terms of the QL, but intuitively foo and bar in your example are very similar. Would it not be possible to ensure that they're covered by the same code path and that constraints on parameters are picked up both through type parameters and through direct types that can be subtyped?

hvitved commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author
  • Shouldn't we take variance into account? For instance, this type propagation does not make sense for contravariant positions such as the return position? Might it makes sense to have variance encoded on access positions with a type that can be invariant/contravariant/covariant?

Ideally, yes, but that is already not supported, but we might consider it for the future.

  • I wonder if it wouldn't be possible to reuse more of the existing contraint propagation? I haven't thought this through in terms of the QL, but intuitively foo and bar in your example are very similar. Would it not be possible to ensure that they're covered by the same code path and that constraints on parameters are picked up both through type parameters and through direct types that can be subtyped?

I thought about doing that, but I went for the simpler solution for now.

paldepind approved these changes Oct 9, 2026
View reviewed changes

paldepind 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the answers. I think we should look into that in the future, but this also LGTM as-is! :)

hvitved merged commit dc7b9bd into github:main Oct 9, 2026
106 of 114 checks passed
hvitved deleted the type-inference-more-base-matching branch October 9, 2026 12:53
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
Show hidden characters
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
paldepind paldepind approved these changes

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

Back | FazBrowse Home | New Git URL