Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hard to read error message with 2D line drawing and multi-line expressions #70935

Closed
steffahn opened this issue Apr 8, 2020 · 3 comments · Fixed by #75020
Closed

Hard to read error message with 2D line drawing and multi-line expressions #70935

steffahn opened this issue Apr 8, 2020 · 3 comments · Fixed by #75020
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. D-papercut Diagnostics: An error or lint that needs small tweaks. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@steffahn
Copy link
Member

steffahn commented Apr 8, 2020

Especially the fact that the lines are broken up seems like a bug.

use core::future::Future;
use futures::future::{BoxFuture, FutureExt};

async fn baz<T>(_c: impl FnMut() -> T) where T: Future<Output=()> {
}

fn foo(tx: std::sync::mpsc::Sender<i32>) -> BoxFuture<'static, ()>{
    async move {
        baz(|| async{
            foo(tx.clone());
        }).await;
    }.boxed()
}

fn bar(_s: impl Future + Send) {
}

fn main() {
    let (tx, _rx) = std::sync::mpsc::channel();
    bar(foo(tx));
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error: future cannot be sent between threads safely
  --> src/main.rs:12:7
   |
12 |     }.boxed()
   |       ^^^^^ future returned by `foo` is not `Send`
   |
   = help: the trait `std::marker::Sync` is not implemented for `std::sync::mpsc::Sender<i32>`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:9:9
   |
9  |            baz(|| async{
   |  __________^___-
   | | _________|
   | ||
10 | ||             foo(tx.clone());
11 | ||         }).await;
   | ||         -      ^- `|| async{
            foo(tx.clone());
        }` is later dropped here
   | ||_________|______|
   | |__________|      await occurs here, with `|| async{
            foo(tx.clone());
        }` maybe used later
   |            has type `[closure@src/main.rs:9:13: 11:10 tx:&std::sync::mpsc::Sender<i32>]`

error: aborting due to previous error

error: could not compile `playground`.

To learn more, run the command again with --verbose.

@rustbot modify labels: A-diagnostics, C-bug, D-papercut, T-compiler

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 8, 2020
@Centril Centril added the A-async-await Area: Async & Await label Apr 8, 2020
@estebank
Copy link
Contributor

There are two things here

  1. We have a label that has embedded newlines which causes the broken down ASCII art
  2. We should find a way to reverse the order in which multiline spans are sorted, but this will require handling multiple labels. The current order is optimized for not having to tweak the label positions, but when no labels are involved, we could do much better.

@tmandry tmandry added AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. P-medium Medium priority labels Apr 21, 2020
@tmandry
Copy link
Member

tmandry commented Apr 21, 2020

So, using raw text from the program as part of our label feels like a hack. We should try to come up with another way of getting a name (and falling back to a description like "async block" or something if that doesn't work). Failing a good way of doing that, I guess we can scan the program text for newlines and only use it if there aren't any..

@estebank
Copy link
Contributor

estebank commented Apr 21, 2020

Agree, we should never be using snippets in labels. The change needs to happen in

let await_span = tables.generator_interior_types.iter().map(|t| t.span).last().unwrap();
let mut span = MultiSpan::from_span(await_span);
span.push_span_label(
await_span,
format!("{} occurs here, with `{}` maybe used later", await_or_yield, snippet),
);
span.push_span_label(
target_span,
format!("has type `{}` which {}", target_ty, trait_explanation),
);
// If available, use the scope span to annotate the drop location.
if let Some(scope_span) = scope_span {
span.push_span_label(
source_map.end_point(*scope_span),
format!("`{}` is later dropped here", snippet),
);
}
err.span_note(
span,
&format!(
"{} {} as this value is used across {}",
future_or_generator, trait_explanation, an_await_or_yield
),
);

You have the HirId which lets you get the Expr and from there use an appropriate description. I think there is an open PR that adds description to all ExprKind variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. D-papercut Diagnostics: An error or lint that needs small tweaks. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants