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

Debug improvements #640

Merged
merged 4 commits into from Mar 5, 2015
Merged

Debug improvements #640

merged 4 commits into from Mar 5, 2015

Conversation

@sfackler
Copy link
Member

@sfackler sfackler commented Jan 21, 2015

Rendered

@sfackler sfackler changed the title Debug improvements RFC Debug improvements Jan 21, 2015
@blaenk
Copy link
Contributor

@blaenk blaenk commented Jan 21, 2015

Yeah, this would be great!

[
"a",
"b",
"c"
Copy link
Contributor

@ftxqxd ftxqxd Jan 21, 2015

Maybe these (and similar things with multiple elements, each on its own line) should be printed with trailing commas? I suppose an exception would have to be made to the last paragraph of this section:

In all cases, pretty printed and non-pretty printed output should differ only in the addition of newlines, whitespace, and trailing commas (i.e., commas that are directly followed by a closing delimiter on the next line, ignoring whitespace).

Copy link
Member Author

@sfackler sfackler Jan 21, 2015

I would be fine with a trailing comma, but I think we should be consistent between the normal and pretty printed outputs.

Copy link
Member

@steveklabnik steveklabnik Mar 4, 2015

I don't think the consistency is important, as I'd write the trailing comma in the pretty-printed style, but not in the compact one.

@kennytm
Copy link
Member

@kennytm kennytm commented Jan 21, 2015

Do you mean we're going to rename Show to Debug? Because I don't think we should add another trait just for the # flag that can be checked at runtime formatter.flags() & (1 << FlagAlternate).

@ftxqxd
Copy link
Contributor

@ftxqxd ftxqxd commented Jan 21, 2015

@kennytm #565 renamed String and Show to Display and Debug respectively. (I only heard about the renaming recently, too.)

@aldanor
Copy link

@aldanor aldanor commented Jan 21, 2015

Great proposal, 👍

@kennytm
Copy link
Member

@kennytm kennytm commented Jan 21, 2015

@P1start I see, thanks.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jan 21, 2015

Thanks for writing this up! It seems like it'd help a lot with various debugging here and there.

  • Will this alter the expansion of derive to use the debugging helper structs by default?
  • Instead of having to import ShowStruct, perhaps this could be an inherent method on Formatter? Something like fmt.debug_struct().field(..).finish() perhaps. Similarly, we could have fmt.padded(n) which would pad all future output by n spaces.
  • Perhaps the field method could take S: Display by value? That would avoid a mandated leading & while still allowing it to be optionally placed.

cc @wycats, @seanmonstar, this seems similar to our work week discussions of Show/String/friends

@sfackler
Copy link
Member Author

@sfackler sfackler commented Jan 21, 2015

Using methods on Formatter seems like a good idea. I'm not sure that padded as described would work because you only want to add the extra padding when formatting the sub-type, and then reset to the current "normal" padding to handle the indent for the next sub-type's line.

@sfackler
Copy link
Member Author

@sfackler sfackler commented Jan 21, 2015

It could return a struct implementing Writer that writes to the Formatter.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jan 21, 2015

Ah yes sorry that's what I was thinking as well, fmt.debug_struct() would consume fmt (the mutable pointer) and then you'd write to the returned struct. Similarly fmt.padded(n) would consume fmt and you would write to the returned struct, aka:

write!(fmt, "data");
write!(fmt.padded(4), "padded data");
write!(fmt, "more non-padded data");

@sfackler
Copy link
Member Author

@sfackler sfackler commented Jan 21, 2015

Yep, that seems pretty reasonable.

@sfackler
Copy link
Member Author

@sfackler sfackler commented Jan 22, 2015

@alexcrichton updated

@Ericson2314
Copy link
Contributor

@Ericson2314 Ericson2314 commented Jan 25, 2015

I very much approve of the idea, but am worried about some of the specifics.

First of all, I think polymorphic types should be instantiated for clarity, i.e. id<T<U>>(...) or id<T<U>> { ... }. This means an additional associated constant for type name reflection.

Second, for good pretty printing, you want to new-line separate vs space separate to respect line length and consistency. I.E.

line length --------------------------|
struct foo<T>{ bar, baz, big_thing: {
        asdf,
        qwer,
    },
    small,
}

Is no good.

I need to consult the exist literature, but I'm pretty sure what this means is that the data structure being printed should first be converted to a doc type that represents its structure, and then THAT is printed "atomically". In this way the thing that traverses the doc data structure is both away of each node's "supertree" and "subtree", both of which effect proper formatting.

I was inclined to say the data structure should be built functionally rather than imperatively (like the calls to fmt), because the alternative is way more complex and potentially unsafe. But unless rustc can deforest that data structure in the ugly print case, perhaps its better to keep the imperative interface so that debugging without dynamic allocation works. Maybe guards can be used to ensure all brackets are closed, and even closed in the correct order (borrow close-guard to make sub expr).

@aturon aturon assigned alexcrichton and unassigned aturon Feb 16, 2015
@aturon
Copy link
Member

@aturon aturon commented Feb 16, 2015

(I'm re-assigning to @alexcrichton as the shepherd, since he's been more involved in the discussion so far.)

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 4, 2015

Seems good to me.

@Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Mar 4, 2015

Typo:

The Debug trait is intended to be implemented by every trait type

Also, in your example:

.field("write_buffer", &format_args!("{}/{}", writer.pos, writer.buf.len()))

Won't this display as:
write_buffer: "x/y" rather than write_buffer: x/y and is that intentional?

@sfackler
Copy link
Member Author

@sfackler sfackler commented Mar 4, 2015

No, it'll display as the second form. Why do you think it wouldn't?

@Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Mar 4, 2015

The reason it currently works is that "fmt::Arguments" implements Debug to return the raw formatted string. However, for consistency with other Debug implementations, it should either return a quoted string, which is what "&str" and "String" currently return, or struct initializer syntax as per this rfc.

Basically, it's somewhat coincidental that it currently works, perhaps there should be another method:
.field_raw("write_buffer", format!("{}/{}", ...))
which just outputs the parameter directly, instead of recursively calling "Debug::fmt" on the argument.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Mar 5, 2015

There seems to be pretty broad support for this at this time and it should help make debugging these forms of structures much easier to write. These APIs will all land as initially #[unstable] so we'll still have some time to tweak the APIs a bit before they become #[stable] to accommodate any surprising use cases that come up!

Thanks again for the RFC @sfackler!

@alexcrichton alexcrichton merged commit be42152 into rust-lang:master Mar 5, 2015
nagisa added a commit to nagisa/rfcs that referenced this issue Mar 16, 2015
nagisa added a commit to nagisa/rfcs that referenced this issue Mar 16, 2015
nagisa added a commit to nagisa/rfcs that referenced this issue Mar 16, 2015
steveklabnik added a commit that referenced this issue Mar 16, 2015
@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Apr 2, 2015

Should assert_eq! default to using this?

@sfackler sfackler deleted the debug-improvements branch Apr 2, 2015
@nwoeanhinnogaehr
Copy link

@nwoeanhinnogaehr nwoeanhinnogaehr commented Apr 4, 2015

Seems like it should to me, asserts are only really for debugging so it would be great to have stuff pretty printed there . There's also lots of stuff in src/libcore/fmt/mod.rs that doesn't account for this yet.

I for one would like to have the ability to change the indentation size since it's quite easy to overflow the terminal width when printing recursive data structures but I understand this may not be desirable since it goes against the style guidelines.

@liigo
Copy link
Contributor

@liigo liigo commented Jul 8, 2015

keywords: pretty debug builders
impl: rust-lang/rust#23162
(to help people finding this rfc)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet