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

allow multiple args to dbg!(..) #59826

Merged
merged 3 commits into from
Apr 20, 2019
Merged

allow multiple args to dbg!(..) #59826

merged 3 commits into from
Apr 20, 2019

Conversation

llogiq
Copy link
Contributor

@llogiq llogiq commented Apr 9, 2019

This closes #59763

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 9, 2019
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

I think a better implementation of this, as I originally envisaged, was to have each part of the tuple on a separate line for better readability. Moreover, dbg!(1,) and dbg!(2, 3,) is supported by this change whereas it is not by your implementation.

src/libstd/macros.rs Outdated Show resolved Hide resolved
src/libstd/macros.rs Outdated Show resolved Hide resolved
@Centril Centril added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Apr 9, 2019
@Centril
Copy link
Contributor

Centril commented Apr 9, 2019

r? @alexcrichton

@Centril
Copy link
Contributor

Centril commented Apr 9, 2019

Moreover, the UI tests for dbg! should be extended to cover the new behavior once we settle on a behavior, assuming we want to make this change.

Use dbg! recursively on each item

Co-Authored-By: llogiq <bogusandre@gmail.com>
@rust-highfive

This comment has been minimized.

@alexcrichton
Copy link
Member

r? @SimonSapin

I think you authored the original RFC, so you're probably best to look at this

@rust-highfive

This comment has been minimized.

@SimonSapin SimonSapin added I-needs-decision Issue: In need of a decision. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 10, 2019
@SimonSapin
Copy link
Contributor

In the RFC, I specifically proposed not doing this:

https://github.com/rust-lang/rfcs/blob/master/text/2361-dbg-macro.md#accepting-a-single-expression-instead-of-many

I think it creates an undesirable “discontinuity”: if dbg!(x, y, z) returns the 3-tuple (x, y, y) and dbg!(x, y) returns the 2-tuple (x, y) (and dbg!() returns the zero-tuple ()) then the consistent continuation is that dbg!(x) returns the 1-tuple (x,). But it has been stabilized as returning x, and a 1-tuple is probably less convenient anyway.

I can’t find it again, but I vaguely remember an argument that you could look at dbg!(x) as dbg! (x), and (x) by itself is x with unnecessary parens, not a tuple. But I disagree with this framing: the foo!(…) syntax of expression-position macro invocation is much more similar to function call syntax than to tuple syntax. (Using a 1-tuple to represent the arguments of a single-argument function appears again in the desugared notation for the Fn(T) trait, which is Fn<(T,)>.)

This PR seems to be going for emulating tuple syntax: it makes dbg!(x,) behave differently than dbg!(x) and return (x,) instead of x. But in a function call, a trailing comma after arguments is not significant. I think we should not make it significant in a function-like macro that takes comma-separated expressions as arguments.


TL;DR: I feel I would still prefer that dbg!(…) only accepts one (or zero) expression, and that making it return a tuple involves passing it an actual tuple value.

That said, is there consensus except for me that the convenience of debugging multiple values with fewer sigils is worth accepting this weird inconsistency?

If so, I’d prefer we embrace the inconsistency and make dbg!(x,) the same as dbg!(x). And maybe the filename and line number should not be repeated for every element of the tuple?

CC @rust-lang/libs

@Centril
Copy link
Contributor

Centril commented Apr 10, 2019

I can’t find it again, but I vaguely remember an argument that you could look at dbg!(x) as dbg! (x), and (x) by itself is x with unnecessary parens, not a tuple.

Yes, that is the framing I was going for. It provides a good internal consistency. Perhaps not the one you want, but I think it works well in practice to do what the users wants 99% of the time.

This PR seems to be going for emulating tuple syntax: it makes dbg!(x,) behave differently than dbg!(x) and return (x,) instead of x. But in a function call, a trailing comma after arguments is not significant. I think we should not make it significant in a function-like macro that takes comma-separated expressions as arguments.

What is the failure mode here? i.e. in what way do you think this will trip users up?

Note that if you write dbg!(x); or dbg!(x,); it will not matter that you actually received back a tuple as the semicolon turns the type into () in any case.

To actually have a problem, you need to actually use the result:

let x = dbg!(x,);

(x must still be passed along to have a problem)

I think this is unlikely and is only probable as the result of a typo. When it happens the compiler already gives a good error message:

fn main() {
    let _: u8 = dbg!(1,);
}

results in:

error[E0308]: mismatched types
  --> src/main.rs:18:9
   |
18 |         ($(dbg!($val)),+,)
   |         ^^^^^^^^^^^^^^^^^^ expected u8, found tuple
...
23 |     let _: u8 = dbg!(1,);
   |                 -------- in this macro invocation
   |
   = note: expected type `u8`
              found type `({integer},)`

If so, I’d prefer we embrace the inconsistency and make dbg!(x,) the same as dbg!(x). And maybe the filename and line number should not be repeated for every element of the tuple?

