-
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
Implement the precise analysis pass for lint disjoint_capture_drop_reorder
#81912
Conversation
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.
This looks fantastic! A few nits and thoughts, but almost ready to merge.
@@ -642,6 +650,279 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
need_migrations | |||
} | |||
|
|||
fn compute_2229_migrations_precise_pass( |
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.
This function could use a comment =) why are there two passes? What does it mean for one to be "precise"?
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.
Those questions are not purely rhetorical. I'm also just legit unsure why there are two passes :)
struct ConstainsDropField(Foo, Foo); | ||
|
||
// Test that if all paths starting at root variable that implement Drop are captured | ||
// then it doesn't trigger the lint. |
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.
It occurs to me that the order in which these drops will execute may be different -- but I think that's ok. It seems pretty unlikely that this will be relevant. Still, I think we should document these assumptions -- maybe in the project-rfc-2229 repo? We're going to want to note them down in the Edition Migration Guide.
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., before, the closure would drop t
, which would in turn drop its fields in order, but now the order of the fields in the closure may not match -- actually, if we wanted, we could sort the fields in the closure and fix that, couldn't we?)
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.
Yea sorting won't be hard,
For place_a, place_b, let c
length of the common starting prefix of these places. Eg: for p.x.y and p.x.z then c = 2.
Then we can write a compartor which is essentially place_a.projections[c] < place_b.projections[c] => place_a < place_b
@nikomatsakis I have updated the PR. There was no specific reason to do a two-pass implementation, so I have combined them |
1934bf3
to
96c12f9
Compare
@bors r+ |
📌 Commit 96c12f9 has been approved by |
Rollup of 11 pull requests Successful merges: - rust-lang#80523 (#[doc(inline)] sym_generated) - rust-lang#80920 (Visit more targets when validating attributes) - rust-lang#81720 (Updated smallvec version due to RUSTSEC-2021-0003) - rust-lang#81891 ([rustdoc-json] Make `header` a vec of modifiers, and FunctionPointer consistent) - rust-lang#81912 (Implement the precise analysis pass for lint `disjoint_capture_drop_reorder`) - rust-lang#81914 (Fixing bad suggestion for `_` in `const` type when a function rust-lang#81885) - rust-lang#81919 (BTreeMap: fix internal comments) - rust-lang#81927 (Add a regression test for rust-lang#32498) - rust-lang#81965 (Fix MIR pretty printer for non-local DefIds) - rust-lang#82029 (Use debug log level for developer oriented logs) - rust-lang#82056 (fix ice (rust-lang#82032)) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The precision pass for the lint prevents the lint from triggering for a variable (that was previously entirely captured by the closure) if all paths that need Drop starting at root variable have been captured by the closure.
r? @nikomatsakis