Rust: Make impl blocks only give rise to direct trait implementation by paldepind · Pull Request #20723 · github/codeql · GitHub

This PR changes how we calculate the trait/implements hierarchy.

Currently an impl Trait for T block makes T directly implement both Trait and any supertraits, SuperTrait, of Trait. However, in order for this impl block to be valid, it must already be the case that T implements SuperTrait by some other impl block. Hence, the only new information from an impl block is the implementation of the specific target trait.

This PR changes how we handle impl blocks when calculating the trait/implements hierarchy, s.t. impl blocks only makes a type implement the specific trait in the impl block.

This restriction is also used in AssocFunctionType where the restriction removes some spurious call targets.

The DCA report shows a small speedup and decent reductions in "Path resolution inconsistencies" and "Nodes With Type At Length Limit". There is a small increase in "Unknown expression types". I think this increase looks reasonable when compared to the decrease in "Path resolution inconsistencies". I also did a quick and small spot check on neon, and the lost types all looked like spurious types.

Pull Request Overview

This PR enhances the type inference module by adding a transitive parameter to control whether constraints should be transitively propagated through trait bounds. The key change distinguishes between impl blocks (non-transitive) and trait bounds (transitive), fixing spurious type inference results.

Added a boolean transitive parameter to conditionSatisfiesConstraint predicate to control constraint transitivity
Updated associated function type resolution to properly track parent traits and constraints
Corrected documentation examples and fixed function name references in comments

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
shared/typeinference/codeql/typeinference/internal/TypeInference.qll	Added transitive parameter to conditionSatisfiesConstraint predicate signature and updated all call sites
rust/ql/lib/codeql/rust/internal/TypeInference.qll	Set transitive = false for impl blocks and transitive = true for trait bounds, supertraits, and related constraints
rust/ql/lib/codeql/rust/internal/typeinference/FunctionType.qll	Refactored associated function type resolution to track parent traits, updated parameter binding sets, and corrected documentation table
rust/ql/test/library-tests/type-inference/main.rs	Removed SPURIOUS markers from test expectations that are now correctly resolved
rust/ql/test/library-tests/type-inference/type-inference.expected	Removed spurious reference type inferences for loop variables
rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected	Removed entries for calls that no longer have multiple targets
rust/ql/test/library-tests/dataflow/sources/stdin/CONSISTENCY/PathResolutionConsistency.expected	Removed spurious multiple call target entry
rust/ql/test/library-tests/dataflow/models/CONSISTENCY/PathResolutionConsistency.expected	Removed spurious multiple call target entry

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Corrected spelling of 'wether' to 'whether' and 'satisifed' to 'satisfied'.

Suggested change

      
                 * - `transitive` corresponds to wether any further constraints satisifed
          
                 * - `transitive` corresponds to whether any further constraints satisfied

The documentation table references functions m4 and m5 that don't exist in the example code. Based on the example, line 153 should reference m1 (from the impl T1 for X block) and line 154 should reference m3 (from the impl T2 for X block).

Suggested change

      
             * `m4` | `impl T2 for X` | `self` | `X`
          
             * `m5` | `impl T2 for X` | `self` | `X`
          
             * `m1` | `impl T1 for X` | `self` | `X`
          
             * `m3` | `impl T2 for X` | `self` | `X`

I'm not sure why the second test did not get fixed, especially since it looks a lot like the fourth test. I'm guessing something more might be needed here, as the AssocFunctionType looked fine when I eval'ed it.

I think I have something to address this follow-up.

Changes on DCA and existing tests LGTM.

Though this calls the version of m in impl MyTrait1 for S4, if that did not override m I believe Rust would in fact fall back to the implementation in MyTrait1. It's also possible that a function in a direct Impl S4 could be called here. I'm seeking reassurance that we will get the right call targets in expanded cases such as these:

trait MyTrait1 {
    // MyTrait1::m
    fn m(&self) {}

    // MyTrait1::n
    fn n(&self) {}

    // MyTrait1::o
    fn o(&self) {}
}

#[derive(Debug)]
struct S4;

impl MyTrait1 for S4 {
    // <S4_as_MyTrait1>::m
    fn m(&self) {}

    // <S4_as_MyTrait1>::n
    fn n(&self) {}
}

impl S4 {
    // S4::n
    fn n(&self) {}
}

S4.m(); // $ target=<S4_as_MyTrait1>::m
S4.n(); // $ target=S4::n
S4.o(); // $ target=MyTrait1::o

LGTM, just one nit comment.

I think I have something to address this follow-up.

nit: apply

Merge conflicts in this file.

github-actions Bot added the Rust Pull requests that update Rust code label Oct 30, 2025

paldepind force-pushed the rust/ti-inheritance branch from 6b38220 to 978e42e Compare October 31, 2025 14:23

paldepind marked this pull request as ready for review November 3, 2025 08:56

paldepind requested review from a team as code owners November 3, 2025 08:56

Copilot AI review requested due to automatic review settings November 3, 2025 08:56

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

paldepind assigned hvitved Nov 3, 2025

Copilot AI reviewed Nov 3, 2025

View reviewed changes

paldepind commented Nov 3, 2025

View reviewed changes

paldepind force-pushed the rust/ti-inheritance branch from fad7b5e to 9a3892c Compare November 3, 2025 09:23

