add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071

Centril · 2018-03-16T04:29:04Z

This adds the BestEffortDebug (formerly WrapDebug) wrapper discussed in rust-lang/rfcs#2361 and rust-lang/rfcs#2173.

Motivation

This allows macros such as debugit, dbg, p-macro to work in generic code without T: Debug bounds infecting the call stack. The reason for having it in libcore is that specialization is unstable, and that type_name is part of core_instrinsics which is forever unstable. In addition, the macros in question can reuse this type and so we get some code reuse.

Does this expose specialization?

Yes in the sense that you have to fall back to type_name and impl<T> BEDInternal for BestEffortDebug<T> if we decide to remove specialization entirely. The output thus becomes effectively useless, but no code will break as a result of removing specialization. The guarantees section in the documentation also explicitly states that the output format is not guaranteed.

No in the sense that specialization is not effectively stabilized as a result of this. The "inner helper trait" hack is used in other places such as: ArcFromSlice.

…libcore

…pe_name

…ebugInternal`

rust-highfive · 2018-03-16T04:29:08Z

r? @Kimundi

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

scottmcm · 2018-03-16T05:58:05Z

I do like the "there's a way to get debug output without threading T:Debug through" capability.

Bikeshed: Wrap is a super-generic word that doesn't really convey any meaning to me. It feels like there ought to be a better word, but the only thing jumping to mind is AlwaysDebug.

Centril · 2018-03-16T06:38:17Z

🚲🏘️

@scottmcm The name comes from: https://docs.rs/wrap-debug/0.1.1/wrap_debug/struct.WrapDebug.html but I'm not super excited about it as well.

AlwaysDebug, is kinda nice / better, but also not super exciting.

Centril · 2018-03-16T07:07:04Z

More 🚲🏘️

BestEffortDebug, this one is clear about the role the type has.
ValiantDebug - a synonym of BestEffortDebug more or less, but probably a less known adjective.
TryDebug / AttemptDebug - this causes confusion with the Try trait and TryFrom (error handling in general).

Since we've been OK historically with 3-worded types such as ExactSizeIterator it suggests that we should be OK with BestEffortDebug, which is most descriptive, so I'm changing to BestEffortDebug.

arthurprs · 2018-03-16T15:07:26Z

MaybeDebug, LazyDebug, ForceDebug also come to mind.

llogiq · 2018-03-21T11:41:39Z

I'd like to see an extension of this that shows the ptr (either directly or hashed) to the object in question for non-Debug references. This has precedence in languages like Java, where the standard toString() method prints out @ (which is both shorter and more informative than [<unknown> of type is !Debug]). Regarding the address, I'm personally fine with writing it out directly in hex (which means 8 digits), but others may prefer a shortened hashed version (e.g. CRC32) that is more readable while keeping the risk of accidential collisions within reasonable bounds.

Centril · 2018-03-22T09:24:43Z

@llogiq I specifically did not opt for such a design; the reason why is explained here: rust-lang/rfcs#2173 (comment) Relevant snippet:

We discussed including the address of expr in dbg!(expr) but since that is just the address of a temporary value inside the macro, it is actively harmful when expr is not a pointer. The user may also do: dbg!(&var as *const _) if they want pointer info.

In Java, the address is stable, so it is useful - this is not the case for the intended use case in Rust. Additionally, the output format of BestEffortDebug<T> is unspecified, so we can make future changes.

bluss · 2018-03-24T18:37:04Z

src/libcore/fmt/mod.rs

+    default fn fmt(&self, fmt: &mut Formatter) -> Result {
+        use intrinsics::type_name;
+        write!(fmt, "[<unknown> of type {} is !Debug]",
+            unsafe { type_name::<T>() })


I think the is !Debug part of the message is jargon-y and hard to understand. My idea of an improvement would be "[<unknown value> of type {}]"

I agree on the hard to understand bit, how about this?

"[<unknown value> of type {}, which does not implement Debug]"

The point of this being that if a library user sees this, they will know that they should bug the library author with a request to add #[derive(Debug)].

(That's much better, and I think the reason for that is good)

Centril · 2018-03-24T19:07:51Z

From my discussions with @nikomatsakis (ping) on WG-traits, it seems that this particular specialization would not be permitted under max-min specialization (bummer :/) and would require specialization predicates. Niko did say that it would be useful, but that the PR should probably be closed. So I'm not sure how to proceed...

shepmaster · 2018-03-30T19:31:17Z

Ping from triage, @Kimundi and @nikomatsakis ! Should we put this PR to bed, for now?

bors · 2018-04-07T14:41:57Z

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

Centril · 2018-04-07T15:34:56Z

An update: @aturon's recent blog post on specialize(..) would make the following possible:

pub struct BED<T>(pub T);

impl<T> Debug for BED<T> { .. }

impl<T> Debug for BED<T> where specialize(T: Debug) { .. }

this would still support all types, but : 'static and TyConstuctor<T, T> would fall back to the first impl.
However, it would be quite uncommon to have such impls and not be generic over 'a and TyCtor<T, U, ..>.

pietroalbini · 2018-04-16T10:24:27Z

Ping from triage! What's the status of this PR?

pietroalbini · 2018-04-23T11:00:12Z

Ping from triage! @Kimundi and @Centril, what's the status of this PR?

Kimundi · 2018-04-25T10:24:36Z

This seems like a generally useful addition. As the comment thread shows, it's building on aspects of rusts specialization rules that are in flux, but in the worst case it just makes the proposed functionality useless, not breaking. Since this developed as part of the much discussed dbg!() proposal, I'd like to get team feedback for this, but I think merging it should be fine.

@bors fcp merge

aturon · 2018-04-28T06:00:56Z

@Kimundi I think you meant:

@rfcbot fcp merge

rfcbot · 2018-04-28T06:00:56Z

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

Concerns:

not-useful-if-specialization-does-not-work (add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071 (comment))

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.

alexcrichton · 2018-04-30T14:33:45Z

@rfcbot concern not-useful-if-specialization-does-not-work

Specialization is still very much in flux AFAIK and it seems like it's still not 100% clear that there's a path to stability for functionality like this. @aturon's recent blog post is indeed one possible path but I don't think we're sure that it's sound and working, right?

Additionally I do not think that functionality like this is useful in libstd if specialization is removed. That's "basically a breaking change" as the whole point of this type would be specialization. Otherwise it's not offering much utility over a crates.io crate that just calls type_name or exposing that intrinsic in a stable fashion.

This to me seems like something that should live on crates.io until we know specialization works, then it can move into libstd.

alexcrichton · 2018-05-17T15:18:19Z

@aturon or @Centril do you have thoughts on the concern I listed above?

Centril · 2018-05-17T15:36:35Z

@alexcrichton Excuse the tardy reply ;)

I'll defer to @aturon re. the pace of implementation and stabilization of specialization.

However, you could leave BestEffortDebug as unstable while specialization is unstable;
you could also use BestEffortDebug as a way to keep dbg! itself unstable.
(Anyone who uses dbg!(..) will get an error about the unstable nature of BestEffortDebug...)

Additionally I do not think that functionality like this is useful in libstd if specialization is removed.

Granted; It would become effectively useless. My biggest fear is not that the helper type itself becomes useless, but that its reverse dependency, and primary usecase, dbg! becomes useless as a result of it.
In this sense, there is a stronger concern ;)

This to me seems like something that should live on crates.io until we know specialization works, then it can move into libstd.

It already does ;) Not as a standalone crate, but it does exist in the dbg and debugit crates. It is named WrapDebug and DebugIt in those crates.

Otherwise it's not offering much utility over a crates.io crate that just calls type_name or exposing that intrinsic in a stable fashion.

Aside: Speaking of type_name... should there be a stable wrapper function for it?

nikomatsakis · 2018-05-17T15:51:30Z

I think it seems fine to have this live in crates.io and not std until such time as stabilization is on a clear path to stability (implemented in the way we would want, etc)

alexcrichton · 2018-05-17T19:55:44Z

Hm ok so given that specialization is not on a clear path to stabilization, @Centril are you ok holding off on this PR for now?

Centril · 2018-05-17T20:37:57Z

@alexcrichton Sure thing :)

alexcrichton · 2018-05-18T17:37:06Z

Ok!

Centril added 6 commits March 16, 2018 04:34

add core/alloc/std::fmt::WrapDebug

198c1ab

add core/alloc/std::fmt::WrapDebug - #![feature(core_intrinsics)] in …

3c314fa

…libcore

add core/alloc/std::fmt::WrapDebug - global path core::intrinsics::ty…

05287c8

…pe_name

add core/alloc/std::fmt::WrapDebug - fix use intrinsics::type_name

d61e0b6

add core/alloc/std::fmt::WrapDebug - rewrite logic using `trait WrapD…

