Remove asterisk suggestion for move errors in borrowck #61332

kungfukennyg · 2019-05-29T22:59:58Z

As per the decision in #54985 completely removes the suggestion to add an asterisk when checking move errors. I believe I've preserved the correct behavior with the "consider borrowing here" branch of the original match arm, but I'm not positive on that.

This is my first PR to rustc so any feedback is greatly appreciated. Thanks.

rust-highfive · 2019-05-29T23:00:01Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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.

kungfukennyg · 2019-05-30T00:29:49Z

I see that the build has failed because the stderr output for some tests in the ui suite are failing. What would be the best course of action here? Assuming my changes are fine as is, should I manually update the stderr output for each failing test?

eddyb · 2019-05-30T01:37:40Z

r? @matthewjasper

matthewjasper · 2019-05-30T12:15:30Z

You can update the files by running ./x.py test src/test/ui --stage 1 --bless

kungfukennyg · 2019-05-30T18:02:10Z

Thanks! Did that, and now there are some tests running ui/suggestions/dont-suggest-ref/simple.rs that are failing due to the expected output changing. I've attached a snippet of the log below. What would be the best course of action here to fix those?

https://gist.github.com/kennethbgoodin/d43178144e121e9cf8ca07cca079b171

matthewjasper · 2019-05-30T19:39:55Z

There's no automated way to fix those. You'll have to manually remove //~| HELP and //~| SUGGESTION that cause the test to fail.

kungfukennyg · 2019-05-30T19:43:54Z

Okay thank you, on it.

kungfukennyg · 2019-05-30T22:54:22Z

Okay tests passed 😄

matthewjasper · 2019-06-02T09:58:57Z

src/librustc_mir/borrow_check/move_errors.rs

-                        Applicability::Unspecified,
-                    );
-                } else {
+                if !try_remove_deref || !snippet.starts_with('*') {


I think this condition should be removed and we should accept that sometimes we will suggest &*

I've made that change. You said we should accept that sometimes we will suggest strange things, but it's worth highlighting some of the weirder:

132aa6d#diff-33c6ee1b44f5207df102942b0c659b94R26

132aa6d#diff-a3121673048182c8d1a0ee3ccbfb9d5fR8

Are those suggestions acceptable? They might end up causing more confusion than the initial suggestion this PR addresses.

The first is fine. The &*& suggestions are weird, but they need someone to write *&. I think that we are generally OK with suggestions that are suboptimal in edge cases. cc @rust-lang/wg-diagnostics

It'd be nice to add the comment // run-rustfix at the beginning of the changed tests (and rerun x.py) to make sure the suggestions are always valid in these cases and do not break going forwards.

I think that we are generally OK with suggestions that are suboptimal in edge cases.

We have a harder stance on incorrect and misleading suggestions over weird suggestions, but we do try to minimize the later :)

Suggestions from borrow checking are always going to be best guesses. They definitely shouldn't be rustfixable, unless rustfix has a flag to apply possibly incorrect suggestions.

Hi, just wanted to confirm -- I should not do Esteban's suggestion?

@kennethbgoodin it's fine, don't bother for now. I'll try to clean it up sometime down the line :)

As per issue rust-lang#54985 removes the not useful suggestion to remove asterisk in move errors. Includes minor changes to tests in the `ui` suite to account for the removed suggestion.

matthewjasper · 2019-06-07T08:46:50Z

@bors r+

bors · 2019-06-07T08:46:51Z

📌 Commit de677b9 has been approved by matthewjasper

…risk-suggestion, r=matthewjasper Remove asterisk suggestion for move errors in borrowck As per the decision in rust-lang#54985 completely removes the suggestion to add an asterisk when checking move errors. I believe I've preserved the correct behavior with the "consider borrowing here" branch of the original match arm, but I'm not positive on that. This is my first PR to rustc so any feedback is greatly appreciated. Thanks.

@ghost

Rollup of 4 pull requests Successful merges: - #61332 (Remove asterisk suggestion for move errors in borrowck) - #61532 ([const-prop] Support Rvalue::{Ref,Len} and Deref) - #61586 (ci: Disable LLVM/debug assertions for asmjs builder) - #61599 (libcore/pin: Minor grammar corrections for module documentation) Failed merges: r? @ghost

bors · 2019-06-07T14:44:17Z

⌛ Testing commit de677b9 with merge 6628785855a797d65c9fbab3930233d2fed11191...

…risk-suggestion, r=matthewjasper Remove asterisk suggestion for move errors in borrowck As per the decision in rust-lang#54985 completely removes the suggestion to add an asterisk when checking move errors. I believe I've preserved the correct behavior with the "consider borrowing here" branch of the original match arm, but I'm not positive on that. This is my first PR to rustc so any feedback is greatly appreciated. Thanks.

Centril · 2019-06-07T14:50:17Z

@bors retry r0lled up

@ghost

Rollup of 7 pull requests Successful merges: - #61332 (Remove asterisk suggestion for move errors in borrowck) - #61532 ([const-prop] Support Rvalue::{Ref,Len} and Deref) - #61586 (ci: Disable LLVM/debug assertions for asmjs builder) - #61599 (libcore/pin: Minor grammar corrections for module documentation) - #61603 (Increases heap size available during testing for SGX) - #61605 (Fix slice const generic length display) - #61618 (make the backtrace field of EvalError private) Failed merges: r? @ghost

rust-highfive assigned eddyb May 29, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 29, 2019

kungfukennyg changed the title ~~#54985 Remove asterisk suggestion for move errors in borrowck~~ Remove asterisk suggestion for move errors in borrowck May 29, 2019

This comment has been minimized.

Sign in to view

rust-highfive assigned matthewjasper and unassigned eddyb May 30, 2019

kungfukennyg force-pushed the borrowck-remove-asterisk-suggestion branch from ebb39b1 to 5fcc753 Compare May 30, 2019 20:34

kungfukennyg force-pushed the borrowck-remove-asterisk-suggestion branch from 5fcc753 to f4e01aa Compare May 31, 2019 00:24

matthewjasper reviewed Jun 2, 2019

View reviewed changes

kungfukennyg force-pushed the borrowck-remove-asterisk-suggestion branch from f4e01aa to 132aa6d Compare June 2, 2019 19:36

This comment has been minimized.

Sign in to view

Remove asterisk suggestion for move errors in borrowck

de677b9

As per issue rust-lang#54985 removes the not useful suggestion to remove asterisk in move errors. Includes minor changes to tests in the `ui` suite to account for the removed suggestion.

kungfukennyg force-pushed the borrowck-remove-asterisk-suggestion branch from 132aa6d to de677b9 Compare June 4, 2019 17:31

estebank approved these changes Jun 6, 2019

View reviewed changes

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 Jun 7, 2019

Centril mentioned this pull request Jun 7, 2019

Rollup of 4 pull requests #61619

Closed

Centril mentioned this pull request Jun 7, 2019

Rollup of 7 pull requests #61622

Merged

bors merged commit de677b9 into rust-lang:master Jun 7, 2019

awaitlink mentioned this pull request Jun 17, 2019

nll: strange suggestion to "consider removing the * #54985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove asterisk suggestion for move errors in borrowck #61332

Remove asterisk suggestion for move errors in borrowck #61332

kungfukennyg commented May 29, 2019

rust-highfive commented May 29, 2019

This comment has been minimized.

kungfukennyg commented May 30, 2019

eddyb commented May 30, 2019

matthewjasper commented May 30, 2019

kungfukennyg commented May 30, 2019

matthewjasper commented May 30, 2019

kungfukennyg commented May 30, 2019

kungfukennyg commented May 30, 2019

matthewjasper Jun 2, 2019

kungfukennyg Jun 2, 2019 •

edited

Loading

matthewjasper Jun 2, 2019

estebank Jun 3, 2019

matthewjasper Jun 3, 2019

kungfukennyg Jun 6, 2019

estebank Jun 6, 2019

This comment has been minimized.

matthewjasper commented Jun 7, 2019

bors commented Jun 7, 2019

bors commented Jun 7, 2019

Centril commented Jun 7, 2019

Remove asterisk suggestion for move errors in borrowck #61332

Remove asterisk suggestion for move errors in borrowck #61332

Conversation

kungfukennyg commented May 29, 2019

rust-highfive commented May 29, 2019

This comment has been minimized.

kungfukennyg commented May 30, 2019

eddyb commented May 30, 2019

matthewjasper commented May 30, 2019

kungfukennyg commented May 30, 2019

matthewjasper commented May 30, 2019

kungfukennyg commented May 30, 2019

kungfukennyg commented May 30, 2019

matthewjasper Jun 2, 2019

Choose a reason for hiding this comment

kungfukennyg Jun 2, 2019 • edited Loading

Choose a reason for hiding this comment

matthewjasper Jun 2, 2019

Choose a reason for hiding this comment

estebank Jun 3, 2019

Choose a reason for hiding this comment

matthewjasper Jun 3, 2019

Choose a reason for hiding this comment

kungfukennyg Jun 6, 2019

Choose a reason for hiding this comment

estebank Jun 6, 2019

Choose a reason for hiding this comment

This comment has been minimized.

matthewjasper commented Jun 7, 2019

bors commented Jun 7, 2019

bors commented Jun 7, 2019

Centril commented Jun 7, 2019

kungfukennyg Jun 2, 2019 •

edited

Loading