Emit clear error for index into zero-sized Arrays #171
Conversation
JulianKnodt
requested review from
eddyb,
LegNeato and
fornwall
as code owners
|
Awesome, thanks for the PR! 🍻 I'm not super familiar with this code but the change looks reasonable to me. I'll wait for some other maintainers to look at it before reviewing 🤙 Looks like you need to run |
|
Somehow this passes locally but not on CI 🤣, I'll have to test a bit more |
JulianKnodt
changed the title
|
I've changed my expectation, previously it was erroring for accessing zero sized arrays, but it was hidden behind a deny that is on by default. Instead, I think we should always panic, but with a clear error message. I'm unsure what the behavior would be if that expression were allowed to be compiled. |
JulianKnodt
changed the title
|
Can we make the error messages match rustc? For example: |
|
I can, I do worry that since it's not the actual error it may be more confusing |
| #[spirv(compute(threads(1, 1, 1)))] | ||
| pub fn compute() { | ||
| let mut array = [0; 0]; | ||
| // for some reason this now compiles? |
There was a problem hiding this comment.
We should look into this case, no?
There was a problem hiding this comment.
yea, I want to see if it compiles to include a panic, but I'm unsure of how to run the rest to see what happens
Is it run automatically by compiletest
There was a problem hiding this comment.
No, sadly compiletest just checks it compiles. We don't have any automated runtime tests right now sadly. I usually just drop the code in one of the examples when I am doing a quick manual test.
There was a problem hiding this comment.
I've checked the output spirv assembly for that example (by adding the compile flags), and it seems that the assignment is getting optimized out somehow. I checked all the spirv builder methods, and none of them seem to be hit. I've found that any reads from the array hit gep and fail, but stores do not. I feel like ideally that should give a runtime panic, but I'm not entirely sure where to inject it.
While investigating this, I also noticed that zst arrays as input parameters do not work.&mut [T; 0] also gives the old cryptic error message. I'm not sure if I should fix that in this PR, or another
There was a problem hiding this comment.
Is the store behavior changed by this PR? If it is unchanged I'd say just leave it and file a followup task.
Feel free to fix the input message in this PR if you want.
There was a problem hiding this comment.
Not sure if this is answered or ready to review, ping if so!
There was a problem hiding this comment.
Oops I forgot to reply because of the holidays, I checked but the behavior isn't changed, this previously also compiled on main
Previously GEP on zero-sized arrays, would fail. This change fixes arrays to instead emit runtime arrays. I do not know if this will lead to any runtime cost, but it fixes all the compile errors.
Previously GEP on zero-sized arrays, would fail. This change fixes arrays to instead emit runtime arrays. I do not know if this will lead to any runtime cost, but it fixes all the compile errors.
I did try out
[(); 0].map(|()| 1), and found that that does not compile.Fixes #151
Currently
[T; 0] = Tstill compiles, will need to file some follow up issues