[WIP] Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB #59497

Osspial · 2019-03-28T18:23:21Z

The problem here is that ensure_drop_predicates_are_implied_by_item_defn in dropck.rs incorrectly rejects all implementations that have HRTB clauses in their where clause, even if the corresponding struct definition has an identical HRTB clause. Digging deep into the types, the issue is a DefId mismatch in RegionKind::ReLateBound when it contains a BoundRegion::BrNamed variant - the BrNamed's DefId in the Drop impl doesn't match up with the corresponding DefId in the struct definition.

The current solution is to just ignore the DefId when we come across that specific pattern, and just compare the overall shape of the HRTB bound. This is an absolutely awful way to do it and probably shouldn't be merged onto master (hence the draft PR status), but it works. There are two better solutions I can come up with:

Use the fulfill machinery, which this comment rules out for reasons I don't have the required type-system vocabulary to understand.
Ensure that the DefIds in the drop impl's HRTB clauses match up with the DefIds in the struct's HRTB clauses.

However, I have very little knowledge of the compiler's internals, and hence don't know where to look to implement either of those solutions.

Questions for whoever reviews this:

What are outlives-predicates and region inference constraints, and why do they make the fulfill machinery not work for checking drop predicates (which the linked comment above claims)?
Which of the two above solutions is the best solution? If neither of them are the proper solution, what other compiler mechanisms should I use to solve this?

