added missing implementation hint #50161

rizakrko · 2018-04-22T17:53:37Z

Fixes #50151.
Actually, i don't know, should following code
let x = |ref x: isize| { x += 1; };
emit
note: an implementation of std::ops::AddAssign might be missing for &isize
or
note: this is a reference to a type that + can be applied to; you need to dereference this variable once for this operation to work
or both

rust-highfive · 2018-04-22T17:53:47Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (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.

michaelwoerister · 2018-04-23T08:19:44Z

r? @estebank

estebank

Great work! Would you be able to address the nitpicks? If not, we can merge as is, but it'd be great if we could add the suggestions to avoid spamming too much diagnostic information in trivial cases.

estebank · 2018-04-26T23:32:15Z

src/librustc_typeck/check/op.rs

                                     "cannot apply unary operator `{}` to type `{}`",
-                                     op.as_str(), actual).emit();
+                                     op.as_str(), actual);


Could you add here the following?

err.span_label(ex.span, format!("cannot apply unary operator `{}`", op.as_str()));

estebank · 2018-04-26T23:49:14Z

src/librustc_typeck/check/op.rs

+                        hir::UnDeref => "std::ops::UnDerf"
+                    };
+                    err.note(&format!("an implementation of `{}` might be missing for `{}`",
+                                             missing_trait, operand_ty));


It would be interesting to check wether actual is an unsigned numeric value, !, () or str to avoid showing this note, maybe even emitting a note explaining that unsigned numeric values cannot be negative when missing_trait == hir::UnNeg.

estebank · 2018-04-26T23:53:11Z

src/test/ui/issue-5239-1.stderr

@@ -5,6 +5,8 @@ LL |     let x = |ref x: isize| { x += 1; };
   |                              -^^^^^
   |                              |
   |                              cannot use `+=` on type `&isize`
+   |
+   = note: an implementation of `std::ops::AddAssign` might be missing for `&isize`


If you add a check for wether *actual could have op applied, I would point that out with a span_suggestion:

error[E0368]: binary assignment operation `+=` cannot be applied to type `&isize` --> $DIR/issue-5239-1.rs:14:30 | LL | let x = |ref x: isize| { x += 1; }; | -^^^^^ | | | cannot use `+=` on type `&isize` | help: `+=` can be used on `isize`, you can dereference `x`: `&x`

rizakrko · 2018-04-27T13:30:58Z

I will try to allly this suggestions

shepmaster · 2018-05-06T21:21:15Z

Ping from triage @rizakrko ! Will you have time soon to apply the review suggestions?

rizakrko · 2018-05-07T14:10:35Z

@shepmaster Sorry for being late. I'l fix this tomorrow

rizakrko · 2018-05-08T19:24:34Z

Ping @estebank
I'v also ignored TyChar and TyArray in check_user_unop.

estebank

Great improvements! A bunch of suggestions inline.

estebank · 2018-05-08T20:53:51Z

src/librustc_typeck/check/op.rs

