[significant_drop_tightening] Fix #10413 #10774
Conversation
|
r? @dswij (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
@dswij I can re-roll if you don't have time to review |
|
Sorry for the delay, I had started to look into this but then fell a bit ill last week. I will take a look asap |
|
Don't worry |
| fn visit_block(&mut self, block: &'tcx hir::Block<'tcx>) { | ||
| self.ap.curr_block_hir_id = block.hir_id; | ||
| self.ap.curr_block_span = block.span; | ||
| for stmt in block.stmts.iter() { | ||
| self.ap.curr_stmt = Cow::Borrowed(stmt); | ||
| self.visit_stmt(stmt); | ||
| self.ap.curr_block_hir_id = block.hir_id; | ||
| self.ap.curr_block_span = block.span; | ||
| self.manage_has_expensive_expr_after_last_attr(); | ||
| } | ||
| if let Some(expr) = block.expr { | ||
| self.ap.curr_stmt = Cow::Owned(dummy_stmt_expr(expr)); | ||
| self.visit_expr(expr); | ||
| self.ap.curr_block_hir_id = block.hir_id; | ||
| self.ap.curr_block_span = block.span; | ||
| self.manage_has_expensive_expr_after_last_attr(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if I understand completely, but I think this is the difference with the previous implementation? i.e. now we check for blocks and any nested block?
There was a problem hiding this comment.
The previous algorithm used to check all individual occurrences of Block through LateLintPass::check_block but now everything starts in the root block of fn through LateLintPass::check_fn to properly track the subsequent nested block children.
|
There is still related work to be done that I would like to continue pursuing with this PR merged, as such, I will try another reviewer. r? clippy |
|
With a quick look this seems ok. With that said, getting this lint to a state where it could be removed from the nursery will requiring rewriting this as a MIR lint. It needs to be able to track lifetimes for it to not have a large number of false positives. e.g. let mut locked = foo.lock();
let borrow = &mut *locked;
let references_borrow = bar(borrow);
// and so on |
|
Yeah, such thing will allow me to close #9399 but something tells me that it will take a long time to finish :) |
|
@Jarcho I can re-roll again if you don't have the time to review |
|
Sorry, I've been sick the past few weeks. Since this is in the nursery I'll consider the lint still a WIP. I'll just reiterate that you can't fully fix this lint at the HIR level. You can reference Since there doesn't seem to be any new issues added with this. @bors r+ |
[significant_drop_tightening] Fix #10413 Fix #10413 This is quite a rewrite that unfortunately took a large amount of time. I tried my best to comment what is going on to easy review but feel free to ask any question. The problem basically is that the current algorithm is only taking into consideration single blocks which means that things like the following don't work or show unpredictable results. ```rust let mutex = Mutex::new(1); { let lock = mutex.lock().unwrap(); { let _ = *lock; } } ``` The solve the issue, each path that refers a lock is now being tracked individually. ``` changelog: [`significant_drop_tightening`]: Lift the restriction of only considerate single blocks ```
|
💔 Test failed - checks-action_test |
|
The CI failure has been fixed.Thank you, @Jarcho! |
|
@bors retry |
|
@bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fix #10413
This is quite a rewrite that unfortunately took a large amount of time. I tried my best to comment what is going on to easy review but feel free to ask any question.
The problem basically is that the current algorithm is only taking into consideration single blocks which means that things like the following don't work or show unpredictable results.
The solve the issue, each path that refers a lock is now being tracked individually.