Reduce false positives of tail-expr-drop-order from consumed values #129864
Conversation
rustbot
added
S-waiting-on-review
dingxiangfei2009
force-pushed
the
reduce-false-positives-from-consumed-droppers
branch
from
c1fce35
to
491fb13
Compare
dingxiangfei2009
force-pushed
the
reduce-false-positives-from-consumed-droppers
branch
from
491fb13
to
de2d256
Compare
|
I also found that |
| type Error = !; | ||
|
|
||
| fn typeck_results(&self) -> Self::TypeckResults<'_> { | ||
| self.0.typeck(self.1) |
There was a problem hiding this comment.
This is going to be extremely expensive for the query, for the record. I would probably make some helper that caches the typeck results after the first call, or better yet just load the typeck results yourself and pass them here.
| match place_with_id.place.base { | ||
| PlaceBase::Rvalue => { | ||
| self.nodes.insert(place_with_id.hir_id); | ||
| } | ||
| PlaceBase::Local(id) => { | ||
| self.nodes.insert(id); | ||
| } | ||
| PlaceBase::Upvar(upvar) => { | ||
| self.nodes.insert(upvar.var_path.hir_id); | ||
| } |
There was a problem hiding this comment.
Doesn't this count partial moves as full moves? Isn't that still not correct for drop ordering?
There was a problem hiding this comment.
Also, doesn't this not respect different paths correctly? What about consuming operations that happen on only one branch of a conditional path? I don't know if this sort of analysis is supported with ExprUseVisitor, which has no "flow" state.
| } | ||
| } | ||
|
|
||
| fn extract_tail_expr_consuming_nodes<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx HirIdSet { |
There was a problem hiding this comment.
Make this into a hook, not a query; I don't believe it needs any sort of caching, since that has overhead. This should also explain very clearly what it does.
|
I opened a zulip thread at https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/More.20precise.20lint.20impl.20for.20Edition.202024.20tail-expr-drop-order to discuss this lint, because as @compiler-errors mentioned I feel like this might almost need to be some kind of flow analysis. |
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #130357) made this pull request unmergeable. Please resolve the merge conflicts. |
r? @jieyouxu
To reduce false positives, the lint does not fire if the locals are consumed/moved, or the values with significant drop are consumed/moved at the tail expression location.
I am also printing the type involved for easier diagnosis.