Check for <&NotClone as Clone>::clone() calls and suggest to add Clone trait appropriately
#112995
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fee1-dead (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
|
@estebank Forgive if this is stealing thunder as I wouldn't have done this so quickly without your work here, please feel free to pull this into your branch or whatever works best for protocol. This covers more cases as per the tests though there are still potentially further improvements I may look into at some point as I notice this issue is probably in the same area: #76643 I think this is OK, but there is a danger of heavy recursion in big code bases but I think they'll always terminate fairly quickly or get caught by stack limits (which are probably quite high in compiler world having said that). Would welcome your thoughts on this one? |
|
Note that there is already the |
|
@fee1-dead Seems fair, the only thing would be this would put it inline with the error itself making it clear this fixes the error where the other would put out a seemingly unrelated warning the user would have to link together, could even get lost in lots of other warnings. At least that's my understanding of it. This would potentially be helpful to beginners and the like, more so than experienced Rust devs certainly. But yeah, if we decide to not go with this for that reason that's cool. I'm happy to close if so. |
The issue with lints is that they don't trigger if other errors are present, which is why I find it reasonable to have some duplication between targeted lints and suggestions in errors, to reduce the number of back and forth that users have when fixing an issue. |
|
Setting to waiting on author per my review. @rustbot author |
rustbot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #113637) made this pull request unmergeable. Please resolve the merge conflicts. |
strottos
force-pushed
the
ref-clone-suggestions
branch
from
6f7bcc3
to
323a141
Compare
|
Pushed some more changes, there's still some patterns not covered but it's more inclusive as can be seen from the new tests. Examples of a pattern not covered is the following, this seems harder: where conversely this is covered: Not opposed to looking at the above, but I suspect this will take longer, would be interested in what people thought for feedback as to whether the above is worth it @fee1-dead @estebank ? |
|
I do think it's an improvement from what it was but I think to cover every pattern will be a lot of work and not sure how much value. |
|
@rustbot review |
rustbot
added
S-waiting-on-review
It is fine to not support every case, particularly when there are generics in place. I think we could eventually look for any arguments in an fn expression for
I am of the position of "handle 80% of the cases with 20% of the effort". If we can expand on the coverage after the fact, that's fine. |
|
That was the conclusion I came to as well, I did try exactly that funny enough using the |
| hir::PatKind::Tuple(pats, ..) => match init.kind { | ||
| ExprKind::Tup(init_tup) => { | ||
| for (pat, init) in zip(pats.iter(), init_tup.iter()) { | ||
| if pat.hir_id == *hir_id { | ||
| return self.note_type_is_not_clone( | ||
| diag, | ||
| expected_ty, | ||
| found_ty, | ||
| init, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| ExprKind::Block( | ||
| hir::Block { | ||
| expr: | ||
| Some(Expr { | ||
| kind: ExprKind::Tup(init_block_expr_tup), .. | ||
| }), | ||
| .. | ||
| }, | ||
| _, | ||
| ) => { | ||
| for (pat, init) in zip(pats.iter(), init_block_expr_tup.iter()) |
There was a problem hiding this comment.
there's a lot of rightward drift here. Perhaps you could add a method to ExprKind that either returns the original expression or the trailing expr of the innermost block? It could be named as something like trailing_expr or innermost_expr. if you add documentation to that it should be fine
There was a problem hiding this comment.
I agree the right drift isn't great. Not sure I see how this will help at the moment unfortunately though, maybe there's a pattern I'm not seeing, however I can't return an original expression in an ExprKind as it's not an Expr. Plus I'd need to somehow interject that into this match statement.
I'll try and have another look next week to see if I can see a way of taming the right drift.
There was a problem hiding this comment.
Yeah I think this is tricky, I think I see what you're saying but if I do something like this it's no good for the case below as I need the expr:
impl<'hir> ExprKind<'hir> {
pub fn innermost_exprkind(&'hir self) -> &'hir ExprKind<'hir> {
match self {
ExprKind::Block(Block { expr: Some(Expr { kind, .. }), .. }, _) => kind,
_ => self,
}
}
}
On the other hand if I make it return an Expr to be useful then I can't just return self as it's the wrong type.
There was a problem hiding this comment.
Think I've managed to tame the right drift now anyway without this. Have a look at the latest when you've a second and see if it's any better. Happy to look again if further improvements can be made.
| // If we're a closure recurse back into that closure | ||
| hir::ExprKind::Closure(hir::Closure { body: body_id, .. }) => { | ||
| let hir::Body { value: body_expr, .. } = self.tcx.hir().body(*body_id); | ||
| return self.note_type_is_not_clone(diag, expected_ty, found_ty, body_expr); | ||
| } | ||
| // Similarly recurse into a block | ||
| hir::ExprKind::Block(hir::Block { expr: Some(block_expr), .. }, _) => { | ||
| return self.note_type_is_not_clone(diag, expected_ty, found_ty, block_expr); | ||
| } |
There was a problem hiding this comment.
with the trailing_expr abstraction suggested above these duplication wouldn't be necessary i think.
| match expr.kind { | ||
| hir::ExprKind::Path(hir::QPath::Resolved( | ||
| None, | ||
| hir::Path { segments: [_], res: crate::Res::Local(binding), .. }, | ||
| )) => { |
There was a problem hiding this comment.
All the other arms of this match block return. Therefore you can extract the binding like so:
let binding = match expr.kind {
hir::ExprKind::Path(hir::QPath::Resolved(
None,
hir::Path { segments: [_], res: crate::Res::Local(binding), .. },
)) => {
binding
}
/* other arms return */
};There was a problem hiding this comment.
Good suggestion but it’s no longer true that all the other branches return now we’re not returning an Option. I think this is clearer as is personally, but happy to change if you still think.
This comment has been minimized.
This comment has been minimized.
strottos
force-pushed
the
ref-clone-suggestions
branch
from
bcedda6
to
02b65d9
Compare
|
Thanks @fee1-dead , think I’ve addressed all but the last one which I don’t think is relevant any more. Thanks for your time. |
| match init.kind { | ||
| ExprKind::Tup(init_tup) => { | ||
| if let Some(init) = find_init(init_tup) { | ||
| self.note_type_is_not_clone_inner_expr(init) | ||
| } else { | ||
| expr | ||
| } | ||
| } | ||
| ExprKind::Block(_, _) => { | ||
| let block_expr = init.peel_blocks(); |
There was a problem hiding this comment.
I meant you call peel_blocks and just do if let ExprKind::Tup(init_tup) = &init.peel_blocks().kind. That way you don't have to worry about matching the block case at all. With that you don't need to have duplicate arms or a closure either. Sorry for not being clear about this.
| self.note_type_is_not_clone_inner_expr(block_expr) | ||
| } | ||
| hir::ExprKind::Call(Expr { kind: call_expr_kind, .. }, _) => { | ||
| if let hir::ExprKind::Path(hir::QPath::Resolved( None, call_expr_path,)) = call_expr_kind |
There was a problem hiding this comment.
| if let hir::ExprKind::Path(hir::QPath::Resolved( None, call_expr_path,)) = call_expr_kind | |
| if let hir::ExprKind::Path(hir::QPath::Resolved(None, call_expr_path)) = call_expr_kind |
| && let Some(closure) = self.tcx.hir().find(self.tcx.hir().parent_id(*hir_id)) | ||
| && let hir::Node::Local(hir::Local { init: Some(init), .. }) = closure | ||
| { | ||
| self.note_type_is_not_clone_inner_expr(init) |
There was a problem hiding this comment.
this seems to be specific to the clone_thing9 test case? If this is so and this is either a closure or something else that we don't care about, I'd prefer this to extract the closure body here instead of in a match case above. Therefore you should remove that match arm and move that here.
Also, could you add some comments on this if let chain, to explain what we are trying to do here (lookup the source of a closure call)?
| // Similarly recurse into a block | ||
| hir::ExprKind::Block(hir::Block { expr: Some(block_expr), .. }, _) => { | ||
| self.note_type_is_not_clone_inner_expr(block_expr) | ||
| } |
There was a problem hiding this comment.
similar to the peel_blocks use above it would be nice if expr.peel_blocks() is used for this match also. Then we'd avoid the unnecessary recursion and reduce the noise from handling not very relevant cases.
strottos
force-pushed
the
ref-clone-suggestions
branch
from
02b65d9
to
25db1fa
Compare
|
@bors r+ |
bors
added
S-waiting-on-bors
Thanks for your time again and no need to apologise, learned loads from doing this and been a great experience 😀 |
|
⌛ Testing commit 25db1fa with merge 0c0090f59cc47c8c6b954e1eed5cdacc28ca53da... |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
Had a quick look and looks kind of like a spurious failure. It failed because the backtrack on a Windows PC stopped spitting out function names and just wrote out When tested on a Linux PC (all I have access to right now, tomorrow can try on a Windows PC tomorrow evening) we get much more output than this that would match the test. Is there a way we can queue a retry @fee1-dead ? No stress on this one, it’s not a biggie. Would be curious to know under what circumstances we get this… could be some obscure Windows internals maybe but would need to look how backtrack gets this detail but it’s bound to be a Windows syscall equivalent (forget the name). |
|
Ran that test 1k times on my Linux box and 1k correct outputs. Wonder if the same will be true on the Windows box tomorrow evening. Either way whatever happens to this I’ll look into that again there tomorrow so even if it is something I’ve done hopefully I’ll be able to see what (though I can’t think what that could have been at present). |
|
Let's just retry and see what happens. @bors retry |
bors
added
S-waiting-on-bors
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#112995 (Check for `<&NotClone as Clone>::clone()` calls and suggest to add Clone trait appropriately) - rust-lang#113578 (Don't say that a type is uncallable if its fn signature has errors in it) - rust-lang#113661 (Double check that hidden types match the expected hidden type) - rust-lang#114044 (factor out more stable impls) - rust-lang#114062 (CI: split nested GHA groups instead of panicking) r? `@ghost` `@rustbot` modify labels: rollup
Added recursive checking back up the HIR to see if a
Clonesuggestion would be helpful.Addresses #112857
Largely based on: #112977