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

Strip code to the left and right in diagnostics for long lines #63402

Merged
merged 9 commits into from
Aug 30, 2019

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Aug 9, 2019

Fix #62999.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 Aug 9, 2019
@rust-highfive

This comment has been minimized.

@estebank
Copy link
Contributor Author

estebank commented Aug 9, 2019

TODO:

  • Rebase
  • Fix tests
  • Handle Unicode slicing correctly
  • Use calculated margins to aim at the span being roughly centered, but bias towards stripping more whitespace than code
  • Add width rustc flag so that cargo can set the current column width (running cargo will default the output to 140 chars)
  • Consider centering the position of the span as much as possible.

Current look on the code in the original report:

Screen Shot 2019-08-08 at 11 17 09 PM


Edit: current look
Screen Shot 2019-08-14 at 11 36 15 AM


Edit:

Screen Shot 2019-08-14 at 2 21 57 PM

@estebank estebank changed the title [WIP] Strip code to the left and right in diagnostics for long lines Strip code to the left and right in diagnostics for long lines Aug 14, 2019
@estebank
Copy link
Contributor Author

estebank commented Aug 14, 2019

@rust-lang/wg-diagnostics @Centril @nikomatsakis you all might be interested in taking a look at this.

What it looks like in practice:

Screen Shot 2019-08-14 at 11 39 50 AM


Update: now centered:

Screen Shot 2019-08-14 at 2 06 37 PM

@Centril
Copy link
Contributor

Centril commented Aug 14, 2019

@estebank how does it look right now?

@estebank
Copy link
Contributor Author

how does it look right now?

@Centril I'm not sure I follow? I have screenshots in the comments.

Right now the logic is:

@rust-highfive

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Aug 14, 2019

@estebank sounds good :)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 15, 2019

Idk how I feel about this. It will make the move to annotate_snippets harder

@eddyb
Copy link
Member

eddyb commented Aug 15, 2019

@oli-obk Can we add this to annotate_snippets instead, and try to speed up the move?

@bors

This comment has been minimized.

@eddyb eddyb 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2019
@eddyb
Copy link
Member

eddyb commented Aug 18, 2019

Blocked on #63402 (comment) (or at least a decision regarding it, maybe this should've been nominated for compiler-team meeting?).

@Centril Centril added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 18, 2019
@estebank
Copy link
Contributor Author

estebank commented Aug 19, 2019

I would be ok with adding the feature to annotate-snippets, but I would like to land this asap as when it is needed, it is really needed, and it'll be useful to gather actual real world usage feedback, even if we never land it in stable. I want to see how people react.

Edit: furthermore, some of the changes include extra flags that cargo needs to know about, so it'd be nice to have it in place to speed up that part of the work.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 20, 2019

ok, let's land this without touching annotate snippets first

@oli-obk
Copy link
Contributor

oli-obk commented Aug 30, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Aug 30, 2019

📌 Commit aaf4dc3 has been approved by oli-obk

@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 Aug 30, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Aug 30, 2019

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb Aug 30, 2019
@bors
Copy link
Contributor

bors commented Aug 30, 2019

⌛ Testing commit aaf4dc3 with merge 19a38de...

bors added a commit that referenced this pull request Aug 30, 2019
Strip code to the left and right in diagnostics for long lines

Fix #62999.
@bors
Copy link
Contributor

bors commented Aug 30, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 19a38de to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 30, 2019
@bors bors merged commit aaf4dc3 into rust-lang:master Aug 30, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #63402!

Tested on commit 19a38de.
Direct link to PR: #63402

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 30, 2019
Tested on commit rust-lang/rust@19a38de.
Direct link to PR: <rust-lang/rust#63402>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @RalfJung @eddyb, @rust-lang/infra).
🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
@RalfJung
Copy link
Member

Interesting. This PR introduced an ICE in a Miri test.

2019-08-30T09:21:21.4479192Z error: failure produced the wrong error: exit code: 101
2019-08-30T09:21:21.4479270Z status: exit code: 101
2019-08-30T09:21:21.4479913Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2-tools-bin/miri" "tests/compile-fail/validity/invalid_char.rs" "-L" "/tmp/compiletestkZNor4" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-C" "prefer-dynamic" "-o" "/tmp/compiletestkZNor4/validity/invalid_char.stage-id" "--edition" "2018" "-Astable-features" "--sysroot" "/home/user/.cache/miri/HOST" "-L" "/tmp/compiletestkZNor4/validity/invalid_char.stage-id.aux" "-A" "unused"
2019-08-30T09:21:21.4480668Z stdout:
2019-08-30T09:21:21.4481022Z ------------------------------------------
2019-08-30T09:21:21.4481081Z 
2019-08-30T09:21:21.4481328Z ------------------------------------------
2019-08-30T09:21:21.4481434Z stderr:
2019-08-30T09:21:21.4481671Z ------------------------------------------
2019-08-30T09:21:21.4482018Z thread 'rustc' panicked at 'attempt to subtract with overflow', src/librustc_errors/emitter.rs:154:38
2019-08-30T09:21:21.4482129Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-08-30T09:21:21.4482208Z 
2019-08-30T09:21:21.4482275Z error: internal compiler error: unexpected panic
2019-08-30T09:21:21.4482342Z 
2019-08-30T09:21:21.4482411Z note: the compiler unexpectedly panicked. this is a bug.
2019-08-30T09:21:21.4482462Z 
2019-08-30T09:21:21.4482813Z note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
2019-08-30T09:21:21.4482896Z 
2019-08-30T09:21:21.4483177Z note: rustc 1.39.0-dev running on x86_64-unknown-linux-gnu
2019-08-30T09:21:21.4483234Z 
2019-08-30T09:21:21.4483485Z note: compiler flags: -C prefer-dynamic
2019-08-30T09:21:21.4483538Z 
2019-08-30T09:21:21.4483574Z 
2019-08-30T09:21:21.4484155Z ------------------------------------------
2019-08-30T09:21:21.4484197Z 
2019-08-30T09:21:21.4484526Z thread '[compile-fail] compile-fail/validity/invalid_char.rs' panicked at 'explicit panic', /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/compiletest_rs-0.3.22/src/runtest.rs:2632:9
2019-08-30T09:21:21.4484644Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-08-30T09:21:21.4493103Z test [compile-fail] compile-fail/validity/invalid_char.rs ... FAILED

@estebank any idea why?

} else if self.span_right - self.span_left <= self.column_width {
// Attempt to fit the code window considering the spans and labels plus padding.
let padding_left = (self.column_width - (self.span_right - self.span_left)) / 5 * 2;
self.computed_left = self.span_left - padding_left;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this subtraction overflows when Miri emits an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RalfJung is that the only test failing? What would be the correct steps for me to repro this? I have never used Miri. Otherwise, would you be up to taking this on? I'm sure it's a small change that's needed and hopefully there are enough comments to follow the somewhat convoluted logic.

Copy link
Member

@RalfJung RalfJung Aug 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the only test failing, yes. And I'm afraid I am leaving for vacation this weekend and probably won't have time for this before I leave. I also looked at the code while on the train today and could not figure out what all the magic constants (5, 2) were about...

What would be the correct steps for me to repro this?

  • Do a local rustc build with debug assertions enabled: ./x.py build src/rustc
  • Set up a rustup toolchain link for the stage2 directory of that build
  • git clone https://github.com/rust-lang/miri/, then cd into that dir
  • rustup override set <the linked toolchain>
  • RUST_BACKTRACE=1 ./miri run-debug tests/compile-fail/validity/invalid_char.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #64029. Off-by-one error due to rounding.

Centril added a commit to Centril/rust that referenced this pull request Sep 1, 2019
Account for rounding errors when deciding the diagnostic boundaries

Fix Miri by fixing the bug raised in rust-lang#63402 (comment).

Fixes rust-lang#64020
@joshtriplett joshtriplett added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 4, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
bors added a commit to rust-lang/cargo that referenced this pull request Jul 9, 2020
Add support for rustc's `-Z terminal-width`.

This PR continues the work started in #7315, adding support for rustc's `-Z terminal-width` flag, which is used to trim diagnostic output to fit within the current terminal and was added in rust-lang/rust#63402 (with JSON emitter support in rust-lang/rust#73763).

At the time of writing, rust-lang/rust#73763 isn't in nightly, so the test added in this PR will fail, but it should pass tomorrow (I've confirmed that it works with a local rustc build).

cc @estebank
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustc should "ellipsis" very long lines when printing warnings
8 participants