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

core: allow messages in unimplemented!() macro #42155

Merged
merged 1 commit into from
Jun 11, 2017

Conversation

seanmonstar
Copy link
Contributor

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
Copy link
Collaborator

r? @sfackler

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

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2017
@@ -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));
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2017
@alexcrichton
Copy link
Member

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

@seanmonstar
Copy link
Contributor Author

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

@alexcrichton
Copy link
Member

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

@alexcrichton alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 1, 2017
@natboehm
Copy link
Contributor

natboehm commented Jun 5, 2017

Hi @sfackler pinging you on IRC as well

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

sfackler commented Jun 5, 2017

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

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jun 5, 2017

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.

@rfcbot
Copy link

rfcbot commented Jun 7, 2017

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

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jun 7, 2017
@ollie27
Copy link
Member

ollie27 commented Jun 7, 2017

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
Copy link
Member

sfackler commented Jun 7, 2017

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

@seanmonstar
Copy link
Contributor Author

I just updated to use the recommendation from @ollie27.

@Mark-Simulacrum
Copy link
Member

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

@sfackler
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 11, 2017

📌 Commit f517719 has been approved by sfackler

@ollie27
Copy link
Member

ollie27 commented Jun 11, 2017

Surely this needs documentation and tests?

@bors
Copy link
Contributor

bors commented Jun 11, 2017

⌛ Testing commit f517719 with merge e2eaef8...

bors added a commit that referenced this pull request Jun 11, 2017
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)`.
@bors
Copy link
Contributor

bors commented Jun 11, 2017

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

@bors bors merged commit f517719 into rust-lang:master Jun 11, 2017
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 6, 2017
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
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Sep 7, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

10 participants