-
Notifications
You must be signed in to change notification settings - Fork 13k
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
[experiment] Make Cell<T>::update
work for T: Default | Copy
.
#89357
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
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.
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.
Who would be the right person to evaluate soundness of hacks like this?
@matthewjasper maybe?
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.
My notes so far: thanks to
CopyOrDefault
, the only wayCell::update
could be unsound is if both traits are implemented, but with different imposed relationships on lifetimes.However, that means
Copy
is used, andDefault
isn't called. So this is just as unsound as any other specialization onCopy
.IOW, when
Default::default
gets called,T: CopyOrDefault
is equivalent toT: Default
and any requirements from theDefault
impl will be imposed on the caller ofCell::update
.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.
Yes, this is right:
IsDefault
here is used to specialize over nothing.In that regard, I find that writing the overriding impl before the default one, and slightly changing the naming of the things involved, can improve the readability of the code, as I've showcased in this Playground (I've also nested some of the trait definitions and impls, which as a bonus scopes the
unsafe_specialization_marker
, since I can fathom the branches better that way, but I understand that that's debatable. I stand by the order of my impls with the/* + !Trait */
annotations, though)Extra comments / remarks regarding the soundness here
So, in the linked Playground one can see that the "parent impl" for the
IsDefault
specialization isCopyOrDefault
, with the specializedCopy
impls stripped. This means that we are only dealing withDefault
types in practice (moduloCopy
specialization issues), so the specialization is effectively specializing over the whole subset / it is as if there effectively were aDefault
supertrait. That's why I consider thisIsDefault
specialization, when contained to this very usage, sound. But that does assume that theCopy
specialization is fine to begin with, which it may technically not be.For instance, if the
Copy
specialization were not to kick in for aCopy + !Default
type, could we reach theunreachable!
impl? 🤔So, if anything, the only potential issue / area I'm not that confident about is whether a "flaw" with the
Copy
specialization could be "amplified" by allowing a different "flaw" with theIsDefault
specialization.And even then, I could only see that causing "unsoundness" if we were to guarantee, at the API level, that for
Copy + Default
typesupdate()
shall necessarilyCopy
(because a hypothetical failed application ofCopy
specialization could break that guarantee). That's why, unless this situation is cleared up of all suspicion, I think we could loosen that guarantee to something likemem::needs_drop()
and other such stuff:Copy + Default
types, the "usingget()
" optimization is "likely" to be favored, but it is not guaranteed to do so:unsafe
code cannot rely on this for soundness.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.
It may be sound in this particular case, but isn't this basically a… I guess you could say soundness hole, in
rustc_unsafe_specialization_marker
? To quote the PR that created the attribute:So it's not completely sound, but it's in some sense 'less unsound' than unrestricted specialization because you can't use it to call methods.
Except you are using it to call methods, by taking advantage of the supertrait
Default
, from a specialized impl (impl<T: IsDefault> DefaultSpec for T
) whose parent impl does not have aDefault
bound.With that ability, how is this any different from unrestricted specialization? In other words, what dangerous thing could you do by specializing on
Default
(which is not allowed) that you cannot do by creating a wrapper trait like thisIsDefault
and specializing on that? It seems like the answer is 'nothing', in which case the idea ofrustc_unsafe_specialization_marker
being limited to marker traits that have no methods is meaningless. But I may be missing something.I note that in the case of some existing
rustc_unsafe_specialization_marker
traits, such asFusedIterator
andTrustedLen
, they do have supertraits – in both cases,Iterator
– but impls that specialize on them always(?) have parent impls that are already bounded onIterator
. On the other hand, another such trait,MarkerEq
, as used inRcEqIdent
andArcEqIdent
, is the same kind of wrapper asIsDefault
. It even has a comment saying: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.
I think this might be a soundness hole in the current
min_specialization
impl that I didn't get around to fixing.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.
I've opened #89413 for this