Note that this only makes the macro more complex, not less. One good advantage of this PR is that it provides a simple implementation that should also be simple to explain: simply remove dbg! and the thing you have without is the value you get back out and of that type. This rule would have zero exceptions with this PR.

@Centril
Copy link
Contributor

Centril commented Apr 10, 2019

As for whether to accept multiple arguments or not, I think that the purpose of dbg! is quick and convenient debugging. To not accept multiple arguments goes against that as the user must actively remember that they can pass a tuple instead (and it gives less readable output). That a diagnostic was suggested in #59763 is in my mind evidence that accepting multiple arguments is intuitive and expected.

@BurntSushi
Copy link
Member

If this PR is accepted, is the type of dbg!(a, b) and dbg!((a, b)) the same?

I do agree with @SimonSapin that if we're going to allow multiple arguments, then the trailing comma in the case of the first argument should behave like a function call. While let x = dbg!(y,); is unlikely to happen in practice, something like this is much more feasible:

let x = dbg!(
    some_long_single_argument_expression,
);

And if that doesn't work---regardless of whether we permit multiple arguments or not---I would honestly be pretty surprised. I'd expect things like this to work as expected as well:

let x = dbg!(
    some_long_single_argument_expression,
    another_long_single_argument_expression,
);

I don't have too many strong opinions on whether we permit multiple arguments. It does honestly seem nice, and the case of wanting a unit tuple is a bit weird/rare anyway, so if you did need that, you'd just have to get used to writing dbg!((x,)) I guess.

@Centril
Copy link
Contributor

Centril commented Apr 17, 2019

  • In particular, dbg!(x) returns x, not (x,).

@SimonSapin You mean dbg!(x,) here, right?

@llogiq
Copy link
Contributor Author

llogiq commented Apr 17, 2019

I read this as dbg!(x,) == dbg!(x) (yes, that means for 1-tuples we need double braces).

@SimonSapin
Copy link
Contributor

Ah yes, that was a typo. Fixed.

@SimonSapin
Copy link
Contributor

I’m personally still undecided (and forgot to bring it up in the meeting) whether it’s better if the file name and line number are printed once per macro call or once per argument.

@Centril
Copy link
Contributor

Centril commented Apr 17, 2019

@SimonSapin Since that's not part of the stable guarantees it seems to me that we can do the simple thing now and iterate / "feel" our way through.

@SimonSapin
Copy link
Contributor

Sure, it’s not set in stone and we can change our minds later. I think it’s still worth discussing now, though! Do you have thoughts either way?

@Centril
Copy link
Contributor

Centril commented Apr 17, 2019

Do you have thoughts either way?

It's tricky... on the one hand, having output like:

[src/main.rs:2] a = 1
[src/main.rs:2] b = 2
[src/main.rs:2] c = 3

feels nicely aligned... on the other it feels repetitive.

A sketch for an alternative for multi-argument output:

