core: allow messages in unimplemented!() macro #42155
Conversation
|
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
arielb1
added
the
S-waiting-on-review
src/libcore/macros.rs
Outdated
| @@ -542,7 +542,9 @@ macro_rules! unreachable { | |||
| #[macro_export] | |||
| #[stable(feature = "rust1", since = "1.0.0")] | |||
| macro_rules! unimplemented { | |||
| () => (panic!("not yet implemented")) | |||
| () => (panic!("not yet implemented")); | |||
| ($msg:expr) => (unimplemented!("{}", $msg)); | |||
There was a problem hiding this comment.
I know this matches unreachable but does this macro really need to accept anything that implements Display as a single argument? Is it that useful to be able to use unimplemented!(x) rather than unimplemented!("{}", x) given that it means unimplemented!("Conn::poll; state={:?}") would compile when it arguably shouldn't?
There was a problem hiding this comment.
This like could be changed to this: panic!(format_args!(concat!("not yet implemented: ", $msg))), and it would catch unimplemented!("foo {}"). The error message it prints:
error: invalid reference to argument `0` (no arguments given)
--> <anon>:6:29
|
6 | panic!(format_args!(concat!("not yet implemented: ", $msg)))
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
13 | unimplemented!("foo {}")
| ----------------- in this macro invocation
It's not super clear, but it would catch that mistake. However, it differs from unreachable, and even panic.
There was a problem hiding this comment.
Well if you only want to accept valid format strings, you could replace this line and the next one with ($($arg:tt)+) => (panic!("not yet implemented: {}", format_args!($($arg)+))); which gives the same error message as other macros accepting format strings:
error: invalid reference to argument `0` (no arguments given)
--> <anon>:7:20
|
7 | unimplemented!("foo {}")
| ^^^^^^^^
Mark-Simulacrum
added
S-waiting-on-author
|
ping @seanmonstar just wanted to make sure this stays on your radar! |
|
Oh it's waiting on me? I was waiting on review. What's the action? |
|
Oh sorry in that case I'll retag for review, ping @sfackler in that case! |
alexcrichton
added
S-waiting-on-review
|
Hi @sfackler pinging you on IRC as well |
sfackler
added
the
T-libs-api
|
LGTM - I guess I'll FCP it since it's an insta-stable change though. @rfcbot fcp merge |
|
Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
the
final-comment-period
|
Does anyone have an opinion on the issue I brought up above? Specifically should this macro follow the slightly bizarre behaviour of |
|
I would prefer to think of unreachable's behavior as an accident and prefer only well-formed format args. |
seanmonstar
force-pushed
the
unimplemented
branch
from
dfeed1c
to
f517719
Compare
|
I just updated to use the recommendation from @ollie27. |
|
@sfackler I believe this is good to go, just needs your approval. |
|
@bors r+ |
|
📌 Commit f517719 has been approved by |
|
Surely this needs documentation and tests? |
core: allow messages in unimplemented!() macro
This makes `unimplemented!()` match `unreachable!()`, allowing a message and possible formatting to be provided to better explain what and/or why something is not implemented.
I've used this myself in hyper for a while, include the type and method name, to better help while prototyping new modules, like `unimplemented!("Conn::poll_complete")`, or `unimplemented!("Conn::poll; state={:?}", state)`.
|
☀️ Test successful - status-appveyor, status-travis |
update unimplemented! docs For rust-lang#42628 (updating docs from changes from rust-lang#42155). Initial changes made to make `unimplemented!` doc comments look more like `unreachable!` and remove statement about the panic message. r? @steveklabnik
update unimplemented! docs For rust-lang#42628 (updating docs from changes from rust-lang#42155). Initial changes made to make `unimplemented!` doc comments look more like `unreachable!` and remove statement about the panic message. r? @steveklabnik
This makes
unimplemented!()matchunreachable!(), allowing a message and possible formatting to be provided to better explain what and/or why something is not implemented.I've used this myself in hyper for a while, include the type and method name, to better help while prototyping new modules, like
unimplemented!("Conn::poll_complete"), orunimplemented!("Conn::poll; state={:?}", state).