-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Generalize dropck to ignore item-less traits #24898
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
Conversation
Fix rust-lang#24805 (see follow-on commit for test.)
Namely, we need to catch cases like `trait Child : Parent { }` where `Parent` itself defines methods.
The new functionality being tested here is that a drop impl bounded by `UserDefined` does not cause dropck to inject its conservative constraints on region inference.
r? @huonw (rust_highfive has picked a reviewer for you, use r? to override) |
(i still need to run |
|
||
let mut has_pred_of_interest = false; | ||
|
||
let mut seen_items = Vec::new(); |
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.
Should this be some sort of O(log n)/O(1) lookup data structure, instead of making this loop theoretically O(n^2)?
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.e. how big is n?)
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.
yeah I had a HashSet here originally when it was traversing every predicate. But when I switched it to being focused on just the set of traits, I became somewhat confident that n
would not get too large -- since I assume you would be unlikely to have a long chain of parent traits where every trait defined no items.
r=me (although maybe you wish to wait for discussion on #24895 before landing?). |
@huonw yeah I'm not going to land this version now. Discussion with @nikomatsakis led me to realize there is a much smaller fix for #24895 that I should land first (and schedule for backport to beta); then after that is done I can work on landing this (which I need for other reasons that have nothing to do with soundness nor the beta). |
(PR #24906 is the aforementioned simpler fix. I plan to rebase this atop that after that lands, but I'll close this for now, since they definitely conflict.) |
Generalize dropck to ignore item-less traits
Fix #24805.
This also should remove the unsoundness of #24895, but at the cost of injecting the dropck constraints in places where they would not be necessary if we did not have
Copy: Clone
.