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

RFC: Quick dbg!(expr) macro #2173

Closed
wants to merge 30 commits into from

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 13, 2017

See RFC #2361 for the "second version" of this RFC

Adds a macro dbg!(expr1 [, expr2, .., exprN]) for quick and dirty Debug:ing of expressions to the terminal. The macro evaluates expressions, prints it to STDERR, and finally yields a flat tuple of (expr1 [, expr2, .. exprN]). On release builds, the macro is the identity function and has no side effects. The macro is added to the prelude of the standard library.

Rendered.

Acknowledgements & ping

@scottmcm, @mbrubeck, @sdleffler, @setharnold

@Centril Centril changed the title Rfc/quick debug macro RFC: Quick dbg!(expr) macro Oct 13, 2017

## On release builds:

The same examples above will print nothing to `STDERR` and will instead simply
Copy link
Member

Choose a reason for hiding this comment

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

Why? dbg!() calls don't seem like something that you'd want to keep around other than when you're looking into a specific bug.

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 think looking into a specific bug is the most likely scenario. However...
There might be some fix you made, but you are not 100% sure, and you'd still like to include it into release builds. With the macro having no effect on release, this is possible.

Does this have any downsides?

Copy link
Member

Choose a reason for hiding this comment

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

Spewing a bunch of stuff to stderr when a downstream crate isn't building with --release is a downside.

Asserts are a more common way of handling those kinds of checks IME.

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 buy the argument regarding assert for at least libraries. However, it might have some value for programs?

@sfackler
Copy link
Member

Does something like this exist in other languages?

@sdleffler
Copy link

@sfackler Haskell has Debug.trace :: a -> a, which I would liken to this.

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2017

@sfackler (You could argue that) It is similar to:

@SimonSapin
Copy link
Contributor

Yes please. I think the lesser verbosity compared to println!("{:#?}", expr) (which requires a number of sigils in a particular order) is worth adding a new macro to the prelude.

@sdleffler
Copy link

sdleffler commented Oct 13, 2017

I want to restate the concern I had on IRC - I think that perhaps this macro should take a single format string argument, with a privileged "first" parameter: e.g. dbg!(x, "DEBUG: {:?}") in front of the format string which is let through and becomes the value of the macro. This would avoid any sort of irritation with the [DEBUGGING ...] etc. formatting - if there's no format string just print out the single argument.

@sfackler
Copy link
Member

@sdleffler this seems more like traceShowId than trace, right? dbg!(x, "DEBUG: {:?}") is just eprintln!("DEBUG: {:?}", x)? It's not clear to me that it really buys anything.

None of those examples wrap the output in [DEBUGGING ..], do they?

@sdleffler
Copy link

@sfackler no, because eprintln!(...) has type (). dbg!(x) where x: T has type T.

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2017

@sfackler The format, including [DEBUGGING ..] is entirely bike-sheddable ;)

@sdleffler
Copy link

@sfackler see the "Reference-level Explanation": ideally I'd want to avoid that formatting and just let the user do it. It doesn't actually wrap the output in [DEBUGGING ..], but it prefixes the eprintln! with it.

@sdleffler
Copy link

And also thus avoid that bikeshedding :P

the identity function on the expression, but the call will be inlined away such
that the overhead is zero.

The file name, line number and column is included for increased utility when included in production quality code. The expression is also stringified, so that
Copy link
Member

Choose a reason for hiding this comment

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

What does "production quality code" mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Things ready to be included in programs & libraries run in production... This is arguably vague and subjective. It could be interpreted as things ready for cargo publish, but probably not.

Copy link
Member

Choose a reason for hiding this comment

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

I would argue that things that should be included in programs & libraries run in production should never be writing something like

[DEBUGGING, foobar.rs]
some-expr => 15

to stderr. If you want people to use your library, let them decide if and how they want to consume the debugging information you produce.

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 would probably only be included when developing/debugging a library, which the developer of the lib does.

Copy link
Member

@sfackler sfackler Oct 13, 2017

Choose a reason for hiding this comment

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

The "when included in production quality code" bit seems out of place if this wouldn't be used in production quality code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should probably be reworded.

