Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Apr 28, 2025

Removes inconsistencies, where a method call could previously dispatch to shadowed impl Trait methods.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Apr 28, 2025
@hvitved hvitved marked this pull request as ready for review April 28, 2025 11:59
Copilot AI review requested due to automatic review settings April 28, 2025 11:59
@hvitved hvitved requested a review from a team as a code owner April 28, 2025 11:59
@hvitved hvitved requested a review from paldepind April 28, 2025 11:59
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 introduces a new test module to verify that inherent methods correctly shadow trait methods in Rust. The key changes include:

  • Adding a new module "impl_overlap" with a structure S1.
  • Defining a trait MyTrait with an m1 method and an inherent method m1 on S1.
  • Including a test function that confirms the inherent method is invoked.
Files not reviewed (4)
  • rust/ql/lib/codeql/rust/internal/Type.qll: Language not supported
  • rust/ql/test/extractor-tests/canonical_path/CONSISTENCY/PathResolutionConsistency.expected: Language not supported
  • rust/ql/test/extractor-tests/canonical_path_disabled/CONSISTENCY/PathResolutionConsistency.expected: Language not supported
  • rust/ql/test/library-tests/type-inference/type-inference.expected: Language not supported

@github github deleted a comment from Copilot AI Apr 29, 2025
@paldepind
Copy link
Contributor

paldepind commented Apr 29, 2025

I was just running into this myself in #19372 since allowing non-universal/non-parametric impl blocks magnifies this issue.

However, #19372 completely changes how methods are resolved (the getMethod member predicate no longer exists for instance). So unless this fix is needed urgently I think it would make more sense to fix this in #19372 (and I think it's the only remaining issue in that PR)?

@paldepind
Copy link
Contributor

This is should now also be handled in #19372.

@hvitved
Copy link
Contributor Author

hvitved commented Apr 29, 2025

This is should now also be handled in #19372.

Excellent, I'll close this PR then.

@hvitved hvitved closed this Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants