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

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

Draft
wants to merge 2 commits into
base: master
from

Conversation

@Osspial
Copy link

commented Mar 28, 2019

Fixes #34426.

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

@pnkfelix

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

(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.))

@pnkfelix 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

@Centril

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

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

@Osspial

This comment has been minimized.

Copy link
Author

commented Apr 10, 2019

Any updates on reviewing this?

@estebank

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

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

This comment has been minimized.

Copy link
Member

commented May 18, 2019

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
Copy link
Contributor

left a comment

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

}
};

if !assumptions_in_impl_context.iter().find(predicate_matches).is_some() {

This comment has been minimized.

Copy link
@oli-obk

oli-obk May 19, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@oli-obk

oli-obk May 19, 2019

Contributor

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

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

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

@arielb1

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.