-                                        to; you need to dereference this variable once for this \
-                                        operation to work",
-                                    op.node.as_str()));
+                    match is_assign{


[nitpick] space after is_assign

estebank · 2018-05-08T20:56:24Z

src/librustc_typeck/check/op.rs

+                                } {
+                                    let codemap = self.tcx.sess.codemap();
+                                    match codemap.span_to_snippet(lhs_expr.span) {
+                                        Ok(lstring) =>{


[nitpick] space between => and {

estebank · 2018-05-08T20:57:32Z

src/librustc_typeck/check/op.rs

+                                            .is_ok()
+                                } {
+                                    let codemap = self.tcx.sess.codemap();
+                                    match codemap.span_to_snippet(lhs_expr.span) {


This can instead be the following to avoid one level of indentation:

if let Ok(lstring) = codemap.span_to_snippet(lhs_expr.span) { // ... }

estebank · 2018-05-08T20:59:01Z

src/librustc_typeck/check/op.rs

+                                        err.note(
+                                            &format!("an implementation of `{}` might \
+                                                        be missing for `{}`",
+                                                        missing_trait, lhs_ty));


[nitpick] indentation

I was not sure about this, so formatted according to the rustfmt

estebank · 2018-05-08T21:01:18Z

src/librustc_typeck/check/op.rs

+                                    // FIXME: point to span of param
+                                    err.note(
+                                        &format!("`{}` might need a bound for `{}`",
+                                                    lhs_ty, missing_trait));


Missing test for this case.

Also, wording. I'm not a native speaker, but "might need to be bound" sounds slightly better to me.

Not sure about correct wording, so i left it as it was before

estebank · 2018-05-08T21:10:02Z

src/test/ui/feature-gate-negate-unsigned.stderr

-   |                       ^^
+   |                       ^^ cannot apply unary operator `-`
+   |
+   = note: unsigned values cannot be negated


estebank · 2018-05-08T21:11:17Z

src/librustc_typeck/check/op.rs

+                                } {
+                                    let codemap = self.tcx.sess.codemap();
+                                    match codemap.span_to_snippet(lhs_expr.span) {
+                                        Ok(lstring) =>{


if let Ok(lstring) = codemap.span_to_snippet(lhs_expr.span) { instead.

estebank · 2018-05-08T21:12:04Z

src/librustc_typeck/check/op.rs

+                                                                Op::Binary(op, is_assign))
+                                            .is_ok()
+                                } {
+                                    let codemap = self.tcx.sess.codemap();


You're assigning codemap in two places, do it once in the innermost common scope instead.

estebank · 2018-05-08T21:16:23Z

src/librustc_typeck/check/op.rs

                            }
+                            if let Some(missing_trait) = missing_trait {
+                                if missing_trait == "std::ops::AddAssign" &&


Comparing op.node == hir::BiAdd would be faster than comparing strings.

estebank · 2018-05-08T21:19:14Z

src/librustc_typeck/check/op.rs

+                                        &format!("`{}` might need a bound for `{}`",
+                                                    lhs_ty, missing_trait));
+                                } else {
+                                    if !suggested_deref{


This can be } else if !suggested_deref { without the need of an extra indentation level.

rizakrko · 2018-05-08T22:01:31Z

Thanks, @estebank! I'll fix this asap.

estebank · 2018-05-09T17:14:18Z

@bors r+

bors · 2018-05-09T17:14:19Z

📌 Commit fa8ac4a has been approved by estebank

bors · 2018-05-10T07:43:54Z

⌛ Testing commit fa8ac4a with merge a854d3e0993ff9f23ec8100f400c5d0b101b69a8...

bors · 2018-05-10T07:52:11Z

💔 Test failed - status-appveyor

kennytm · 2018-05-10T10:27:29Z

@bors retry #50604

added missing implementation hint Fixes [rust-lang#50151](rust-lang#50151). Actually, i don't know, should following code `let x = |ref x: isize| { x += 1; };` emit `note: an implementation of std::ops::AddAssign might be missing for &isize` or `note: this is a reference to a type that + can be applied to; you need to dereference this variable once for this operation to work` or both

bors · 2018-05-10T16:39:14Z

☔ The latest upstream changes (presumably #50395) made this pull request unmergeable. Please resolve the merge conflicts.

Rollup of 18 pull requests Successful merges: - #49423 (Extend tests for RFC1598 (GAT)) - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)) - #50447 (Fix update-references for tests within subdirectories.) - #50514 (Pull in a wasm fix from LLVM upstream) - #50524 (Make DepGraph::previous_work_products immutable) - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.) - #50538 ( Make CrateNum allocation more thread-safe. ) - #50564 (Inline `Span` methods.) - #50565 (Use SmallVec for DepNodeIndex within dep_graph.) - #50569 (Allow for specifying a linker plugin for cross-language LTO) - #50572 (Clarify in the docs that `mul_add` is not always faster.) - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022)) - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`) - #50588 (Move "See also" disambiguation links for primitive types to top) - #50590 (Fix tuple struct field spans) - #50591 (Restore RawVec::reserve* documentation) - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize) - #50606 (Retry when downloading the Docker cache.) Failed merges: - #50161 (added missing implementation hint) - #50558 (Remove all reference to DepGraph::work_products)

estebank · 2018-05-11T22:48:52Z

@bors r+

bors · 2018-05-11T22:48:53Z

📌 Commit 17c56d4 has been approved by estebank

bors · 2018-05-11T23:06:34Z

⌛ Testing commit 17c56d4 with merge 388df82...

added missing implementation hint Fixes [#50151](#50151). Actually, i don't know, should following code `let x = |ref x: isize| { x += 1; };` emit `note: an implementation of std::ops::AddAssign might be missing for &isize` or `note: this is a reference to a type that + can be applied to; you need to dereference this variable once for this operation to work` or both

bors · 2018-05-12T01:48:00Z

☀️ Test successful - status-appveyor, status-travis
Approved by: estebank
Pushing 388df82 to master...

rust-highfive assigned michaelwoerister Apr 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2018

rust-highfive assigned estebank and unassigned michaelwoerister Apr 23, 2018

estebank approved these changes Apr 26, 2018

View reviewed changes

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 26, 2018

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 8, 2018

estebank reviewed May 8, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 9, 2018

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2018

kennytm mentioned this pull request May 10, 2018

Spurious access violation (exit code: 3221225477) when compiling libsyntax with MSVC #50604

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2018

This was referenced May 10, 2018

Rollup of 19 pull requests #50608

Closed

Rollup of 18 pull requests #50611

Merged

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 10, 2018

rizakrko force-pushed the impl_note branch from fed8a56 to fa8ac4a Compare May 11, 2018 12:10

rizakrko added 5 commits May 11, 2018 15:12

added missing implementation hint

2f6945c

unary op filter, dereference hint

56fbc63

[wip] fixed some error, added missing test

747d5d9

fixed double ref hint

d9b71d2

resolved merge conflict

17c56d4

rizakrko force-pushed the impl_note branch from fa8ac4a to 17c56d4 Compare May 11, 2018 12:14

kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 11, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 11, 2018

bors merged commit 17c56d4 into rust-lang:master May 12, 2018

added missing implementation hint #50161

added missing implementation hint #50161

Conversation

rizakrko commented Apr 22, 2018

rust-highfive commented Apr 22, 2018

michaelwoerister commented Apr 23, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rizakrko commented Apr 27, 2018 • edited Loading

shepmaster commented May 6, 2018

rizakrko commented May 7, 2018

rizakrko commented May 8, 2018

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rizakrko commented May 8, 2018

estebank commented May 9, 2018

bors commented May 9, 2018

bors commented May 10, 2018

bors commented May 10, 2018

kennytm commented May 10, 2018

bors commented May 10, 2018

estebank commented May 11, 2018

bors commented May 11, 2018

bors commented May 11, 2018

bors commented May 12, 2018

rizakrko commented Apr 27, 2018 •

edited

Loading