geoffw0 reviewed Nov 10, 2025

View reviewed changes

hvitved reviewed Nov 12, 2025

View reviewed changes

Rust: Make impl blocks only give rise to direct trait implementation

089bfff

paldepind force-pushed the rust/ti-inheritance branch from 9a3892c to f8e5a1e Compare November 17, 2025 09:30

hvitved requested changes Nov 17, 2025

View reviewed changes

hvitved mentioned this pull request Nov 17, 2025

Rust: Make impl blocks only give rise to direct trait implementation #20847

Closed

paldepind added 3 commits November 17, 2025 12:39

Rust: Accept changes to expected files

a07f015

Rust: Improvements to docs from review comments

9971936

Rust: Fix grammar

4eb22a7

paldepind force-pushed the rust/ti-inheritance branch from f8e5a1e to 4eb22a7 Compare November 17, 2025 11:39

hvitved approved these changes Nov 17, 2025

View reviewed changes

hvitved merged commit b0dc48e into github:main Nov 17, 2025
42 checks passed

hvitved mentioned this pull request Nov 19, 2025

Rust: Refine Self resolution inside impl blocks #20853

Merged

                    *   free in `condition` and `constraint`,
                    * - and for every instantiation of the type parameters from `abs` the
                    *   resulting `condition` satisifies the constraint given by `constraint`.
+                   * - `transitive` corresponds to wether any further constraints satisifed

-                      S5(0i32).m(); // $ target=<S5<i32>_as_MyTrait1>::m $ SPURIOUS: target=MyTrait1::m
-                      S5::m(&S5(0i32)); // $ target=<S5<i32>_as_MyTrait1>::m $ SPURIOUS: target=MyTrait1::m
+                      S5(0i32).m(); // $ target=<S5<i32>_as_MyTrait1>::m
+                      S5::m(&S5(0i32)); // $ target=<S5<i32>_as_MyTrait1>::m

-                      S5(0i32).m(); // $ target=<S5<i32>_as_MyTrait1>::m $ SPURIOUS: target=MyTrait1::m
-                      S5::m(&S5(0i32)); // $ target=<S5<i32>_as_MyTrait1>::m $ SPURIOUS: target=MyTrait1::m
+                      S5(0i32).m(); // $ target=<S5<i32>_as_MyTrait1>::m
+                      S5::m(&S5(0i32)); // $ target=<S5<i32>_as_MyTrait1>::m

-                   *   resulting `condition` satisifies the constraint given by `constraint`.
+                   *   resulting `condition` satisfies the constraint given by `constraint`.
+                   * - `transitive` corresponds to whether any further constraints satisfied
+                   *   through `constraint` also applies to `condition`.

               | main.rs:2561:13:2561:31 | ...::from(...) |
               | main.rs:2562:13:2562:31 | ...::from(...) |
               | main.rs:2563:13:2563:31 | ...::from(...) |
+              =======

Rust: Make impl blocks only give rise to direct trait implementation by paldepind · Pull Request #20723 · github/codeql · GitHub

Uh oh!

Rust: Make impl blocks only give rise to direct trait implementation - #20723

Rust: Make impl blocks only give rise to direct trait implementation#20723
hvitved merged 4 commits into
github:mainfrom
paldepind:rust/ti-inheritance

paldepind commented Oct 30, 2025 •
edited
Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 3, 2025

Uh oh!

Copilot AI Nov 3, 2025

Uh oh!

paldepind Nov 3, 2025

Uh oh!

hvitved Nov 12, 2025

Uh oh!

geoffw0 left a comment

Uh oh!

geoffw0 Nov 10, 2025

Uh oh!

hvitved left a comment

Uh oh!

hvitved Nov 12, 2025

Uh oh!

hvitved Nov 12, 2025

Uh oh!

hvitved Nov 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	* - `transitive` corresponds to wether any further constraints satisifed
	* - `transitive` corresponds to whether any further constraints satisfied

		* `m4` \| `impl T2 for X` \| `self` \| `X`
		* `m5` \| `impl T2 for X` \| `self` \| `X`

                       println!("{:?}", S3::m(&w, x)); // $ target=S3<T>::m
-                      S4.m(); // $ target=<S4_as_MyTrait1>::m $ SPURIOUS: target=MyTrait1::m
+                      S4.m(); // $ target=<S4_as_MyTrait1>::m

Rust: Make impl blocks only give rise to direct trait implementation by paldepind · Pull Request #20723 · github/codeql · GitHub

Uh oh!

Rust: Make impl blocks only give rise to direct trait implementation - #20723

Rust: Make impl blocks only give rise to direct trait implementation#20723hvitved merged 4 commits intogithub:maingithub/codeql:mainfrom paldepind:rust/ti-inheritancepaldepind/codeql:rust/ti-inheritanceCopy head branch name to clipboard

Conversation

paldepind commented Oct 30, 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

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

paldepind Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

geoffw0 Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

hvitved Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Nov 12, 2025

Choose a reason for hiding this comment

Uh oh!

hvitved Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rust: Make impl blocks only give rise to direct trait implementation#20723
hvitved merged 4 commits into
github:mainfrom
paldepind:rust/ti-inheritance

paldepind commented Oct 30, 2025 •
edited
Loading