Skip to content

Conversation

Noratrieb
Copy link
Member

This solves a regression where 0.0.cmp() was ambiguous when a custom trait with a cmp method was in scope.

For integers it shouldn't be a problem in practice so I wasn't able to add a test.

I'm not sure whether there could be more issues hidden in the shadows as mentioned in the issue, but this should at least fix the problematic regression immediately.

fixes #109892

r? oli-obk

This solves a regression where `0.0.cmp()` was ambiguous when a custom
trait with a `cmp` method was in scope.

FOr integers it shouldn't be a problem in practice so I wasn't able to
add a test.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 3, 2023
@compiler-errors
Copy link
Member

Thanks @Nilstrieb.

r? @compiler-errors @bors r+ rollup

@bors
Copy link
Collaborator

bors commented Apr 3, 2023

📌 Commit ca79b82 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2023
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this so quickly! I won't approve this myself because I'm not 100% sure I have all the background on the version of FnPtr that landed, but the change makes sense to me.

EDIT: heh, I raced with @compiler-errors's r+, so it all worked out nicely.

Comment on lines +1008 to 1009
ty::Infer(ty::InferTy::TyVar(_) | ty::InferTy::FreshTy(_)) => {
candidates.ambiguous = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is the only case I can think of that could still cause issues, I just hope this was already always ambiguous anyway 🤞.

Copy link
Member Author

@Noratrieb Noratrieb Apr 3, 2023

Choose a reason for hiding this comment

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

I tried to call a method on a ty infer var but that caused errors in typeck about not being able to do that

Copy link
Member

Choose a reason for hiding this comment

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

if obligation.predicate.skip_binder().self_ty().is_ty_var() {

_: Trait is always ambiguous, and similarly treating fresh vars here is also fine here, since we'll only get those from freshening ty vars.

Copy link
Member

@eddyb eddyb Apr 3, 2023

Choose a reason for hiding this comment

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

I tried to call a method on a ty infer var but that caused errors in typeck about not being able to do that

Yeah, that's what I ran into as well, when trying to reproduce it w/o floats.

AFAIK you can't get it to pick a trait based on method name alone, without something like Default::default().method() or <_>::method but those give up too early because of how ambiguous they are, only int/float inference vars go farther (because they represent an unresolved choice in a small finite set of types etc.).

The fuzzier ideas I've had would involve guiding inference, but _: FnPtr will never succeed early enough to taint inference, and blocking inference would likely require associated types etc. - and the moment you pin down a trait, FnPtr can only affect it through a blanket impl of your choice, which would also apply to all the existing types too (before FnPtr was added).

EDIT: @compiler-errors' reply while I was playing around with more ideas for potential triggers, kind of invalidates all of those silly ideas, so yeah don't mind me, heh.

bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#109783 (Update contributing links for rustc-dev-guide changes)
 - rust-lang#109883 (Add links to <cell.rs>)
 - rust-lang#109889 (Update book, rustc-dev-guide, rust-by-example)
 - rust-lang#109896 (Never consider int and float vars for `FnPtr` candidates)
 - rust-lang#109902 (Add async-await test for rust-lang#107414)
 - rust-lang#109903 (Add Chris Denton to `.mailmap`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7d3207b into rust-lang:master Apr 4, 2023
@rustbot rustbot added this to the 1.70.0 milestone Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FnPtr blanket impls cause "multiple applicable items in scope" for unrelated types.
6 participants