b472dc2

…ebugInternal`

add core/alloc/std::fmt::WrapDebug - fix use intrinsics::type_name

f36dc46

Centril added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 16, 2018

rust-highfive assigned Kimundi Mar 16, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2018

Merge branch 'master' into feature/wrap-debug

beeabd0

rename WrapDebug to BestEffortDebug

88a196b

Centril changed the title ~~add WrapDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug~~ add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug Mar 16, 2018

bluss reviewed Mar 24, 2018

View reviewed changes

sinkuu mentioned this pull request Mar 30, 2018

Implement RFC 2011 (nicer assert messages) #48973

Closed

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Apr 28, 2018

shepmaster added 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 May 6, 2018

alexcrichton closed this May 18, 2018

rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label May 18, 2018

Centril mentioned this pull request Jul 11, 2019

Add Option::expect_none(msg) and unwrap_none() #62596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071

add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071

Centril commented Mar 16, 2018 •

edited

Loading

rust-highfive commented Mar 16, 2018

scottmcm commented Mar 16, 2018

Centril commented Mar 16, 2018

Centril commented Mar 16, 2018

arthurprs commented Mar 16, 2018

llogiq commented Mar 21, 2018

Centril commented Mar 22, 2018

bluss Mar 24, 2018

Centril Mar 24, 2018

bluss Apr 1, 2018

Centril commented Mar 24, 2018 •

edited

Loading

shepmaster commented Mar 30, 2018

bors commented Apr 7, 2018

Centril commented Apr 7, 2018

pietroalbini commented Apr 16, 2018

pietroalbini commented Apr 23, 2018

Kimundi commented Apr 25, 2018

aturon commented Apr 28, 2018

rfcbot commented Apr 28, 2018 •

edited by dtolnay

Loading

alexcrichton commented Apr 30, 2018

alexcrichton commented May 17, 2018

Centril commented May 17, 2018

nikomatsakis commented May 17, 2018

alexcrichton commented May 17, 2018

Centril commented May 17, 2018

alexcrichton commented May 18, 2018

add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071

add BestEffortDebug<T>, an always-Debug wrapper over T irrespective of T: Debug / T: !Debug #49071

Conversation

Centril commented Mar 16, 2018 • edited Loading

Motivation

Does this expose specialization?

rust-highfive commented Mar 16, 2018

scottmcm commented Mar 16, 2018

Centril commented Mar 16, 2018

🚲🏘️

Centril commented Mar 16, 2018

More 🚲🏘️

arthurprs commented Mar 16, 2018

llogiq commented Mar 21, 2018

Centril commented Mar 22, 2018

bluss Mar 24, 2018

Choose a reason for hiding this comment

Centril Mar 24, 2018

Choose a reason for hiding this comment

bluss Apr 1, 2018

Choose a reason for hiding this comment

Centril commented Mar 24, 2018 • edited Loading

shepmaster commented Mar 30, 2018

bors commented Apr 7, 2018

Centril commented Apr 7, 2018

pietroalbini commented Apr 16, 2018

pietroalbini commented Apr 23, 2018

Kimundi commented Apr 25, 2018

aturon commented Apr 28, 2018

rfcbot commented Apr 28, 2018 • edited by dtolnay Loading

alexcrichton commented Apr 30, 2018

alexcrichton commented May 17, 2018

Centril commented May 17, 2018

nikomatsakis commented May 17, 2018

alexcrichton commented May 17, 2018

Centril commented May 17, 2018

alexcrichton commented May 18, 2018

Centril commented Mar 16, 2018 •

edited

Loading

Centril commented Mar 24, 2018 •

edited

Loading

rfcbot commented Apr 28, 2018 •

edited by dtolnay

Loading