Shared, type inference: Add inference for type parameters with constraints (base type mentions) by paldepind · Pull Request #19102 · github/codeql

paldepind · 2025-03-24T13:01:32Z

The first commit in this PR is a small refactor that tries to:

Combine baseTypeMentionHasTypeParameterAt and baseTypeMentionHasNonTypeParameterAt into a single predicate, to cut down some duplication.
Reduce the number of tuples in hasBaseTypeMention by restrict the TypeMentions that it considers. On the type inference test database this refactor changes the number of tuples from 126 + 381 + 66 = 573 to 0 + 62 + 0 = 62 in that predicate, with no change to the results.

In addition the PR improves type inference for calls that target functions with type parameters that have trait bounds. For instance, when calling a function like

struct MyThing<T> { ... }
fn call_trait_thing_m1<T1, T2: MyTrait<T1>>(x: MyThing<T2>) -> T1 { ... }

the new typeParameterConstraintHasTypeParameter handles inferring T1. It does this by noticing

that T1 appears inside another type parameter T2,
that T2 occurs inside the declared type of x at the path T,
considering the type of the argument at T as a base type mention of MyTrait<T1> and inferring T1 from that.

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)

shared/typeinference/codeql/typeinference/internal/TypeInference.qll: Language not supported

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

paldepind · 2025-03-24T17:16:31Z

DCA is not looking good. I think maybe the change to declarationBaseType made that predicate grow in size. I'll investigate.

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

paldepind · 2025-03-25T15:17:07Z

I can't get DCA to work right now, but when running locally on rust it seems that the bad join is fixed, so I think this is ready for review 😃

Edit: DCA is working again and the results are now fine.

hvitved

Impressive work, very nice! My comments are mostly minor; one thing that I note is that by introducing the pathToSub generalization, we add even more assumptions about covariance, but we already have a follow-up task about handling contravariance properly.

hvitved · 2025-03-28T08:26:25Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

       *   of `Mid`,
-       * - ``C`1`` is mentioned at `0` for immediate base type mention `Mid<C<T4>>`
+       * - ``C`1`` is mentioned at `T3` for immediate base type mention `Mid<C<T4>>`
       *   of `Sub`, and


Remove and

hvitved · 2025-03-28T08:26:33Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

       *   of `Sub`, and
-       * - ``C`1`` is mentioned at `0` and implicitly at `0.0` for transitive base type
+       * - `T4` is mentioned at `T3.T1` for immediate base type mention `Mid<C<T4>>`
+       *   of `Sub`, and


Remove and

hvitved · 2025-03-28T08:26:46Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

+       * - `T4` is mentioned at `T3.T1` for immediate base type mention `Mid<C<T4>>`
+       *   of `Sub`, and
+       * - ``C`1`` is mentioned at `T2` and implicitly at `T2.T1` for transitive base type
       *   mention `Base<C<T3>>` of `Sub`.


Replace . with , and

hvitved · 2025-03-28T08:39:12Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