The intent is:

  • In the case of a library: If the library is used in production and you are the developer/contributor to it, and you want to make some changes and debug those, the macro should be useful to you when doing debug builds.
  • In the case of a program: The macro should be useful to you when doing debug builds and you shouldn't have to remove them in --release.

Copy link
Member

Choose a reason for hiding this comment

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

But how is the production-ness of the library relevant in the first case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed production quality to non-trivial per discussion here and on IRC.

@scottmcm scottmcm added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Oct 13, 2017
@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2017

ping @bluss for non-Debug types & specialization.


By using `dbg!(expr);`, the burden of a common papercut: writing `println!("{:?}", expr);` every time you want to see the evaluted-to value of an expression, can be significantly reduced. The developer no longer has to remember the formatting args and has to type significantly less (12 characters to be exact).

The shortness is also beneficial when asking `playbot-mini` on #rust@irc.mozilla.org to evaluate and print an expression.
Copy link
Member

Choose a reason for hiding this comment

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

I think IRC bots would rather a very different output format from what's described currently:

  • The [DEBUGGING, src/main.rs:1:12]: is clearly of no value there
  • Most bots only keep one or two lines (to avoid spamming), so {:#?} is worse than {:?}
  • The expression is one the line right above, so isn't really worth showing again

playbot (RIP) just used fn show<T:Debug>(x: T) -> T { print!("{:?}", x); x } which worked great for it.

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 buy this =) I'll remove the argument from the RFC.

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2017

  • Added optional formatting argument as requested by @sdleffler
  • Added notes on the choice of macro name.

@vi
Copy link

vi commented Oct 13, 2017

Maybe it should be sensitive to environment variables, like in the RUST_BACKTRACE case, so one can opt-in (out-out) to the output from the macro?


