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

Point out the known type when field doesn't satisfy bound #38150

Merged
merged 2 commits into from
Dec 21, 2016

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Dec 3, 2016

For file

use std::path::Path;

fn f(p: Path) { }

provide the following error

error[E0277]: the trait bound `[u8]: std::marker::Sized` is not satisfied in `std::path::Path`
 --> file.rs:3:6
  |
3 | fn f(p: Path) { }
  |      ^ within `std::path::Path`, the trait `std::marker::Sized` is not implemented for `[u8]`
  |
  = note: `[u8]` does not have a constant size known at compile-time
  = note: required because it appears within the type `std::path::Path`
  = note: all local variables must have a statically known size

Fix #23286.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

let (post_message, pre_message) =
if let ObligationCauseCode::BuiltinDerivedObligation(ref data)
= obligation.cause.code
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brace should be on the previous line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

For file

```rust
use std::path::Path;

fn f(p: Path) { }
```

provide the following error

```nocode
error[E0277]: the trait bound `[u8]: std::marker::Sized` is not satisfied in `std::path::Path`
 --> file.rs:3:6
  |
3 | fn f(p: Path) { }
  |      ^ within `std::path::Path`, the trait `std::marker::Sized` is not implemented for `[u8]`
  |
  = note: `[u8]` does not have a constant size known at compile-time
  = note: required because it appears within the type `std::path::Path`
  = note: all local variables must have a statically known size
```
let (post_message, pre_message) =
if let ObligationCauseCode::BuiltinDerivedObligation(ref data)
= obligation.cause.code {
let parent_trait_ref = self.resolve_type_vars_if_possible(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing an indent level here.

Copy link
Contributor Author

@estebank estebank Dec 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it? the if let is part of the previous line, the only reason is below is because I couldn't get to fit nicely in under 100 chars. In this were to fit in one line it would be:

let (post_message, pre_message) = if let ObligationCauseCode::BuiltinDerivedObligation(ref data) = obligation.cause.code {

I don't see why then line 493 would have to have double indentation. :)

I agree that it is slightly confusing as it is now, but haven't come up with a better way of presenting this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh indeed, you're absolutely right, my bad!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one better way might be using match :)

let (post_message, pre_message) = match obligation.cause.code {
    ObligationCauseCode::BuiltinDerivedObligation(ref data) => {
        let parent_trait_ref = self.resolve_type_vars_if_possible(
        ...
    }
    _ => {
        (String::new(), String::new())
    }
};

@GuillaumeGomez
Copy link
Member

All good for me. I'm not sure I'm supposed to merge this so I'll wait for others' opinion.

cc @jonathandturner

let (post_message, pre_message) =
if let ObligationCauseCode::BuiltinDerivedObligation(ref data)
= obligation.cause.code {
let parent_trait_ref = self.resolve_type_vars_if_possible(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one better way might be using match :)

let (post_message, pre_message) = match obligation.cause.code {
    ObligationCauseCode::BuiltinDerivedObligation(ref data) => {
        let parent_trait_ref = self.resolve_type_vars_if_possible(
        ...
    }
    _ => {
        (String::new(), String::new())
    }
};

= obligation.cause.code {
let parent_trait_ref = self.resolve_type_vars_if_possible(
&data.parent_trait_ref);
(format!(" in `{}`", parent_trait_ref.0.self_ty()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern I have is that this only goes up one level, but there are similar cases where multiple levels are needed. For example, it'd be nice if this patch addressed this example: https://is.gd/BZjfP6

The current output is

error[E0277]: the trait bound `*const u8: std::marker::Send` is not satisfied
  --> <anon>:16:5
   |
16 |     is_send::<Foo>();
   |     ^^^^^^^^^^^^^^ trait `*const u8: std::marker::Send` not satisfied
   |
   = note: `*const u8` cannot be sent between threads safely
   = note: required because it appears within the type `Baz`
   = note: required because it appears within the type `Bar`
   = note: required because it appears within the type `Foo`
   = note: required by `is_send`

and I think your suggested approach would be a big improvement:

error[E0277]: within `Foo`, the trait bound `*const u8: std::marker::Send` is not satisfied
  --> <anon>:16:5
   |
16 |     is_send::<Foo>();
   |     ^^^^^^^^^^^^^^ trait `*const u8: std::marker::Send` not satisfied
   |
   = note: `*const u8` cannot be sent between threads safely
   = note: required because it appears within the type `Baz`
   = note: required because it appears within the type `Bar`
   = note: required because it appears within the type `Foo`
   = note: required by `is_send`

But I believe your patch as is will print "within Baz" instead.

should be easy enough though -- just have to iterate on the cause chain rather than just going up one level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test for new case and a recursive search up the cause chain.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 19, 2016

📌 Commit 5c13041 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 20, 2016

⌛ Testing commit 5c13041 with merge 27122ab...

@bors
Copy link
Contributor

bors commented Dec 20, 2016

💔 Test failed - auto-win-gnu-64-opt

@alexcrichton
Copy link
Member

alexcrichton commented Dec 20, 2016 via email

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 20, 2016
Point out the known type when field doesn't satisfy bound

For file

```rust
use std::path::Path;

fn f(p: Path) { }
```

provide the following error

```nocode
error[E0277]: the trait bound `[u8]: std::marker::Sized` is not satisfied in `std::path::Path`
 --> file.rs:3:6
  |
3 | fn f(p: Path) { }
  |      ^ within `std::path::Path`, the trait `std::marker::Sized` is not implemented for `[u8]`
  |
  = note: `[u8]` does not have a constant size known at compile-time
  = note: required because it appears within the type `std::path::Path`
  = note: all local variables must have a statically known size
```

Fix rust-lang#23286.
bors added a commit that referenced this pull request Dec 20, 2016
@bors bors merged commit 5c13041 into rust-lang:master Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants