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

NLL: On "cannot move out of type" error, print original before rewrite #52907

Conversation

Projects
None yet
4 participants
@pnkfelix
Copy link
Member

pnkfelix commented Jul 31, 2018

NLL: On "cannot move out of type" error, print original source before rewrite.

  • Arguably this change is sometimes injecting noise into the output (namely in the cases where the suggested rewrite is inline with the suggestion and we end up highlighting the original source code). I would not be opposed to something more aggressive/dynamic, like revising the suggestion code to automatically print the original source when necessary (e.g. when the error does not have a span that includes the span of the suggestion).

  • Also, as another note on this change: The doc comment for Diagnostic::span_suggestion says:

    /// The message
    ///
    /// * should not end in any punctuation (a `:` is added automatically)
    /// * should not be a question
    /// * should not contain any parts like "the following", "as shown"
  • but the : is not added when the emitted line appears out-of-line relative to the suggestion. I find that to be an unfortunate UI experience.

As a drive-by fix, also changed code to combine multiple suggestions for a pattern into a single multipart suggestion (which vastly improves user experience IMO).


Includes the updates to expected NLL diagnostics.

Fix #52877

NLL: On "cannot move out of type" error, print original source before…
… rewrite.

 * Arguably this change is sometimes injecting noise into the output
   (namely in the cases where the suggested rewrite is inline with the
   suggestion and we end up highlighting the original source code).
   I would not be opposed to something more aggressive/dynamic, like
   revising the suggestion code to automatically print the original
   source when necessary (e.g. when the error does not have a span
   that includes the span of the suggestion).

 * Also, as another note on this change: The doc comment for `Diagnostic::span_suggestion`
   says:
    /// The message
    ///
    /// * should not end in any punctuation (a `:` is added automatically)
    /// * should not be a question
    /// * should not contain any parts like "the following", "as shown"

   but the `:` is *not* added when the emitted line appears
   out-of-line relative to the suggestion. I find that to be an
   unfortunate UI experience.

----

As a drive-by fix, also changed code to combine multiple suggestions
for a pattern into a single multipart suggestion (which vastly
improves user experience IMO).

----

Includes the updates to expected NLL diagnostics.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jul 31, 2018

r? @petrochenkov

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jul 31, 2018

LGTM, @bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 31, 2018

📌 Commit a5a3f76 has been approved by petrochenkov

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 1, 2018

Rollup merge of rust-lang#52907 - pnkfelix:issue-52877-original-sourc…
…e-should-precede-suggestions, r=petrochenkov

NLL: On "cannot move out of type" error, print original before rewrite

NLL: On "cannot move out of type" error, print original source before rewrite.

 * Arguably this change is sometimes injecting noise into the output  (namely in the cases where the suggested rewrite is inline with the   suggestion and we end up highlighting the original source code).   I would not be opposed to something more aggressive/dynamic, like   revising the suggestion code to automatically print the original  source when necessary (e.g. when the error does not have a span   that includes the span of the suggestion).

 * Also, as another note on this change: The doc comment for `Diagnostic::span_suggestion`  says:
```rust
    /// The message
    ///
    /// * should not end in any punctuation (a `:` is added automatically)
    /// * should not be a question
    /// * should not contain any parts like "the following", "as shown"
```
  *  but the `:` is *not* added when the emitted line appears  out-of-line relative to the suggestion. I find that to be an  unfortunate UI experience.

----

As a drive-by fix, also changed code to combine multiple suggestions for a pattern into a single multipart suggestion (which vastly improves user experience IMO).

----

Includes the updates to expected NLL diagnostics.

Fix rust-lang#52877

bors added a commit that referenced this pull request Aug 1, 2018

Auto merge of #52931 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 31 pull requests

