Skip to content

Rust: Fix type inference for trait objects for traits with associated types #20122

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paldepind
Copy link
Contributor

This PR adds tests that shows two problems:

  • For a subtrait of a supertrait with associated types, we don't handle the associated type for correctly for the subtrait.
  • For a dyn Trait type we don't handle any associated types that Trait might have.

I stumbled into both of these issues while trying to implement type inference for closures since the traits for functions (Fn, FnOnce, FnMut) use associated types.

This PR also fixes the second issue which is done by letting dyn Trait have matching type parameters for the associated types of Trait similar to what is done for the normal type parameters.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jul 24, 2025
@paldepind paldepind force-pushed the rust/type-inference-dyn-assoc branch from 2348657 to 466bf85 Compare July 24, 2025 14:08
@paldepind paldepind marked this pull request as ready for review July 24, 2025 15:57
@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 15:57
@paldepind paldepind requested a review from a team as a code owner July 24, 2025 15:57
Copy link
Contributor

@Copilot 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 fixes type inference for trait objects (dyn Trait) when traits have associated types, specifically addressing two issues: handling associated types for subtraits of supertraits with associated types, and properly handling associated types in dyn Trait types. The fix involves extending the type parameter system to include both generic type parameters and associated type parameters for trait objects.

  • Refactors the DynTraitTypeParameter class to handle both TypeParam and TypeAlias nodes
  • Adds comprehensive test coverage for associated types in trait objects and supertrait scenarios
  • Updates type mention resolution logic to properly handle the new type parameter structure

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rust/ql/test/library-tests/type-inference/main.rs Adds test module for supertrait associated types and renames existing module
rust/ql/test/library-tests/type-inference/dyn_type.rs Adds comprehensive tests for trait objects with associated types
rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected Updates line numbers in consistency test expectations
rust/ql/lib/codeql/rust/internal/TypeMention.qll Updates type resolution logic for dyn trait type parameters
rust/ql/lib/codeql/rust/internal/TypeInference.qll Extends type parameter ID generation to handle associated types
rust/ql/lib/codeql/rust/internal/Type.qll Major refactor of DynTraitTypeParameter to support both generic and associated type parameters
Comments suppressed due to low confidence (1)

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

  • [nitpick] The method name toStringInner is ambiguous. Consider renaming it to getNodeDisplayName or formatNodeName to better indicate its purpose of formatting the underlying node for display.
  private string toStringInner() {

@paldepind paldepind added the no-change-note-required This PR does not need a change note label Jul 24, 2025
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Looks good, couple of minor suggestions.

id = idOfTypeParameterAstNode(tp0.(DynTraitTypeParameter).getTypeParam())
id =
idOfTypeParameterAstNode([
tp0.(DynTraitTypeParameter).getTypeParam().(AstNode),
Copy link
Contributor

Choose a reason for hiding this comment

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

.(AstNode) appears to be redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to help QL figure out what the element type of the set is. It complains without the .(AstNode) :)

Copy link
Contributor

Choose a reason for hiding this comment

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

That's surprising, but fair enough.

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