Skip to content
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

regression: int_roundings conflicts with existing APIs #88971

Closed
Mark-Simulacrum opened this issue Sep 15, 2021 · 9 comments
Closed

regression: int_roundings conflicts with existing APIs #88971

Mark-Simulacrum opened this issue Sep 15, 2021 · 9 comments
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Sep 15, 2021

Crater run for 1.56 detected a considerable amount of breakage due to int_roundings on nightly. We have some support in the compiler to avoid stable breakage due to unstable features, but it does not work for this case (@cuviper theorized this is due to "the self inherent method is resolved before the &self trait method is considered at all."

There are:

This is a level of breakage significant enough that it seems prudent to back this out or rename the functions, to avoid breaking a good portion of the ecosystem.

cc:

@Mark-Simulacrum Mark-Simulacrum added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Sep 15, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.56.0 milestone Sep 15, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 15, 2021
@cuviper
Copy link
Member

cuviper commented Sep 15, 2021

This may be a nitpick on what is defined as a root cause, but num-integer itself shouldn't be failing to compile as a dependency. The direct log errors you show are only in its tests, fixed in rust-num/num-integer#44 but not yet published. Other crate errors come from use of its Integer trait, so it is a root cause in that sense, but I can't fix that in num-integer.

@cuviper
Copy link
Member

cuviper commented Sep 15, 2021

I know it's not a blame game or anything, but I think the distinction matters when considering how many places have to be fixed to deal with this change.

@Mark-Simulacrum
Copy link
Member Author

Yeah, unfortunately, with this scope of breakage the number of root causes may not be terribly important -- many of those need somewhat manual intervention for the .lock bumps, in practice, which can take time.

@joshtriplett
Copy link
Member

These are unstable methods, so can we come up with reasonable names to rename them to, knowing that we don't have to commit to those names long-term if we come up with better ones before stabilization?

@m-ou-se m-ou-se added P-high High priority and removed I-nominated I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 22, 2021
@joshtriplett
Copy link
Member

Conclusion from the libs-api meeting: let's temporarily rename these to something non-conflicting that we know we don't want to use long-term; for instance, let's prefix them with unstable_.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 22, 2021

Discussed in the libs-api meeting. We'll rename them for now and beta-backport that.

@m-ou-se
Copy link
Member

m-ou-se commented Sep 22, 2021

Race condition.

@joshtriplett
Copy link
Member

Long-term fix:

  • We should fix the unstable-vs-stable resolution here: if the compiler finds an inherent method that's unstable and not opted into, and we're going to error out anyway, keep going and see if we find a stable method we can use.
  • We could potentially do autoderef here, since the type is Copy.

@joshtriplett
Copy link
Member

#89184

@m-ou-se m-ou-se closed this as completed Oct 6, 2021
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 19, 2021
Try all stable method candidates first before trying unstable ones

Currently we try methods in this order in each step:
* Stable by value
* Unstable by value
* Stable autoref
* Unstable autoref
* ...

This PR changes it to first try pick methods without any unstable candidates, and if none is found, try again to pick unstable ones.

Fix rust-lang#90320
CC rust-lang#88971, hopefully would allow us to rename the "unstable_*" methods for integer impls back.

`@rustbot` label T-compiler T-libs-api
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants