[naga valid]: Clean up validation of Statement::ImageStore.

[jimblandy]

• Ensure that the type we obtain for the image operand is correct:
  "see through" a binding array to its element type only when image is
  actually an Access or AccessIndex expression. (This changes the
  set of programs the validator will pass, but it turns out not to
  affect the set of WGSL programs that Naga will accept, since the WGSL
  front end is already checking the types of the texture arguments to
  textureStore function calls, in order to decide which overload
  applies.)
  Rename the variables to better reflect their values.

• Move image class validation to a more natural spot.

• Use let-else statements to keep the main flow of control at the
  left. Briefly describe the conditions we're testing.

[teoxoy]

I find that let-else looks odd but maybe I just need to get used to it.

Something I wrote in a previous review:

I'm not a fan of if let else because the thing being matched is after the pattern (reversed compared to a match) which disrupts the flow when reading the code. I find let else to be even worse in this regard because there is no indication that it's a condition unless you look for the else.

I see value in using if let else & let else when they are used for pattern matching + binding but I think there are better alternatives when pattern matching.

from #6424 (comment)

This and the other usages in this PR are the pattern matching + binding case and so I think there is value in using let-else but I still find that "there is no indication that it's a condition unless you look for the else" disrupting the flow when reading the code.

We shouldn't block the PR on this, I'd just like to hear your thoughts on it.

[cwfitzgerald]

I agree when the match condition is very long, causing the wrapping to be all weird,

let Some(blah) = blah else {
    continue;

is amazing tho

[jimblandy]

What I'm aiming for here is to keep the successful path of execution all the way to the left. That is, as you read down the main spine of the function, you should be following the path that we expect most calls to take. Nested code is reporting an error or handling some special case. This is a style that we followed in SpiderMonkey, and I found it very helpful.

The problem with match statements is that you can only use pattern-matched bindings in code that is nested inside the match, so if you want to follow the above principle, you end up writing stuff like this:

let (foo, bar, baz) = match thing {
    ArmA { a, b,c } => (a, b, c),
    ArmB => return Err(...);
}

That seems harder to follow than this:

let ArmA { a: foo, b: bar: c: baz } = thing else {
    return Err(...);
};

I think it's reasonable to be concerned that "there's no indication that it's a condition", but the thing about let else is that the else block must diverge. It has to break, return, continue, or something like that. This is because the success path introduces bindings that are in scope following the let else, and in the else case there are no values for the pattern's variables. So you almost (almost) don't even care whether it's a condition, because the else just isn't going to impact what happens next.

I think let else could be abused, but I think this PR uses it in a helpful way: after the change, the code is just a flat list of conditions being checked for.