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

Tracking issue for RFC 2361, "Simpler alternative dbg!() macro" #54306

Closed
Centril opened this Issue Sep 17, 2018 · 27 comments

Comments

@Centril
Copy link
Contributor

Centril commented Sep 17, 2018

This is a tracking issue for the RFC "Simpler alternative dbg!() macro" (rust-lang/rfcs#2361).

Steps:

Unresolved questions:

There are none.

@Centril Centril added the T-libs label Sep 17, 2018

@Centril Centril self-assigned this Sep 17, 2018

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Sep 17, 2018

I am working on this :)

kennytm added a commit to kennytm/rust that referenced this issue Sep 20, 2018

Rollup merge of rust-lang#54317 - Centril:feature/dbg_macro, r=SimonS…
…apin

Implement the dbg!(..) macro

Implements the `dbg!(..)` macro due to rust-lang#54306.
cc rust-lang/rfcs#2361

r? @alexcrichton

kennytm added a commit to kennytm/rust that referenced this issue Sep 21, 2018

Rollup merge of rust-lang#54317 - Centril:feature/dbg_macro, r=SimonS…
…apin

Implement the dbg!(..) macro

Implements the `dbg!(..)` macro due to rust-lang#54306.
cc rust-lang/rfcs#2361

r? @alexcrichton

bors added a commit that referenced this issue Sep 23, 2018

Auto merge of #54317 - Centril:feature/dbg_macro, r=SimonSapin
Implement the dbg!(..) macro

Implements the `dbg!(..)` macro due to #54306.
cc rust-lang/rfcs#2361

r? @alexcrichton

bors added a commit that referenced this issue Sep 25, 2018

Auto merge of #54317 - Centril:feature/dbg_macro, r=SimonSapin
Implement the dbg!(..) macro

Implements the `dbg!(..)` macro due to #54306.
cc rust-lang/rfcs#2361

r? @alexcrichton
@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Sep 25, 2018

I just tried this out, and something that was unexpected for me was that it dbg!() can only take a single value. I think this might be because most other printers I've used to this point support this (e.g. JS console.log(x, y), Rust println!("{:?}, {:?}", x, y), Bash echo $x $y).

I didn't see this mentioned in the RFC anywhere, but it's possible this was brought up before and now falls under "Unbounded bikeshedding". I don't know if it does, but figured it might be worth sharing my experience in the off chance this hadn't been brought up before.

edit (2018-09-25): I just re-read the proposal and found the tuples entry. I guess that answers it, but yeah it still was something unexpected to me. Ah well.

Expected

let x = 1;
let y = 2;
dbg!(x, y);

Current

let x = 1;
let y = 2;
dbg!(x);
dbg!(y);
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 25, 2018

I suppose one alternative to the tuple discontinuity could be to return nothing at all when more than one argument is provided.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Sep 25, 2018

The original RFC, rust-lang/rfcs#2173, considered it and dealt with this by giving you back a tuple; e.g. dbg!(a, b, c) : (typeof(a), typeof(b), typeof(c)). If we're going to support multiple arguments then I think that is the natural approach. Not returning anything would be surprising imo.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 25, 2018

And, to repeat the argument there: if dbg!(a, b, c) : (typeof(a), typeof(b), typeof(c)) and dbg!(a, b) : (typeof(a), typeof(b)) etc, should dbg!(a) : (typeof(a),) (a one-item tuple)? If not, why not?

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Sep 25, 2018

@SimonSapin because if you consider (a) then typeof( (a) ) == typeof( a ), but if you consider (a, b, c) then typeof( (a, b, c) ) == ( typeof(a), typeof(b), typeof(c) ), and finally if you consider (a,) then typeof( (a,) ) == ( typeof(a), ).

I think the consistent thing to do if you want to pass multiple arguments is to consider what the result would be if you removed the prefix dbg!, and then you would have:

dbg! (a) : typeof(a)
     (a) : typeof(a)

dbg! (a,) : (typeof(a),) // note the one-tuple type
     (a,) : (typeof(a),) // note the one-tuple type

dbg! (a, b) : (typeof(a), typeof(b))
     (a, b) : (typeof(a), typeof(b))

dbg! (a, b, c) : (typeof(a), typeof(b), typeof(c))
     (a, b, c) : (typeof(a), typeof(b), typeof(c))

This is symmetrical with how the language behaves and so surprises should be fewer.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 25, 2018

Exclamation mark macros invocations with parentheses are very close in syntax to function calls. The single-item tuple trailing comma syntax special case is specific to tuples, it does not apply to function calls: Box::new(foo,) is the same as Box::new(foo). In your dbg! (a,) example you inserted a space between the macro name and its arguments that is to present in more typical style.

Whatever we do about other cases, I think dbg!(a,) should be the same as dbg!(a).

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Sep 25, 2018

@SimonSapin I inserted the space to make the consistency I'm talking about more clear ;)

I think it is useful to think specifically about dbg! (rather than other macros...) in terms of what happens when you remove it, because that is how I think it will be used; e.g. you tack on dbg! and what tacked it onto gets printed.

For example, one starts out with let foo = (a, b);, then one just changes it to let foo = dbg!(a, b);. This is as easy as copy-pasting dbg! in front of all the places you want debugged and it generally works (up to precedence issues with binary operators and method calls) -- I think that's a valuable property to have.

