-
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
Add lint for 2229 migrations #80629
Add lint for 2229 migrations #80629
Conversation
The job Click to see the possible cause of the failure (guessed by this bot)
|
☔ The latest upstream changes (presumably #76219) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
By and large, this looks great. I left a few nits.
self.compute_min_captures(closure_def_id, delegate.capture_information); | ||
|
||
let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id); | ||
if should_do_migration_analysis(self.tcx, closure_hir_id) { |
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.
nit: can we pull the body of this if
out into a separate method?
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.
Done
|
||
let is_moved = root_var_min_capture_list | ||
.iter() | ||
.find(|capture| matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue(_))) |
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.
nit: you can use .any()
instead =)
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.
Done
// 2. If we only capture one path starting at the root variable, it's still possible | ||
// that it isn't the root variable completely. | ||
if is_moved | ||
&& ((root_var_min_capture_list.len() > 1) |
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.
Nit: maybe simpler to do root_var_min_capture_list.iter().any(|l| l.place.projections.len() > 0)
?
would we ever capture a
and a.b
? no, right?
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.
Done
let (level, _) = | ||
tcx.lint_level_at_node(lint::builtin::DISJOINT_CAPTURE_DROP_REORDER, closure_id); | ||
|
||
!matches!(level, lint::Level::Allow) |
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 wonder if we want to consider the Edition too .. e.g., in Edition 2021, maybe this lint should be a no-op
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.
Not important
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.
When I initially wrote this the PR to introduce 2021 edition was in progress, so I didn't think too much at the time. I don't know if we should just ignore lints that are no-op without letting the user know about it.
|
||
// Test migration analysis in case of Drop + Non Drop aggregates. | ||
// Note we need migration here only because the non-copy (because Drop type) is captured, | ||
// otherwise we won't need to, since we can get away with just by ref capture in that case. |
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.
We don't really need migration here, right? The only drop that will run is still going to run at the same point either way (when the closure is dropped). I agree the analysis as implemented in this PR would be expected to warn, it's just that it's overly conservative.
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.
We don't need migrations here, but this PR doesn't implement the precise path yet. I can add another String to the tuple so that we don't need to move the test when I implement the precise path.
497100e
to
b75dd0d
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
f6e6601
to
eea0668
Compare
@bors r+ |
📌 Commit eea0668100c15869891597eec40c0ac2a1b84712 has been approved by |
☔ The latest upstream changes (presumably #81461) made this pull request unmergeable. Please resolve the merge conflicts. |
eea0668
to
d8e89fb
Compare
☔ The latest upstream changes (presumably #81493) made this pull request unmergeable. Please resolve the merge conflicts. |
d8e89fb
to
11abaa1
Compare
📌 Commit 11abaa1 has been approved by |
✌️ @arora-aman can now approve this pull request |
Cargo.lock
Outdated
@@ -5388,7 +5388,7 @@ dependencies = [ | |||
"chrono", | |||
"lazy_static", | |||
"matchers", | |||
"parking_lot 0.11.0", | |||
"parking_lot 0.9.0", |
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.
Was this change to Cargo.lock intentional?
…sakis Add lint for 2229 migrations Implements the first for RFC 2229 where we make the decision to migrate a root variable based on if the type of the variable needs Drop and if the root variable would be moved into the closure when the feature isn't enabled. r? `@nikomatsakis`
@bors r- based on #80629 (comment) |
- This allows us add fake information after handling migrations if needed. - Capture analysis also priortizes what we see earlier, which means fake information should go in last.
5a3b85c
to
d96041b
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
d96041b
to
84f0a0a
Compare
Updated PR to handle issues some conflict with #80092 |
@bors r+ |
📌 Commit 84f0a0a has been approved by |
@bors r=nikomatsakis |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit 84f0a0a has been approved by |
…as-schievink Rollup of 11 pull requests Successful merges: - rust-lang#80629 (Add lint for 2229 migrations) - rust-lang#81022 (Add Frames Iterator for Backtrace) - rust-lang#81481 (move some tests) - rust-lang#81485 (Add some tests for associated-type-bounds issues) - rust-lang#81492 (rustdoc: Note why `rustdoc::html::markdown` is public) - rust-lang#81577 (const_evaluatable: consider sub-expressions to be evaluatable) - rust-lang#81599 (Implement `TrustedLen` for `Fuse<I: TrustedLen>`) - rust-lang#81608 (Improve handling of spans around macro result parse errors) - rust-lang#81609 (Remove the remains of query categories) - rust-lang#81630 (Fix overflowing text on mobile when sidebar is displayed) - rust-lang#81631 (Remove unneeded `mut` variable) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Original message: Rollup merge of rust-lang#80629 - sexxi-goose:migrations_1, r=nikomatsakis Add lint for 2229 migrations Implements the first for RFC 2229 where we make the decision to migrate a root variable based on if the type of the variable needs Drop and if the root variable would be moved into the closure when the feature isn't enabled. r? `@nikomatsakis`
Implements the first for RFC 2229 where we make the decision to migrate a root variable based on if the type of the variable needs Drop and if the root variable would be moved into the closure when the feature isn't enabled.
r? @nikomatsakis