Never consider int and float vars for FnPtr candidates#109896
Conversation
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.
|
Thanks @Nilstrieb. r? @compiler-errors @bors r+ rollup |
There was a problem hiding this comment.
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 , but the change makes sense to me.FnPtr that landed
EDIT: heh, I raced with @compiler-errors's r+, so it all worked out nicely.
| ty::Infer(ty::InferTy::TyVar(_) | ty::InferTy::FreshTy(_)) => { | ||
| candidates.ambiguous = true; |
There was a problem hiding this comment.
This is the only case I can think of that could still cause issues, I just hope this was already always ambiguous anyway 🤞.
There was a problem hiding this comment.
I tried to call a method on a ty infer var but that caused errors in typeck about not being able to do that
There was a problem hiding this comment.
_: Trait is always ambiguous, and similarly treating fresh vars here is also fine here, since we'll only get those from freshening ty vars.
There was a problem hiding this comment.
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.
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
This solves a regression where
0.0.cmp()was ambiguous when a custom trait with acmpmethod 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