Point out implicit deref coercions in borrow #81629

sledgehammervampire · 2021-02-01T16:22:43Z

@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 · 2021-02-01T16:22:47Z

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

sledgehammervampire · 2021-02-01T16:23:03Z

r? @Aaron1011

sledgehammervampire · 2021-02-02T07:39:20Z

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.

b-naber · 2021-02-02T16:43:10Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

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


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

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.

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.

Clean up code

Aaron1011 · 2021-02-18T19:31:17Z

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

rust/compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

Line 370 in cb2effd

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

Update conflict_errors.rs Add deref definition location

sledgehammervampire · 2021-02-19T14:10:00Z

@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 · 2021-02-20T00:19:52Z

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

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

estebank · 2021-02-22T07:49:50Z

src/test/ui/borrowck/issue-81365-1.rs

+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;
+    }
+}


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

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.

estebank · 2021-02-22T07:51:19Z

compiler/rustc_mir/src/borrow_check/diagnostics/conflict_errors.rs

+        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");
+                }
+            }
+        }


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

I've moved the code in my latest commit.

Aaron1011 · 2021-02-23T01:45:33Z

@1000teslas Thanks for all of your work!

@bors r+

bors · 2021-02-23T01:45:35Z

📌 Commit 1847a6c has been approved by Aaron1011

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

rust-highfive assigned lcnr Feb 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2021

rust-highfive assigned Aaron1011 and unassigned lcnr Feb 1, 2021

b-naber reviewed Feb 2, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

sledgehammervampire force-pushed the issue-81365-fix branch from 081dfe6 to 8b2b07b Compare February 7, 2021 16:43

sledgehammervampire marked this pull request as ready for review February 8, 2021 01:26

sledgehammervampire added 2 commits February 8, 2021 13:24

Point out implicit deref coercions in borrow

b2eed3a

Clean up code

Add tests

8412da6

sledgehammervampire force-pushed the issue-81365-fix branch from 8b2b07b to 8412da6 Compare February 8, 2021 02:30

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

This comment has been minimized.

Sign in to view

Add deref definition location

99f4573

Update conflict_errors.rs Add deref definition location

sledgehammervampire force-pushed the issue-81365-fix branch from e5b1e1c to 99f4573 Compare February 19, 2021 07:45

estebank reviewed Feb 22, 2021

View reviewed changes

estebank approved these changes Feb 22, 2021

View reviewed changes

Extract deref coercion explanation into method

1847a6c

estebank approved these changes Feb 22, 2021

View reviewed changes

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 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

Dylan-DPC-zz mentioned this pull request Feb 23, 2021

Rollup of 10 pull requests #82443

Merged

bors merged commit 18d1284 into rust-lang:master Feb 23, 2021

rustbot added this to the 1.52.0 milestone Feb 23, 2021

sledgehammervampire deleted the issue-81365-fix branch February 24, 2021 01:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point out implicit deref coercions in borrow #81629

Point out implicit deref coercions in borrow #81629

sledgehammervampire commented Feb 1, 2021

rust-highfive commented Feb 1, 2021

sledgehammervampire commented Feb 1, 2021

sledgehammervampire commented Feb 2, 2021

b-naber Feb 2, 2021

sledgehammervampire Feb 3, 2021

sledgehammervampire Feb 7, 2021

This comment has been minimized.

Aaron1011 commented Feb 18, 2021

This comment has been minimized.

sledgehammervampire commented Feb 19, 2021 •

edited

Loading

Aaron1011 commented Feb 20, 2021

estebank Feb 22, 2021

sledgehammervampire Feb 22, 2021 •

edited

Loading

estebank Feb 22, 2021

sledgehammervampire Feb 22, 2021

Aaron1011 commented Feb 23, 2021

bors commented Feb 23, 2021

Point out implicit deref coercions in borrow #81629

Point out implicit deref coercions in borrow #81629

Conversation

sledgehammervampire commented Feb 1, 2021

rust-highfive commented Feb 1, 2021

sledgehammervampire commented Feb 1, 2021

sledgehammervampire commented Feb 2, 2021

b-naber Feb 2, 2021

Choose a reason for hiding this comment

sledgehammervampire Feb 3, 2021

Choose a reason for hiding this comment

sledgehammervampire Feb 7, 2021

Choose a reason for hiding this comment

This comment has been minimized.

Aaron1011 commented Feb 18, 2021

This comment has been minimized.

sledgehammervampire commented Feb 19, 2021 • edited Loading

Aaron1011 commented Feb 20, 2021

estebank Feb 22, 2021

Choose a reason for hiding this comment

sledgehammervampire Feb 22, 2021 • edited Loading

Choose a reason for hiding this comment

estebank Feb 22, 2021

Choose a reason for hiding this comment

sledgehammervampire Feb 22, 2021

Choose a reason for hiding this comment

Aaron1011 commented Feb 23, 2021

bors commented Feb 23, 2021

sledgehammervampire commented Feb 19, 2021 •

edited

Loading

sledgehammervampire Feb 22, 2021 •

edited

Loading