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

Pretty print error value on Result.unwrap() panic #34320

Closed
wants to merge 1 commit into from

Conversation

liigo
Copy link
Contributor

@liigo liigo commented Jun 17, 2016

Old (one long line, hard to read):

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 2, message: "No such file or directory" } }', ../src/libcore/result.rs:746

New (pretty readable):

thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Error {
    repr: Os { 
        code: 2, 
        message: "No such file or directory" 
    } 
}', ../src/libcore/result.rs:746

Old (one long line, hard to read):
```
thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 2, message: "No such file or directory" } }', ../src/libcore/result.rs:746
```

New (pretty readable):
```
thread '<main>' panicked at 'called `Result::unwrap()` on an `Err` value: Error {
    repr: Os { 
        code: 2, 
        message: "No such file or directory" 
    } 
}', ../src/libcore/result.rs:746
```
@rust-highfive
Copy link
Collaborator

r? @aturon

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

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 17, 2016
@alexcrichton
Copy link
Member

This change has the likelihood of being a relatively significant breaking change, and we've had discussion on topics like this before which concluded that output on multiple lines has the likelihood of becoming garbled with multiple threads in play.

cc @rust-lang/libs

@liigo
Copy link
Contributor Author

liigo commented Jun 19, 2016

This info is mainly for develop/debug purpose, almostly not occurs in productions. Maybe running crater to find if it really break anything?

I treat this as implementation detail. Didn't change any stable interface, or semantic.

It's unfortunate under multiple threads scenes. Maybe we can prefix tid on every line? Anyway, it did panicked, something imperfect is acceptable, at least for me.

@alexcrichton
Copy link
Member

Right yeah I understand that it's intended for debugging, but this isn't something that exclusively shows up when debugging. This change will affect production logs and such. Unfortunately crater would not turn up any regressions that this would cause.

This is somewhat of an implementation detail, but the fact of the matter is that it may cause breakage regardless if we start spreading out information on multiple lines.

@brson
Copy link
Contributor

brson commented Jun 20, 2016

In light of previous similar discussions I think it's unlikely we would do this. Perhaps this kind of behavior can be enabled in a panic handler and published as a crate?

@aturon
Copy link
Member

aturon commented Jun 21, 2016

Curious to get @sfackler's perspective in particular -- but I'm inclined to agree with @alexcrichton that this is a pretty risky behavioral change to make, sadly.

@liigo
Copy link
Contributor Author

liigo commented Jun 22, 2016

per discussions above, I'm closing this pr.

Perhaps we have chance to do this by adding Result::unwrap2() (or other great name) in the future?

@liigo liigo closed this Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants