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

Include trailing comma in multiline Debug representation #59076

Merged
merged 1 commit into from
Apr 5, 2019

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Mar 10, 2019

This PR changes the behavior of Formatter::debug_struct, debug_tuple, debug_list, debug_set, and debug_map to render trailing commas in {:#?} mode, which is the dominant style in modern Rust code.

Before:

Language {
    name: "Rust",
    trailing_commas: false
}

After:

Language {
    name: "Rust",
    trailing_commas: true,
}

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 10, 2019
@rust-highfive
Copy link
Collaborator

r? @kennytm

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 10, 2019
@Mark-Simulacrum
Copy link
Member

Is there any reason to do this, beyond being nicer from a formatting perspective?

@rustbot modify labels to relnotes.

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Mar 11, 2019
@dtolnay
Copy link
Member Author

dtolnay commented Mar 11, 2019

I would say that's the only reason. I won't try to argue that this makes debugging easier. 😉

The current behavior of DebugStruct was designed before the code style for Rust had been as well established. You can see in RFC 640 which proposed {:#?} there is this code fragment under Motivation without a trailing comma:

pub struct BufferedStream<S> {
    inner: BufferedReader<InternalBufferedWriter<S>>
}

I believe if the feature were being implemented from scratch today it would certainly be done with a trailing comma. This PR only closes the gap between the way it was built in 2015 and the way we would build it today.

@petrochenkov
Copy link
Contributor

Is there any reason to do this, beyond being nicer from a formatting perspective?

Code in libcore/fmt is simpler, for example.
Being friendly to code generators is one of the primary reasons why trailing separators are supported in general.

@Mark-Simulacrum
Copy link
Member

I will make the note that this is likely to cause somewhat widespread test breakage in the community; I don't think that's a reason to not do this, but worth noting. We can run crater if we want to know ahead of time.

@bors try in case we decide to go ahead with a crater run

I presume we'll want an FCP for libs team? Let's try r? @Amanieu for the review

@rust-highfive rust-highfive assigned Amanieu and unassigned kennytm Mar 11, 2019
@bors
Copy link
Contributor

bors commented Mar 11, 2019

⌛ Trying commit ddcfe2b5d359b9a4b925f037a1874a0a4fcdc801 with merge 0f4bc26daf93fffc14b0a6557b12eab1844ecfda...

@bors
Copy link
Contributor

bors commented Mar 11, 2019

☀️ Try build successful - checks-travis
Build commit: 0f4bc26daf93fffc14b0a6557b12eab1844ecfda

@Centril Centril added this to the 1.35 milestone Mar 18, 2019
@Dylan-DPC-zz
Copy link

ping from triage @Amanieu / @rust-lang/libs any updates?

@alexcrichton
Copy link
Member

I would personally think that we can land this at any time (and should) but @Mark-Simulacrum do you feel that this definitely needs a crater run? I think I probably expect less breakage than you though

@Mark-Simulacrum
Copy link
Member

Not at all; I expect breakage but probably just local (i.e., tests) and we're generally pretty free to break those. We'll (someone from the release team) probably just open an issue once next beta is cut with this in it and we get a crater run on that, we can assess breakage then. Seems fine to land for now though!

@bors r=alexcrichton (I also glanced through but tests and nothing seems obviously bad)

@bors
Copy link
Contributor

bors commented Mar 19, 2019

📌 Commit ddcfe2b5d359b9a4b925f037a1874a0a4fcdc801 has been approved by alexcrichton

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2019
@bors
Copy link
Contributor

bors commented Mar 19, 2019

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 19, 2019
@KamilaBorowska
Copy link
Contributor

Well, it is a breaking change, and I saw code (although not in Rust) that parsed formatting output in order to get information that is otherwise unobtainable. However, while I can believe some Rust programmers did that, I would assume most were reasonable enough to use {:?} instead of {:#?} to avoid unnecessary whitespace.

@Centril
Copy link
Contributor

Centril commented Mar 30, 2019

Ping from triage, @dtolnay, you have some rebasing todo.

@bors

This comment has been minimized.

@dtolnay
Copy link
Member Author

dtolnay commented Apr 3, 2019

Rebased to fix conflict with #57847 in src/test/ui/rfc-2361-dbg-macro/dbg-macro-expected-behavior.rs.
Passed ./x.py test locally.

@bors r=alexcrichton

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 3, 2019
bors added a commit to rust-lang/cargo that referenced this pull request Apr 3, 2019
Accept trailing comma in test of impl Debug for PackageId

The standard library is planning to begin emitting trailing commas in multiline Debug representations to align with the dominant style in modern Rust code -- rust-lang/rust#59076.

```diff
  PackageId {
      name: "foo",
      version: "1.0.0",
-     source: "registry `https://github.com/rust-lang/crates.io-index`"
+     source: "registry `https://github.com/rust-lang/crates.io-index`",
  }
```

For now, change this tests to accept both with and without trailing comma. Once the trailing comma change reaches the stable channel we will be able to remove one of the cases.
@dtolnay dtolnay mentioned this pull request Apr 3, 2019
@dtolnay
Copy link
Member Author

dtolnay commented Apr 4, 2019

Working on it -- I submitted rust-lang/cargo#6818 to fix the test case in Cargo, currently waiting on the Cargo update in rust-lang/rust: #59681.

Centril added a commit to Centril/rust that referenced this pull request Apr 5, 2019
Update cargo

20 commits in
63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c
2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000
- Fix Init for Fossil SCM project (rust-lang/cargo#6792)
- Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799)
- Don't include email if it is empty (rust-lang/cargo#6802)
- Fix unused import warning (rust-lang/cargo#6807)
- Add some help and documentation for unstable flags (rust-lang/cargo#6791)
- Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803)
- Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804)
- Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805)
- Warn on version req with metadata (rust-lang/cargo#6806)
- cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801)
- Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800)
- Cargo clippy (rust-lang/cargo#6759)
- Don't include metadata in wasm binary examples (rust-lang/cargo#6812)
- Update glossary for `feature` (rust-lang/cargo#6809)
- Include proc-macros in `build-override` (rust-lang/cargo#6811)
- Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776)
- Add some docs for `Downloads` (rust-lang/cargo#6815)
- Resolve: Be less strict while offline (rust-lang/cargo#6814)
- Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)
- Fix doc link (rust-lang/cargo#6820)

<br>

I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock rust-lang#59076.

Mentioning @ehuss.
bors added a commit that referenced this pull request Apr 5, 2019
Update cargo

20 commits in
63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c
2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000
- Fix Init for Fossil SCM project (rust-lang/cargo#6792)
- Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799)
- Don't include email if it is empty (rust-lang/cargo#6802)
- Fix unused import warning (rust-lang/cargo#6807)
- Add some help and documentation for unstable flags (rust-lang/cargo#6791)
- Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803)
- Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804)
- Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805)
- Warn on version req with metadata (rust-lang/cargo#6806)
- cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801)
- Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800)
- Cargo clippy (rust-lang/cargo#6759)
- Don't include metadata in wasm binary examples (rust-lang/cargo#6812)
- Update glossary for `feature` (rust-lang/cargo#6809)
- Include proc-macros in `build-override` (rust-lang/cargo#6811)
- Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776)
- Add some docs for `Downloads` (rust-lang/cargo#6815)
- Resolve: Be less strict while offline (rust-lang/cargo#6814)
- Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)
- Fix doc link (rust-lang/cargo#6820)

<br>

I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock #59076.

Mentioning @ehuss.
This commit changes the behavior of Formatter::debug_struct,
debug_tuple, debug_list, debug_set, and debug_map to render trailing
commas in {:#?} mode, which is the dominant style in modern Rust code.

Before:

    Language {
        name: "Rust",
        trailing_commas: false
    }

After:

    Language {
        name: "Rust",
        trailing_commas: true,
    }
@dtolnay
Copy link
Member Author

dtolnay commented Apr 5, 2019

Rebased on #59681 and tested again with ./x.py test. Is there a more exhaustive test command that I could run locally that would run the test suites of whichever tools are tested in Travis?

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Apr 5, 2019

📌 Commit cfd31fb has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 5, 2019
@bors
Copy link
Contributor

bors commented Apr 5, 2019

⌛ Testing commit cfd31fb with merge 20dbf28...

bors added a commit that referenced this pull request Apr 5, 2019
Include trailing comma in multiline Debug representation

This PR changes the behavior of [`Formatter::debug_struct`](https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_struct), [`debug_tuple`](https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_tuple), [`debug_list`](https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_list), [`debug_set`](https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_set), and [`debug_map`](https://doc.rust-lang.org/std/fmt/struct.Formatter.html#method.debug_map) to render trailing commas in `{:#?}` mode, which is the dominant style in modern Rust code.

#### Before:

```console
Language {
    name: "Rust",
    trailing_commas: false
}
```

#### After:

```console
Language {
    name: "Rust",
    trailing_commas: true,
}
```
@bors
Copy link
Contributor

bors commented Apr 5, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 20dbf28 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 5, 2019
@bors bors merged commit cfd31fb into rust-lang:master Apr 5, 2019
@dtolnay dtolnay deleted the comma branch April 5, 2019 17:54
teskje added a commit to teskje/lalrpop that referenced this pull request Apr 28, 2019
rust-lang/rust/pull/59076 changed the multiline Debug representation to
always include a trailing comma. This change currently breaks our tests
on beta and nightly, since we use this Debug representation to compare
expected parsing output.

We cannot simply add the trailing commas to the expected output strings,
since that would break on older rustc versions we support. Instead, this
commit makes it so that all trailing commas are stripped before the
comparison.

This workaround can be removed once rust-lang/rust/pull/59076 reaches
our minimum supported rustc version.
Marwes pushed a commit to lalrpop/lalrpop that referenced this pull request Apr 30, 2019
* Ignore trailing commas in `ExpectedDebug`

rust-lang/rust/pull/59076 changed the multiline Debug representation to
always include a trailing comma. This change currently breaks our tests
on beta and nightly, since we use this Debug representation to compare
expected parsing output.

We cannot simply add the trailing commas to the expected output strings,
since that would break on older rustc versions we support. Instead, this
commit makes it so that all trailing commas are stripped before the
comparison.

This workaround can be removed once rust-lang/rust/pull/59076 reaches
our minimum supported rustc version.

* Bump minimum Rust version to 1.31.0

That's required by the current version rustc-demangle, which is a
dependency of mdbook.
Marwes pushed a commit to lalrpop/lalrpop that referenced this pull request Apr 30, 2019
* Ignore trailing commas in `ExpectedDebug`

rust-lang/rust/pull/59076 changed the multiline Debug representation to
always include a trailing comma. This change currently breaks our tests
on beta and nightly, since we use this Debug representation to compare
expected parsing output.

We cannot simply add the trailing commas to the expected output strings,
since that would break on older rustc versions we support. Instead, this
commit makes it so that all trailing commas are stripped before the
comparison.

This workaround can be removed once rust-lang/rust/pull/59076 reaches
our minimum supported rustc version.

* Bump minimum Rust version to 1.31.0

That's required by the current version rustc-demangle, which is a
dependency of mdbook.

* Replace all uses of `try!` with `?`

The `?` operator was added to replace the `try!` macro and should be
used in modern Rust code. `try` is also a reserved keyword in Rust 2018,
so this change prepares for a possible edition switch too.
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 31, 2019
Version 1.35.0 (2019-05-23)
==========================

Language
--------
- [`FnOnce`, `FnMut`, and the `Fn` traits are now implemented for `Box<FnOnce>`,
  `Box<FnMut>`, and `Box<Fn>` respectively.][59500]
- [You can now coerce closures into unsafe function pointers.][59580] e.g.
  ```rust
  unsafe fn call_unsafe(func: unsafe fn()) {
      func()
  }

  pub fn main() {
      unsafe { call_unsafe(|| {}); }
  }
  ```


Compiler
--------
- [Added the `armv6-unknown-freebsd-gnueabihf` and
  `armv7-unknown-freebsd-gnueabihf` targets.][58080]
- [Added the `wasm32-unknown-wasi` target.][59464]


Libraries
---------
- [`Thread` will now show its ID in `Debug` output.][59460]
- [`StdinLock`, `StdoutLock`, and `StderrLock` now implement `AsRawFd`.][59512]
- [`alloc::System` now implements `Default`.][59451]
- [Expanded `Debug` output (`{:#?}`) for structs now has a trailing comma on the
  last field.][59076]
- [`char::{ToLowercase, ToUppercase}` now
  implement `ExactSizeIterator`.][58778]
- [All `NonZero` numeric types now implement `FromStr`.][58717]
- [Removed the `Read` trait bounds
  on the `BufReader::{get_ref, get_mut, into_inner}` methods.][58423]
- [You can now call the `dbg!` macro without any parameters to print the file
  and line where it is called.][57847]
- [In place ASCII case conversions are now up to 4× faster.][59283]
  e.g. `str::make_ascii_lowercase`
- [`hash_map::{OccupiedEntry, VacantEntry}` now implement `Sync`
  and `Send`.][58369]

Stabilized APIs
---------------
- [`f32::copysign`]
- [`f64::copysign`]
- [`RefCell::replace_with`]
- [`RefCell::map_split`]
- [`ptr::hash`]
- [`Range::contains`]
- [`RangeFrom::contains`]
- [`RangeTo::contains`]
- [`RangeInclusive::contains`]
- [`RangeToInclusive::contains`]
- [`Option::copied`]

Cargo
-----
- [You can now set `cargo:rustc-cdylib-link-arg` at build time to pass custom
  linker arguments when building a `cdylib`.][cargo/6298] Its usage is highly
  platform specific.

Misc
----
- [The Rust toolchain is now available natively for musl based distros.][58575]

[59460]: rust-lang/rust#59460
[59464]: rust-lang/rust#59464
[59500]: rust-lang/rust#59500
[59512]: rust-lang/rust#59512
[59580]: rust-lang/rust#59580
[59283]: rust-lang/rust#59283
[59451]: rust-lang/rust#59451
[59076]: rust-lang/rust#59076
[58778]: rust-lang/rust#58778
[58717]: rust-lang/rust#58717
[58369]: rust-lang/rust#58369
[58423]: rust-lang/rust#58423
[58080]: rust-lang/rust#58080
[57847]: rust-lang/rust#57847
[58575]: rust-lang/rust#58575
[cargo/6298]: rust-lang/cargo#6298
[`f32::copysign`]: https://doc.rust-lang.org/stable/std/primitive.f32.html#method.copysign
[`f64::copysign`]: https://doc.rust-lang.org/stable/std/primitive.f64.html#method.copysign
[`RefCell::replace_with`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.replace_with
[`RefCell::map_split`]: https://doc.rust-lang.org/stable/std/cell/struct.RefCell.html#method.map_split
[`ptr::hash`]: https://doc.rust-lang.org/stable/std/ptr/fn.hash.html
[`Range::contains`]: https://doc.rust-lang.org/std/ops/struct.Range.html#method.contains
[`RangeFrom::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeFrom.html#method.contains
[`RangeTo::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeTo.html#method.contains
[`RangeInclusive::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeInclusive.html#method.contains
[`RangeToInclusive::contains`]: https://doc.rust-lang.org/std/ops/struct.RangeToInclusive.html#method.contains
[`Option::copied`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.copied
waywardmonkeys added a commit to waywardmonkeys/petgraph that referenced this pull request Aug 11, 2019
Rust 1.35 changed the behavior of the Debug trait to include the
trailing comma in structs and other things. This deals with that
by not including all of the whitespace.

The Rust change that introduced this was rust-lang/rust#59076

Fixes petgraph#255.
waywardmonkeys added a commit to petgraph/petgraph that referenced this pull request Aug 12, 2019
Rust 1.35 changed the behavior of the Debug trait to include the
trailing comma in structs and other things. This deals with that
by not including all of the whitespace.

The Rust change that introduced this was rust-lang/rust#59076

Fixes #255.
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.