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

zackw · 2017-04-10T17:07:44Z

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 · 2017-04-10T17:07:49Z

r? @aturon

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

steveklabnik · 2017-04-10T19:01:15Z

src/libstd/macros.rs

+/// 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


we don't use () after function names

steveklabnik · 2017-04-10T19:01:59Z

src/libstd/macros.rs

+///
+/// # Panics
+///
+/// Panics if writing to `io::stderr()` fails.


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

print!

io::stderr

io::stdout

io::stderr

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

steveklabnik · 2017-04-10T19:02:06Z

src/libstd/macros.rs

+///
+/// # Panics
+///
+/// Panics if writing to `io::stderr()` fails.


same comments here

alexcrichton · 2017-04-12T18:09:49Z

src/libstd/io/stdio.rs

+    // 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.


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

Yes, the implementation can be factored out.

alexcrichton · 2017-04-13T16:05:02Z

@bors: r+

Thanks @zackw!

bors · 2017-04-13T16:05:03Z

📌 Commit 1d00098 has been approved by alexcrichton

ollie27 · 2017-04-13T16:14:06Z

src/libstd/io/mod.rs

@@ -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")]


This should probably be feature = "eprint_internal".

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

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.

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

ollie27 · 2017-04-13T16:16:38Z

src/libstd/macros.rs

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


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

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

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

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

ollie27 · 2017-04-13T16:21:50Z

src/libstd/macros.rs

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


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.

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.

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

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.

@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 · 2017-04-13T16:49:39Z

@bors: r-

( active comments )

bors · 2017-04-20T04:42:43Z

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

zackw · 2017-04-20T21:54:36Z

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 · 2017-04-20T22:08:37Z

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 · 2017-04-23T04:37:16Z

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

arielb1 · 2017-04-25T09:54:11Z

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

alexcrichton · 2017-04-25T19:09:25Z

@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 · 2017-04-25T19:48:45Z

@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 · 2017-04-26T14:32:56Z

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

cc @rust-lang/libs

BurntSushi · 2017-04-26T15:54:40Z

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

aturon · 2017-04-27T03:20:12Z

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 · 2017-05-05T04:23:13Z

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

brson · 2017-05-09T20:14:49Z

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.

* 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 · 2017-05-10T13:54:54Z

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

alexcrichton · 2017-05-10T14:14:41Z

@bors: r+

Thanks @zackw!

bors · 2017-05-10T14:14:42Z

📌 Commit 4ab3bcb has been approved by alexcrichton

bors · 2017-05-10T15:10:30Z

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

bors · 2017-05-10T17:10:10Z

💔 Test failed - status-travis

zackw · 2017-05-10T17:33:34Z

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 · 2017-05-10T18:01:58Z

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 · 2017-05-10T18:02:07Z

er didn't mean to close

alexcrichton · 2017-05-10T22:19:43Z

@bors: r+

bors · 2017-05-10T22:19:44Z

📌 Commit 72588a2 has been approved by alexcrichton

@steveklabnik

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.

@steveklabnik

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.

Rollup of 5 pull requests - Successful merges: #41192, #41724, #41873, #41877, #41889 - Failed merges:

bors · 2017-05-11T09:39:30Z

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

rust-highfive assigned aturon Apr 10, 2017

steveklabnik reviewed Apr 10, 2017

View reviewed changes

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

alexcrichton reviewed Apr 12, 2017

View reviewed changes

ollie27 reviewed Apr 13, 2017

View reviewed changes

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

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

aturon added the I-nominated label May 5, 2017

aturon removed the I-nominated label May 9, 2017

zackw added 2 commits May 10, 2017 09:41

Fix up stability annotations per feedback.

4ab3bcb

zackw force-pushed the eprintln branch from 0b63c0c to 4ab3bcb Compare May 10, 2017 13:54

alexcrichton closed this May 10, 2017

alexcrichton reopened this May 10, 2017

Skip print-stdout-eprint-stderr test on emscripten

72588a2

frewsxcv mentioned this pull request May 11, 2017

Rollup of 10 pull requests #41901

Closed

frewsxcv mentioned this pull request May 11, 2017

Rollup of 5 pull requests #41905

Merged

bors added a commit that referenced this pull request May 11, 2017

Auto merge of #41905 - frewsxcv:rollup, r=frewsxcv

24ea08e

Rollup of 5 pull requests - Successful merges: #41192, #41724, #41873, #41877, #41889 - Failed merges:

bors merged commit 72588a2 into rust-lang:master May 11, 2017

zackw deleted the eprintln branch May 11, 2017 15:37

ollie27 mentioned this pull request May 12, 2017

println!, eprintln! and writeln! accept other literals not just format strings #30143

Closed

jan-schreib mentioned this pull request Jul 21, 2017

eprint! and eprintln! in Rust 1.19 rust-lang/book#823

Closed

zackw mentioned this pull request Jul 27, 2017

Tracking issue for addition of eprint/eprintln macros #40528

Closed

shepmaster mentioned this pull request Jan 15, 2018

print! and println! (and other similar macro pairs) accept differing types of "format strings" #47441

Closed

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

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

Conversation

zackw commented Apr 10, 2017 • edited Loading

rust-highfive commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Apr 13, 2017

bors commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackw Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Apr 13, 2017

bors commented Apr 20, 2017

zackw commented Apr 20, 2017 • edited Loading

alexcrichton commented Apr 20, 2017

bors commented Apr 23, 2017

arielb1 commented Apr 25, 2017

alexcrichton commented Apr 25, 2017

zackw commented Apr 25, 2017

alexcrichton commented Apr 26, 2017

BurntSushi commented Apr 26, 2017

aturon commented Apr 27, 2017

aturon commented May 5, 2017

brson commented May 9, 2017

zackw commented May 10, 2017

alexcrichton commented May 10, 2017

bors commented May 10, 2017

bors commented May 10, 2017

bors commented May 10, 2017

zackw commented May 10, 2017

alexcrichton commented May 10, 2017

alexcrichton commented May 10, 2017

alexcrichton commented May 10, 2017

bors commented May 10, 2017

bors commented May 11, 2017

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

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

zackw commented Apr 10, 2017 •

edited

Loading

zackw Apr 13, 2017 •

edited

Loading

zackw commented Apr 20, 2017 •

edited

Loading