Rust: Support non-universal `impl` blocks #19372

paldepind · 2025-04-24T14:48:57Z

Adds type inference support for non-universal impl blocks. By "non-universal" we mean impl blocks that target generic types but which are not valid for all instantiations of the generic type.

For instance

impl<T> Option<Option<T>> {
  fn flatten(self) -> Option<T> { ... }
}

where the flatten method is only valid for some Option instantiations.

Non-universal impl block affect both method resolution and trait implementations as the method/trait implementation can be valid only for some instantiations of a type. A Foo<i64> might have a different set of methods/traits than a Foo<String>. Finding the right method/trait implementation is the crux of the matter. The tests have examples of this.

I've tried to document the new additions in this PR with QLdoc, but here are some additional high-level comments on the changes:

As mentioned above, with non-universal impl blocks it is no longer enough to know the root of a type to determine which traits it implements and which methods it supports. This affects a bunch of things.
- The getMethod and getABaseTypeMention member predicate on Type is removed, as a Type is now not enough to determine these things.
- The new conditionSatisfiesConstraint takes a TypeMention as its second parameter as a Type is not enough.
In this PR I've split subtype handling (inferring type parameters through supertypes) from constraint handling (inferring type parameters from type parameter interface constraints (in C#)/trait bounds (in Rust). The former is now the sole job of the AccessBaseType sub-module and the later is done in the new AccessConstraint sub-module. There's also a predicate for each in the module signature: getABaseTypeMention and conditionSatisfiesConstraint.

This has both pros and cons:
- PRO: Languages (like Rust) that don't have subtyping can leave getABaseTypeMention as none() and avoid any computation related to subtyping.
- PRO: Constraints are now more complicated and I don't think any languages have subtyping that is similarly complicated. So subtyping is supported in a simpler and potentially more efficient way.
- CON: It leads to more code within the shared library compared to handling these two things uniformly as before.
- CON: In languages where these things are identical, i.e., C# where the question "does T implement the interface I" is equal to asking "is T a subtype of I" doing it as one thing could be more performant.
All in all I'm not sure which approach is best, but when I made this change performance improved quite a bit (but that was before making some other optimizations) so that's why I ended up with this. Another approach (in follow up work) could be to 1/ merge the two things back again, 2/ add a getVariance predicate for access positions than can be covariant or invariant, 3/ only do subtyping for covariant positions, 4/ make all positions invariant for Rust. That should give equal performance for Rust, cut down on the duplicated code and hopefully the optimization with countConstraintImplementations would mean that subtyping for languages like C# wouldn't regress in performance (but we'd have no way to measure that).

rust/ql/lib/codeql/rust/internal/Type.qll

rust/ql/lib/codeql/rust/internal/TypeInference.qll

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

…rait implementation

geoffw0

Results on Rust LGTM - fewer consistency check failures, DCA looks good, except there appears to be an analysis time slowdown. It might be worth checking what's causing that, whether it affects other languages, and whether it's easy to fix.

geoffw0 · 2025-05-01T11:11:21Z

rust/ql/lib/codeql/rust/elements/internal/MethodCallExprImpl.qll

+      result = this.getStaticTargetFrom(true)
+      or
+      not exists(this.getStaticTargetFrom(true)) and
+      result = this.getStaticTargetFrom(false)


Why do we prioritize results with fromSource above everything else?

That's because the extractor currently duplicates function in source: once as a function from the source code and once as a function from library code. Hence we favor those in the source as the non-source one is probably a duplicate of the same thing.

I've added a comment now to make this clear.

geoffw0 · 2025-05-01T11:13:32Z

rust/ql/lib/codeql/rust/internal/Type.qll

+  override Location getLocation() { result = trait.getLocation() }
+}
+
+/** A type abstraction. */


I'd appreciate a bit more QLDoc here. What is a "type abstraction" in this context? (assuming it's not a Rust term)

A tried to explain this over in the shared module. Would it make sense to repeat the Rust specific part of that here?

Ah, that looks good, but a little difficult to find for someone who is here. I think a reference to it (as in, "see TypeAbstraction in ...") would be sufficient.