rust-highfive · 2019-03-28T18:23:25Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (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 · 2019-03-28T20:25:03Z

r? @nikomatsakis

pnkfelix · 2019-04-01T12:03:11Z

(marking as WIP to reflect "draft PR status" as noted in PR description above.)

((i was unware of github's support for "draft" status as an explicit state. however, given that I am also unsure whether bors knows about such state, I am pretty confident that an explicit "WIP" in the title is sensible.))

Centril · 2019-04-01T12:05:43Z

cc rust-lang/homu#14

bors · 2019-04-10T18:50:08Z

☔ The latest upstream changes (presumably #59810) made this pull request unmergeable. Please resolve the merge conflicts.

Osspial · 2019-04-10T21:25:17Z

Any updates on reviewing this?

estebank · 2019-04-12T01:43:41Z

Pinged @nikomatsakis over zulip. The code doesn't seem bad on its own, but your assessment of this not being the best way of approaching this is correct. I'll defer to Niko for a more in depth review.

Mark-Simulacrum · 2019-05-18T13:55:37Z

cc @rust-lang/compiler, is there anyone else who can review this PR? It looks like @nikomatsakis probably doesn't have the time to do so.

oli-obk

I'm a little bit out of my depth here, so take my idea with a grain of salt. Calling p.relate(relater, predicate) will probably do the right thing, but I have not been able to figure out which relater you would want or if you'll need to roll your own

oli-obk · 2019-05-19T16:55:16Z

src/librustc_typeck/check/dropck.rs

+            }
+        };
+
+        if !assumptions_in_impl_context.iter().find(predicate_matches).is_some() {


nit: .iter().find(f).is_some() should be .iter().any(f)

oli-obk · 2019-05-19T18:06:12Z

src/librustc_typeck/check/dropck.rs

+        let predicate_matches = |p: &'_ &Predicate<'_>| {
+            match (p, predicate) {
+                (Predicate::Trait(p), Predicate::Trait(predicate)) => {
+                    let predicate_substs = &*predicate.skip_binder().trait_ref.substs;


So... I have been told that skip_binder is always a signal that something with traits is going wrong. Digging around in the (4 years old) PR that created the "fulfill" comment above and in the docs around TraitRef I believe that using relate instead of == may do the right thing^TM for what you are doing here.

eddyb · 2019-05-22T09:28:37Z

r? @pnkfelix

bors · 2019-06-18T02:19:41Z

☔ The latest upstream changes (presumably #61915) made this pull request unmergeable. Please resolve the merge conflicts.

arielb1 · 2019-06-26T19:30:56Z

Hi!

Screwing with late-bound regions like that tends to cause all sort of weird troubles. Have you tried using anonymize_late_bound_regions in a recursive fashion, like RegionEraserVisitor?

I don't think you can just use plain RegionEraserVisitor, because you don't want to erase the non-LBRs, so you'll probably want a version of it without the fold_region fn.

JohnTitor · 2019-10-27T09:13:55Z

Ping from triage: @Osspial any updates on this?

TommasoBianchi · 2019-11-26T05:30:59Z

Hi everyone, came here while working on #58311. I think I can refactor @Osspial code to account for both HRTB (using anonymize_late_bound_regions as suggested) and for correctly equating associated types in ProjectionPredicate (which was the problem in #58311). I have written a simple impl of TypeRelation in order to use relate instead of ==.

Is it ok if I open a new PR or should we keep the two issues separate? I think that there's some benefit in doing a single refactor of the code that checks for Predicate equality for the dropck.

…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.

Osspial · 2019-12-21T20:41:04Z

I'll go ahead and close this now, since the underlying issue's been resolved. Thank you @TommasoBianchi for looking into it!'

I figured out how to avoid the design pattern that was causing this to be a problem in my code, and I'd had a hard time getting into a groove working on the compiler so fixing this PR up fell to the wayside. I'm sorry for not communicating that earlier.

crlf0710 · 2020-03-29T10:12:36Z

@rustbot modify labels to -S-inactive +S-inactive-closed

Osspial added 2 commits March 15, 2019 17:16

Add regression test for rust-lang#34426

d959799

Add hacky solution to make hrtb dropck work

31f6517

rust-highfive assigned estebank Mar 28, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 28, 2019

rust-highfive assigned nikomatsakis and unassigned estebank Mar 28, 2019

pnkfelix changed the title ~~Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB~~ [WIP] Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB Apr 1, 2019

oli-obk reviewed May 19, 2019

View reviewed changes

rust-highfive assigned pnkfelix and unassigned nikomatsakis May 22, 2019

Mark-Simulacrum assigned arielb1 and unassigned pnkfelix Jul 9, 2019

Mark-Simulacrum added S-inactive and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 9, 2019

TommasoBianchi mentioned this pull request Dec 5, 2019

Fix too restrictive checks on Drop impls #67059

Merged

Osspial closed this Dec 21, 2019

rustbot added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-inactive labels Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB #59497

[WIP] Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB #59497

Osspial commented Mar 28, 2019 •

edited

Loading

rust-highfive commented Mar 28, 2019

Centril commented Mar 28, 2019

pnkfelix commented Apr 1, 2019 •

edited

Loading

Centril commented Apr 1, 2019

bors commented Apr 10, 2019

Osspial commented Apr 10, 2019

estebank commented Apr 12, 2019

Mark-Simulacrum commented May 18, 2019

oli-obk left a comment

oli-obk May 19, 2019

oli-obk May 19, 2019

eddyb commented May 22, 2019

bors commented Jun 18, 2019

arielb1 commented Jun 26, 2019

JohnTitor commented Oct 27, 2019

TommasoBianchi commented Nov 26, 2019

Osspial commented Dec 21, 2019

crlf0710 commented Mar 29, 2020

[WIP] Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB #59497

[WIP] Fix compiler incorrectly rejecting Drop impls where parent struct uses HRTB #59497

Conversation

Osspial commented Mar 28, 2019 • edited Loading

rust-highfive commented Mar 28, 2019

Centril commented Mar 28, 2019

pnkfelix commented Apr 1, 2019 • edited Loading

Centril commented Apr 1, 2019

bors commented Apr 10, 2019

Osspial commented Apr 10, 2019

estebank commented Apr 12, 2019

Mark-Simulacrum commented May 18, 2019

oli-obk left a comment

Choose a reason for hiding this comment

oli-obk May 19, 2019

Choose a reason for hiding this comment

oli-obk May 19, 2019

Choose a reason for hiding this comment

eddyb commented May 22, 2019

bors commented Jun 18, 2019

arielb1 commented Jun 26, 2019

JohnTitor commented Oct 27, 2019

TommasoBianchi commented Nov 26, 2019

Osspial commented Dec 21, 2019

crlf0710 commented Mar 29, 2020

Osspial commented Mar 28, 2019 •

edited

Loading

pnkfelix commented Apr 1, 2019 •

edited

Loading