Avoid complex diagnostics in snippets which contain newlines #75020

JohnTitor · 2020-08-01T17:25:46Z

Fixes #70935

r? @estebank @tmandry

JohnTitor · 2020-08-01T17:26:48Z

@estebank said:

I think there is an open PR that adds description to all ExprKind variants.

I guess you meant DefKind?

src/test/ui/async-await/issue-70935-complex-spans.stderr

Dylan-DPC-zz · 2020-08-18T12:33:38Z

@estebank this is ready to review

tmandry · 2020-08-18T20:57:54Z

src/test/ui/async-await/issue-70935-complex-spans.stderr

+LL | |             foo(tx.clone());
+LL | |         }).await;
+   | |________________^
+note: ...but, the value is later dropped here


I don't think ...but is clearer here. To me, it implies that the fact that it is dropped is part of the problem. And in the case of the borrow check errors that sound like this, it is.

Sometimes that's true in that you could drop the value earlier, before the await point. But in both of these cases it's not; the extra note is just information. (And not very useful information in these cases, but that's something we can clean up later.)

Anyway, it might be helpful to represent that this is a continuation of the above note, in which case I'd ask that we keep only the ellipsis, e.g. note: ...the value is later dropped here

tmandry · 2020-08-18T21:04:17Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

+                                    await_or_yield, snippet
+                                ),
+                            );
+                            err.span_note(


Removing the type information makes the error less verbose, but it's quite useful in a lot of cases. If I were to pick one thing to remove, it would be the "Foo is later dropped here" message.

Or we can emit the same set of messages in both cases, in which case I think we could deduplicate a lot of this code.

bors · 2020-08-30T17:52:21Z

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

crlf0710 · 2020-09-18T12:25:59Z

Triage: there are merge conflicts now.

estebank · 2020-08-03T01:54:10Z

src/librustc_trait_selection/traits/error_reporting/suggestions.rs

+                    // LL | /         baz(|| async{
+                    // LL | |             foo(tx.clone());
+                    // LL | |         }).await;
+                    //    | |________________^


These two spans don't seem right. The case we're proactively handling here originally had a span pointing until the end of the await and another pointing at the end of the call if baz.

estebank · 2020-09-19T23:23:16Z

src/test/ui/async-await/issues/issue-65436-raw-ptr-not-send.stderr

+  --> $DIR/issue-65436-raw-ptr-not-send.rs:14:9
+   |
+LL |         bar(Foo(std::ptr::null())).await;
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


Would it make sense to combine these two along the lines of

note: future is not `Send` as this value is used across an await --> $DIR/issue-65436-raw-ptr-not-send.rs:14:9 | LL | bar(Foo(std::ptr::null())).await; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ first, await occurs here, with `std::ptr::null()` maybe used later... note: `std::ptr::null()` is later dropped here --> $DIR/issue-65436-raw-ptr-not-send.rs:14:41 | LL | bar(Foo(std::ptr::null())).await; | ---------------- ^ `std::ptr::null()` is later dropped here | | | has type `*const u8` which is not `Send`

tmandry · 2020-10-06T17:38:52Z

The PR needs a rebase and there are some review comments to address.

Ping @JohnTitor, are you still interested in working on this?

JohnTitor · 2020-10-10T19:49:54Z

Sorry for the delay! Rebased and tweaked some spans and wording.

@estebank @tmandry Could you take a look?

tmandry · 2020-10-21T22:35:11Z

Sorry for the delay on my end.

I still think the new message should include the has type {} which is not {} message that we have in the other case.

Otherwise, I think this looks good.

JohnTitor · 2020-10-22T05:19:39Z

src/test/ui/async-await/issue-70935-complex-spans.stderr

+note: this has type `[closure@$DIR/issue-70935-complex-spans.rs:13:13: 15:10]` which is not `Send`
+  --> $DIR/issue-70935-complex-spans.rs:13:13
+   |
+LL |           baz(|| async{
+   |  _____________^
+LL | |             foo(tx.clone());
+LL | |         }).await;
+   | |_________^


Added this note separately, since this span has some lines and loses readability when we do as @estebank suggested (#75020 (comment)). I feel it's redundant a bit.

@tmandry Does this make sense for you?

@JohnTitor something I've done in other errors i check whether Spans are exactly the same, if one encloses the other, or if one of them is multiline to provide different output depending on what looks best. In this case just checking whether the "inner" span is multiline to decide between span_labels and span_notes should be easy to do and reasonable as a solution. What do you two think?

r=me with or without the proposed changes.

Sounds good! Addressed via 54d9ffc.

tmandry · 2020-10-28T00:52:31Z

@bors delegate+

bors · 2020-10-28T00:52:33Z

✌️ @JohnTitor can now approve this pull request

JohnTitor · 2020-11-02T08:02:24Z

Applied the suggestion from @estebank via 54d9ffc and added 8d65101 to make the prior commit diff-less.
@bors r=estebank,tmandry

bors · 2020-11-02T08:02:25Z

📌 Commit 8d65101 has been approved by estebank,tmandry

bors · 2020-11-02T08:17:41Z

⌛ Testing commit 8d65101 with merge 234099d...

bors · 2020-11-02T10:42:40Z

☀️ Test successful - checks-actions
Approved by: estebank,tmandry
Pushing 234099d to master...

rust-highfive assigned estebank Aug 1, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2020

estebank reviewed Aug 2, 2020

View reviewed changes

src/test/ui/async-await/issue-70935-complex-spans.stderr Outdated Show resolved Hide resolved

tmandry reviewed Aug 18, 2020

View reviewed changes

crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 18, 2020

estebank reviewed Sep 19, 2020

View reviewed changes

JohnTitor force-pushed the fix-multispan branch from db80f28 to aabc1c9 Compare October 10, 2020 19:48

JohnTitor removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 10, 2020

tmandry 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 21, 2020

JohnTitor force-pushed the fix-multispan branch from aabc1c9 to 02b3bb6 Compare October 22, 2020 05:15

JohnTitor commented Oct 22, 2020

View reviewed changes

JohnTitor added 6 commits November 2, 2020 15:53

Avoid complex diagnostics in snippets which contain newlines

c269525

Separate complex multispan into some notes

ad978e5

Address some code reviews

66226ca

Add "this has type {} which {}" note

2da86a1

Only separate notes if span is multiline

54d9ffc

Fix format

8d65101

JohnTitor force-pushed the fix-multispan branch from 02b3bb6 to 8d65101 Compare November 2, 2020 07:59

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 Nov 2, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 2, 2020

bors merged commit 234099d into rust-lang:master Nov 2, 2020

rustbot added this to the 1.49.0 milestone Nov 2, 2020

JohnTitor deleted the fix-multispan branch November 2, 2020 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid complex diagnostics in snippets which contain newlines #75020

Avoid complex diagnostics in snippets which contain newlines #75020

JohnTitor commented Aug 1, 2020

JohnTitor commented Aug 1, 2020

Dylan-DPC-zz commented Aug 18, 2020

tmandry Aug 18, 2020

tmandry Aug 18, 2020

bors commented Aug 30, 2020

crlf0710 commented Sep 18, 2020

estebank Aug 3, 2020

estebank Sep 19, 2020 •

edited

Loading

tmandry commented Oct 6, 2020

JohnTitor commented Oct 10, 2020

tmandry commented Oct 21, 2020

JohnTitor Oct 22, 2020

estebank Oct 26, 2020

estebank Oct 26, 2020

JohnTitor Nov 2, 2020

tmandry commented Oct 28, 2020

bors commented Oct 28, 2020

JohnTitor commented Nov 2, 2020

bors commented Nov 2, 2020

bors commented Nov 2, 2020

bors commented Nov 2, 2020

Avoid complex diagnostics in snippets which contain newlines #75020

Avoid complex diagnostics in snippets which contain newlines #75020

Conversation

JohnTitor commented Aug 1, 2020

JohnTitor commented Aug 1, 2020

Dylan-DPC-zz commented Aug 18, 2020

tmandry Aug 18, 2020

Choose a reason for hiding this comment

tmandry Aug 18, 2020

Choose a reason for hiding this comment

bors commented Aug 30, 2020

crlf0710 commented Sep 18, 2020

estebank Aug 3, 2020

Choose a reason for hiding this comment

estebank Sep 19, 2020 • edited Loading

Choose a reason for hiding this comment

tmandry commented Oct 6, 2020

JohnTitor commented Oct 10, 2020

tmandry commented Oct 21, 2020

JohnTitor Oct 22, 2020

Choose a reason for hiding this comment

estebank Oct 26, 2020

Choose a reason for hiding this comment

estebank Oct 26, 2020

Choose a reason for hiding this comment

JohnTitor Nov 2, 2020

Choose a reason for hiding this comment

tmandry commented Oct 28, 2020

bors commented Oct 28, 2020

JohnTitor commented Nov 2, 2020

bors commented Nov 2, 2020

bors commented Nov 2, 2020

bors commented Nov 2, 2020

estebank Sep 19, 2020 •

edited

Loading