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.

Sign up for GitHub

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

Jump to bottom

When encountering move error on an Option, suggest using as_ref #61147

Merged
merged 2 commits into from
May 27, 2019
Merged

When encountering move error on an Option, suggest using as_ref #61147

merged 2 commits into from
May 27, 2019

Conversation

estebank
Copy link
Contributor

@estebank estebank commented May 25, 2019 •
edited
Loading

Fix #61109, cc #15457.

@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 May 25, 2019
@estebank
Copy link
Contributor Author

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned eddyb May 25, 2019
@czipperz
Copy link
Contributor

Does this correctly handle as_mut as well?

@czipperz
Copy link
Contributor

Perhaps this should support Result as well?

@estebank
Copy link
Contributor Author

@czipperz adding Result support now.

Does this correctly handle as_mut as well?

Could you expand?

@estebank estebank mentioned this pull request May 25, 2019
Closed
@czipperz
Copy link
Contributor

I don't know if this is possible, but it would be nice if it suggested as_mut when you need mutable access to call a method. as_ref().unwrap() converts Option<T> to &T but sometimes you need &mut T.

oli-obk
oli-obk reviewed May 25, 2019
View reviewed changes
src/librustc_mir/borrow_check/move_errors.rs
original_path.ty(self.mir, self.infcx.tcx).ty,
);
let snippet = self.infcx.tcx.sess.source_map().span_to_snippet(span).unwrap();
let is_option = orig_path_ty.starts_with("std::option::Option");
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about waiting a bit with merging this PR (a bit being until #60966 is merged)? Then you can annotate Option and Result with #[rustc_diagnostic_item = "option"] and "result" respectively and then check for these items via

if let Some(adt) = original_path.ty(self.mir, self.infcx.tcx).adt_def() {
	if self.infcx.tcx.is_diagnostic_item(adt.did, sym::option) {
        ...
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are a couple of places where we're already doing string comparisons like this, so I love the fact that we'll be able to get rid of them soon!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... or we can just merge this and fix it up later, doesn't seem like a big deal in fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine either way

@estebank
Copy link
Contributor Author

@czipperz suggesting both is bit heavy, but can be done. I'm a bit ambivalent on whether it is worth it. On one hand, it teaches the API surface nicely, on the other hand it is verbose and might confuse newcomers.

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2019

My vote is for just suggesting as_ref and figuring out how to make sure we emit good errors on subsequent errors that may occur due to the code actually requiring mutable references

@estebank
Copy link
Contributor Author

Should I just remove the last commit or are there any further changes needed (beyond #60966)?

@oli-obk
Copy link
Contributor

oli-obk commented May 26, 2019

r=me with last commit removed. I agree with your assessment that we'd be overwhelming users for little gain

@estebank
Copy link
Contributor Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented May 26, 2019

📌 Commit 1cc42ea 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 May 26, 2019
@bors
Copy link
Contributor

bors commented May 27, 2019

⌛ Testing commit 1cc42ea with merge ed2a511...

bors added a commit that referenced this pull request May 27, 2019
@bors
Auto merge of #61147 - estebank:suggest-as-ref, r=oli-obk
ed2a511 
When encountering move error on an `Option`, suggest using `as_ref`

Fix #61109, cc #15457.
@bors
Copy link
Contributor

bors commented May 27, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing ed2a511 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2019
@bors bors merged commit 1cc42ea into rust-lang:master May 27, 2019
@nnethercote
Copy link
Contributor

https://twitter.com/nnethercote/status/1083507450249043969

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

@oli-obk oli-obk oli-obk left review comments

@czipperz czipperz czipperz requested changes

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Suggest as_ref() in E0507 if applicable
7 participants
@estebank @rust-highfive @czipperz @oli-obk @bors @nnethercote @eddyb