-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Make Step
trait safe to implement
#83772
Conversation
This comment has been minimized.
This comment has been minimized.
601494d
to
f74329b
Compare
This comment has been minimized.
This comment has been minimized.
f74329b
to
af7bee0
Compare
This comment has been minimized.
This comment has been minimized.
CI is green. |
Ping @sfackler for review. This will likely need the libs team to discuss this change more generally as well. |
☔ The latest upstream changes (presumably #85150) made this pull request unmergeable. Please resolve the merge conflicts. |
Happy to rebase after a review. |
Has there been discussion somewhere about why Step being unsafe is necessarily a bad thing? I would not expect many users to implement it, but those that do likely have a pretty easy correctness proof - the methods don't look too complicated. IOW, I'm not sure I see the reasoning behind removing the unsafe trait impl in favor of specialization. |
As far as I could tell, the only reason While the proofs would likely be simple, that is not always be the case. If I had implemented Basically, I don't think it's necessary to have it In the future, it would presumably be possible to implement |
Hm. It seems like restricting high-performance Step to only in-std types is worse than making it unsafe; ideally I guess the relationship would be closer to that of TrustedLen and ExactSizeIterator -- that is, I think specialization would still be needed in this case, but based on the existence of TrustedLen I'd guess that it shouldn't be a problem? Does that sound like a plausible path to you? |
If I am understanding what you're saying, that sounds like a perfectly reasonable option. It would have the same benefits and drawbacks of the existing Implementation-wise, I presume I could look at the existing |
Yes, that sounds correct. I can't precisely comment on whether the Trusted and untrusted split is a good path towards stabilization of Step; stabilization of the trusted variant would likely be blocked on getting closer to specialization being a reality, based on #37572 (comment) though that's a pretty old comment. |
For sure. To an end user there wouldn't be any difference (on stable) between the PR as it currently is and what you've said; the only difference would be between the unstable "trusted number of steps" bit. I don't personally need the latter, but it's unquestionably useful for optimization. I'll read up on the exact differences between the two traits and see what I can throw together. |
7eea680
to
017e08c
Compare
@Mark-Simulacrum would you mind taking a look at the current head? Aside from documentation of the new trait, I believe I've done what you were saying. |
While stdlib implementations of the unchecked methods require unchecked math, there is no reason to gate it behind this for external users. The reasoning for a separate `step_trait_ext` feature is unclear, and as such has been merged as well.
aaf4d88
to
741b9a4
Compare
This comment has been minimized.
This comment has been minimized.
@bors r+ rollup=never |
📌 Commit 741b9a4 has been approved by |
⌛ Testing commit 741b9a4 with merge 7dfc146a070facf6afcf501c0914aecb7550ebd4... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
After asking on Zulip, apparently that test isn't the best (#83262) and a retry should work. |
☀️ Test successful - checks-actions |
Tested on commit rust-lang/rust@9a72afa. Direct link to PR: <rust-lang/rust#83772> 💔 rls on windows: test-pass → build-fail (cc @Xanewok). 💔 rls on linux: test-pass → build-fail (cc @Xanewok).
This PR makes a few modifications to the
Step
trait that I believe better position it for stabilization in the short term. In particular,unsafe trait TrustedStep
is introduced, indicating that the implementation ofStep
for a given type upholds all stated invariants (which have remained unchanged). This is gated behind a newtrusted_step
feature, as stabilization is realistically blocked on min_specialization.Step
trait is internally specialized on theTrustedStep
trait, which avoids a serious performance regression.TrustedLen
is implemented forT: TrustedStep
as the latter's invariants subsume the former's.Step
trait is no longerunsafe
, as the invariants must not be relied upon by unsafe code (unless the type implementsTrustedStep
).TrustedStep
is implemented for all types that implementStep
in the standard library and compiler.step_trait_ext
feature is merged into thestep_trait
feature. I was unable to find any reasoning for the features being split; the_unchecked
methods need not necessarily be stabilized at the same time, but I think it is useful to have them under the same feature flag.All existing implementations of
Step
will be broken, as it is not possible tounsafe impl
a safe trait. Given this trait only exists on nightly, I feel this breakage is acceptable. The blanketimpl<T: Step> TrustedLen for T
will likely cause some minor breakage, but this should be covered by the equivalent impl forTrustedStep
.Hopefully these changes are sufficient to place
Step
in decent position for stabilization, which would allow user-defined types to be used witha..b
syntax.