Can it be made enabled (by default) only from crate being actively developed (e.g current crate, it's examples, path = dependencies), not dependencies downloaded from crates.io. For example, by some Cargo magic?

@SimonSapin
Copy link
Contributor

FWIW my use case for this is to sprinkle it during debugging and remove it before making a git commit. So any extra step required to get some visible output (setting an env variable, adding a crate dependency, typing a formatting string, …) is a hindrance.

@vi
Copy link

vi commented Oct 13, 2017

@SimonSapin , Then why bother removing it in release? What if debug and release take different code paths (due to timing, for example) and you are debugging it?

@Centril
Copy link
Contributor Author

Centril commented Oct 13, 2017

@vi If timing matters, I think a considerably more rigorous approach to debugging & verification for the code under test is required. This makes this macro less interesting for those use cases, which is OK.

@dlight
Copy link

dlight commented Oct 14, 2017

Hello, I wrote some time ago a crate called p-macro, which defines a macro p!() that is somewhat similar to this proposal. A debugging macro better than println! was reinvented a number of times in the crates ecosystem, which supports the idea that it should be included on the stdlib.

One interesting thing about p-macro is that p!(a + b) prints a + b = 5 (for example), while p!(: a + b) would print just 5. Printing the source expression alongside its value is, in my opinion, the appropriate default for debugging.

Another thing is that you can prefix with a _ to print using Display instead of Debug. Like p!(_ "Test").

Another is that you can do p!(a, b) to print a = 2, b = 3.

And another thing is being able to print many lines, like

p! {
    _"Hello";
    a, b;
   heh;
}

@Centril
Copy link
Contributor Author

Centril commented Oct 14, 2017

@vi If an environment variable is used to opt-in to printing the macro will also be quite useless for the Rust Playground as there is no way to specify environment variables there (AFAIK), and if there were, it wouldn't be ergonomic for newcomers to the language or experienced rustaceans.

@Centril
Copy link
Contributor Author

Centril commented Mar 13, 2018

(I'd suggest making stabilizing WrapDebug a non-goal for this RFC, though.)

Does that work? If WrapDebug is unstable, and the macro dbg!(e) expands in part to ::std::fmt::WrapDebug(e); then the use site dbg!(e) should raise an error.

EDIT: Of course, one can also see this as a benefit... dbg!(e) will not compile until WrapDebug is stabilized, so in effect this makes dbg!(e) unstable.

@sfackler
Copy link
Member

Standard library macros can bypass stability. I would argue that WrapDebug should be omitted entirely in an initial proposal.

I also don't agree that the macro is massively complex. It is certainly not more complex than println!(..) (if you take in the dependencies) and similar macros.

#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
#[allow_internal_unstable]
macro_rules! print {
    ($($arg:tt)*) => ($crate::io::_print(format_args!($($arg)*)));
}

#[macro_export]
#[stable(feature = "rust1", since = "1.0.0")]
macro_rules! println {
    () => (print!("\n"));
    ($fmt:expr) => (print!(concat!($fmt, "\n")));
    ($fmt:expr, $($arg:tt)*) => (print!(concat!($fmt, "\n"), $($arg)*));
}

@Centril
Copy link
Contributor Author

Centril commented Mar 13, 2018

@sfackler

Standard library macros can bypass stability. I would argue that WrapDebug should be omitted entirely in an initial proposal.

Interesting... But what is the rationale for omitting WrapDebug initially?

Also, if you expand format_args! you have a very different story, wherefore:

(if you take in the dependencies)

@sfackler
Copy link
Member

But what is the rationale for omitting WrapDebug initially?

It can only be implemented through specialization which is not stable. There is currently nothing in the standard library API that exposes specialization.

Also, if you expand format_args!

It does not seem particularly reasonable to consider the complexity of an API by the transitive size of its dependencies. Are we going to include the macro_rules! implementation as well?

@Centril
Copy link
Contributor Author

Centril commented Mar 13, 2018

It can only be implemented through specialization which is not stable. There is currently nothing in the standard library API that exposes specialization.

Including WrapDebug without stabilizing specialization will not expose specialization in the sense that we must stabilize specialization after since you can always remove impl<T: Debug> Debug for WrapDebug<T> { .. } if we should decide that we don't want specialization. Granted, the dbg!(e) macro will become mostly useless if we remove WrapDebug.

If we don't include WrapDebug initially, can we at least agree to do this without going through the RFC process again once specialization lands in stable?

It does not seem particularly reasonable to consider the complexity of an API by the transitive size of its dependencies. Are we going to include the macro_rules! implementation as well?

Then simply consider the complexity of the format_args! macro (which is part of a public API) directly instead. Your assertion It is significantly larger than any other macro in the standard library is not accurate, at least if you count format_args! as part of the standard library.

@sfackler
Copy link
Member

I was specifically referring to macro_rules macros, but I think it's hopefully clear that format_args earns its complexity. It's one of the fundamental and most commonly used components of the standard library.

@Centril
Copy link
Contributor Author

Centril commented Mar 13, 2018

@sfackler

I was specifically referring to macro_rules macros, but I think it's hopefully clear that format_args earns its complexity. It's one of the fundamental and most commonly used components of the standard library.

Fair enough =)

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Mar 13, 2018
@rfcbot
Copy link
Collaborator

rfcbot commented Mar 13, 2018

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

SimonSapin added a commit to SimonSapin/rfcs that referenced this pull request Mar 13, 2018
This is a simpler and more opinionated counter-proposal to [RFC 2173](rust-lang#2173).
@SimonSapin
Copy link
Contributor

I’ve expressed before that I’d prefer this macro to be kept very simple (and in particular configuration-less). I wrote up a full counter-proposal at #2361.

@vi
Copy link

vi commented Mar 13, 2018

Is it a good idea to restrict cargo publish if some dgb!s are left in?

@SimonSapin
Copy link
Contributor

@vi That sounds like a fine lint for Clippy or other project-specific linter, but maybe doesn’t belong in rustc or cargo.

@Centril
Copy link
Contributor Author

Centril commented Mar 14, 2018

@repax

The initial proposal of this RFC seemed like a nice addition. It has since, along with its 30 commits, expanded out of reasonable proportions. I think that the support, as indicated by the votes, should be taken with a pinch of salt.

Irrespective of this RFC, I want to note that I find this comment to be unhelpful and also wrong.

  1. The number of commits are not indicative of the complexity of an RFC; many commits are simply recording discussions and (un)resolved questions as they go, other commits are fixing typos or improving explanations.

  2. Your comments add no new useful information and is essentially devoid of arguments at all. Arguments of the form "this is (not) popular" are not really arguments.

  3. You are not a mind reader and can't speak for the people who indicated their support for an RFC. I expect that the people who lend their support are not a homogeneous group.

  4. Speaking of votes, there are none. The RFC process is not a referendum. Instead, the process consists of an author that puts forth a proposal, and then there are various people who either ask for changes or voice concerns or generally agree. In the end, once the discussion has settled, there's a responsible team who makes a decision, and they should only care about evaluating the text of the RFC and whether there are notable concerns.

@repax
Copy link

repax commented Mar 14, 2018

@Centril: Meh. you're overreacting. I'm not here to bicker. I'm simply pointing out that I think that what started as a good idea has grown too much in complexity. It's easy to see how the complexity of the proposal has grown if you read the commit history. I think this is a valid thing to point out. Did not mean to hurt your feelings.

@Centril
Copy link
Contributor Author

Centril commented Mar 14, 2018

@repax

I'm simply pointing out that I think that what started as a good idea has grown too much in complexity.

This formulation is much better.

Did not mean to hurt your feelings.

Don't worry; they were not hurt - while I would have wanted a more feature complete macro, I am excited to see dbg!(expr), even if in a more minimal form.

My comment was more general about RFCs in general and not specific to this RFC. I think these points are important to understand how the RFC process works so that people don't see a lot of upvotes or downvotes as indicative of what the responsible subteam will do; if people have this perception, and argue about popularity, they will become disappointed. Moreover, arguments about popularity do not advance discussion or bring clarity into trade-offs.

@repax
Copy link

repax commented Mar 14, 2018

@Centril: I should choose my words more carefully. Thank you for all the work you put into this.

@aturon
Copy link
Member

aturon commented Mar 16, 2018

@Centril I just want to thank you for the huge amount of work and patience here, regardless of the outcome. The detail in "formerly unresolved questions" is super valuable, and I wish that all RFCs contained such great summarizations.

Your comparison to println I think is an apt one. That is certainly a complex, "gadgetty" subsystem within std today (for better or worse). It was grandfathered in to 1.0 without the fanfare of a stabilization RFC, and I strongly suspect that the Libs Team would be resistant to adding such an API if it were proposed now.

Over time, we've come to be increasingly conservative in our approach to std, strongly preferring the crates.io ecosystem whenever possible:

The fact that std is effectively coupled with rustc means that upgrading the compiler entails upgrading the standard library, like it or not. That means that the two need to provide essentially the same backwards-compatibility guarantees. TL;DR, it’s simply not feasible to do a new, major version of std with breaking changes. Moreover, std is forcibly tied to the Rust release schedule, meaning that new versions arrive every six weeks, period. Given these constraints, we’ve chosen to take a minimalist route with std, to avoid accumulating a mass of deprecated APIs over time.

On the flip side, our fantastic packaging story with Cargo, together with other ongoing work on improving discoverability and curation in the ecosystem, allow us to provide a very strong user experience despite a lean std. To me, the core problem that sparked this RFC is that our Cargo story isn't quite up to par for these kinds of tools. I'd personally love to see the attention focused there, so that we can take all of the great work going into the design of this RFC and put it out there in the ecosystem.

So to be clear: I think everyone acknowledges that debugging ergonomics are extremely important, and worth investing in. But I think folks are skeptical that, even with this lengthy discussion, we've arrived at the Best Way that will work for everyone (even taking into account the provided knobs). To me, this seems like the perfect kind of thing to provide in the crates.io ecosystem, joining the ranks of many other crates providing highly sophisticated, ergonomic -- and competing -- answers to important development questions.

@rfcbot
Copy link
Collaborator

rfcbot commented Mar 23, 2018

The final comment period is now complete.

@Centril
Copy link
Contributor Author

Centril commented Mar 23, 2018

As the FCP is now complete with a motion to close this RFC is now closed.

The discussion on a more streamlined macro, roughly based on #2173 (comment) continues in RFC #2361.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.