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

Add eprint! and eprintln! macros to the prelude. #41192

Merged
merged 4 commits into from
May 11, 2017
Merged

Conversation

zackw
Copy link
Contributor

@zackw zackw commented Apr 10, 2017

These are exactly the same as print! and println! except that they write to stderr instead of stdout. Issues #39228 and #40528; previous PR #39229; accepted RFC rust-lang/rfcs#1869; proposed revision to The Book rust-lang/book#615.

I have not revised this any since the original submission; I will do that later this week. I wanted to get this PR in place since it's been quite a while since the RFC was merged.

Known outstanding review comments:

  • @steveklabnik requested a new chapter for the unstable version of The Book -- please see if the proposed revisions to the second edition cover it.
  • @nodakai asked if it were possible to merge the internal methods _print and _eprint - not completely, since they both refer to different internal globals which we don't want to expose, but I will see if some duplication can be factored out.

Please let me know if I missed anything.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

/// Macro for printing to the standard error.
///
/// Equivalent to the `print!` macro, except that output goes to
/// `io::stderr()` instead of `io::stdout()`. See `print!` for
Copy link
Member

Choose a reason for hiding this comment

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

we don't use () after function names

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

///
/// # Panics
///
/// Panics if writing to `io::stderr()` fails.
Copy link
Member

Choose a reason for hiding this comment

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

in general, it'd be good to link up all the stuff here:

  • print!
  • io::stderr
  • io::stdout
  • io::stderr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you be more specific? I don't understand what you have in mind, besides some degree of cross-reference hyperlinks.

///
/// # Panics
///
/// Panics if writing to `io::stderr()` fails.
Copy link
Member

Choose a reason for hiding this comment

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

same comments here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also fixed

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 11, 2017
// 2. If the local stderr is currently in use (e.g. we're in the middle of
// already printing) then accessing again would cause a panic.
//
// If, however, the actual I/O causes an error, we do indeed panic.
Copy link
Member

Choose a reason for hiding this comment

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

Instead of line-for-line copying the comment above in _print, can this and _print bove either share an implementation or reduce duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the implementation can be factored out.

@alexcrichton
Copy link
Member

@bors: r+

Thanks @zackw!

@bors
Copy link
Contributor

bors commented Apr 13, 2017

📌 Commit 1d00098 has been approved by alexcrichton

@@ -290,6 +290,8 @@ pub use self::util::{copy, sink, Sink, empty, Empty, repeat, Repeat};
pub use self::stdio::{stdin, stdout, stderr, _print, Stdin, Stdout, Stderr};
#[stable(feature = "rust1", since = "1.0.0")]
pub use self::stdio::{StdoutLock, StderrLock, StdinLock};
#[unstable(feature = "eprint", issue="40528")]
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be feature = "eprint_internal".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that is incorrect. See how _print, immediately above, is marked stable, not unstable(print_internal)?

Copy link
Member

Choose a reason for hiding this comment

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

Well strictly speaking the import for _print shouldn't be marked stable but I don't think stability attributes on imports have any effect anyway so I guess it doesn't really matter.

Copy link
Member

Choose a reason for hiding this comment

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

Stability on pub use is ignored, it's just advisory for readers.

///
/// Panics if writing to `io::stderr` fails.
#[macro_export]
#[unstable(feature = "eprint", issue="40528")]
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure stability attributes don't apply the macro_rules macros so these should be stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dunno if the compiler can enforce it, but it seems worthwhile for documentation reasons anyway?

Copy link
Member

Choose a reason for hiding this comment

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

If you can use these marcos in stable rust then why would you want the documentation to say otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

@zackw the test you added has #![feature(eprint)], is that not necessary? (I assumed it was)

If not, let's change this to #[stable] to reflect the (unforutnate) reality.

