Skip to content

Fix incorrect suggestions for E0605 #84968

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

Merged
merged 1 commit into from
May 28, 2021
Merged

Conversation

FabianWolff
Copy link
Contributor

@FabianWolff FabianWolff commented May 6, 2021

Fixes #84598. Here is a simplified version of the problem presented in issue #84598:

#![allow(unused_variables)]
#![allow(dead_code)]

trait T { fn t(&self) -> i32; }

unsafe fn foo(t: *mut dyn T) {
    (t as &dyn T).t();
}

fn main() {}

The current output is:

error[E0605]: non-primitive cast: `*mut (dyn T + 'static)` as `&dyn T`
 --> src/main.rs:7:5
  |
7 |     (t as &dyn T).t();
  |     ^^^^^^^^^^^^^ invalid cast
  |
help: borrow the value for the cast to be valid
  |
7 |     (&t as &dyn T).t();
  |      ^

This is incorrect, though: The cast will not be valid when writing &t instead of t:

error[E0277]: the trait bound `*mut (dyn T + 'static): T` is not satisfied
 --> t4.rs:7:6
  |
7 |     (&t as &dyn T).t();
  |      ^^ the trait `T` is not implemented for `*mut (dyn T + 'static)`
  |
  = note: required for the cast to the object type `dyn T`

The correct suggestion is &*t, which I have implemented in this pull request. Of course, this suggestion will always require an unsafe block, but arguably, that's what the user really wants if they're trying to cast a pointer to a reference.

In any case, claiming that the cast will be valid after implementing the suggestion is overly optimistic, as the coercion logic doesn't seem to resolve all nested obligations, i.e. the cast may still be invalid after implementing the suggestion. I have therefore rephrased the suggestion slightly ("consider borrowing the value" instead of "borrow the value for the cast to be valid").

Additionally, I have fixed another incorrect suggestion not mentioned in #84598, which relates to casting immutable references to mutable ones:

fn main() {
    let mut x = 0;
    let m = &x as &mut i32;
}

currently leads to

error[E0605]: non-primitive cast: `&i32` as `&mut i32`
 --> t5.rs:3:13
  |
3 |     let m = &x as &mut i32;
  |             ^^^^^^^^^^^^^^ invalid cast
  |
help: borrow the value for the cast to be valid
  |
3 |     let m = &mut &x as &mut i32;
  |             ^^^^

which is obviously incorrect:

error[E0596]: cannot borrow data in a `&` reference as mutable
 --> t5.rs:3:13
  |
3 |     let m = &mut &x as &mut i32;
  |             ^^^^^^^ cannot borrow as mutable

I've changed the suggestion to a note explaining the problem:

error[E0605]: non-primitive cast: `&i32` as `&mut i32`
 --> t5.rs:3:13
  |
3 |     let m = &x as &mut i32;
  |             ^^^^^^^^^^^^^^ invalid cast
  |
note: this reference is immutable
 --> t5.rs:3:13
  |
3 |     let m = &x as &mut i32;
  |             ^^
note: trying to cast to a mutable reference type
 --> t5.rs:3:19
  |
3 |     let m = &x as &mut i32;
  |                   ^^^^^^^^

In this example, it would have been even nicer to suggest replacing &x with &mut x, but this would be much more complex because we would have to take apart the expression to be cast (currently, we only look at its type), and &x could be stored in a variable, where such a suggestion would not even be directly applicable:

fn main() {
    let mut x = 0;
    let r = &x;
    let m = r as &mut i32;
}

My solution covers this case, too.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 6, 2021
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 28, 2021

📌 Commit 71d1b2a has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 28, 2021
@bors
Copy link
Collaborator

bors commented May 28, 2021

⌛ Testing commit 71d1b2a with merge 8eef79c...

@bors
Copy link
Collaborator

bors commented May 28, 2021

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 8eef79c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 28, 2021
@bors bors merged commit 8eef79c into rust-lang:master May 28, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

E0605 suggests invalid cast
5 participants