Detect double reference when applying binary op #34420
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| match lhs_ty.sty { | ||
| TypeVariants::TyRef(_, ref ty_mut) => { | ||
| match ty_mut.ty.sty { | ||
| TypeVariants::TyRef(_, ref ty_mut) => { |
|
I like this note because I've seen plenty of people confused by this mistake. I have a little doubt about the wording though, the error message for something so confusing should be as simple as possible. Since the message already recommends deref'ing once, perhaps the "reference of a reference" part can be simplified? Something like:
(This may be too short instead.) |
|
@rkruppe I incorporated the review comments, as well as added a new test for the &String + str case. Slightly reworded the NOTE, trying to have a middle point between the terseness of your proposed wording and the specificity of mine. Please let me know what you think. |
| err.span_note( | ||
| lhs_expr.span, | ||
| &format!( | ||
| "this is a reference of type that `{}` can be \ |
There was a problem hiding this comment.
"reference of type that + can be applied to" sounds ungrammatical to me. I am not a native speaker, so I would like to invite others to chip in.
|
So I thought a bit more about Perhaps you could add code to check if the lhs type is |
```rust
let vr = v.iter().filter(|x| {
x % 2 == 0
});
```
will now yield the following compiler output:
```bash
ERROR binary operation `%` cannot be applied to type `&&_`
NOTE this is a reference of a reference to a type that `%` can be applied to,
you need to dereference this variable once for this operation to work
NOTE an implementation of `std::ops::Rem` might be missing for `&&_`
```
The first NOTE is new.
Bug rust-lang#33877
estebank
force-pushed
the
double-reference
branch
from
961e371 to
3cdadcf
Compare
|
@rkruppe, I haven't been able to retake this since I last pushed. I am not sure how to check wether the lefthand span Beyond that, is there a way to determine wether the lhs is an argument to the current context, and if it is, getting the span for it? Having that I could expand the help text for those cases with something along the lines of: |
|
Checking whether a No idea how you might check for a parameter, but I wouldn't sweat it since cc @Manishearth do you know? |
|
I think |
|
@estebank do you think you'll be able to incorporate the changes to check for |
|
@pnkfelix I've had some issues with the lifetimes associated to |
|
ping r? @pnkfelix |
|
Sorry for the delay, I will try to assist. |
|
☔ The latest upstream changes (presumably #38057) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@estebank okay so while one could try to use I'll try to push a commit to your branch (a feature I've heard of but never used) that makes these changes (as well as the update to the |
|
Update: I was not able to push to the In case @estebank would like to see, here is the key bit of the patch I added: diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs
index 442fdb6..ca809a3 100644
--- a/src/librustc_typeck/check/op.rs
+++ b/src/librustc_typeck/check/op.rs
@@ -188,7 +188,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
lhs_ty);
if let TypeVariants::TyRef(_, ref ty_mut) = lhs_ty.sty {
- if self.lookup_op_method(expr, ty_mut.ty, vec![rhs_ty_var],
+ if !self.infcx.type_moves_by_default(ty_mut.ty, lhs_expr.span) &&
+ self.lookup_op_method(expr, ty_mut.ty, vec![rhs_ty_var],
token::intern(name), trait_def_id,
lhs_expr).is_ok() {
err.span_note( |
|
A semi-related issue here is that there has been talk of extending the language so that references to copy-types just behave more like the underlying type itself in various ways; e.g. inserting auto-deref when passing See e.g. https://internals.rust-lang.org/t/roadmap-2017-productivity-learning-curve-and-expressiveness/4097 But it may be a long time until that is proposed, gets through the RFC process, and is implemented. So I'll still plan to land this note in the meantime. |
Detect double reference when applying binary op
``` rust
let vr = v.iter().filter(|x| {
x % 2 == 0
});
```
will now yield the following compiler output:
``` bash
ERROR binary operation `%` cannot be applied to type `&&_`
NOTE this is a reference of a reference to a type that `%` can be applied to,
you need to dereference this variable once for this operation to work
NOTE an implementation of `std::ops::Rem` might be missing for `&&_`
```
The first NOTE is new.
Fix rust-lang#33877
----
Thanks to @estebank for providing the original PR rust-lang#34420 (of which this is a tweaked rebase).
Detect double reference when applying binary op
``` rust
let vr = v.iter().filter(|x| {
x % 2 == 0
});
```
will now yield the following compiler output:
``` bash
ERROR binary operation `%` cannot be applied to type `&&_`
NOTE this is a reference of a reference to a type that `%` can be applied to,
you need to dereference this variable once for this operation to work
NOTE an implementation of `std::ops::Rem` might be missing for `&&_`
```
The first NOTE is new.
Fix rust-lang#33877
----
Thanks to @estebank for providing the original PR rust-lang#34420 (of which this is a tweaked rebase).
will now yield the following compiler output:
The first NOTE is new.
Bug #33877