geoffw0 · 2025-05-01T11:16:01Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

+    result = tp.(SelfTypeParameter).getTrait()
+  }
+
+  predicate conditionSatisfiesConstraint(


This could also do with QLDoc, in particular explaining the condition and constraint parameters.

I've added QLDoc for this and TypeAbstraction as mentioned above.

hvitved

First batch of review comments, mostly concerning performance.

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

hvitved · 2025-05-01T17:31:10Z

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

+     * Note that the type parameters in `abs` significantly change the meaning
+     * of type parameters that occur in `condition`. For instance, in the Rust
+     * example
+     * ```rust
+     * fn foo<T: Trait>() { }
+     * ```
+     * we have that the type parameter `T` satisfies the constraint `Trait`. But,
+     * only that specific `T` satisfy the constraint. Hence we would not have
+     * `T` in `abs`. On the other hand, in the Rust example
+     * ```rust
+     * impl<T> Trait for T { }
+     * ```
+     * the constraint `Trait` is in fact satisfied for all types, and we would
+     * have `T` in `abs` to make it free in the condition.
+     */


This bit is hard to follow.

I've tried to rework this text. Hopefully it's a bit clearer now. I wanted to highlight this example, as it's the reason why I had to add abs in the first place.

hvitved · 2025-05-01T17:33:56Z

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

+       * Holds if `abs` is a type abstraction under which `tm` occurs and if
+       * `app` is potentially the result of applying the abstraction to type
+       * some type argument.


Can you elaborate what under which `tm` occurs means?

This just mean that the type parameters introduced in abs are in scope in tm, that is, tm is "under" the type abstraction. I've added an example and a bit more text to make this clearer.

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

+       * `potentialInstantiationOf`. Defaults to simply projecting the third
+       * argument of `potentialInstantiationOf`.
+       */
+      default predicate relevantTypeMention(TypeMention tm) { potentialInstantiationOf(_, _, tm) }


paldepind · 2025-05-12T09:47:54Z

I think the PR is now ready for another look :)

DCA shows a big slowdown on rust but I can't reproduce anything locally. For me both getStaticTarget, resolveMethodCallExpr, and inferType quick eval at basically the same time as on main. I can try an investigate some more, but perhaps someone know of a good way to see what might be up?

This fixes a test failure where duplicated functions from extraction caused a bunch of spurious results to pop up

hvitved · 2025-05-14T09:31:50Z

DCA shows a big slowdown on rust but I can't reproduce anything locally.

I can reproduce; will work on a fix.

paldepind · 2025-05-14T09:46:59Z

I can reproduce; will work on a fix.

Great that you can reproduce and thanks for looking into it. What did you do to reproduce?

hvitved · 2025-05-14T09:49:49Z

What did you do to reproduce?

I ran DataFlowConsistencyCounts.ql, which was reported in the stage stats report by DCA.

hvitved · 2025-05-14T18:39:49Z

It only now occurred to me that it was not my proposed changes that resulted in the slowdown on rust; it actually improved things significantly, as the analysis timed out before those changes 😌

hvitved

Very impressive work. It would be nice if we can, as you hint at in the description, simplify the code via generalization, but let's postpone that as follow-up work.

hvitved · 2025-05-15T12:29:40Z

rust/ql/test/query-tests/security/CWE-089/CONSISTENCY/PathResolutionConsistency.expected

@@ -1,13 +0,0 @@
-multipleMethodCallTargets


This file can now be deleted.

hvitved · 2025-05-15T12:32:33Z

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

@@ -212,6 +237,17 @@ module Make1<LocationSig Location, InputSig1<Location> Input1> {
    TypePath cons(TypeParameter tp, TypePath suffix) { result = singleton(tp).append(suffix) }
  }