[src/main.rs:2]
[ a = 1
[ b = 2
[ c = 3

@llogiq
Copy link
Contributor Author

llogiq commented Apr 18, 2019

I've implemented the libs team's suggestions as @SimonSapin wrote them up. I did not yet change the output of the macro, we can iterate on that later.

@Centril Centril added this to the 1.36 milestone Apr 18, 2019
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@llogiq
Copy link
Contributor Author

llogiq commented Apr 20, 2019

@SimonSapin if you are OK with this I'd like to land this as is now and iterate on the output in a follow-up PR.

@SimonSapin
Copy link
Contributor

Looks good, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 20, 2019

📌 Commit b641fd3 has been approved by SimonSapin

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Apr 20, 2019
@bors
Copy link
Contributor

bors commented Apr 20, 2019

⌛ Testing commit b641fd3 with merge 33fe113...

bors added a commit that referenced this pull request Apr 20, 2019
allow multiple args to `dbg!(..)`

This closes #59763
@bors
Copy link
Contributor

bors commented Apr 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: SimonSapin
Pushing 33fe113 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2019
@bors bors merged commit b641fd3 into rust-lang:master Apr 20, 2019
@Centril Centril removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Apr 25, 2019
@Centril Centril removed the I-needs-decision Issue: In need of a decision. label Jul 2, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jul 5, 2019
Pkgsrc changes:
 * NetBSD/sparc64 disabling of "packed" removed ("packed" removed upstream)
 * Adapt src_libstd_build.rs patch, update sed'ing of .cargo-checksum.json

Build verified on NetBSD 8.0/amd64.

Upstream changes:

Version 1.36.0 (2019-07-04)
==========================

Language
--------
- [Non-Lexical Lifetimes are now enabled on the 2015 edition.][59114]
- [The order of traits in trait objects no longer affects the semantics of that
  object.][59445] e.g. `dyn Send + fmt::Debug` is now equivalent to
  `dyn fmt::Debug + Send`, where this was previously not the case.

Libraries
---------
- [`HashMap`'s implementation has been replaced with `hashbrown::HashMap` implem
entation.][58623]
- [`TryFromSliceError` now implements `From<Infallible>`.][60318]
- [`mem::needs_drop` is now available as a const fn.][60364]
- [`alloc::Layout::from_size_align_unchecked` is now available as a const fn.][6
0370]
- [`String` now implements `BorrowMut<str>`.][60404]
- [`io::Cursor` now implements `Default`.][60234]
- [Both `NonNull::{dangling, cast}` are now const fns.][60244]
- [The `alloc` crate is now stable.][59675] `alloc` allows you to use a subset
  of `std` (e.g. `Vec`, `Box`, `Arc`) in `#![no_std]` environments if the
  environment has access to heap memory allocation.
- [`String` now implements `From<&String>`.][59825]
- [You can now pass multiple arguments to the `dbg!` macro.][59826] `dbg!` will
  return a tuple of each argument when there is multiple arguments.
- [`Result::{is_err, is_ok}` are now `#[must_use]` and will produce a warning if
  not used.][59648]

Stabilized APIs
---------------
- [`VecDeque::rotate_left`]
- [`VecDeque::rotate_right`]
- [`Iterator::copied`]
- [`io::IoSlice`]
- [`io::IoSliceMut`]
- [`Read::read_vectored`]
- [`Write::write_vectored`]
- [`str::as_mut_ptr`]
- [`mem::MaybeUninit`]
- [`pointer::align_offset`]
- [`future::Future`]
- [`task::Context`]
- [`task::RawWaker`]
- [`task::RawWakerVTable`]
- [`task::Waker`]
- [`task::Poll`]

Cargo
-----
- [Cargo will now produce an error if you attempt to use the name of a required
dependency as a feature.][cargo/6860]
- [You can now pass the `--offline` flag to run cargo without accessing the netw
ork.][cargo/6934]

You can find further change's in [Cargo's 1.36.0 release notes][cargo-1-36-0].

Clippy
------
There have been numerous additions and fixes to clippy, see [Clippy's 1.36.0 rel
ease notes][clippy-1-36-0] for more details.

Misc
----

Compatibility Notes
-------------------
- [`std::arch::x86::_rdtsc` returns `u64` instead of `i64`][stdsimd/559]
- [`std::arch::x86_64::_mm_shuffle_ps` takes an `i32` instead of `u32` for `mask
`][stdsimd/522]
- With the stabilisation of `mem::MaybeUninit`, `mem::uninitialized` use is no
  longer recommended, and will be deprecated in 1.38.0.

[60318]: rust-lang/rust#60318
[60364]: rust-lang/rust#60364
[60370]: rust-lang/rust#60370
[60404]: rust-lang/rust#60404
[60234]: rust-lang/rust#60234
[60244]: rust-lang/rust#60244
[58623]: rust-lang/rust#58623
[59648]: rust-lang/rust#59648
[59675]: rust-lang/rust#59675
[59825]: rust-lang/rust#59825
[59826]: rust-lang/rust#59826
[59445]: rust-lang/rust#59445
[59114]: rust-lang/rust#59114
[cargo/6860]: rust-lang/cargo#6860
[cargo/6934]: rust-lang/cargo#6934
[`VecDeque::rotate_left`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_left
[`VecDeque::rotate_right`]: https://doc.rust-lang.org/std/collections/struct.VecDeque.html#method.rotate_right
[`Iterator::copied`]: https://doc.rust-lang.org/std/iter/trait.Iterator.html#tymethod.copied
[`io::IoSlice`]: https://doc.rust-lang.org/std/io/struct.IoSlice.html
[`io::IoSliceMut`]: https://doc.rust-lang.org/std/io/struct.IoSliceMut.html
[`Read::read_vectored`]: https://doc.rust-lang.org/std/io/trait.Read.html#method.read_vectored
[`Write::write_vectored`]: https://doc.rust-lang.org/std/io/trait.Write.html#method.write_vectored
[`str::as_mut_ptr`]: https://doc.rust-lang.org/std/primitive.str.html#method.as_mut_ptr
[`mem::MaybeUninit`]: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html
[`pointer::align_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.align_offset
[`future::Future`]: https://doc.rust-lang.org/std/future/trait.Future.html
[`task::Context`]: https://doc.rust-lang.org/beta/std/task/struct.Context.html
[`task::RawWaker`]: https://doc.rust-lang.org/beta/std/task/struct.RawWaker.html
[`task::RawWakerVTable`]: https://doc.rust-lang.org/beta/std/task/struct.RawWakerVTable.html
[`task::Waker`]: https://doc.rust-lang.org/beta/std/task/struct.Waker.html
[`task::Poll`]: https://doc.rust-lang.org/beta/std/task/enum.Poll.html
[clippy-1-36-0]: https://github.com/rust-lang/rust-clippy/blob/master/CHANGELOG.md#rust-136
[cargo-1-36-0]: https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-136-2019-07-04
[stdsimd/522]: rust-lang/stdarch#522
[stdsimd/559]: rust-lang/stdarch#559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Suggest parenthesising multiple arguments to dbg!()
9 participants