Add filtering option to `rustc_on_unimplemented` and reword `Iterator` E0277 errors #54946

estebank · 2018-10-10T02:04:41Z

Add more targetting filters for arrays to rustc_on_unimplemented (Fix Add filter to rustc_on_unimplemented for Self: array #53766)
Detect one element array of Range type, which is potentially a typo:
for _ in [0..10] {} where iterating between 0 and 10 was intended.
(Fix Add a special case error for [Range<T>; 1] #23141)
Suggest .bytes() and .chars() for String.
Suggest borrowing or .iter() on arrays (Fix Better suggestion when attempting to iterate over array #36391)
Suggest using range literal when iterating on integers (Fix Suggest using ranges in loops on primitive types, instead of calling .iter() #34353)
Do not suggest .iter() by default (Fix Don't suggest calling .iter() when unsatified Iterator bound does not originate from an expression #50773, fix Wrong suggestion for struct's type-param that is not iter::Iterator but should #46806)
Add regression test (Fix ICE: assertion failed: leak_check(infcx, &skol_map, snapshot).is_ok() #22872)

- Detect one element array of `Range` type, which is potentially a typo: `for _ in [0..10] {}` where iterating between `0` and `10` was intended. (rust-lang#23141) - Suggest `.bytes()` and `.chars()` for `String`. - Suggest borrowing or `.iter()` on arrays (rust-lang#36391) - Suggest using range literal when iterating on integers (rust-lang#34353) - Do not suggest `.iter()` by default (rust-lang#50773, rust-lang#46806)

rust-highfive · 2018-10-10T02:04:44Z

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2018-10-10T02:06:55Z

CC @csmoe I think this covers all that #52178 did

varkor

These diagnostic changes are a big improvement!

It'd be nice if we could get rid of the false positives (e.g. the "if you want to iterate between 0 until a value end" message), but it's hard to tell how often they'd come up in practice, so if that's awkward to support, I'd probably be okay with leaving it how it is: I think it probably helps more than it confuses.

src/libcore/iter/iterator.rs

src/librustc/traits/error_reporting.rs

varkor · 2018-10-11T16:54:59Z

src/test/ui/issues/issue-50480.stderr

   |
   = help: the trait `std::iter::Iterator` is not implemented for `i32`
+   = note: if you want to iterate between `0` until a value `end`, use the range syntax: `0..end`


This doesn't seem perfect: it's specific to loops, but I imagine people writing <{integer} as Iterator> is so uncommon that this isn't really a problem.

That was my thinking.

This would also be fixed by marking all the spans part of a for loop as being part of a desugaring context, which we can filter on in rustc_on_unimplemented and would improve the situation for many of this diagnostics that only apply when using a binding and not in type signatures.

Ah, that would be good!

varkor · 2018-10-11T16:57:49Z

src/test/ui/iterators/array-of-ranges.stderr

+  --> $DIR/array-of-ranges.rs:7:14
+   |
+LL |     for _ in array_of_range {}
+   |              ^^^^^^^^^^^^^^ if you meant to iterate between two values, remove the square brackets


This isn't ideal. Any way we could only make this happen for the literal syntax?

We would have to change rustc_on_unimplemented. I've tried to "poison" the span for the iterator with a "desugaring context", but was unable to get it working. It should be possible to add it, but given that the likelihood of this case in particular happening is so slim (you have to 1. create an array of ranges, already uncommon and 2. try to iterate over the array), I felt confident leaving it as is (at least for now).

That's true; it could always be a follow-up.

src/libcore/iter/iterator.rs

- reword messages - apply custom comments to all types of ranges - fix indentation

varkor · 2018-10-11T19:52:20Z

src/libcore/iter/iterator.rs

+    ),
+    on(
+        _Self="[std::ops::RangeTo<Idx>; 1]",
+        label="if you meant to iterate until a value, remove the square brackets",


You can't iterate over a RangeTo, so I'm not sure this one is necessary.

You're absolutely right, I just never had thought about it enough to look it up. I will still add a note to it in order to let users know that.

varkor · 2018-10-11T19:52:47Z

src/libcore/iter/iterator.rs

+    ),
+    on(
+        _Self="[std::ops::RangeToInclusive<Idx>; 1]",
+        label="if you meant to iterate until a value, remove the square brackets",


Same with RangeToInclusive.

varkor · 2018-10-11T19:55:12Z

r=me after the last couple of comments, if Travis passes.

estebank · 2018-10-11T21:56:20Z

@bors r=varkor

bors · 2018-10-11T21:56:21Z

📌 Commit 5beeb53 has been approved by varkor

bors · 2018-10-13T03:30:52Z

⌛ Testing commit 5beeb53 with merge 73008786a90f5da800834e1540ca6521d8b7c7ba...

bors · 2018-10-13T04:22:22Z

💔 Test failed - status-travis

estebank · 2018-10-15T17:13:05Z

@bors try

Add filtering option to `rustc_on_unimplemented` and reword `Iterator` E0277 errors - Add more targetting filters for arrays to `rustc_on_unimplemented` (Fix #53766) - Detect one element array of `Range` type, which is potentially a typo: `for _ in [0..10] {}` where iterating between `0` and `10` was intended. (Fix #23141) - Suggest `.bytes()` and `.chars()` for `String`. - Suggest borrowing or `.iter()` on arrays (Fix #36391) - Suggest using range literal when iterating on integers (Fix #34353) - Do not suggest `.iter()` by default (Fix #50773, fix #46806) - Add regression test (Fix #22872)

bors · 2018-10-15T20:14:42Z

☀️ Test successful - status-travis
State: approved= try=True

estebank · 2018-10-15T20:27:36Z

@bors r=varkor

bors · 2018-10-15T20:27:37Z

📌 Commit f5cc6b5 has been approved by varkor

bors · 2018-10-16T01:16:19Z

⌛ Testing commit f5cc6b5 with merge d502144fd36634a938d73bf1893580d420194443...

estebank · 2018-10-16T17:29:10Z

src/librustc/traits/error_reporting.rs

+                    Some(format!("[{}]", self.tcx.type_of(def.did).to_string())),
+                ));
+                if let Some(len) = len.val.try_to_scalar().and_then(|scalar| {
+                    scalar.to_i64().ok()


Hi @oli-obk!

Could you take a look at how I'm using Scalar::to_i64 here? In travis I'm getting the following test error, and looking at the interface of Scalar I don't see a safe way of figuring wether the value is 1 without tripping the assert on some platforms. I need to check wether len.val is 1 (at the very least) or the actual value (more flexibility, but not needed now).

[00:46:27] thread 'main' panicked at 'assertion failed: `(left == right)` [00:46:27] left: `8`, [00:46:27] right: `4`', librustc/mir/interpret/value.rs:242:17 [00:46:27] note: Run with `RUST_BACKTRACE=1` for a backtrace. [00:46:27] [00:46:27] error: internal compiler error: unexpected panic

You can use scalar.to_usize(self.tcx).ok() which actually takes the target platform's usize size and should thus work on all platforms

Thanks! I didn't realize that TyCtxt implemented HasDataLayout.

estebank · 2018-10-16T18:06:14Z

@bors r=varkor

bors · 2018-10-16T18:06:15Z

📌 Commit def0f54 has been approved by varkor

bors · 2018-10-17T11:48:00Z

⌛ Testing commit def0f54 with merge 1dceadd...

Add filtering option to `rustc_on_unimplemented` and reword `Iterator` E0277 errors - Add more targetting filters for arrays to `rustc_on_unimplemented` (Fix #53766) - Detect one element array of `Range` type, which is potentially a typo: `for _ in [0..10] {}` where iterating between `0` and `10` was intended. (Fix #23141) - Suggest `.bytes()` and `.chars()` for `String`. - Suggest borrowing or `.iter()` on arrays (Fix #36391) - Suggest using range literal when iterating on integers (Fix #34353) - Do not suggest `.iter()` by default (Fix #50773, fix #46806) - Add regression test (Fix #22872)

bors · 2018-10-17T14:38:21Z

☀️ Test successful - status-appveyor, status-travis
Approved by: varkor
Pushing 1dceadd to master...

clarfonthey · 2018-10-17T14:44:24Z

Late but... I think this just replaced several clippy lints with just one diagnostic change.

estebank added 4 commits October 9, 2018 18:44

Add more targetting filters for arrays to rustc_on_unimplemented

cd7c818

Add regression test (rust-lang#22872)

330b7ed

Add tests for handled cases

2305d02

rust-highfive assigned varkor Oct 10, 2018

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

This comment has been minimized.

Sign in to view

fix tidy

a0fd68b

varkor suggested changes Oct 11, 2018

View reviewed changes

review comments

c71228e

- reword messages - apply custom comments to all types of ranges - fix indentation

varkor reviewed Oct 11, 2018

View reviewed changes

Reword Range*/[Range*]: Iterator E0277 messages

5beeb53

estebank mentioned this pull request Oct 11, 2018

Add filtering to rustc_on_unimplemented to avoid misleading suggestion #55000

Open

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 Oct 11, 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 Oct 13, 2018

This comment has been minimized.

Sign in to view

kennytm 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 Oct 13, 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 15, 2018

This comment has been minimized.

Sign in to view

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 Oct 16, 2018

kennytm 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 Oct 16, 2018

estebank commented Oct 16, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

Change Scalar to numeric cast

def0f54

estebank force-pushed the iterator branch from f5cc6b5 to def0f54 Compare October 16, 2018 18:04

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 16, 2018

bors merged commit def0f54 into rust-lang:master Oct 17, 2018

estebank mentioned this pull request Oct 17, 2018

"maybe try calling .iter()" on missing trait implementation #55159

Closed

estebank mentioned this pull request Oct 2, 2019

Extend rustc_on_unimplemented to allow pointing at enclosing function/closure #61709

Closed

estebank deleted the iterator branch November 9, 2023 05:22

Add filtering option to rustc_on_unimplemented and reword Iterator E0277 errors #54946

Add filtering option to rustc_on_unimplemented and reword Iterator E0277 errors #54946

Conversation

estebank commented Oct 10, 2018 • edited by varkor Loading

rust-highfive commented Oct 10, 2018

estebank commented Oct 10, 2018

This comment has been minimized.

varkor 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

varkor commented Oct 11, 2018

estebank commented Oct 11, 2018

bors commented Oct 11, 2018

bors commented Oct 13, 2018

bors commented Oct 13, 2018

This comment has been minimized.

estebank commented Oct 15, 2018

bors commented Oct 15, 2018

estebank commented Oct 15, 2018

bors commented Oct 15, 2018

bors commented Oct 16, 2018

This comment has been minimized.

This comment has been minimized.

estebank Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

estebank commented Oct 16, 2018

bors commented Oct 16, 2018

bors commented Oct 17, 2018

bors commented Oct 17, 2018

clarfonthey commented Oct 17, 2018

Add filtering option to `rustc_on_unimplemented` and reword `Iterator` E0277 errors #54946

Add filtering option to `rustc_on_unimplemented` and reword `Iterator` E0277 errors #54946

estebank commented Oct 10, 2018 •

edited by varkor

Loading

estebank Oct 16, 2018 •

edited

Loading