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: BITS constant conflicts in lexical crates #85667

Closed
Mark-Simulacrum opened this issue May 25, 2021 · 13 comments · Fixed by #86380
Closed

regression: BITS constant conflicts in lexical crates #85667

Mark-Simulacrum opened this issue May 25, 2021 · 13 comments · Fixed by #86380
Labels
P-critical Critical 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

u/iN::BITS was re-stabilized in 1.53 after being backed out of 1.51 due to breakage, primarily in the lexical-core crate. This breakage has not been fixed, as far as I can tell, in a vast majority of crates: we're still seeing ~1300 regressions, majority of which are lexical-core in the beta crater run.

There was no discussion on the re-stabilization PR #82565 (comment) as to whether this level of breakage is acceptable. Comments on the original breakage issue suggest that 0.7.5 was published with a fix, but older versions (0.4, 0.5, 0.6) did not receive similar treatment.

The majority of the breakage is in crates locked at 0.7.4; the other versions do share ~400 different crates amongst them, so I think it'd be good to try and help get those series patches as well.

@Mark-Simulacrum Mark-Simulacrum added 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. P-critical Critical priority labels May 25, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.53.0 milestone May 25, 2021
@Mark-Simulacrum
Copy link
Member Author

cc @m-ou-se @myrrlyn

Even with 0.7.5 out, it does look like it'll be some time before crates actually update lockfiles (if ever). We don't have a firm policy on whether cargo update being sufficient is "enough" that breakage is OK -- it's a new decision each time pretty much -- but this level of breakage is pretty bad. I guess there may not be much we can do -- IIRC a prelude trait for example adding BITS wouldn't help here, right? Those still introduce conflicts IIRC.

@myrrlyn
Copy link

myrrlyn commented May 25, 2021

There’s no way around the symbol collision. I think it’s important that ::BITS stabilize and I’d like us to get out of the way on that.

I can take the list of broken crates and start walking it. I can also work on a backport on the older minor series; the reason I did not do so last time is that I was unfamiliar with lexical-core’s dependents and didn’t know whether the older series were still in use.

What is the most complete course of action we can take to clear ::BITS for stabilization? Should I yank the conflicting versions in each minor series?
Alternatively, would it make sense, and is it even possible, to stabilize ::BITS only in e2021?

@Mark-Simulacrum
Copy link
Member Author

I wouldn't worry about filing pull requests with 1300+ crates bumping lock files (in part because those are unlikely to get merged quickly).

I do think getting patch releases out for each series would already be a significant step forward. I think yanking is likely not the right step to take at this time.

In terms of stabilizing only with e2021- no, that's not really possible. In theory we could stabilize with a different name to avoid all the breakage, but I doubt there's a great name...

@est31
Copy link
Member

est31 commented May 25, 2021

is it even possible, to stabilize ::BITS only in e2021?

The constant would have to be available everywhere, and stable everywhere, but one could think about adding a syntactic hack to interpret the ::BITS syntax differently depending on the edition, similarly to how it was done for into_iter. That would have the desired effect of avoiding breakage on editions 2018 and before, but I'm not sure it's a good idea to double down on adding such hacks.

@m-ou-se
Copy link
Member

m-ou-se commented May 25, 2021

Attaching it to an edition would not only require compiler changes, but also a lint, a suggestion for rustfix, documentation, etc.

@m-ou-se
Copy link
Member

m-ou-se commented May 25, 2021

IIRC a prelude trait for example adding BITS wouldn't help here, right?

Indeed.

We don't have a firm policy on whether cargo update being sufficient is "enough" that breakage is OK

Such a policy would be useful. If the decision is that that's fine, we might want to make crater update the lock files when something fails before it considers a crate broken.

As another data point: cargo install also ignores the lock files and picks the latest matching version of everything.

@myrrlyn
Copy link

myrrlyn commented May 25, 2021

I assumed the edition route was an infeasible idea but hey might as well get a “no” in writing.

I’ll start cutting fixes for the minor series listed in the regression report this weekend.

@m-ou-se
Copy link
Member

m-ou-se commented May 25, 2021

I’ll start cutting fixes for the minor series listed in the regression report this weekend.

@myrrlyn Thank you! Let me know if I can help.

@m-ou-se
Copy link
Member

m-ou-se commented May 25, 2021

I'm wondering if we should have a std_version = "1.52.0" in Cargo.toml (just like edition = ..) that will make Rustc consider everything that was stabilized in a later version as still unstable. cargo publish could add it to the uploaded crate if it's not explicitly set, and it makes it easier for crates to track an explicit MSRV.

@Mark-Simulacrum
Copy link
Member Author

@myrrlyn did you get a chance to release new patch releases on each of the channels to avoid this breakage for the lexical crates, at least? I believe that was the plan but I haven't seen updates on this issue at least :)

@myrrlyn
Copy link

myrrlyn commented Jun 16, 2021

Hi; I am so sorry for the radio silence; I have had quite the month since then. Fortunately, Alex Huszagh already did the patch-release issuance back in April.

lexical-core versions 0.4.8, 0.5.1, 0.6.8, and 0.7.6 all appear to have the necessary disambiguation in place. These versions also do not appear in the regression report from Bors. I've also run cargo check on each of these releases with #![feature(int_bits_const)] and passed, so these tips ought to be correct when the constants stabilize.

I hope day-before isn't too late and I apologize again for the delay. If you're comfortable with a "run cargo update to un-break your project", then lexical-core is ready.

@Mark-Simulacrum
Copy link
Member Author

No worries. We are planning to move ahead with a compat note indicating cargo update.

@Alexhuszagh
Copy link
Contributor

There was no discussion on the re-stabilization PR #82565 (comment) as to whether this level of breakage is acceptable. Comments on the original breakage issue suggest that 0.7.5 was published with a fix, but older versions (0.4, 0.5, 0.6) did not receive similar treatment.

Just an FYI: this was fixed ~2 months ago, with v0.4.8, v0.5.1, and v0.6.8. It means that any branch used in the wild should be able to be updated to a compatible version now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-critical Critical 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

Successfully merging a pull request may close this issue.

5 participants