-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Dedup auto traits in trait objects. #51276
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
src/librustc_typeck/astconv.rs
Outdated
let mut auto_traits = | ||
auto_traits.into_iter().map(ty::ExistentialPredicate::AutoTrait).collect::<Vec<_>>(); | ||
auto_traits.sort_by(|a, b| a.cmp(tcx, b)); | ||
auto_traits.dedup_by(|a, b| (&*a).cmp(tcx, b) == Ordering::Equal); |
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.
The (&*a) is necessary here because std::cmp::Ord::cmp
shadows ty::ExistentialPredicate::cmp
for an &mut ty::ExistentialPredicate
, but not necessary in the sort_by
on the previous line because the shadowing is reversed for &ty::ExistentialPreciate
(and the unborrowed version).
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 wonder if it's possible for you to just collect into an FxHashSet
here?
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.
The cmp
used by a HashSet doesn't compare using the TyCtx, so would fail if the DefIds for the auto trait are different, which I think can happen through type or use aliases. Also, in the vast majority of cases, there's going to be less than three auto traits (and many times zero), so I don't think it would be much of an optimization.
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.
First off, I would recommend invoking ty::ExistentialPredicate::cmp
— but maybe we should rename that method, if it is indeed not equivalent to Ord::cmp
?
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.
Put another way, why can't we invoke .dedup()
here?
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.
Hmm, I thought that the DefIds
might be different based on which use
clause you use, but switching to just .dedup()
works here. But then, why is there both ty::ExistentialPredicate::cmp
and Ord::cmp
if they give the same results? Are there actually cases where two DefId
s could point to the same trait?
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.
So, that function (a) has a bad name and (b) needs a comment. I think the distinction is this:
ExistentialPredicate::cmp
sorts via a stable ordering that will not change if modules are ordered or other changes are made to the tree. In particular, this ordering is preserved across incremental compilations.Ord::Cmp
sorts in an order that is not stable across compilations.
For purposes of equality, they are equivalent.
It should really be called ExistentialPredicate::stable_cmp
-- would you be game to try and rename it? (And add a comment.)
That could be either part of this PR or outside of it...
...given the unfortunate shadowing, the way I would go about it would be to add stable_cmp
and mark the existing method as deprecated (and maybe make it panic!
for good measure). Then a ./x.py test
should shake out all the callers. =)
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.
In any case, I think we can also (in this PR) just call dedup
. We probably don't need a stable ordering, either.
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.
In that case, we don't need to put the auto traits into the ExistentialPredicate before sorting and deduping, so can just sort and dedup the returned auto traits. Which makes this change ultimately more readable than the whole wrapping and then comparing.
--
I'll tackle the refactor in a separate PR, since it has nothing to do with this one. Due to the argument count difference, anybody calling the inherent method will get an argument count error at compile time, so I don't think the old name needs to be deprecated.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
8fa478d
to
d307c00
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
d307c00
to
13399a5
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
But why did that pass on my end? |
d0e4f82
to
1990b45
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1990b45
to
03fd89c
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
03fd89c
to
86ff9fa
Compare
r? @nikomatsakis for re-assignment |
@bors r+ |
📌 Commit eccd2ed has been approved by |
🌲 The tree is currently closed for pull requests below priority 1, this pull request will be tested once the tree is reopened |
…tsakis Dedup auto traits in trait objects. Fixes rust-lang#47010 Note that the test file `run-pass/trait-object-auto-dedup.rs` passes before and after this change. It's the `ui` test that changed from compiling to not compiling. Which does make this a breaking change, but I cannot imagine anybody actually being broken by it.
🔒 Merge conflict |
Rollup of 13 pull requests Successful merges: - #50143 (Add deprecation lint for duplicated `macro_export`s) - #51099 (Fix Issue 38777) - #51276 (Dedup auto traits in trait objects.) - #51298 (Stabilize unit tests with non-`()` return type) - #51360 (Suggest parentheses when a struct literal needs them) - #51391 (Use spans pointing at the inside of a rustdoc attribute) - #51394 (Use scope tree depths to speed up `nearest_common_ancestor`.) - #51396 (Make the size of Option<NonZero*> a documented guarantee.) - #51401 (Warn on `repr` without hints) - #51412 (Avoid useless Vec clones in pending_obligations().) - #51427 (compiletest: autoremove duplicate .nll.* files (#51204)) - #51436 (Do not require stage 2 compiler for rustdoc) - #51437 (rustbuild: generate full list of dependencies for metadata) Failed merges:
…table_cmp See rust-lang#51276 (comment) for rationale.
Refactor: Rename ExistentialPredicate::cmp to ExistentialPredicate::stable_cmp See #51276 (comment) for rationale. Because stable_cmp takes three arguments and Ord::cmp takes two, I am confident that there is no shadowing happening here. r? @nikomatsakis
Fixes #47010
Note that the test file
run-pass/trait-object-auto-dedup.rs
passes before and after this change. It's theui
test that changed from compiling to not compiling. Which does make this a breaking change, but I cannot imagine anybody actually being broken by it.