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

Implement `DebugStruct::non_exhaustive` and `DebugStruct::opaque_field`. #66716

Open
wants to merge 3 commits into
base: master
from

Conversation

@derekdreery
Copy link
Contributor

derekdreery commented Nov 24, 2019

I suggested adding a method to DebugStruct that indicated there is more data contained in the struct that is hidden. It was suggested on internals that this would just be a PR, since it's a small change that goes in as unstable.

The opaque_field method was super-simple to add so I added it, but maybe it doesn't belong in this PR. If so I'll remove it.

Open questions

  1. Should the method opaque_field be removed?
  2. Should the non_exhaustive functionality be added to DebugTuple and DebugMap as well?
  3. (note to myself) How do I make this mergeable (issue number at least)?

Example

#![feature(debug_non_exhaustive)]
use std::fmt;

struct Bar {
    bar: i32,
    hidden: f32,
}

impl fmt::Debug for Bar {
    fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
        fmt.debug_struct("Bar")
           .field("bar", &self.bar)
           .non_exhaustive(true) // Show that some other field(s) exist.
           .finish()
    }
}

assert_eq!(
    format!("{:?}", Bar { bar: 10, hidden: 1.0 }),
    "Bar { bar: 10, .. }",
);

EDIT

The implementation of finish is a little complex, because there are three bimodal dimensions (alternate format, non-exhaustive, and has_fields), and so 2^3 = 8 possible permutations. If you include existing tests, most of these are covered by tests.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 24, 2019

r? @kennytm

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

@derekdreery

This comment has been minimized.

Copy link
Contributor Author

derekdreery commented Nov 24, 2019

r? @scottmcm since they saw the internals post.

@rust-highfive rust-highfive assigned scottmcm and unassigned kennytm Nov 24, 2019
/// # #![feature(debug_non_exhaustive)]
/// use std::fmt;
///
/// struct Foo;

This comment has been minimized.

Copy link
@CAD97

CAD97 Nov 24, 2019

Contributor

I'd add a field foo to this structure, just for clarity's sake.

Probably Box<dyn (pick your favorite object safe trait that isn't `Debug`)>.

@CAD97

This comment has been minimized.

Copy link
Contributor

CAD97 commented Nov 24, 2019

Would it be simpler to print non_exhaustive directly, like as a field? (Though this would mean that it would be possible to make the output awkward by calling non_exhaustive not directly before finish; maybe it could even be e.g. finish_non_exhaustive to avoid that?)

e.g. smth like

    pub fn non_exhaustive(&mut self) -> &mut DebugStruct<'a, 'b> {
        self.result = self.result.and_then(|_| {
            if self.is_pretty() {
                if !self.has_fields {
                    self.fmt.write_str(" {\n")?;
                }
                let mut slot = None;
                let mut state = Default::default();
                let mut writer = PadAdapter::wrap(&mut self.fmt, &mut slot, &mut state);
                writer.write_str("..\n")
            } else {
                let prefix = if self.has_fields { ", " } else { " { " };
                self.fmt.write_str(prefix)?;
                self.fmt.write_str("..")
            }
        });
        self.has_fields = true;
        self
    }

    pub fn finish_non_exhaustive(&mut self) -> fmt::Result {
        self.non_exhaustive().finish()
    }
@derekdreery

This comment has been minimized.

Copy link
Contributor Author

derekdreery commented Nov 24, 2019

@CAD97

Would it be simpler to print non_exhaustive directly, like as a field? (Though this would mean that it would be possible to make the output awkward by calling non_exhaustive not directly before finish; maybe it could even be e.g. finish_non_exhaustive to avoid that?)

That is certainly another possibility, I'd be interested to hear other people's opinions.

@@ -190,7 +195,7 @@ impl<'a, 'b: 'a> DebugStruct<'a, 'b> {
/// }
///
/// assert_eq!(
/// format!("{:?}", Foo),
/// format!("{:?}", Foo { foo: Box::new([1, 2].into_iter().copied()) }),

This comment has been minimized.

Copy link
@CAD97

CAD97 Nov 24, 2019

Contributor

🚩🚩🚩🚩🚩🚩

You do not want to use .into_iter().copied() here. This takes an autoref to &[_] and will break when we eventually add impl IntoIterator for [_]. Use .iter() instead.

This comment has been minimized.

Copy link
@derekdreery

derekdreery Nov 24, 2019

Author Contributor

okies thanks!

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Nov 30, 2019

Ping from triage: @scottmcm - all checks pass, can you please review this PR?

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