core: allow messages in unimplemented!() macro

[seanmonstar]

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).

[rust-highfive]

r? @sfackler

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

[ollie27]

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?

[seanmonstar]

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.

[ollie27]

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 {}")
| ^^^^^^^^

[alexcrichton]

ping @seanmonstar just wanted to make sure this stays on your radar!

[seanmonstar]

Oh it's waiting on me? I was waiting on review. What's the action?

[alexcrichton]

Oh sorry in that case I'll retag for review, ping @sfackler in that case!

[natboehm]

Hi @sfackler pinging you on IRC as well

[sfackler]

LGTM - I guess I'll FCP it since it's an insta-stable change though.

@rfcbot fcp merge

[rfcbot]

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @dtolnay
• @sfackler

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.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[ollie27]

Does anyone have an opinion on the issue I brought up above? Specifically should this macro follow the slightly bizarre behaviour of unreachable and let things like unimplemented!(1 + 2) and unimplemented!("foo: {}") compile or just add a simple rule like ($($arg:tt)+) => (panic!("not yet implemented: {}", format_args!($($arg)+))); which would only accept valid format strings?

[sfackler]

I would prefer to think of unreachable's behavior as an accident and prefer only well-formed format args.

[seanmonstar]

I just updated to use the recommendation from @ollie27.

[Mark-Simulacrum]

@sfackler I believe this is good to go, just needs your approval.

[sfackler]

@bors r+

[bors]

📌 Commit f517719 has been approved by sfackler

[ollie27]

Surely this needs documentation and tests?

[bors]

⌛ Testing commit f517719 with merge e2eaef8...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing e2eaef8 to master...