Skip to content

Accessing fields through a Deref impl leads to confusing error messages #81365

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Aaron1011 opened this issue Jan 25, 2021 · 4 comments · Fixed by #81629
Closed

Accessing fields through a Deref impl leads to confusing error messages #81365

Aaron1011 opened this issue Jan 25, 2021 · 4 comments · Fixed by #81629
Assignees
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Jan 25, 2021

The following code:

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

causes the following error message:

error[E0506]: cannot assign to `self.container_field` because it is borrowed
  --> src/lib.rs:21:9
   |
20 |         let first = &self.target_field;
   |                      ---- borrow of `self.container_field` occurs here
21 |         self.container_field = true;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^ assignment to borrowed `self.container_field` occurs here
22 |         first;
   |         ----- borrow later used here

This error is caused by the fact that &self.target_field does through the Deref impl on Container. As a result, &self is borrowed, not just the desired field. Changing this line to let first = &self.target.target_field makes the code compile.

However, the error message doesn't explain any of this. It claims that the usage of self is a borrow of self.container_field, without noting that this is due to the Deref impl.

We should do something similar to #75304 for borrows, and make it clear when lifetimes are being affected by implicit Deref calls.

@Aaron1011 Aaron1011 added A-diagnostics Area: Messages for errors, warnings, and lints A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels Jan 25, 2021
@estebank estebank added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 25, 2021
@sledgehammervampire
Copy link
Contributor

@rustbot claim

@sledgehammervampire
Copy link
Contributor

@Aaron1011 How can I check if a Deref in the list of projections of a place is implicitly inserted?

@Aaron1011
Copy link
Member Author

@1000teslas: You should be able to adapt this code:

// We are trying to find MIR of the form:
// ```
// _temp = _moved_val;
// ...
// FnSelfCall(_temp, ...)
// ```
//
// where `_moved_val` is the place we generated the move error for,
// `_temp` is some other local, and `FnSelfCall` is a function
// that has a `self` parameter.
let target_temp = match stmt.kind {
StatementKind::Assign(box (temp, _)) if temp.as_local().is_some() => {
temp.as_local().unwrap()
}
_ => return normal_ret,
};
debug!("move_spans: target_temp = {:?}", target_temp);
if let Some(Terminator {
kind: TerminatorKind::Call { fn_span, from_hir_call, .. }, ..
}) = &self.body[location.block].terminator
{
let (method_did, method_substs) = if let Some(info) =
crate::util::find_self_call(self.infcx.tcx, &self.body, target_temp, location.block)
{
info
} else {
return normal_ret;
};
let tcx = self.infcx.tcx;
let parent = tcx.parent(method_did);
let is_fn_once = parent == tcx.lang_items().fn_once_trait();
let is_operator = !from_hir_call
&& parent.map_or(false, |p| tcx.lang_items().group(LangItemGroup::Op).contains(&p));
let is_deref = !from_hir_call && tcx.is_diagnostic_item(sym::deref_method, method_did);
let fn_call_span = *fn_span;
let self_arg = tcx.fn_arg_names(method_did)[0];
debug!(
"terminator = {:?} from_hir_call={:?}",
self.body[location.block].terminator, from_hir_call
);
// Check for a 'special' use of 'self' -
// an FnOnce call, an operator (e.g. `<<`), or a
// deref coercion.
let kind = if is_fn_once {
Some(FnSelfUseKind::FnOnceCall)
} else if is_operator {
Some(FnSelfUseKind::Operator { self_arg })
} else if is_deref {
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);
Some(FnSelfUseKind::DerefCoercion {
deref_target: tcx.def_span(instance.def_id()),
deref_target_ty,
})
} else {
None
}
} else {
None
};
let kind = kind.unwrap_or_else(|| {
// This isn't a 'special' use of `self`
debug!("move_spans: method_did={:?}, fn_call_span={:?}", method_did, fn_call_span);
let implicit_into_iter = matches!(
fn_call_span.desugaring_kind(),
Some(DesugaringKind::ForLoop(ForLoopLoc::IntoIter))
);
FnSelfUseKind::Normal { self_arg, implicit_into_iter }
});
return FnSelfUse {
var_span: stmt.source_info.span,
fn_call_span,
fn_span: self
.infcx
.tcx
.sess
.source_map()
.guess_head_span(self.infcx.tcx.def_span(method_did)),
kind,
};
}
normal_ret

and re-use the FnSelfUse code. The actual check for a non-user-supplied Deref call is done by:

let is_deref = !from_hir_call && tcx.is_diagnostic_item(sym::deref_method, method_did);

@sledgehammervampire
Copy link
Contributor

@Aaron1011 It doesn't look like from_hir_call distinguishes between issue-81365.rs, where the deref in self.target_field is implicit and issue-81365-8.rs, where the deref is explicit.

@bors bors closed this as completed in 18d1284 Feb 23, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants