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.

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

Improve reference cast help message #37375

Merged
merged 1 commit into from
Nov 17, 2016
Merged

Conversation

GuillaumeGomez
Copy link
Member

Fixes #37338.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Oct 24, 2016

You completely changed the meaning though, now it's bad if the user wanted to do the pointer thing.
cc @rust-lang/compiler

@GuillaumeGomez
Copy link
Member Author

But at least it's not incorrect anymore.

@TimNN
Copy link
Contributor

TimNN commented Oct 24, 2016

@GuillaumeGomez: I'm not sure, but wouldn't your patch suggest dereferencing the pointer also in the following case:

let _ = &String::new() as usize;

Here, the original message is more accurate, I think.

@fhartwig
Copy link
Contributor

fhartwig commented Oct 24, 2016

In @TimNN's example, the original message clearly makes more sense. In the example from #37338, there is genuine ambiguity about which one the user wanted to do, but I would argue that the new suggestion is probably better because I suspect that the deref case is the more common one.

Would it makes sense to check the RPtr's type and only make the new suggestion if it's a numeric type? Or maybe suggest both solutions in that case, since there isn't a way to figure out which one the user meant.

@eddyb
Copy link
Member

eddyb commented Oct 24, 2016

I'd argue that only usize is a resonable target for a pointer-to-integer cast, so we could just not suggest anything otherwise - simplest option - besides, @jonathandturner has some general type mismatch suggestion ideas which should apply here as well.

@GuillaumeGomez
Copy link
Member Author

@TimNN: Well saw and not well saw at the same time. In this case, you should have to explicitly cast into a pointer before casting to usize. However, it just puts under the light a big issue: the compiler cannot guess what the user intends to do in some cases and only provide the simplest solution.

In this case, more people will certainly think that you just want to get the length and you put & by mistake.

@fhartwig: Still difficult. But we could provide both helps. What do you think about this @eddyb?

@eddyb
Copy link
Member

eddyb commented Oct 24, 2016

@GuillaumeGomez IMO having both is more confusing.
Like I said, simplest thing to do is only provide the ptr help when as usize is being attempted.

@GuillaumeGomez
Copy link
Member Author

Ok, I'll update it this way then.

@estebank
Copy link
Contributor

estebank commented Oct 28, 2016

@GuillaumeGomez, I hadn't seen that this PR was already out, so I took a stab at it as well. I still feel there's some value in PR #37442, as it handles the case @TimNN pushed forward as it is now, and specializes the cases of &{integer} and &{float} being cast to {integer} or {float} (as @fhartwig indicated).

For a given file:

fn main() {
    vec![0.0].iter().map(|s| s as i16).collect::<Vec<i16>>();
    let _ = &String::new() as usize;
}

PR #37442 outputs:

error: casting `&f64` as `i16` is invalid
 --> file3.rs:2:35
  |
2 |     vec![0.0].iter().map(|s| s as i16).collect::<Vec<i16>>();
  |                              -    ^^^ casting `&f64` as `i16` is invalid
  |                              |
  |                              try dereferencing for the cast to work

error: casting `&std::string::String` as `usize` is invalid
 --> file3.rs:3:13
  |
3 |     let _ = &String::new() as usize;
  |             ^^^^^^^^^^^^^^^^^^^^^^^ cast through a raw pointer first

error: aborting due to 2 previous errors

@GuillaumeGomez
Copy link
Member Author

@estebank: I still need to update it anyway. I intend to do it this week-end.

@GuillaumeGomez
Copy link
Member Author

Updated.

@frewsxcv
Copy link
Member

frewsxcv commented Nov 2, 2016

Travis failure looks unrelated.

Building stage2 tool linkchecker (x86_64-unknown-linux-gnu)
 Downloading url v1.2.2
 Downloading matches v0.1.3
 Downloading idna v0.1.0
 Downloading unicode-normalization v0.1.2
warning: spurious network error (2 tries remaining): [28] Timeout was reached
error: unable to get packages from source

Caused by:
  failed to verify the checksum of `unicode-normalization v0.1.2`

@GuillaumeGomez
Copy link
Member Author

I restart it.

@GuillaumeGomez
Copy link
Member Author

cc @eddyb

@eddyb
Copy link
Member

eddyb commented Nov 6, 2016

You seem to have removed too many messages.

r? @jonathandturner

@rust-highfive rust-highfive assigned sophiajt and unassigned eddyb Nov 6, 2016
@sophiajt
Copy link
Contributor

sophiajt commented Nov 8, 2016

Agree with @eddyb - why remove the help messages?

@GuillaumeGomez
Copy link
Member Author

@jonathandturner: It comes after a discussion between @eddyb and me. Since a lot of messages were incorrect in some cases, it should be better to just remove them. However, after the last messages, I think I misunderstood him. Any further explanation would be appreciated. :)

@eddyb
Copy link
Member

eddyb commented Nov 8, 2016

What I mean is only providing the "pointer to integer" help if usize is the target, i.e. hiding it for other integer types.

@GuillaumeGomez GuillaumeGomez force-pushed the cast_message branch 2 times, most recently from d4c7a85 to 5c341b1 Compare November 10, 2016 13:36
@GuillaumeGomez
Copy link
Member Author

Updated.

@@ -54,7 +54,7 @@ fn main()
//~^^ HELP through a usize first
let _ = &v as usize;
//~^ ERROR casting
//~^^ HELP through a raw pointer first
//~| HELP cast through a raw pointer first
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to change anything in this file.

b as usize; //~ ERROR non-scalar cast
p as usize;
//~^ ERROR casting
//~^^ HELP cast through a thin pointer
Copy link
Member

Choose a reason for hiding this comment

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

These should still emit the help. It's only when that usize is replaced with another type that it's wrong.

@@ -13,5 +13,4 @@ struct Inches(i32);
fn main() {
Inches as f32;
//~^ ERROR casting
//~^^ cast through a usize first
Copy link
Member

Choose a reason for hiding this comment

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

How could this have disappeared?

Copy link
Contributor

Choose a reason for hiding this comment

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

Inches is a function pointer.

@GuillaumeGomez
Copy link
Member Author

@eddyb: Didn't pay attention to the tests I updated. Fixed now.

@eddyb
Copy link
Member

eddyb commented Nov 10, 2016

@GuillaumeGomez Can you add non-usize cases to the existing tests, to show that they don't suggest?
IMO we should also never suggest anything for NeedViaUsize, who casts pointers to floating-point?!

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Nov 10, 2016

That's another debate I think haha. If you can imagine something weird, someone will do something weirder.

@GuillaumeGomez GuillaumeGomez force-pushed the cast_message branch 3 times, most recently from 9bda5fb to 3f0c2d2 Compare November 10, 2016 22:01
@GuillaumeGomez
Copy link
Member Author

cc @eddyb

@arielb1
Copy link
Contributor

arielb1 commented Nov 16, 2016

NeedViaUsize was introduced because all directions of casts used to be OK under the pre-1.4.0 cast system. It can now be removed and replaced by IllegalCast.

@arielb1
Copy link
Contributor

arielb1 commented Nov 16, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Nov 16, 2016

📌 Commit 37903bf has been approved by arielb1

@bors
Copy link
Contributor

bors commented Nov 17, 2016

⌛ Testing commit 37903bf with merge f22fdb0...

bors added a commit that referenced this pull request Nov 17, 2016
Improve reference cast help message

Fixes #37338.
@bors bors merged commit 37903bf into rust-lang:master Nov 17, 2016
@GuillaumeGomez GuillaumeGomez deleted the cast_message branch November 17, 2016 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants