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

Add better error message for partial move #58199

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@clintfred
Copy link

clintfred commented Feb 5, 2019

closes #56657

r? @davidtwco

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 5, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @davidtwco (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@clintfred

This comment has been minimized.

Copy link
Author

clintfred commented Feb 5, 2019

I have several tests failing on my branch, but am hoping this PR will allow others to help me move forward.

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 5, 2019

@clintfred Excited to see this PR! Could you update those test outputs and add them to the PR so that we can see what outputs you are getting and add review comments with steps to take?

@clintfred

This comment has been minimized.

Copy link
Author

clintfred commented Feb 5, 2019

@clintfred Excited to see this PR! Could you update those test outputs and add them to the PR so that we can see what outputs you are getting and add review comments with steps to take?

@davidtwco I'm not sure I understand your request. Won't we see the failing tests when CI runs? Or does it run only some intelligent subset of the tests based on what's changed?

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 5, 2019

@clintfred We will, but it's easier to put review comments and have discussions on your changes than the test results.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 6, 2019

@clintfred you can run ./x.py test src/test/ui --stage 1 --bless, which will update the .stderr files. After doing that I think the PR can be approved. You can see the current test errors in Travis (they all seem to be stderr output mismatch and nothing else).

@clintfred

This comment has been minimized.

Copy link
Author

clintfred commented Feb 6, 2019

@estebank I think that's the same set of failures I was seeing on my machine, so that's a good sign. When reviewing them on my own, it wasn't clear to me that all of those error messages were correct after my change. I will look again, but I would appreciate confirmation from others that they look correct as well.

I will run with --bless so we can get all the files into this PR regardless per @davidtwco's instructions.

@davidtwco
Copy link
Member

davidtwco left a comment

Looking great. Here are some small changes.

Show resolved Hide resolved src/test/ui/borrowck/borrowck-describe-lvalue.mir.stderr Outdated
Show resolved Hide resolved src/librustc_mir/borrow_check/error_reporting.rs Outdated
Show resolved Hide resolved src/test/ui/borrowck/borrowck-issue-48962.stderr Outdated
@clintfred

This comment has been minimized.

Copy link
Author

clintfred commented Feb 13, 2019

I have implemented the suggested changes. Some tests will start failing again and hopefully they will be all the tests that were previously erroneously marked as partial moves... If so, I can rerun the --bless

@estebank

This comment has been minimized.

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 14, 2019

travis-ci.com/rust-lang/rust/jobs/177505407#L2819

A quick glance makes me think that these are all correct. Feel free to bless and push @clintfred.

@estebank estebank changed the title [WIP] Add better error message for partial move Add better error message for partial move Feb 18, 2019

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 18, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 18, 2019

📌 Commit de05548 has been approved by estebank

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 18, 2019

🌲 The tree is currently closed for pull requests below priority 160, this pull request will be tested once the tree is reopened

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 18, 2019

Oh, sorry, didn't realize that you were the reviewer, @davidtwco. Feel free to r- if you disagree.

@davidtwco

This comment has been minimized.

Copy link
Member

davidtwco commented Feb 18, 2019

Oh, sorry, didn't realize that you were the reviewer, @davidtwco. Feel free to r- if you disagree.

No worries, this looks great! Thanks @clintfred!

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Feb 18, 2019

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Feb 18, 2019

Rollup merge of rust-lang#58199 - clintfred:partial-move-err-msg, r=e…
…stebank

Add better error message for partial move

closes rust-lang#56657

r? @davidtwco

bors added a commit that referenced this pull request Feb 19, 2019

Auto merge of #58566 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 16 pull requests

Successful merges:

 - #55632 (Deny the `overflowing_literals` lint for all editions)
 - #56470 (Modify doctest's auto-`fn main()` to allow `Result`s)
 - #58198 (Suggest removing parentheses surrounding lifetimes)
 - #58199 (Add better error message for partial move)
 - #58303 (Improve stability tags display)
 - #58336 (Fix search results interactions)
 - #58392 (Use less explicit shifting in std::net::ip)
 - #58528 (Don't use an allocation for ItemId in StmtKind)
 - #58530 (Monomorphize less code in fs::{read|write})
 - #58534 (Mention capping forbid lints)
 - #58536 (Remove UB in pointer tests)
 - #58539 (Add alias methods to PathBuf for underlying OsString (#58234))
 - #58544 (Fix doc for rustc "-g" flag)
 - #58545 (Add regression test for a specialization-related ICE (#39448))
 - #58551 (Explain a panic in test case net::tcp::tests::double_bind )
 - #58555 (Add a note about 2018e if someone uses `try {` in 2015e)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Feb 19, 2019

Auto merge of #58566 - Mark-Simulacrum:rollup, r=Mark-Simulacrum
Rollup of 16 pull requests

Successful merges:

 - #55632 (Deny the `overflowing_literals` lint for all editions)
 - #56470 (Modify doctest's auto-`fn main()` to allow `Result`s)
 - #58198 (Suggest removing parentheses surrounding lifetimes)
 - #58199 (Add better error message for partial move)
 - #58303 (Improve stability tags display)
 - #58336 (Fix search results interactions)
 - #58392 (Use less explicit shifting in std::net::ip)
 - #58528 (Don't use an allocation for ItemId in StmtKind)
 - #58530 (Monomorphize less code in fs::{read|write})
 - #58534 (Mention capping forbid lints)
 - #58536 (Remove UB in pointer tests)
 - #58539 (Add alias methods to PathBuf for underlying OsString (#58234))
 - #58544 (Fix doc for rustc "-g" flag)
 - #58545 (Add regression test for a specialization-related ICE (#39448))
 - #58551 (Explain a panic in test case net::tcp::tests::double_bind )
 - #58555 (Add a note about 2018e if someone uses `try {` in 2015e)

Failed merges:

r? @ghost
@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Feb 19, 2019

@bors r- rollup-

Failed in NLL mode in #58566 (comment). Needs to update the text (bless) these 6 test cases:

[01:30:22] failures:
[01:30:22]     [ui (nll)] ui/borrowck/borrowck-uninit-field-access.rs#ast
[01:30:22]     [ui (nll)] ui/moves/moves-based-on-type-cyclic-types-issue-4821.rs
[01:30:22]     [ui (nll)] ui/moves/moves-based-on-type-match-bindings.rs
[01:30:22]     [ui (nll)] ui/ref-suggestion.rs
[01:30:22]     [ui (nll)] ui/unsized-locals/borrow-after-move.rs
[01:30:22]     [ui (nll)] ui/unsized-locals/double-move.rs
@clintfred

This comment has been minimized.

Copy link
Author

clintfred commented Feb 19, 2019

@kennytm I'll take a look.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 19, 2019

I think all you have to do is run the same ./x.py ... --bless command with --compare-mode=nll to update these files.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 20, 2019

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 20, 2019

📌 Commit 02fe6a7 has been approved by estebank

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 20, 2019

@bors rollup-

@clintfred

This comment has been minimized.

Copy link
Author

clintfred commented Feb 21, 2019

@estebank I'm not sure of the status here. Do I need to do anything?

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 21, 2019

@clintfred not for the moment, I removed the rollup because this has the potential to clash with other PRs that incorporate partial move errors. Because the stderr files would be new, and generated from a parent commit that didn't have your changes there would be no merge conflict, but the tests would fail because of the discrepancy between both changes. This PR is still approved and will be merged on its own. There's the potential of a different PR being merged in the meantime that will necessitate you to rebase and re-bless, but no other action will be required of you.

Thank you for all the effort you've put into this! These small incremental improvements add up and are critical to maintaining the quality we want to see in rustc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment