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

Point out implicit deref coercions in borrow #81629

Merged
merged 4 commits into from
Feb 23, 2021

Conversation

sledgehammervampire
Copy link
Contributor

Fixes #81365

@Aaron1011 I'm not sure why my code shows the note even in an implicit Deref call. See the output for issue-81365-8.rs.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Feb 1, 2021
@sledgehammervampire
Copy link
Contributor Author

r? @Aaron1011

@rust-highfive rust-highfive assigned Aaron1011 and unassigned lcnr Feb 1, 2021
@sledgehammervampire
Copy link
Contributor Author

For some reason, in issue-81365-8.rs, from_hir_call is false even when the dereference is explicitly done.

It looks like auto-dereferencing occurs when lowering HIR to THIR https://rustc-dev-guide.rust-lang.org/overview.html?highlight=Deref#intermediate-representations. There should be some information about when the dereferencing was implicitly inserted or not somewhere, but I'm not sure where to find it.

loan.reserve_location.block,
),
) {
let is_deref = !from_hir_call && tcx.is_diagnostic_item(sym::deref_method, method_did);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just out of interest: can you explain why we only want to output this error note if from_hir_call is false?

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 would like to add the note in the output for something like issue-81365.rs where the deref is implicit, v. something like issue-81365-8.rs, where the deref is explicit and shouldn't be added. In fact, it is what I'm working on now, trying to figure out if the deref was explicit or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it seems like from_hir_call is true for a syntactic call like f(...) or x.f() but false for an operator, perhaps overloaded, like *x or the implicitly inserted deref in something like (&x).0.

@rust-log-analyzer

This comment has been minimized.

@Aaron1011
Copy link
Member

Sorry for the delay in getting this!

Overall, this looks good. I think there may be some other borrow-related errors where we might want to emit this diagnostic. However, that can be done in follow-up PRs as needed.

Could you add a 'deref defined here' note, similar to

err.span_note(deref_target, "deref defined here");

@Aaron1011 Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 18, 2021
@rust-log-analyzer

This comment has been minimized.

Update conflict_errors.rs

Add deref definition location
@sledgehammervampire
Copy link
Contributor Author

sledgehammervampire commented Feb 19, 2021

@Aaron1011 any idea why the check took so long? i ran the ui tests on my machine and it took about 12 minutes to run them all.

@Aaron1011
Copy link
Member

I'm not sure - I suspect it was an issue on GitHub's end.

cc @estebank: Does this PR look good to you?

Comment on lines +1 to +24
use std::ops::Deref;

struct DerefTarget {
target_field: bool,
}
struct Container {
target: DerefTarget,
container_field: bool,
}

impl Deref for Container {
type Target = DerefTarget;
fn deref(&self) -> &Self::Target {
&self.target
}
}

impl Container {
fn bad_borrow(&mut self) {
let first = &self.target_field;
self.container_field = true; //~ ERROR E0506
first;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason the different tests couldn't be merged into fewer files?

Copy link
Contributor Author

@sledgehammervampire sledgehammervampire Feb 22, 2021

Choose a reason for hiding this comment

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

Well, I don't understand the previous phases that transformed the code into MIR, so I made some tests with the intent of something like "what happens to double implicit derefs?", "what happens to indexing and function calls?", "what happens with implicitly inserted v. explicitly inserted derefs?". The only reasons I haven't merged them is because I don't know enough to decide what distinctions in the tests are important, and also that I'm not sure if any existing tests cover what I've already written. If you think that some tests are redundant, I will remove them.

Comment on lines 1545 to 1574
let tcx = self.infcx.tcx;
// point out implicit deref coercion
if let (
Some(Terminator { kind: TerminatorKind::Call { from_hir_call: false, .. }, .. }),
Some((method_did, method_substs)),
) = (
&self.body[loan.reserve_location.block].terminator,
crate::util::find_self_call(
tcx,
self.body,
loan.assigned_place.local,
loan.reserve_location.block,
),
) {
if tcx.is_diagnostic_item(sym::deref_method, method_did) {
let deref_target =
tcx.get_diagnostic_item(sym::deref_target).and_then(|deref_target| {
Instance::resolve(tcx, self.param_env, deref_target, method_substs)
.transpose()
});
if let Some(Ok(instance)) = deref_target {
let deref_target_ty = instance.ty(tcx, self.param_env);
err.note(&format!(
"borrow occurs due to deref coercion to `{}`",
deref_target_ty
));
err.span_note(tcx.def_span(instance.def_id()), "deref defined here");
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this code to a method with an appropriate name?

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've moved the code in my latest commit.

@Aaron1011
Copy link
Member

@1000teslas Thanks for all of your work!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2021

📌 Commit 1847a6c has been approved by Aaron1011

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 23, 2021
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 23, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#81629 (Point out implicit deref coercions in borrow)
 - rust-lang#82113 (Improve non_fmt_panic lint.)
 - rust-lang#82258 (Implement -Z hir-stats for nested foreign items)
 - rust-lang#82296 (Support `pub` on `macro_rules`)
 - rust-lang#82297 (Consider auto derefs before warning about write only fields)
 - rust-lang#82305 (Remove many RefCells from DocContext)
 - rust-lang#82308 (Lower condition of `if` expression before it's "then" block)
 - rust-lang#82311 (Jsondocck improvements)
 - rust-lang#82362 (Fix mir-cfg dumps)
 - rust-lang#82391 (disable atomic_max/min tests in Miri)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 18d1284 into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@sledgehammervampire sledgehammervampire deleted the issue-81365-fix branch February 24, 2021 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessing fields through a Deref impl leads to confusing error messages
9 participants