-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add impl From<&CompactDecimal> for PluralOperands #4828
Conversation
Oof, the resolver was being too smart before, and now it's biting us. This is in error[E0283]: type annotations needed
--> src/main.rs:16:24
|
16 | let result = rules.category_for(&3.into());
| ^^^^^^^^^^^^ ---- type must be known at this point
| |
| cannot infer type of the type parameter `I` declared on the method `category_for`
|
= note: cannot satisfy `_: From<i32>`
= note: required for `i32` to implement `Into<_>` I thought adding impls was supposed to always be semver safe. |
Adding @Manishearth as reviewer because of the resolver issue |
No, it's actually expected: In general Rust does not consider "you will need to disambiguate" as a breaking change because otherwise you would never be able to add anything to your API. Adding |
ok so does this mean we can land this without a semver bump? |
Yes |
However we should use our judgement on how commonly we expect this breakage to cause actual problems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yet Another Reason™ to not use (CC @zbraniecki @robertbastian who like to do this sometimes) |
It's specifically the chaining of two Intos that's the problem here, I think. Ideally an API should never require that: there should be Into impls for integers as well. It's rarely a real problem imo, and Into accepting functions are fine as long as you never need to feed them the output of a second into. Which you shouldn't need to. |
Looking at the code, I'm surprised that the resolver looks at |
Yes, sometimes, it doesn't always work, I usually don't rely on it except in specific contexts. |
Well don't call functions accepting |
Yeah like I said going through Into twice is known brittle and nobody really does it,you should have a single bridging Into impl if possible |
We all agree that clients shouldn't do this. My separate position is that functions should prefer accepting concrete types over things like I guess the question for us at the moment is, given that we have had this docs example for quite some time, do we care that it breaks in 1.5? |
IMO no, but I think we should invest in adding bridging impls. |
Discussion: the call site is fragile and should be re-written anyway; it is only in a tutorial, not a docs test, which lowers the risk. This should be a Clippy lint. So this PR can proceed. LGTM: @Manishearth @robertbastian @sffc |
We already have |
@robertbastian If you LGTM this PR, including the tutorial change, I will merge it. |
Waiting on review from @eggrobin |
(cherry picked from commit 782a01a)
CC @sven-oly