+  /** A class that represents a type tree. */
+  private signature class TypeTreeSig {


Used in the exposed IsInstantiationOf module, so should be public.

I wonder if we could choose a better name; perhaps Resolvable or similar? The three instantiations are TypeMention, RelevantAccess, and ReceiverExpr, neither of which are types themselves, but which can resolve to types.

hvitved · 2025-05-15T12:45:34Z

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

+     * Holds if
+     * - `abs` is a type abstraction that introduces type variables that are
+     *   free in `condition` and `constraint`,
+     * - and for every instantiation of the type parameters the resulting


Perhaps make it explicit: of the type parameters from `abs`

hvitved · 2025-05-15T12:45:47Z

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

+     *
+     * Example in Rust:
+     * ```rust
+     * impl<A> Trait<i64, B> for Type<String, A> { }


Should the B be A?

hvitved · 2025-05-15T12:46:30Z

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

@@ -265,8 +362,272 @@ module Make1<LocationSig Location, InputSig1<Location> Input1> {
      result = tm.resolveTypeAt(TypePath::nil())
    }

+    signature module IsInstantiationOfInputSig<TypeTreeSig App> {


QL doc, simply say that it provides the input to IsInstantiationOf.

hvitved · 2025-05-16T11:22:14Z

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

              path = prefix.append(suffix)
            )
          )
        }
      }

+      private module AccessConstraint {
+        private newtype TTRelevantAccess =
+          TRelevantAccess(Access a, AccessPosition apos, TypePath path, Type constraint) {


It feels wrong that constraint is part of an access; I tried removing it locally, and that seemed to work (it requires exposing Type constraint in the predicate hasConstraintMention.

hvitved · 2025-05-16T12:25:50Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

+   * //  ^^^^^^ a type abstraction
+   * ```
+   */
+  class TypeAbstraction = T::TypeAbstraction;


I would move this QL doc in to the class being aliased.

hvitved · 2025-05-16T12:30:04Z

rust/ql/lib/codeql/rust/internal/Type.qll

-    // The `Self` type parameter is an implementation of the trait, so it has
-    // all the trait's methods.
-    result = trait.(ItemNode).getASuccessor(name)
+final class ImplTypeAbstraction extends TypeAbstraction, Impl {


Would it make sense to collect ImplTypeAbstraction and TraitTypeAbstraction as simply GenericParamList? That way it would also better match the ^^^^ markers in the comments.

hvitved · 2025-05-16T12:36:48Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

+  pragma[nomagic]
+  private Type receiverRootType(Expr e) {
+    any(MethodCallExpr mce).getReceiver() = e and
+    result = inferType(e)
+  }
+
+  pragma[nomagic]
+  private Type inferReceiverType(Expr e, TypePath path) {
+    exists(Type root | root = receiverRootType(e) |
+      // for reference types, lookup members in the type being referenced
+      if root = TRefType()
+      then result = inferType(e, TypePath::cons(TRefTypeParameter(), path))
+      else result = inferType(e, path)


I think these two can be replaced by

pragma[nomagic] private Type inferReceiverType(Expr e, TypePath path) { exists(TypePath path0 | any(MethodCallExpr mce).getReceiver() = e and result = inferType(e, path0) | path0 = TypePath::cons(TRefTypeParameter(), path) or not path0.isCons(TRefTypeParameter(), _) and path = path0 ) }

which is better, because it does not involve non-linear recursion.

And the predicate logic may just as well be moved directly into resolveTypeAt below (still with nomagic).

hvitved · 2025-05-16T12:47:36Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

+  pragma[inline]
+  private Type inferRootTypeDeref(AstNode n) {
+    exists(Type t |
+      t = inferType(n) and
+      // for reference types, lookup members in the type being referenced
+      if t = TRefType()
+      then result = inferType(n, TypePath::singleton(TRefTypeParameter()))
+      else result = t
+    )


Again, better to avoid non-linear recursion when possible:

pragma[inline] private Type inferRootTypeDeref(AstNode n) { result = inferType(n) and result != TRefType() or // for reference types, lookup members in the type being referenced result = inferType(n, TypePath::singleton(TRefTypeParameter())) }

github-actions bot added the Rust Pull requests that update Rust code label Apr 24, 2025

github-advanced-security bot found potential problems Apr 24, 2025

View reviewed changes

github-advanced-security bot found potential problems Apr 28, 2025

View reviewed changes

paldepind force-pushed the rust-ti-implementing-type-method branch from c66a71a to a9afbb3 Compare April 28, 2025 13:15

github-advanced-security bot found potential problems Apr 28, 2025

View reviewed changes

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

paldepind force-pushed the rust-ti-implementing-type-method branch 2 times, most recently from 76baa34 to 02115f6 Compare April 29, 2025 09:18

paldepind mentioned this pull request Apr 29, 2025

Rust: Model impl shadowing #19392

Closed

paldepind marked this pull request as ready for review April 29, 2025 12:54

paldepind requested a review from a team as a code owner April 29, 2025 12:54

paldepind requested a review from hvitved April 29, 2025 12:54

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

paldepind added 5 commits May 1, 2025 11:58

Rust: Add type inference test for non-universal impl blocks

22407ca

Rust: Implement type inference support for non-universal impl blocks

e45b5c5

Rust: Add type inference test for inherent implementation shadowing t…

4513106

…rait implementation

Rust: Handle inherent implementations shadowing trait implementations

3ee8989

Rust: Workaround for method existing both as source and as dependency

ecead2c

paldepind force-pushed the rust-ti-implementing-type-method branch from 439b202 to 68860b1 Compare May 1, 2025 10:00

Rust: Accept test changes

a545361

paldepind force-pushed the rust-ti-implementing-type-method branch from 68860b1 to a545361 Compare May 1, 2025 10:36

geoffw0 reviewed May 1, 2025

View reviewed changes

hvitved reviewed May 1, 2025

View reviewed changes

paldepind added 2 commits May 5, 2025 15:21

Rust: Address format fixes suggested in review

06cfa9a

Rust: Address comments on documentation

0cf60c4

paldepind force-pushed the rust-ti-implementing-type-method branch 2 times, most recently from d55edd7 to 0cf60c4 Compare May 12, 2025 09:12

Merge branch 'main' into rust-ti-implementing-type-method

bc85801

github-advanced-security bot found potential problems May 12, 2025

View reviewed changes

Rust: Use getStaticTarget in type inference test

7bd1612

This fixes a test failure where duplicated functions from extraction caused a bunch of spurious results to pop up

hvitved requested changes May 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rust: Support non-universal `impl` blocks #19372

Rust: Support non-universal `impl` blocks #19372

paldepind commented Apr 24, 2025 •

edited

Loading

geoffw0 left a comment

geoffw0 May 1, 2025

paldepind May 12, 2025

geoffw0 May 1, 2025

paldepind May 6, 2025

geoffw0 May 7, 2025

geoffw0 May 1, 2025

paldepind May 12, 2025

hvitved left a comment

hvitved May 1, 2025

paldepind May 12, 2025

hvitved May 1, 2025

paldepind May 12, 2025

Check warning

paldepind commented May 12, 2025

hvitved commented May 14, 2025

paldepind commented May 14, 2025

hvitved commented May 14, 2025

hvitved commented May 14, 2025

hvitved left a comment

hvitved May 15, 2025

hvitved May 15, 2025

hvitved May 15, 2025

hvitved May 15, 2025

hvitved May 15, 2025

hvitved May 15, 2025

hvitved May 16, 2025

hvitved May 16, 2025

hvitved May 16, 2025

hvitved May 16, 2025

hvitved May 16, 2025

hvitved May 16, 2025

Rust: Support non-universal impl blocks #19372

Are you sure you want to change the base?

Rust: Support non-universal impl blocks #19372

Conversation

paldepind commented Apr 24, 2025 • edited Loading

geoffw0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvitved left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Check warning

paldepind commented May 12, 2025

hvitved commented May 14, 2025

paldepind commented May 14, 2025

hvitved commented May 14, 2025

hvitved commented May 14, 2025

hvitved left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rust: Support non-universal `impl` blocks #19372

Rust: Support non-universal `impl` blocks #19372

paldepind commented Apr 24, 2025 •

edited

Loading