Fix ICE when casting on constant expressions #9909
Conversation
|
Can you add a test for this as well? Would just want to make sure that it doesn't regress in the future. |
|
Done. Is this sufficient ? I'm not quite familiar with test suites... :/ |
|
Ok for the comment. For the second part, I'm not sure to understand. How do I do this ? |
|
That looks good to me, thanks! |
|
@LeoTestard: not sure what's wrong with the fix for constant expressions, so I sent in another pull request for the |
|
Should be ok now. |
|
Could you rebase to remove the |
|
We discussed this with @alexcrichton on IRC and decided this PR was not ready for merge yet, there are a couple of special cases to be handled if we want to avoid the weird LLVM error in the future. |
|
Should be ok now. |
| // option. This file may not be copied, modified, or distributed | ||
| // except according to those terms. | ||
|
|
||
| // xfail-test |
There was a problem hiding this comment.
This will actually not run the test. You'll need to have //~ ERROR directives below, although you can't cfg-off error directives so you should probably just cast it to a u16 instead of having different cases for 32/64 bit
There was a problem hiding this comment.
Do I have to retype the exact error message in the //~ERROR directive ?
There was a problem hiding this comment.
The problem is that if you have two ERROR directives the test harness will expect two errors to get emitted (which won't happen the way it's currently set up)
There was a problem hiding this comment.
Ok, I see. I will use u16. But what must I type next to //~ERROR exactly ? The whole error message "cannot cast... " etc. ?
|
@alexcrichton I added the error message and the comment as you requested |
|
If the value isn't known at compile-time, how can it be statically used in an array size? It doesn't seem like it would be meeting the requirements of Rust's |
|
FWIW, C++ doesn't permit this with the equivalent feature: It allows it in a immutable (const) global but not a true constant. Rust's |
|
I guess #9866 is related, since we are already doing stuff like this. |
|
It does seem like this cast is just not valid. |
|
@brson I don't think it is so simple. If we consider the current meaning of static (I.E. "must be known at compile-time"), then it is indeed, but foreign statics are not valid too. Just like mutable statics (though they require an unsafe block to be accessed. |
|
@brson @LeoTestard Is there a semantic ambiguity that needs to be decided on here? Is it something that could become an RFC issue and/or be discussed in an upcoming meeting? Or can this go ahead and be merged? |
|
@catamorphism I don't think this PR can be merged "as it". But yes, I think there is indeed a semantic ambiguity on the current meaning on static. If we allow this cast, static is not anymore a "fully compile time known constant".
And whatever we decide about the semantics of static (here is already an issue about this, doing "statically" (by static I don't necessarily mean that I want this to be the meaning of "static" in Rust. I just mean "ahead of time") this kind of things with values that are only known at link (and/or load-time) may be useful in some cases, in particular with FFI or something like that (this issue is related too). I (and maybe other people on #rust-osdev) have use cases for this. We should decide if we allow this, and how. |
|
Closing due to inactivity, it sounds like this needs more thought to see how much of this we're willing to allow. Feel free to reopen if progress is made, though! |
[`get_unwrap`]: include a borrow in the suggestion if argument is not an integer literal Fixes rust-lang#9909 I have to say, I don't really understand what the previous logic was trying to do, but this fixes the linked bug. It was checking if the argument passed to `.get()` can be parsed as a usize (i.e. if it's an integer literal, probably?), and if not, it wouldn't include a borrow? I don't know how we came to that conclusion, but that logic doesn't work: ```rs let slice = &[1, 2]; let _r: &i32 = slice.get({ 1 }).unwrap(); // previous suggestion: slice[{ 1 }] // the suggestion should be: &slice[{ 1 }] ``` Here the argument passed to it isn't an integer literal, but it should still include a borrow, because it would otherwise change the type from `&i32` to `i32`. The exception is that if the parent of the `get().unwrap()` expr is a dereference or a method call or the like, we don't need an explicit borrow because it's automatically inserted by the compiler changelog: [`get_unwrap`]: include a borrow in the suggestion if argument is not an integer literal
This pull request fixes the bug I opened ( #9867 )
It also fixes a small issue that was causing Rustc build to fail on my setup, by removing a rusti target in the Makefile.