-          exists(Declaration target |
+        /**
+         * Holds if inferring types at `a` might depend on the type at `apos`
+         * having `baseMention` as a transitive base type mention.


Should be

having `base` as a transitive base type.

hvitved · 2025-03-28T08:42:08Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

+            not t = immediateBase.getATypeParameter() and
+            baseTypeMentionHasTypeAt(immediateBase, baseMention, path, t)


This pattern (and the dual) appears twice, so I think it perhaps still makes sense to have the predicates baseTypeMentionHasNonTypeParameterAt and baseTypeMentionHasTypeParameterAt, which then simply call baseTypeMentionHasTypeAt and add appropriate restrictions.

hvitved · 2025-03-28T08:45:50Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

+       * For this example
+       * ```csharp
+       * interface IFoo<A> { }
+       * void M<T1, T2>(T2 item) where T2 : IFoo<T1> { }


Perhaps change the return type from void to T1, to make it an example where type inference would actually be relevant.

hvitved · 2025-03-28T08:47:16Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

+       * - `path1 = ""`,
+       * - `tp1 = T2`,
+       * - `constraint = IFoo`,
+       * - `path2 = "A"`,


, -> , and

hvitved · 2025-03-28T08:47:26Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

+       * - `tp1 = T2`,
+       * - `constraint = IFoo`,
+       * - `path2 = "A"`,
+       * - `tp2 = T1`


Add a final .

hvitved · 2025-03-28T08:53:50Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

+         * type of `a` at position `apos` at path `pathToSub`, and `t` is
+         * mentioned (implicitly) at `path` inside `base`. For example, in
         *
         * ```csharp


The example should mention that pathToSub = "". Perhaps it would also be worth adding an example where pathToSub != ""?

For now I've just mentioned pathToSub = "". I think adding another example is a good idea. Now that relevantAccess is added perhaps the existing example needs revamping as well? Now the predicate only finds the supertypes when necessary, and ToString probably doesn't need to do that?

hvitved · 2025-03-28T12:32:09Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

        )
      }
+
+      /** Similar to `baseTypeMentionHasTypeAt` but FIXME: */


Should be fixed ;-)

Fine to simply the remove the QL doc.

hvitved · 2025-03-28T12:32:22Z

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

        /**
         * Holds if inferring types at `a` might depend on the type at `path` of
-         * `apos` having `baseMention` as a transitive base type mention.
+         * `apos` having `base` as a transitive base type mention.


remove mention.

paldepind added the Rust Pull requests that update Rust code label Mar 24, 2025

paldepind marked this pull request as ready for review March 24, 2025 14:08

Copilot AI review requested due to automatic review settings March 24, 2025 14:08

Copilot AI reviewed Mar 24, 2025

View reviewed changes

paldepind added the no-change-note-required This PR does not need a change note label Mar 24, 2025

paldepind requested a review from hvitved March 24, 2025 14:44

paldepind removed the request for review from hvitved March 24, 2025 17:16

github-actions bot removed the Rust Pull requests that update Rust code label Mar 24, 2025

Shared: Refactor type inference

ba9edf8

paldepind force-pushed the rust-ti-refactor branch from c6ad485 to ba9edf8 Compare March 25, 2025 05:33

paldepind added the Rust Pull requests that update Rust code label Mar 25, 2025

paldepind added 2 commits March 25, 2025 08:41

Rust: Expand on type parameter bounds type inference test

831413b

Shared: Infer types for type parameters with contraints

06c8963

paldepind changed the title ~~Shared: Refactor type inference~~ Shared, type inference: Add inference for type parameters with constraints (base type mentions) Mar 25, 2025

github-advanced-security bot found potential problems Mar 25, 2025

View reviewed changes

shared/typeinference/codeql/typeinference/internal/TypeInference.qll Fixed Show fixed Hide fixed

paldepind added 3 commits March 25, 2025 14:26

Rust: Fix a bad join by adding bindingset to resolveTypeMentionRoot

54e7bb7

Merge branch 'main' into rust-ti-refactor

efcffc5

Shared: Fix typos in qldoc

520e27c

paldepind requested a review from hvitved March 25, 2025 15:16

hvitved requested changes Mar 28, 2025

View reviewed changes

Rust: Address PR comments

623bc23

hvitved requested changes Mar 28, 2025

View reviewed changes

Rust: Minor doc tweaks based on PR comments

989c144

hvitved approved these changes Mar 28, 2025

View reviewed changes

paldepind merged commit 884c828 into github:main Mar 28, 2025
36 of 37 checks passed

paldepind deleted the rust-ti-refactor branch March 28, 2025 13:02

		not t = immediateBase.getATypeParameter() and
		baseTypeMentionHasTypeAt(immediateBase, baseMention, path, t)

Conversation

paldepind commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

paldepind commented Mar 24, 2025

Uh oh!

Uh oh!

paldepind commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

paldepind commented Mar 24, 2025 •

edited

Loading

paldepind commented Mar 25, 2025 •

edited

Loading