Skip to content

NLL Diagnostic Review 3: missing a note describing the type of the non-Copy moved variable #56654

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

Closed
davidtwco opened this issue Dec 9, 2018 · 1 comment
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal

Comments

@davidtwco
Copy link
Member

davidtwco commented Dec 9, 2018

The following note is missing on the output of the NLL borrow checker for ui/use/use-after-move-self.nll.stderr:

   |
   = note: move occurs because `self` has type `S`, which does not implement the `Copy` trait
@davidtwco davidtwco added A-NLL Area: Non-lexical lifetimes (NLL) NLL-diagnostics Working towards the "diagnostic parity" goal labels Dec 9, 2018
@davidtwco davidtwco changed the title NLL Diagnostic Review 3: ui/use/use-after-move-self.nll.stderr missing a note describing the type of the non-Copy moved variable NLL Diagnostic Review 3: missing a note describing the type of the non-Copy moved variable Dec 9, 2018
@davidtwco
Copy link
Member Author

Here are some mentoring instructions to get started on this issue. If you're reading this and want to give this issue a go, drop a comment here and feel free to ask for any help or clarification on Zulip.

If this is your first contribution then there are instructions on getting a local build of the compiler available in the rustc guide.


These instructions are up to date as of master being 850fc6a4791b3b0ab668f9fb67c35dddd838b01f.

If we grep in the source, we can find that both the AST borrow checker and the NLL borrow checker have code that is supposed to emit this note, so rather than adding that note, we'll want to improve how we're detecting when to emit it.

This error is emitted from the cannot_act_on_moved_value function:

let err = struct_span_err!(
self,
use_span,
E0382,
"{} of {}moved value{}{OGN}",
verb,
optional_adverb_for_moved,
moved_path,
OGN = o
);

If we grep in the source, we can find that it is invoked in the report_use_of_moved_or_uninitialized function:

let mut err = self.infcx.tcx.cannot_act_on_moved_value(
span,
desired_action.as_noun(),
msg,
self.describe_place_with_options(&moved_place, IncludingDowncast(true)),
Origin::Mir,
);

In that same function, we can see where the diagnostic note should be being added:

if let Some(ty) = self.retrieve_type_for_place(place) {
let note_msg = match self.describe_place_with_options(
place,
IncludingDowncast(true),
) {
Some(name) => format!("`{}`", name),
None => "value".to_owned(),
};
err.note(&format!(
"move occurs because {} has type `{}`, \
which does not implement the `Copy` trait",
note_msg, ty
));
}

Given we know this note should be being added (because the AST borrow checker has it) then that means one of the conditions between the main body of the function and the diagnostic being emitted isn't robust enough and is stopping the error from being reported.

If we look at the used_place variable (the first condition just outside what is shown in the preview above), then we'll see that it is printed as (*(_1.0: std::box::Boxed<isize>)) (equivalent of the *self.x line in the actual source) - a deref of a field projection. Notably, the retrieve_type_of_place function doesn't handle the ProjectionElem::Deref case:

fn retrieve_type_for_place(&self, place: &Place<'tcx>) -> Option<ty::Ty> {
match place {
Place::Local(local) => {
let local = &self.mir.local_decls[*local];
Some(local.ty)
}
Place::Promoted(ref prom) => Some(prom.1),
Place::Static(ref st) => Some(st.ty),
Place::Projection(ref proj) => match proj.elem {
ProjectionElem::Field(_, ty) => Some(ty),
_ => None,
},
}
}

That may be the issue, consider trying to use the Place::ty (then PlaceTy::to_ty) function instead.


If you've got this working then you should run the tests and update any that change correctly and then open a PR with a line saying r? @davidtwco in the description so I'll be assigned to review it.

@davidtwco davidtwco added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Dec 9, 2018
@csmoe csmoe self-assigned this Dec 13, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this issue Dec 20, 2018
Add a note describing the type of the non-Copy moved variable

Closes rust-lang#56654
r?@davidtwco
Centril added a commit to Centril/rust that referenced this issue Dec 22, 2018
Add a note describing the type of the non-Copy moved variable

Closes rust-lang#56654
r?@davidtwco
bors added a commit that referenced this issue Dec 29, 2018
Add a note describing the type of the non-Copy moved variable

Closes #56654
r?@davidtwco
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non-lexical lifetimes (NLL) E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. NLL-diagnostics Working towards the "diagnostic parity" goal
Projects
None yet
Development

No branches or pull requests

2 participants