fix(naga): Improve validation of non-constructible types

[andyleiserson]

Somewhat a follow-up to #8741. Fixes #4720, and fixes the test cases in #7393, but not the issue itself.

After studying the spec more and the related CTS test cases, I think the necessary zero-value validation can be simplified to "type must be constructible". So this changes the zero-value validation from #8741 to operate that way.

Also adds other validation in the WGSL front-end for non-constructible types in places they are not permitted.

This change is adjacent to the template-list discovery change (#8386), and may have some conflicts, but as best I could tell the two changes don't directly overlap.

Testing
Enables CTS tests and adds naga wgsl_errors and validation tests.

Squash or Rebase? Squash

Checklist

• Run cargo fmt.
• Run taplo format.
• Run cargo clippy --tests. If applicable, add:
  • --target wasm32-unknown-unknown
• Run cargo xtask test to run tests.
• Need to add a CHANGELOG.md entry.

[jimblandy]

It looks to me like the changes in construction.rs need to be reimplemented for the post-#8386 world. In particular, I don't think it's true any more that:

the non-constructible types not excluded by !is_dynamically_sized are rejected by the parser.

I apologize for not reviewing this more promptly, since that resulted in throwing the rewrite back at you. But given the changes in #8386, this would have needed to be rewritten one way or another.

[andyleiserson]

I don't think this necessarily needs major changes after #8386.

It's true that the parser doesn't reject everything that it did before. In particular, the zero-value texture_2d test case that was previously rejected by the parser no longer is. But it is still rejected in the validator. I believe there are also other cases that will not be rejected prior to the validator, and weren't even in the pre-#8386 state of this PR, e.g. a struct containing a texture_2d is not considered dynamically sized in the frontend, so is accepted for zero-value construction, but then the validator rejects the struct as invalid.

I think the question is, how much effort do we want to undertake to catch these cases in the frontend for the sake of generating better error messages? I could add something in type_methods to give a conservative indication of types that are never constructible, and use that alongside !is_dynamically_sized in the constructor lowering? Not sure what to call it... is_definitely_not_constructible?

[jimblandy]

Okay. Let's get this branch rebased on current trunk, and then I'll review it.

[jimblandy]

I deleted a test that assumed texture_2d<f32>() would be validated out in the front end; it is now caught in the validator.

[jimblandy]

It seems like all the CTS tests added to tests.lst by this PR pass on trunk even without this PR. Is that expected?

[jimblandy]

I was wishing we could keep the front end simple and just generate invalid WGSL that the validator can complain about, but now I think I understand why Lowerer::construct needs to bother trying to decide if the type is constructible. Consider code like this (from the tests):

let x = array<u32>();

If lowering array<u32>() doesn't complain, then it turns out that it is the front end's lowering of the let declaration that does. WGSL let expressions are not visible in the IR, except as potentially-multiply-referenced Expressions, so the validator cannot enforce WGSL's requirement that the types of let bindings be constructible. Thus, that responsibility falls on the front end.

Unfortunately, that gets us a mediocre error whose span points at x: although in this specific case it's obvious that it's the zero-value construction expression that's at fault, the let lowering code only knows that it's got an initializer with a bad type, and in general there is no smoking gun that elicited that bad type, so it doesn't go looking inside that expression to find the best span to blame it on.

So the early type-checking on let expressions prevents the bad zero-value construction expression from making it to the validator, which would give us a decent error message.

[jimblandy]

@andyleiserson I went ahead and added a TypeInner::is_constructable method. Could you give it a look?

[jimblandy]

Looks good to me.

[andyleiserson]

WGSL let expressions are not visible in the IR, except as potentially-multiply-referenced Expressions, so the validator cannot enforce WGSL's requirement that the types of let bindings be constructible. Thus, that responsibility falls on the front end.

I am leaving #7393 open to improve the handling of this. (And it's linked from the comment on this check in the front-end.)

It seems like all the CTS tests added to tests.lst by this PR pass on trunk even without this PR. Is that expected?

Yes. There are some improvements to the CTS tests in gpuweb/cts#4550, see the description of that PR for explanation of why the CTS was passing before. There's also the directed tests added by this PR, which distinguish between things that are rejected by the frontend and that are rejected by the validator (which the CTS does not).

I went ahead and added a TypeInner::is_constructable method. Could you give it a look?

I don't see it, did you push this change?

[jimblandy]

I don't see it, did you push this change?

Indeed, I had forgotten to push it. Now visible as 64f86c0

[jimblandy]

Oh, I see what I did: I gave the jj-generated branch name a more legible name for my own purposes, and then misconfigured it.

[jimblandy]

In ae0852b I refined TypeInner::is_constructible per your feedback in chat:

• I made it recurse on array element and struct member types, to be fully compliant with WGSL.
• I added a debug assertion to the validator that TypeInner::is_constructible reaches the same conclusion as Validator::validate_type.

[andyleiserson]

LGTM.

I think in the latest version, my change to make is_dynamically_sized return true for override-sized arrays no longer immediately matters. (Now, the only new call is in handling of let declarations, which can't be override-sized arrays, and all the pre-existing call sites only cared about distinguishing runtime-sized arrays.)

I think it still makes sense to have return true for override-sized arrays, though, because that way it's more likely to get appropriate attention on the override case for new uses that might get added in the future.