Successful merges:

 - #52332 (dead-code lint: say "constructed" for structs)
 - #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr)
 - #52628 (Cleanup some rustdoc code)
 - #52732 (Remove unstable and deprecated APIs)
 - #52745 (Update clippy to latest master)
 - #52756 (rustc: Disallow machine applicability in foreign macros)
 - #52771 (Clarify thread::park semantics)
 - #52810 ([NLL] Don't make "fake" match variables mutable)
 - #52821 (pretty print for std::collections::vecdeque)
 - #52822 (Fix From<LocalWaker>)
 - #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper)
 - #52831 (remove references to AUTHORS.txt file)
 - #52835 (Fix Alias intra doc ICE)
 - #52842 (update comment)
 - #52846 (Add timeout to use of `curl` in bootstrap.py.)
 - #52851 (Make the tool_lints actually usable)
 - #52853 (Improve bootstrap help on stages)
 - #52859 (Use Vec::extend in SmallVec::extend when applicable)
 - #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.)
 - #52867 (releases.md: fix 2 typos)
 - #52870 (Implement Unpin for FutureObj and LocalFutureObj)
 - #52876 (run-pass/const-endianness: negate before to_le())
 - #52878 (Fix wrong issue number in the test name)
 - #52883 (Include lifetime in mutability suggestion in NLL messages)
 - #52904 (NLL: sort diagnostics by span)
 - #52905 (Fix a typo in unsize.rs)
 - #52907 (NLL: On "cannot move out of type" error, print original before rewrite)
 - #52908 (Use SetLenOnDrop in Vec::truncate())
 - #52914 (Only run the sparc-abi test on sparc)
 - #52918 (Backport 1.27.2 release notes)
 - #52929 (Update compatibility note for 1.28.0 to be correct)

Failed merges:

 - #52758 (Cleanup for librustc::session)
 - #52799 (Use BitVector for global sets of AttrId)

r? @ghost

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 1, 2018

Rollup merge of rust-lang#52907 - pnkfelix:issue-52877-original-sourc…
…e-should-precede-suggestions, r=petrochenkov

NLL: On "cannot move out of type" error, print original before rewrite

NLL: On "cannot move out of type" error, print original source before rewrite.

 * Arguably this change is sometimes injecting noise into the output  (namely in the cases where the suggested rewrite is inline with the   suggestion and we end up highlighting the original source code).   I would not be opposed to something more aggressive/dynamic, like   revising the suggestion code to automatically print the original  source when necessary (e.g. when the error does not have a span   that includes the span of the suggestion).

 * Also, as another note on this change: The doc comment for `Diagnostic::span_suggestion`  says:
```rust
    /// The message
    ///
    /// * should not end in any punctuation (a `:` is added automatically)
    /// * should not be a question
    /// * should not contain any parts like "the following", "as shown"
```
  *  but the `:` is *not* added when the emitted line appears  out-of-line relative to the suggestion. I find that to be an  unfortunate UI experience.

----

As a drive-by fix, also changed code to combine multiple suggestions for a pattern into a single multipart suggestion (which vastly improves user experience IMO).

----

Includes the updates to expected NLL diagnostics.

Fix rust-lang#52877

bors added a commit that referenced this pull request Aug 1, 2018

Auto merge of #52937 - pietroalbini:rollup, r=pietroalbini
Rollup of 30 pull requests

Successful merges:

 - #52340 (Document From trait implementations for OsStr, OsString, CString, and CStr)
 - #52628 (Cleanup some rustdoc code)
 - #52732 (Remove unstable and deprecated APIs)
 - #52745 (Update clippy to latest master)
 - #52771 (Clarify thread::park semantics)
 - #52778 (Improve readability of serialize.rs)
 - #52810 ([NLL] Don't make "fake" match variables mutable)
 - #52821 (pretty print for std::collections::vecdeque)
 - #52822 (Fix From<LocalWaker>)
 - #52824 (Fix -Wpessimizing-move warnings in rustllvm/PassWrapper)
 - #52825 (Make sure #47772 does not regress)
 - #52831 (remove references to AUTHORS.txt file)
 - #52842 (update comment)
 - #52846 (Add timeout to use of `curl` in bootstrap.py.)
 - #52851 (Make the tool_lints actually usable)
 - #52853 (Improve bootstrap help on stages)
 - #52859 (Use Vec::extend in SmallVec::extend when applicable)
 - #52861 (Add targets for HermitCore (https://hermitcore.org) to the Rust compiler and port libstd to it.)
 - #52867 (releases.md: fix 2 typos)
 - #52870 (Implement Unpin for FutureObj and LocalFutureObj)
 - #52876 (run-pass/const-endianness: negate before to_le())
 - #52878 (Fix wrong issue number in the test name)
 - #52883 (Include lifetime in mutability suggestion in NLL messages)
 - #52888 (Use suggestions for shell format arguments)
 - #52904 (NLL: sort diagnostics by span)
 - #52905 (Fix a typo in unsize.rs)
 - #52907 (NLL: On "cannot move out of type" error, print original before rewrite)
 - #52914 (Only run the sparc-abi test on sparc)
 - #52918 (Backport 1.27.2 release notes)
 - #52929 (Update compatibility note for 1.28.0 to be correct)

Failed merges:

r? @ghost

@bors bors merged commit a5a3f76 into rust-lang:master Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.