Make Step trait safe to implement
#83772
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
T-libs-api
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
jhpratt
force-pushed
the
revamp-step-trait
branch
from
601494d to
f74329b
Compare
This comment has been minimized.
This comment has been minimized.
jhpratt
force-pushed
the
revamp-step-trait
branch
from
f74329b to
af7bee0
Compare
This comment has been minimized.
This comment has been minimized.
Member
Author
|
CI is green. |
JohnCSimon
added
S-waiting-on-review
Member
Author
|
Ping @sfackler for review. This will likely need the libs team to discuss this change more generally as well. |
Collaborator
|
☔ The latest upstream changes (presumably #85150) made this pull request unmergeable. Please resolve the merge conflicts. |
Member
Author
|
Happy to rebase after a review. |
Member
|
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. |
Member
Author
|
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 |
Member
|
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? |
Mark-Simulacrum
added
S-waiting-on-author
Member
Author
|
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 |
Member
|
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. |
Member
Author
|
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. |
jhpratt
force-pushed
the
revamp-step-trait
branch
2 times, most recently
from
7eea680 to
017e08c
Compare
Member
Author
|
@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. |
the8472
reviewed
May 17, 2021
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.
jhpratt
force-pushed
the
revamp-step-trait
branch
from
aaf4d88 to
741b9a4
Compare
This comment has been minimized.
This comment has been minimized.
rustbot
added
S-waiting-on-review
Member
|
@bors r+ rollup=never |
Collaborator
|
📌 Commit 741b9a4 has been approved by |
bors
added
S-waiting-on-bors
Collaborator
|
⌛ Testing commit 741b9a4 with merge 7dfc146a070facf6afcf501c0914aecb7550ebd4... |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Collaborator
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
Member
Author
|
After asking on Zulip, apparently that test isn't the best (#83262) and a retry should work. |
Member
bors
added
S-waiting-on-bors
Collaborator
Collaborator
|
☀️ Test successful - checks-actions |
Contributor
rust-highfive
added a commit
to rust-lang-nursery/rust-toolstate
that referenced
this pull request
May 30, 2021
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).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes a few modifications to the
Steptrait that I believe better position it for stabilization in the short term. In particular,unsafe trait TrustedStepis introduced, indicating that the implementation ofStepfor a given type upholds all stated invariants (which have remained unchanged). This is gated behind a newtrusted_stepfeature, as stabilization is realistically blocked on min_specialization.Steptrait is internally specialized on theTrustedSteptrait, which avoids a serious performance regression.TrustedLenis implemented forT: TrustedStepas the latter's invariants subsume the former's.Steptrait is no longerunsafe, as the invariants must not be relied upon by unsafe code (unless the type implementsTrustedStep).TrustedStepis implemented for all types that implementStepin the standard library and compiler.step_trait_extfeature is merged into thestep_traitfeature. I was unable to find any reasoning for the features being split; the_uncheckedmethods 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
Stepwill be broken, as it is not possible tounsafe impla safe trait. Given this trait only exists on nightly, I feel this breakage is acceptable. The blanketimpl<T: Step> TrustedLen for Twill likely cause some minor breakage, but this should be covered by the equivalent impl forTrustedStep.Hopefully these changes are sufficient to place
Stepin decent position for stabilization, which would allow user-defined types to be used witha..bsyntax.