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.

Sign up for GitHub

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

Jump to bottom

Add a note about implicit temporaries on &mut (fn or const) #104857

Closed
wants to merge 3 commits into from
Closed

Add a note about implicit temporaries on &mut (fn or const) #104857

wants to merge 3 commits into from

Conversation

sharnoff
Copy link
Contributor

@sharnoff sharnoff commented Nov 25, 2022 •
edited
Loading

Ran into a confusing error message and thought I'd open a PR to improve it.

My confusion came about because something like

fn do_nothing() {}

fn return_fn() -> &'static dyn Fn() {
    &do_nothing
}

is valid, but

fn do_nothing() {}

fn return_fn_mut() -> &'static mut dyn FnMut() {
    &mut do_nothing
}

is not. The error message didn't help much:

error[E0515]: cannot return reference to temporary value
  --> $file:4:5
  |
4 |     &mut do_nothing
  |     ^^^^^----------
  |     |    |
  |     |    temporary value created here
  |     returns a reference to data owned by the current function

"why is it creating a temporary only for &mut and not &?"

A similar case is true of constants (although they have the const_item_mutation lint).

Changes to error messages

This PR adds a note to the error message for borrowck failures when a constants or functions are mutably borrowed and a referenece to the temporary is returned (at least, that's what it should do - I've only superficially tested it).

The note is:

  • (for constants) "mutably borrowing a constant copies the value to a temporary"
  • (for functions) "mutably borrowing a function copies the function pointer to a temporary"

The text of the notes can be improved; I just wanted to get something here for feedback :)

I'm also not sure that just having it in a "note" is sufficient, but I don't know what the ideal error construction would be. Maybe changing the "temporary value created here" line?

Other changes

  • Added mir::LocalInfo::FnDefRef to accompany ConstRef - required for detecting this (otherwise local_info = None)
  • Renamed thir::ExprKind::ZstLiteral to NamedFn, because it was only used for functions (and now it's used to set `local_info = FnDefRef``)
    • This maybe should be renamed, because it also includes Self constructors, which might not really be considered "named" functions

r? rust-lang/diagnostics

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 25, 2022
@deltragon
Copy link
Contributor

It probably makes sense to add a test for this.

@sharnoff sharnoff force-pushed the mut-borrow-const-or-fn-borrowck-error branch from 3d9895a to 8bcbfef Compare November 25, 2022 21:28
@sharnoff
Copy link
Contributor Author

Rebased, removed "implicitly" from the error message(s), and added some tests.

@bors
Copy link
Contributor

bors commented Nov 26, 2022

☔ The latest upstream changes (presumably #99798) made this pull request unmergeable. Please resolve the merge conflicts.

@sharnoff sharnoff force-pushed the mut-borrow-const-or-fn-borrowck-error branch from 8bcbfef to 30e7429 Compare November 26, 2022 06:20
@sharnoff
Copy link
Contributor Author

Rebased

@bors
Copy link
Contributor

bors commented Dec 1, 2022

☔ The latest upstream changes (presumably #104975) made this pull request unmergeable. Please resolve the merge conflicts.

@sharnoff sharnoff force-pushed the mut-borrow-const-or-fn-borrowck-error branch from 30e7429 to 2450ccb Compare December 2, 2022 01:53
@sharnoff
Copy link
Contributor Author

sharnoff commented Dec 2, 2022

Rebased

@pnkfelix pnkfelix added A-diagnostics Area: Messages for errors, warnings, and lints WG-diagnostics Working group: Diagnostics labels Jan 12, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Jan 12, 2023

r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned TaKO8Ki Jan 12, 2023
@albertlarsan68
Copy link
Member

The tests directory moved from src/test to tests, please rebase.
@rustbot author

@rustbot rustbot 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 Jan 12, 2023
oli-obk
oli-obk reviewed Jan 12, 2023
View reviewed changes
compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
// note about this.
match &self.body.local_decls[local].local_info {
Some(box LocalInfo::FnDefRef) => {
implicit_temporary = Some(("function", "function pointer"));
Copy link
Contributor

Choose a reason for hiding this comment

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

it actually doesn't put the function pointer into a temporary, but creates a temporary of the function item type (a zst). Maybe it could literally mention the zero sized nature, as that should make it obvious that you get no data to mutate (in case someone expected a function pointer).

oli-obk
oli-obk reviewed Jan 12, 2023
View reviewed changes
compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
_ => {}
}
}
match local_kind {
LocalKind::ReturnPointer | LocalKind::Temp => {
("temporary value".to_string(), "temporary value created here".to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

As you already mentioned, this message is the misleading part. Maybe pick a different message here for ConstRef and FnDefRef instead of adding a new note?

@sharnoff sharnoff force-pushed the mut-borrow-const-or-fn-borrowck-error branch from 2450ccb to 3818536 Compare January 13, 2023 06:16
@rustbot
Copy link
Collaborator

rustbot commented Jan 13, 2023

Some changes occured in rustc_ty_utils::consts.rs

cc @BoxyUwU

@Dylan-DPC
Copy link
Member

@sharnoff any updates to this?

@sharnoff
Copy link
Contributor Author

sharnoff commented Mar 8, 2023

@Dylan-DPC Hey! Sorry, meant to reply, but have been overwhelmed with other stuff for the past couple months. No code updates - will get on that this weekend.

@bors
Copy link
Contributor

bors commented Mar 16, 2023

☔ The latest upstream changes (presumably #108944) made this pull request unmergeable. Please resolve the merge conflicts.

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this May 17, 2023
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

Labels
A-diagnostics Area: Messages for errors, warnings, and lints S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@sharnoff @deltragon @bors @oli-obk @albertlarsan68 @rustbot @Dylan-DPC @pnkfelix @TaKO8Ki