Skip to content

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 25, 2025

#20179 follow-up: The not certainType.getATypeParameter() = tp check in certainTypeConflict does not work as intended for synthetic type parameters.

DCA is uneventful.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Aug 25, 2025
@hvitved hvitved force-pushed the rust/type-synth-type-param branch from 008231b to 9ef839d Compare August 25, 2025 11:13
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Aug 25, 2025
@hvitved hvitved marked this pull request as ready for review August 25, 2025 18:22
@hvitved hvitved requested a review from a team as a code owner August 25, 2025 18:22
@Copilot Copilot AI review requested due to automatic review settings August 25, 2025 18:22
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 modifies the type parameter handling in Rust to properly include synthetic type parameters (such as associated types in traits) in Type.getATypeParameter(). The changes address an issue where the certainTypeConflict check wasn't working correctly for synthetic type parameters.

  • Separates positional type parameters from all type parameters by introducing getPositionalTypeParameter()
  • Updates getATypeParameter() to include both positional and synthetic type parameters
  • Adds specific handling for trait and dynamic trait types to include their associated types

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
rust/ql/lib/codeql/rust/internal/Type.qll Refactors type parameter methods and adds synthetic type parameter support for trait types
rust/ql/lib/codeql/rust/internal/TypeMention.qll Updates call to use the new positional type parameter method

* This includes both positional type parameters and synthetic type parameters,
* such as associated types in traits.
*/
TypeParameter getATypeParameter() { result = this.getPositionalTypeParameter(_) }
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

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

The default implementation of getATypeParameter() only returns positional type parameters. This means that types like StructType, EnumType, etc. won't include their synthetic type parameters unless they override this method. Consider making this method abstract or providing a more comprehensive default implementation.

Suggested change
TypeParameter getATypeParameter() { result = this.getPositionalTypeParameter(_) }
abstract TypeParameter getATypeParameter();

Copilot uses AI. Check for mistakes.

@hvitved hvitved requested a review from paldepind August 26, 2025 12:12
Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

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

I think this is a great change!

The way I understand it, #19847 got rid of the annoying traitAliasIndex that assigned arbitrary indices to associated types for traits. But, it also meant that getTypeParameter on traits no longer gave the tp's for associated types.

With this PR we get the best of both worlds: A getATypeParameter that gives the full set of type parameters (without requiring one to invent positions where it doesn't make much sense) and a getPositionalTypeParameter that holds for the subset of the TPs that have a meaningful position.

@hvitved hvitved merged commit 3527fca into github:main Aug 26, 2025
21 checks passed
@hvitved hvitved deleted the rust/type-synth-type-param branch August 26, 2025 12:42
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