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

Fix too restrictive checks on Drop impls #67059

Merged
merged 3 commits into from Dec 21, 2019
Merged

Conversation

@TommasoBianchi
Copy link
Contributor

TommasoBianchi commented Dec 5, 2019

Fixes #34426. Fixes #58311.

This PR completes and extends #59497 (which has been inactive for a while now).
The problem generating both issues was that when checking that the Predicates of the Drop impl are exactly the same as the ones of the struct definition, the check was essentially performed by a simple == operator, which was not handling correctly HRTBs and involved Fn types.

The implemented solution relies on the relate machinery to more correctly equate Predicates, and on anonymize_late_bound_regions to handle HRTB in a more general way. As the Relate trait currently is implemented only for TraitPredicate and ProjectionPredicate (and as they were the ones generating problems), relate is used only for them while for other Predicates the equality check is kept. I'm currently considering whether it would make sense to implement the Relate trait also for all other Predicates to render the proposed solution more general.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 5, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @zackmdavis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 5, 2019

@rust-highfive rust-highfive assigned pnkfelix and unassigned zackmdavis Dec 5, 2019
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Dec 5, 2019

@@ -83,10 +87,7 @@ fn ensure_drop_params_and_item_params_correspond<'tcx>(
let fresh_impl_self_ty = drop_impl_ty.subst(tcx, fresh_impl_substs);

let cause = &ObligationCause::misc(drop_impl_span, drop_impl_hir_id);
match infcx

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Dec 11, 2019

Member

please try to avoid making pure-formatting changes in the same commit that has effectual semantic changes.

(If the reformatting is being injected by e.g. running rustfmt, then try to do those runs in a separate commit, please.)

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Dec 11, 2019

Member

(the reason I'm asking for it to be in a separate commit is that it eases review: It allows the reviewer to just quickly skim the commits that are formatting fixes, while diving into the commits that have the real meat!)

This comment has been minimized.

Copy link
@Centril

Centril Dec 21, 2019

Member

Btw, ticking the "hide whitespace changes" checkbox usually helps a bit with this when reviewing.

item_span,
"Use same sequence of generic type and region \
parameters that is on the struct/enum definition",
)
.emit();
.emit();

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Dec 11, 2019

Member

follow-up to other note about formatting changes: while I stand by my claim that I prefer formatting changes to be segregated into their own commits, I will say that if the formatting "fixes" were solely to formatting "bugs" that are this egregious, I myself would probably leave them in the same commit too.

@@ -212,20 +216,40 @@ fn ensure_drop_predicates_are_implied_by_item_defn<'tcx>(
// the analysis together via the fulfill , rather than the
// repeated `contains` calls.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Dec 11, 2019

Member

there is comment here that only makes sense based on the prior version of the code that called .contains(..). Can you update the comment accordingly for future code readers?

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Dec 11, 2019

this looks good. r=me once the formatting changes are factored out and the noted comment about contains is updated in some reasonable way.

@TommasoBianchi TommasoBianchi force-pushed the TommasoBianchi:dropck_fix_pr branch from e02de5c to b08d697 Dec 16, 2019
@TommasoBianchi

This comment has been minimized.

Copy link
Contributor Author

TommasoBianchi commented Dec 16, 2019

r=@pnkfelix thank you for the review, I should have fixed both problems.
I am still curious about whether it would be beneficial to implement equality for all Predicates using the Relate trait; probably it is better to just be conservative and change as little as possible while still fixing the open issues.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Dec 21, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

📌 Commit b08d697 has been approved by pnkfelix

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 21, 2019

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

Centril added a commit to Centril/rust that referenced this pull request Dec 21, 2019
…felix

Fix too restrictive checks on Drop impls

Fixes rust-lang#34426. Fixes rust-lang#58311.

This PR completes and extends rust-lang#59497 (which has been inactive for a while now).
The problem generating both issues was that when checking that the `Predicate`s of the `Drop` impl are exactly the same as the ones of the struct definition, the check was essentially performed by a simple `==` operator, which was not handling correctly HRTBs and involved `Fn` types.

The implemented solution relies on the `relate` machinery to more correctly equate `Predicate`s, and on `anonymize_late_bound_regions` to handle HRTB in a more general way. As the `Relate` trait currently is implemented only for `TraitPredicate` and `ProjectionPredicate` (and as they were the ones generating problems), `relate` is used only for them while for other `Predicate`s the equality check is kept. I'm currently considering whether it would make sense to implement the `Relate` trait also for all other `Predicate`s to render the proposed solution more general.
Centril added a commit to Centril/rust that referenced this pull request Dec 21, 2019
…felix

Fix too restrictive checks on Drop impls

Fixes rust-lang#34426. Fixes rust-lang#58311.

This PR completes and extends rust-lang#59497 (which has been inactive for a while now).
The problem generating both issues was that when checking that the `Predicate`s of the `Drop` impl are exactly the same as the ones of the struct definition, the check was essentially performed by a simple `==` operator, which was not handling correctly HRTBs and involved `Fn` types.

The implemented solution relies on the `relate` machinery to more correctly equate `Predicate`s, and on `anonymize_late_bound_regions` to handle HRTB in a more general way. As the `Relate` trait currently is implemented only for `TraitPredicate` and `ProjectionPredicate` (and as they were the ones generating problems), `relate` is used only for them while for other `Predicate`s the equality check is kept. I'm currently considering whether it would make sense to implement the `Relate` trait also for all other `Predicate`s to render the proposed solution more general.
bors added a commit that referenced this pull request Dec 21, 2019
Rollup of 7 pull requests

Successful merges:

 - #67059 (Fix too restrictive checks on Drop impls)
 - #67355 (Merge `ast::Mutability` and `mir::Mutability`)
 - #67393 (Enable opting out of specific default LLVM arguments.)
 - #67422 (Cleanup err codes)
 - #67462 (Make ptr::slice_from_raw_parts a const fn available under a feature flag)
 - #67467 (Test slice patterns more)
 - #67478 (Fix src/libcore/str/mod.rs doc comments)

Failed merges:

r? @ghost
@bors bors merged commit b08d697 into rust-lang:master Dec 21, 2019
4 checks passed
4 checks passed
pr #20191216.33 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.