I do think that the behavior I noted above is more consistent than not outputting anything when multiple arguments are passed.

Of course, another option is to not accept multiple arguments at all.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 25, 2018

I think dbg!(a,) should be the same as dbg!(a)

I feel strongly about this.

It seems that proposing otherwise was only a reaction to my earlier comment about the discontinuity between the single-arg v.s. multiple args. I’d rather live with that discontinuity than making a trailing comma significant.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Sep 25, 2018

For example, one starts out with let foo = (a, b);, then one just changes it to let foo = dbg!(a, b);.

This seems like a very artificial example constructed specifically for this argument. I think it is exceedingly rare that the debugged expression just happens to be a tuple literal. The normal case is that you add dbg!( and ) around an expression, not just dbg! with parens that just happen to already be in the right place.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Sep 25, 2018

If you are not tacking on dbg! onto an already existing expression such as (a, b) or (a,) - which I do agree is the normal case, then it seems to me that dbg!(a, b); (note the semi) is being written specifically to write things faster and to save characters (e.g. for ergonomics).

However, writing dbg!(a,); seems like a mistake someone made and not something written intentionally; if you do make such a mistake, then it will still work and a will be printed out because you are not using the resulting computation and instead you are throwing the value away, and so the resulting type is irrelevant.

The only problem arises when you remove the ; and write something like let foo = dbg!(a,); -- in these cases I think it is more likely that you already had let foo = (a, b, c); and so you want let foo = dbg!(a, b, c); to be a tuple.

@yoshuawuyts

This comment has been minimized.

Copy link
Member

yoshuawuyts commented Oct 9, 2018

It's been two weeks since the last update on this thread, so in order to help keep the discussion progressing I'm making an attempt to summarize the discussion so far. My hope is that by making explicit which points are agreed / disagreed on, we can keep this proposal going.

I hope I got everything right here; if I didn't let me know and I'll update this post. I hope this comes in useful. Thanks!

Summary

Resolved

  • Being able to do dbg!(a, b, c) is useful when debugging.
  • The return type of dbg!(a, b, c) should be a tuple of (a, b, c).
  • The return type of dbg!(a) should be a.

Unresolved

  • Should dbg!(a,) be allowed?
  • If dbg!(a,) is allowed, what should the return type be?

Discussion

I personally hope that the parts that are agreed on are enough that they can be moved forward with, allowing to focus the remainder of the discussion on the unresolved questions. That would hopefully put the dbg!() API further on the path to stabilization, making it available for more people!

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Oct 9, 2018

@yoshuawuyts Thanks for the summary :)

That would hopefully put the dbg!() API further on the path to stabilization, making it available for more people!

I think it should be possible to stabilize the macro as-is and then extend it further with multiple arguments, specialization (when that is stable), and other nice things in a backwards compatible way... Are there any concerns that prevent that?

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Oct 9, 2018

cc @oli-obk @alexreg As a general question... do we have any thoughts about how you debug during CTFE? It would be nice if there were some mechanism to do so in a const fn but which is a no-op at runtime? (this is unlikely to be solved on this tracking issue but I thought I'd record it somewhere less ephemeral... we should probably spawn of an issue from this...)

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Oct 9, 2018

Probably needs a separate issue, yeah. It's going to require special compiler support to get around the usual const-restrictions on this sort of thing (I/O).

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Oct 10, 2018

I agree, let's discuss this in a separate issue. We can get around the const restrictions by making the debug printer an intrinsic

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Oct 10, 2018

@oli-obk Aha yes, that crossed my mind, but I wasn't sure if it worked. Good to hear.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 21, 2018

@SimonSapin How do you feel about landing this as-is? We can revisit extensions such as multiple arguments and such later? Would be nice to have this in 1.32. :)

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Nov 21, 2018

The unresolved question is what to do about dbg!(x,). It is currently not matched by the macro, so we can compatibly add it later. Landing as-is sounds good to me.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 21, 2018

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), 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

This comment has been minimized.

Copy link

rfcbot commented Nov 21, 2018

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

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Nov 21, 2018

🎉🎈 Woohoo...! 1.32 is gonna be amazing! =P 🎈🎉

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 1, 2018

Stabilization PR filed in #56395.

@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 1, 2018

After stabilizing, we might want to? keep this issue open for discussion about extensions and as a landing place for users who want to discuss dbg!.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 1, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.
@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 1, 2018

The final comment period, with a disposition to merge, as per the review above, is now complete.

Centril added a commit to Centril/rust that referenced this issue Dec 1, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.

Centril added a commit to Centril/rust that referenced this issue Dec 2, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.
@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Dec 3, 2018

@Centril

After stabilizing, we might want to? keep this issue open for discussion

I don't think it should stay open. Once the RFC is implemented and stabilized, this is done. People can use IRLO threads or the RFCs repo if they want to have discussions about things or propose changes.

kennytm added a commit to kennytm/rust that referenced this issue Dec 3, 2018

Rollup merge of rust-lang#56395 - Centril:stabilize-dbg-macro, r=Simo…
…nSapin

Stabilize dbg!(...)

Per FCP in rust-lang#54306 (which is ~1 day from completion).

r? @SimonSapin

The PR is fairly isolated so a rollup should probably work.
@Centril

This comment has been minimized.

Copy link
Contributor Author

Centril commented Dec 3, 2018

The stabilization PR was just merged so there's nothing left to do and thus we can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.