Skip to content

Conversation

@paldepind
Copy link
Contributor

@paldepind paldepind commented Oct 30, 2025

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.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Oct 30, 2025
@paldepind paldepind force-pushed the rust/ti-inheritance branch from 6b38220 to 978e42e Compare October 31, 2025 14:23
@paldepind paldepind marked this pull request as ready for review November 3, 2025 08:56
@paldepind 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 paldepind added the no-change-note-required This PR does not need a change note label Nov 3, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
Comment on lines 153 to 154
* `m4` | `impl T2 for X` | `self` | `X`
* `m5` | `impl T2 for X` | `self` | `X`
Copy link

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

Copilot uses AI. Check for mistakes.
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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@paldepind paldepind force-pushed the rust/ti-inheritance branch from fad7b5e to 9a3892c Compare November 3, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants