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

Suggest dereferencing when `Deref` is implemented. #60155

Merged
merged 3 commits into from Apr 23, 2019

Conversation

Projects
None yet
6 participants
@davidtwco
Copy link
Member

commented Apr 21, 2019

Fixes #59819.

r? @oli-obk
cc @estebank

davidtwco added some commits Apr 21, 2019

Add existing behaviour test for deref suggestions.
This commit adds a test that demonstrates the current behaviour where
suggestions to add a dereference aren't given for non-references.
Suggest dereferencing when `Deref` is implemented.
This commit suggests dereferencing a type when it implements `Deref`
with the correct `Output` associated type.
@@ -463,6 +463,57 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
}
}
}
_ if sp == expr.span => {
// If neither type is a reference, then check for `Deref` implementations by

This comment has been minimized.

Copy link
@Centril

Centril Apr 22, 2019

Member

Seems like a good place to refactor into function(s)?

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

General note: When I wrote the issue I hoped that we could deduplicate the logic on references with the new logic you wrote. References also implement &T: Deref<Target = T>, so it should hopefully work out of the box?

@davidtwco

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

General note: When I wrote the issue I hoped that we could deduplicate the logic on references with the new logic you wrote. References also implement &T: Deref<Target = T>, so it should hopefully work out of the box?

I just tried this and I don't think it does, when I check if &{integer}: Deref<Target = i32> then it returns false. I assume that this is because &{integer}: Deref<Target = {integer}> and not i32 specifically but that's just a guess.

@davidtwco davidtwco force-pushed the davidtwco:issue-59819 branch from aa190cc to 6c494ac Apr 22, 2019

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

I just tried this and I don't think it does, when I check if &{integer}: Deref<Target = i32> then it returns false. I assume that this is because &{integer}: Deref<Target = {integer}> and not i32 specifically but that's just a guess.

Maybe we need to try something different then. Check whether the type implements Deref, get the Target type, try to coerce to the destination type.

@davidtwco davidtwco force-pushed the davidtwco:issue-59819 branch from 6c494ac to 1b6fad6 Apr 22, 2019

@davidtwco

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

Maybe we need to try something different then. Check whether the type implements Deref, get the Target type, try to coerce to the destination type.

I have no idea what I was trying earlier, pushed a version that de-duplicates that logic.

Only make suggestion when type is `Copy`.
This commit makes the suggestion to dereference when a type implements
`Deref` only apply if the dereference would succeed (ie. the type is
`Copy`, otherwise a borrow check error would occur).

@davidtwco davidtwco force-pushed the davidtwco:issue-59819 branch from 1b6fad6 to 7ab1bfd Apr 22, 2019

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@bors r+

This is absolutely awesome!

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

📌 Commit 7ab1bfd has been approved by oli-obk

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

⌛️ Testing commit 7ab1bfd with merge 4eff852...

bors added a commit that referenced this pull request Apr 23, 2019

Auto merge of #60155 - davidtwco:issue-59819, r=oli-obk
Suggest dereferencing when `Deref` is implemented.

Fixes #59819.

r? @oli-obk
cc @estebank
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

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

@bors bors added the merged-by-bors label Apr 23, 2019

@bors bors merged commit 7ab1bfd into rust-lang:master Apr 23, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-59819 branch Apr 24, 2019

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.