Skip to content

Rust: fix missing canonical paths for trait impls on builtin numeric types #20001

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 2 commits into
base: main
Choose a base branch
from

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Jul 8, 2025

@hvitved This is probably the wrong way to fix this.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jul 8, 2025
@aibaars aibaars force-pushed the aibaars/trait-impl-int branch from 588c791 to d64a6cd Compare July 9, 2025 08:36
Comment on lines 598 to 602
(
c1 = c2 or
c2 = c1.getADependency() or
c1 = c2.getADependency()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Parens can be removed if you replace the preceding and with |.

Copy link
Contributor Author

@aibaars aibaars Jul 9, 2025

Choose a reason for hiding this comment

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

I think I found a fix that matches better with the original intent of the code. Hopefully the tests agree as well.

@aibaars aibaars force-pushed the aibaars/trait-impl-int branch from d64a6cd to ffd107d Compare July 9, 2025 09:09
@aibaars aibaars force-pushed the aibaars/trait-impl-int branch from ffd107d to 0bf11da Compare July 9, 2025 09:11
@aibaars aibaars marked this pull request as ready for review July 9, 2025 09:23
@aibaars aibaars requested a review from a team as a code owner July 9, 2025 09:23
@aibaars aibaars added the no-change-note-required This PR does not need a change note label Jul 9, 2025
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

LGTM; let's see what DCA says.

@@ -595,7 +595,7 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl {
exists(TraitItemNode trait |
trait = this.resolveTraitTy() and
trait.hasCanonicalPath(c2) and
if this.hasCanonicalPath(c2)
if trait.hasCanonicalPath(c1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I figured out what happens; this needs to be reverted, and then we need to add c = this and before the line this.getName() = "core" and inside CrateItemNode.providesCanonicalPathPrefixFor.

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