macro_rules! eprintln {
() => (eprint!("\n"));
($fmt:expr) => (eprint!(concat!($fmt, "\n")));
($fmt:expr, $($arg:tt)*) => (eprint!(concat!($fmt, "\n"), $($arg)*));
Copy link
Member

Choose a reason for hiding this comment

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

This suffers from #30143 just like println and writeln. Maybe the aim was to copy println bug for bug but in case it wasn't, these last two rules can be replaced with ($($arg:tt)*) => (eprint!("{}\n", format_args!($($arg)*))); to get sensible behaviour.

Copy link
Contributor Author

@zackw zackw Apr 13, 2017

Choose a reason for hiding this comment

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

The aim is, in fact, bug-for-bug equivalence with println. If this is to be changed, it should be changed via #30143, for all of the printing macros at once.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree. The reason println and writeln were not fixed was to avoid breaking existing programs. That isn't a concern here.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for pointing this out @ollie27, @zackw I agree that we should fix this bug while we can, it's always backwards compatible to take the currently implemented strategy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ollie27's suggested change to the macro does work, but I am surprised that it works - it appears to be a type error. I am reluctant to do something that may only work by accident. It also appears to have double formatting overhead, which is not desirable. (It is disappointing that macro_rules! macros have no direct way to specify that a particular token tree should be a string literal. Freaking CPP can do this.)

But more importantly, I really do think eprintln should be bug-for-bug compatible with println, because people should be able to go through existing programs and change println to eprintln where appropriate, without thinking about it apart from "should this properly go to stderr?" Again, if this is to be changed, it should be changed for both println and eprintln at the same time.

@alexcrichton
Copy link
Member

@bors: r-

( active comments )

@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017
@bors
Copy link
Contributor

bors commented Apr 20, 2017

☔ The latest upstream changes (presumably #41411) made this pull request unmergeable. Please resolve the merge conflicts.

@zackw
Copy link
Contributor Author

zackw commented Apr 20, 2017

Fixed up the merge conflicts and the stability annotations. For now, have not changed the eprintln! macro - see response to @ollie27 in "outdated review comments" above.

@alexcrichton
Copy link
Member

I still personally feel that we should fix #30143 if we can for eprint/eprintln. @ollie27's suggestion works and I believe not accidentally so, so it should be fine to insert. I also don't really think performance is much of an issue here, eprintln! is doing unbuffered writes to stderr which is far more costly than one extra virtual dispatch.

I also disagree that this should have the same bug to make transitioning easier. It's highly unlikely to exhibit #30143 anyway and it's an opportunity to make it more idiomatic Rust by using a format string.

@bors
Copy link
Contributor

bors commented Apr 23, 2017

☔ The latest upstream changes (presumably #41437) made this pull request unmergeable. Please resolve the merge conflicts.

@arielb1 arielb1 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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-author Status: This is awaiting some action (such as code changes or more information) from the author. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2017
@arielb1
Copy link
Contributor

arielb1 commented Apr 25, 2017

@zackw & @alexcrichton - is there a decision on the #30143 question?

@alexcrichton
Copy link
Member

@zackw do you feel firmly this should be bug-for-bug compatible? If so I can bring this up with the libs team to see what they think as well.

@zackw
Copy link
Contributor Author

zackw commented Apr 25, 2017

@alexcrichton

It is my considered opinion that either we should implement the bug in eprintln!, or we should fix the bug in println! immediately -- not waiting for 2.0. My logic is thus:

If we believe that the bug in println! cannot be fixed before 2.0, then we believe that there are a nontrivial number of existing programs that depend on the bug. Some unknown fraction of the println! calls in those programs should be calls to eprintln!. People going through those programs to fix up inappropriate printing to stdout should not have to make any other changes at the same time, both for their own sanity and to facilitate code review. Therefore, eprintln! should also exhibit the bug.

On the other hand, if we believe that there is no need for eprintln! to exhibit the bug, then we believe that people going through existing programs to fix up inappropriate printing to stdout will not encounter any println! calls that need changing that rely on the bug. This is only plausible to me if there are, in fact, no println! calls at all that rely on the bug -- and in that case there is no reason not to fix the bug.

@alexcrichton
Copy link
Member

Ok, in that case let's get some more opinions!

cc @rust-lang/libs

@alexcrichton alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 26, 2017
@BurntSushi
Copy link
Member

Even if we don't fix the bug in println!, I don't think we should purposefully reproduce it with eprintln!.

@aturon
Copy link
Member

aturon commented Apr 27, 2017

I'm ambivalent, mostly because this seems like a very minor issue. (I personally don't really have a problem with the current println! behavior).

Given that, as far as I know, there aren't any footguns here, I'd personally lean toward matching the "bug" in the new macros. (I have yet to see a compelling argument that this "bug" is problematic, aside from being inconsistent with print!, which we could change to allow arbitrarily literals.)

@aturon
Copy link
Member

aturon commented May 5, 2017

Nominating for discussion at the next libs triage (next Tuesday). Sorry for the delay!

@brson
Copy link
Contributor

brson commented May 9, 2017

Even if we think the existing println! behavior is undesirable, my inclination is to keep the behavior consistent between println! and eprintln! to reduce the possibility for surprises.

@aturon aturon removed the I-nominated label May 9, 2017
zackw added 2 commits May 10, 2017 09:41
 * Factor out the nigh-identical bodies of `_print` and `_eprint` to a helper
   function `print_to` (I was sorely tempted to call it `_doprnt`).
 * Update the issue number for the unstable `eprint` feature.
 * Add entries to the "unstable book" for `eprint` and `eprint_internal`.
 * Style corrections to the documentation.
@zackw
Copy link
Contributor Author

zackw commented May 10, 2017

@alexcrichton Rebased now. Let me know if you also want it squashed.

@alexcrichton
Copy link
Member

@bors: r+

Thanks @zackw!

@bors
Copy link
Contributor

bors commented May 10, 2017

📌 Commit 4ab3bcb has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented May 10, 2017

⌛ Testing commit 4ab3bcb with merge 9e9f527...

@bors
Copy link
Contributor

bors commented May 10, 2017

💔 Test failed - status-travis

@zackw
Copy link
Contributor Author

zackw commented May 10, 2017

Looks like the new test is failing on emscripten only; there's not enough detail for me to be sure, but I suspect the problem is emscripten doesn't actually have a stderr. Any objection to skipping the test there?

@alexcrichton
Copy link
Member

Oh I think it's just that emscripten can't spawn new processes, you'll probably just want to ignore the test on emscripten

@alexcrichton
Copy link
Member

er didn't mean to close

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented May 10, 2017

📌 Commit 72588a2 has been approved by alexcrichton

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
Add `eprint!` and `eprintln!` macros to the prelude.

These are exactly the same as `print!` and `println!` except that they write to stderr instead of stdout.  Issues rust-lang#39228 and rust-lang#40528; previous PR rust-lang#39229; accepted RFC rust-lang/rfcs#1869; proposed revision to The Book rust-lang/book#615.

I have _not_ revised this any since the original submission; I will do that later this week.  I wanted to get this PR in place since it's been quite a while since the RFC was merged.

Known outstanding review comments:

* [x] @steveklabnik requested a new chapter for the unstable version of The Book -- please see if the proposed revisions to the second edition cover it.
* [x] @nodakai asked if it were possible to merge the internal methods `_print` and `_eprint` - not completely, since they both refer to different internal globals which we don't want to expose, but I will see if some duplication can be factored out.

Please let me know if I missed anything.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 11, 2017
Add `eprint!` and `eprintln!` macros to the prelude.

These are exactly the same as `print!` and `println!` except that they write to stderr instead of stdout.  Issues rust-lang#39228 and rust-lang#40528; previous PR rust-lang#39229; accepted RFC rust-lang/rfcs#1869; proposed revision to The Book rust-lang/book#615.

I have _not_ revised this any since the original submission; I will do that later this week.  I wanted to get this PR in place since it's been quite a while since the RFC was merged.

Known outstanding review comments:

* [x] @steveklabnik requested a new chapter for the unstable version of The Book -- please see if the proposed revisions to the second edition cover it.
* [x] @nodakai asked if it were possible to merge the internal methods `_print` and `_eprint` - not completely, since they both refer to different internal globals which we don't want to expose, but I will see if some duplication can be factored out.

Please let me know if I missed anything.
bors added a commit that referenced this pull request May 11, 2017
Rollup of 5 pull requests

- Successful merges: #41192, #41724, #41873, #41877, #41889
- Failed merges:
@bors
Copy link
Contributor

bors commented May 11, 2017

⌛ Testing commit 72588a2 with merge 24ea08e...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.