For E0277 on `for` loops, point at the "head" expression #47690

estebank · 2018-01-23T23:01:04Z

When E0277's span points at a for loop, the actual issue is in the
element being iterated. Instead of pointing at the entire loop, point
only at the first line (when possible) so that the span ends in the
element for which E0277 was triggered.

rust-highfive · 2018-01-23T23:01:07Z

r? @eddyb

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

eddyb · 2018-01-23T23:04:21Z

r? @nikomatsakis

zackmdavis · 2018-01-24T00:22:18Z

src/librustc/traits/error_reporting.rs

@@ -551,6 +551,24 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                        let OnUnimplementedNote { message, label }
                            = self.on_unimplemented_note(trait_ref, obligation);
                        let have_alt_message = message.is_some() || label.is_some();
+                        let span = match self.tcx.sess.codemap().span_to_snippet(span) {
+                            Ok(ref _s) if _s.starts_with("for ") => {


style nit: Ok(ref s) if s.starts_with("for ") (pretty sure the unused-variables lint is in fact smart enough to count the if guard as a use)

For some reason I was convinced that there was a lint that complained at patterns that where only used in match guards... Must have been a different case.

You might be thinking of how the dead-code lint complains about enum variants that are never constructed, even if they're matched on?

When E0277's span points at a `for` loop, the actual issue is in the element being iterated. Instead of pointing at the entire loop, point only at the first line (when possible) so that the span ends in the element for which E0277 was triggered.

nikomatsakis

I like the span but feel like we could do it in a more elegant way...

Also, what if people do something like

for x in
    something
{
    ...
}

I do that from time to time...

nikomatsakis · 2018-01-24T17:39:53Z

src/librustc/traits/error_reporting.rs

@@ -551,6 +551,24 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
                        let OnUnimplementedNote { message, label }
                            = self.on_unimplemented_note(trait_ref, obligation);
                        let have_alt_message = message.is_some() || label.is_some();
+                        let span = match self.tcx.sess.codemap().span_to_snippet(span) {
+                            Ok(ref s) if s.starts_with("for ") => {
+                                // On for loops, this error is caused by the element being iterated


Hmm, I like this span a lot, but I wonder if this is the right way to do this. Maybe we should adjust the span in HIR lowering instead? Roughly here:

rust/src/librustc/hir/lowering.rs

Lines 3167 to 3173 in a0dcecf

// `match ::std::iter::IntoIterator::into_iter(<head>) { ... }`

let into_iter_expr = {

let into_iter_path = &["iter", "IntoIterator", "into_iter"];

let into_iter = P(self.expr_std_path(e.span, into_iter_path,

ThinVec::new()));

P(self.expr_call(e.span, into_iter, hir_vec![head]))

};

Reasonable, it's definitely an overuse of span_until_char, it was just slightly faster to figure how to do this than the proper way. Should it point at only the iterator expression or at the entire for _ in iterator_expr?

@estebank good question, I don't know the answer. Just pointing at the span of the iterator expression is obviously easier... and maybe better? I guess that showing the for may help convey that it's "because of the for" that IntoIterator is needed... but that seems sort of obvious...

If we do it in lowering, we can also introduce a "synthetic" span, right, that would let us give a very tailored message if we wanted?

If we do it in lowering, we can also introduce a "synthetic" span, right, that would let us give a very tailored message if we wanted?

I think so. If we want to point at the for, we could just do that, which would look better ni the case where for and expr are not in the same line:

X | for _ in expr { | --- ^^^^ `&str` is not an iterator | | | needed because of this `for` loop

oooh, that's nice

Modified the expansion code to use the appropriate span, but that makes it harder to add the appropriate span under for.

nikomatsakis · 2018-01-26T15:13:35Z

src/librustc/hir/lowering.rs

@@ -2969,7 +2969,7 @@ impl<'a> LoweringContext<'a> {
                    let body = if let Some(else_expr) = wildcard_arm {
                        P(self.lower_expr(else_expr))
                    } else {
-                        self.expr_tuple(e.span, hir_vec![])
+                        self.expr_tuple(body.span, hir_vec![])


Why the change to body.span here? It's not entirely intuitive to me that the (synthetic) "else clause" should get the span of the code that executes when else does not apply. =) At minimum, a comment seems good.

nikomatsakis · 2018-01-26T15:15:36Z

src/test/ui/const-fn-error.stderr

+17 |     for i in 0..x { //~ ERROR calls in constant functions
+   |              ^^^^
+   |              |
+   |              in this macro invocation


macro invocation? what macro? =)

Is this referring to the for loop desugaring?

Fixed, it was macro desugaring been confused with macro invocations in emitter.rs.

nikomatsakis · 2018-01-26T15:15:55Z

src/test/ui/suggestions/for-c-in-str.rs

+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// E0277 should point exclusively at line 14, not the entire for loop span


Modify the spans used for `for`-loop expression expansion, instead of creating a new span during error creation.

nikomatsakis

this turned out really good

nikomatsakis · 2018-01-26T22:04:50Z

src/test/ui/suggestions/try-operator-on-main.stderr

-   |     |
-   |     cannot use the `?` operator in a function that returns `()`
-   |     in this macro invocation
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot use the `?` operator in a function that returns `()`


nice drive-by fix here 🚗

nikomatsakis · 2018-01-26T22:05:15Z

src/test/ui/suggestions/for-c-in-str.stderr

+   |              ^^^^^^ `&str` is not an iterator; maybe try calling `.iter()` or a similar method
+   |
+   = help: the trait `std::iter::Iterator` is not implemented for `&str`
+   = note: required by `std::iter::IntoIterator::into_iter`


it'd be cool if we could highlight the for, but this is indeed pretty good still =)

I agree. That will require some extra changes to report_selection_error in order to figure out that this is part of a for-loop desugaring in order to find the leading for. For now I think this is enough of an improvement.

nikomatsakis · 2018-01-26T22:05:41Z

@bors r+

bors · 2018-01-26T22:05:41Z

📌 Commit 106e5c5 has been approved by nikomatsakis

bors · 2018-01-27T08:04:18Z

⌛ Testing commit 106e5c5 with merge 6272b60...

For E0277 on `for` loops, point at the "head" expression When E0277's span points at a `for` loop, the actual issue is in the element being iterated. Instead of pointing at the entire loop, point only at the first line (when possible) so that the span ends in the element for which E0277 was triggered.

bors · 2018-01-27T10:56:51Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 6272b60 to master...

rust-highfive assigned eddyb Jan 23, 2018

estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 23, 2018

rust-highfive assigned nikomatsakis and unassigned eddyb Jan 23, 2018

zackmdavis reviewed Jan 24, 2018

View reviewed changes

estebank force-pushed the for-block-277 branch from 1bf1a76 to 3a530ba Compare January 24, 2018 01:05

nikomatsakis requested changes Jan 24, 2018

View reviewed changes

nikomatsakis 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 Jan 25, 2018

estebank 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 Jan 25, 2018

estebank changed the title ~~For E0277 on for loops, point at first line~~ For E0277 on for loops, point at the "head" expression Jan 26, 2018

estebank force-pushed the for-block-277 branch 2 times, most recently from db7be26 to 9fc426e Compare January 26, 2018 08:41

nikomatsakis reviewed Jan 26, 2018

View reviewed changes

Modify spans of expanded expression

f90c445

Modify the spans used for `for`-loop expression expansion, instead of creating a new span during error creation.

estebank force-pushed the for-block-277 branch from 9fc426e to f90c445 Compare January 26, 2018 19:05

Don't add "in this macro invocation" label to desugared spans

106e5c5

nikomatsakis approved these changes Jan 26, 2018

View reviewed changes

nikomatsakis 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 Jan 26, 2018

bors merged commit 106e5c5 into rust-lang:master Jan 27, 2018

Mark-Simulacrum mentioned this pull request Feb 12, 2018

Compiler slowdown on lage module in nightly #48153

Closed

estebank deleted the for-block-277 branch November 9, 2023 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For E0277 on `for` loops, point at the "head" expression #47690

For E0277 on `for` loops, point at the "head" expression #47690

estebank commented Jan 23, 2018

rust-highfive commented Jan 23, 2018

eddyb commented Jan 23, 2018

zackmdavis Jan 24, 2018

estebank Jan 24, 2018

zackmdavis Jan 24, 2018

nikomatsakis left a comment

nikomatsakis Jan 24, 2018

estebank Jan 24, 2018

nikomatsakis Jan 24, 2018

estebank Jan 24, 2018

nikomatsakis Jan 24, 2018

estebank Jan 25, 2018

nikomatsakis Jan 26, 2018

estebank Jan 26, 2018

nikomatsakis Jan 26, 2018

estebank Jan 26, 2018 •

edited

Loading

nikomatsakis Jan 26, 2018

nikomatsakis left a comment

nikomatsakis Jan 26, 2018

estebank Jan 26, 2018

nikomatsakis Jan 26, 2018

estebank Jan 26, 2018

nikomatsakis commented Jan 26, 2018

bors commented Jan 26, 2018

bors commented Jan 27, 2018

bors commented Jan 27, 2018

	// `match ::std::iter::IntoIterator::into_iter(<head>) { ... }`
	let into_iter_expr = {
	let into_iter_path = &["iter", "IntoIterator", "into_iter"];
	let into_iter = P(self.expr_std_path(e.span, into_iter_path,
	ThinVec::new()));
	P(self.expr_call(e.span, into_iter, hir_vec![head]))
	};

For E0277 on for loops, point at the "head" expression #47690

For E0277 on for loops, point at the "head" expression #47690

Conversation

estebank commented Jan 23, 2018

rust-highfive commented Jan 23, 2018

eddyb commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis 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

estebank Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis 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

nikomatsakis commented Jan 26, 2018

bors commented Jan 26, 2018

bors commented Jan 27, 2018

bors commented Jan 27, 2018

For E0277 on `for` loops, point at the "head" expression #47690

For E0277 on `for` loops, point at the "head" expression #47690

estebank Jan 26, 2018 •

edited

Loading