Skip to content

Conversation

dpc
Copy link
Contributor

@dpc dpc commented May 17, 2025

This is something that has been bothering me for years, which (naively) seems simple to fix.

The message passed to .expect(msg) is printed in (at least potentially) confusing way to the user:

let x: Option<u32> = None;
x.expect("valid x value");

// thread 'main' panicked at ...:
// valid x value

IMO, it would be much better if it got reported as:

// thread 'main' panicked at ...:
// expected: valid x value

Then, no matter how the message is formulated, it
should be clear that it was the expectation, not the failing condition itself.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 17, 2025
P: FnOnce(&mut T) -> bool,
{
if self.as_mut().map_or(false, predicate) { self.take() } else { None }
if self.as_mut().map_or(false, predicate) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. My automatic formatting seems to have different opinions. TODO: clean up

@dpc dpc force-pushed the dpc/jj-ksyuprkwynnu branch from b186cd7 to b6e7fdf Compare May 17, 2025 18:10
@rust-log-analyzer

This comment has been minimized.

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented May 17, 2025

Unfortunately the clearer panic message for the expect("valid X") style comes at the expense of the “expect as error message” style discussed in the error module docs. If I understand correctly, the PR would display the example given there as

…
expected: env variable `IMPORTANT_PATH` is not set

While the docs give reasons to prefer the “expect as precondition” style, it also says the other error message is common as well, and I have no reason to doubt that. (Also, if everyone took that advice to heart, we wouldn't have expect("valid X") style either.)

This is something that has been bothering me for years,
which (naively) seems simple to fix.

The message passed to `.expect(msg)` is printed in
(at least potentially) confusing way to the user:

```
let x: Option<u32> = None;
x.expect("valid x value");

// thread 'main' panicked at ...:
// valid x value
```

IMO, it would be much better if it got reported as:

```
// thread 'main' panicked at ...:
// expected: valid x value
```

Then, no matter how the message is formulated, it
should be clear that it was the *expectation*, not
the failing condition itself.
@dpc dpc force-pushed the dpc/jj-ksyuprkwynnu branch from b6e7fdf to 1cec012 Compare May 17, 2025 20:53
@dpc
Copy link
Contributor Author

dpc commented May 17, 2025

Due to current panic-in-const-context limitations, I don't think it's doable ATM.

@dpc dpc closed this May 17, 2025
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
[RUSTC-TIMING] build_script_build test:false 0.289
error: argument to `panic!()` in a const context must have type `&str`
    --> library/core/src/option.rs:2067:5
     |
2067 |     panic_display(&MsgWithPrefix { prefix, msg })
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[RUSTC-TIMING] core test:false 7.090
error: could not compile `core` (lib) due to 1 previous error
Build completed unsuccessfully in 0:00:40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants