Emit clear error for index into zero-sized Arrays - #171
JulianKnodt wants to merge 1 commit into
Conversation
|
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 |
LegNeato
left a comment
LegNeato
left a comment
There was a problem hiding this comment.
Sorry, for the error messages I meant matching the same case in rustc to the case here where it makes sense. I'll leave it up to you to change them so they make sense!
A couple of small comments, thanks again for the PR and sorry for the slow response!
| #[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.
|
@eddyb can you review this when you get a chance? |
This branch has not been deployed
3 participants
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