[naga] parse and validate `@must_use` attribute #6801

turbocrime · 2024-12-21T23:07:48Z

ready for review

~~to be revised now that i can run conformance tests~~

not worrying about conformance tests, as cts_runner seems disused.

Connections

initial work on #5186

Description

When the wgsl parser encounters the @must_use function attribute, it should not error.

This PR will recognize the attribute and emit a new field in function AST. Both the parser and the validator stage will use this attribute.

The parser requires that the annotated function should return a value, and requires that the function call appears in an expression.

The validator requires that return value is referenced or assigned.

A few tests are added covering these behaviors and more.

Testing

tested with wgpu test suite
tested by building Firefox nightly with this change

Checklist

Run cargo fmt.
Run taplo format.
Run cargo clippy. If applicable, add:
- --target wasm32-unknown-unknown
- --target wasm32-unknown-emscripten
Run cargo xtask test to run tests.
Add change to CHANGELOG.md. See simple instructions inside file.

turbocrime · 2024-12-22T00:32:17Z

Looking at discussion in the gpuweb repository linked in the issue, it seems like

wgsl builtins may have the attribute must_use
a wgsl compiler should reject bare calls to must_use
must_use validation should happen at compile-time and not at run-time

I expect that many particularities may complicate validation, so tests are provided for slightly complex valid examples.

I'm unfamiliar with how naga's AST behaves, and looking through the validation code, it didn't seem obvious where/how to validate that a CallResult is 'used', and what qualifies as 'used'.

turbocrime · 2024-12-22T00:34:45Z

~~Are there likely to be important validation differences between constant and variable values?~~

turbocrime

some commentary

turbocrime · 2024-12-22T00:42:24Z

naga/src/front/wgsl/parse/ast.rs

 pub struct FunctionResult<'a> {
    pub ty: Handle<Type<'a>>,
    pub binding: Option<Binding<'a>>,
+    pub must_use: bool,
 }


must_use is a required member of wgsl ast FunctionResult.

turbocrime · 2024-12-22T00:42:53Z

naga/src/front/wgsl/parse/mod.rs

+            Some(ast::FunctionResult {
+                ty,
+                binding,
+                must_use,
+            })


ast::FunctionResult creations are updated

turbocrime · 2024-12-22T00:46:15Z

naga/src/front/wgsl/parse/mod.rs

@@ -2455,6 +2460,7 @@ impl Parser {
        let (mut bind_index, mut bind_group) =
            (ParsedAttribute::default(), ParsedAttribute::default());
        let mut id = ParsedAttribute::default();
+        let mut must_use = ParsedAttribute::default();


attribute is handled in global_decl

is there another location in which @must_use might validly appear?

should invalid locations recognize the attribute at all?

possibly, some global declarations should not have the @must_use attribute. no checks are performed.

turbocrime · 2024-12-22T00:47:39Z

naga/src/front/wgsl/parse/mod.rs

@@ -2333,6 +2333,7 @@ impl Parser {
        diagnostic_filter_leaf: Option<Handle<DiagnosticFilterNode>>,
        out: &mut ast::TranslationUnit<'a>,
        dependencies: &mut FastIndexSet<ast::Dependency<'a>>,
+        must_use: bool,


i added a parameter to function_decl for this attribute.

naga/tests/in/must-use-questionable.wgsl

turbocrime · 2024-12-31T09:57:46Z

i'm going to revise this now that i can run conformance tests after #6840

it is not presently ready for review

Signed-off-by: sagudev <[email protected]>

{"fail_fast": false, "matrix": [{"name": "WebGPU CTS", "workflow": "linux", "wpt_layout": "2020", "profile": "production", "unit_tests": false, "bencher": false, "wpt_args": "_webgpu"}]}

sagudev · 2025-01-01T09:17:34Z

i'm going to revise this now that i can run conformance tests after #6840

Unfortunately am not sure how relevant information from cts_runner actually is, as there is no concept of (bad) expectations (so only selected tests are run) and it uses old CTS version. The most useful results come from browsers running CTS, either Firefox or Servo as both run all test and have expectations properly set, so you know what results become different using your PR.

In servo you can simply override wgpu in Cargo.toml (ensuring that your PR bases from same revision of wgpu previous used in servo) then run ./mach try webgpu in servo folder to trigger CI run of CTS in GitHub Actions. I did initial CTS run for this PR here: https://github.com/sagudev/servo/actions/runs/12568503599/attempts/1#summary-35037584122 (note that there are some flaky crashes in webgpu:shader,execution,expression due to cyclic JS imports, so you can ignore them).

turbocrime · 2025-01-01T23:19:40Z

yes, realizing this since i patched things together. i was hoping to have canonical tests for edge cases and ambiguities, that could execute in a nice automated way, but it seems like the validation tests can't run in cts_runner without more api work.

maybe this repo should remove cts_runner and the documentation that suggests using it, if it's no longer relevant.

thanks for the instructions for executing in servo's ci! this is very useful

jamienicol · 2025-01-17T12:12:40Z

I've ran the CTS without and with this in firefox and it looks good. There are no new failures, and there are a few new passes as expected. There are also quite a few more places where previously we failed due to parsing error: unknown attribute: must_use, but with that fixed now we fail for another reason. (Seems to be type conversion mostly AFAICS)

As you noted above this does have the limitation that it doesn't ensure the result of @must_use builtin functions are actually used. But let's not let perfect be the enemy of good. We can save that for a follow-up. I think this is mostly ready to go.

@turbocrime do you still have changes planned for this, or would you like me to review as-is? Or if you're too busy I'm happy to take this over the line for you. LMK!

turbocrime · 2025-01-18T00:15:35Z

naga/tests/wgsl_errors.rs

+#[test]
+#[ignore = "validation can't be tested if parser fails"]
+fn must_use_unused_validation() {
+    check_validation! {
+        "
+        @must_use
+        fn use_me(a: i32) -> i32 {
+            return 10;
+        }
+
+        fn useless() -> i32 {
+            use_me(1);
+            return 0;
+        }
+        ":
+        Err(
+            naga::valid::ValidationError::Function {
+                handle: _caller,
+                name: caller_name,
+                source: naga::valid::FunctionError::InvalidCall {
+                    function: _callee,
+                    error: naga::valid::CallError::ResultNotUsed(_result),
+                },
+            },
+        )
+        if caller_name == "useless"
+    }
+}
+


not sure how to test validation if the parser will reject the shader. perhaps i could manually create the AST by parsing a shader without the attribute and then modifying it.

turbocrime · 2025-01-18T00:22:38Z

As you noted above this does have the limitation that it doesn't ensure the result of @must_use builtin functions are actually used. But let's not let perfect be the enemy of good. We can save that for a follow-up. I think this is mostly ready to go.

i assume the builtins aren't presently 'annotated'. if must_use builtins are expressed correctly in AST, perhaps validation phase can catch that.

validation code is present and works if i remove the parser error, but i'm not sure how to test it when the parser throws. i'm not sure if the validation code is actually useful - would the parser know about builtins' attributes at parsing time? will the parsing step ever be skipped? do we care about that validation if the parsing step is skipped?

@turbocrime do you still have changes planned for this, or would you like me to review as-is? Or if you're too busy I'm happy to take this over the line for you. LMK!

i had a local commit sitting around that reverted some random junk. pushed that up, ready for you to take a look.

i will have way less free time starting monday so feel free to take over.

turbocrime · 2025-01-18T01:30:49Z

if you'd like to see the validation phase successfully detect an unused call, see f028d07 where i have disabled the parser failures

turbocrime · 2025-01-18T03:00:46Z

this is covered by the parser, but perhaps the validator should also error if it encounters a call to some @must_use that somehow returns nothing, whether it's in an expression or a statement. i experimented briefly but couldn't get the parser to emit an example.

turbocrime force-pushed the turbocrime/must_use branch 2 times, most recently from 17da613 to 1331f0f Compare December 22, 2024 00:00

turbocrime changed the title ~~parse @must_use attribute~~ [naga] parse @must_use attribute Dec 22, 2024

turbocrime force-pushed the turbocrime/must_use branch from 1331f0f to 201ce67 Compare December 22, 2024 00:19

turbocrime marked this pull request as ready for review December 22, 2024 00:32

turbocrime requested a review from a team as a code owner December 22, 2024 00:32

turbocrime commented Dec 22, 2024

View reviewed changes

turbocrime marked this pull request as draft December 24, 2024 19:26

ErichDonGubler self-assigned this Dec 28, 2024

ErichDonGubler self-requested a review December 28, 2024 13:05

turbocrime force-pushed the turbocrime/must_use branch from 201ce67 to 31b3e47 Compare December 31, 2024 23:41

sagudev added a commit to sagudev/servo that referenced this pull request Jan 1, 2025

try gfx-rs/wgpu#6801

aa3b33e

Signed-off-by: sagudev <[email protected]>

turbocrime commented Jan 18, 2025

View reviewed changes

turbocrime marked this pull request as ready for review January 18, 2025 00:50

turbocrime added 5 commits January 17, 2025 16:51

feat: parse and enforce @must_use attribute

ea0baaa

feat: refuse @must_use of void function

962a209

generated naga/tests/out/ir

bfa3cbb

revert extraneous changes

b37277f

fix clippy

cfe925f

turbocrime force-pushed the turbocrime/must_use branch from 6247e79 to cfe925f Compare January 18, 2025 00:51

update repeated attribute error to trunk format

04922e3

struct members can't have @must_use

e176586

turbocrime changed the title ~~[naga] parse @must_use attribute~~ [naga] parse and validate @must_use attribute Jan 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[naga] parse and validate `@must_use` attribute #6801

[naga] parse and validate `@must_use` attribute #6801

turbocrime commented Dec 21, 2024 •

edited

Loading

turbocrime commented Dec 22, 2024

turbocrime commented Dec 22, 2024 •

edited

Loading

turbocrime left a comment

turbocrime Dec 22, 2024

turbocrime Dec 22, 2024

turbocrime Dec 22, 2024 •

edited

Loading

turbocrime Dec 22, 2024

turbocrime commented Dec 31, 2024

sagudev commented Jan 1, 2025

turbocrime commented Jan 1, 2025 •

edited

Loading

jamienicol commented Jan 17, 2025 •

edited

Loading

turbocrime Jan 18, 2025

turbocrime commented Jan 18, 2025 •

edited

Loading

turbocrime commented Jan 18, 2025

turbocrime commented Jan 18, 2025 •

edited

Loading

[naga] parse and validate @must_use attribute #6801

Are you sure you want to change the base?

[naga] parse and validate @must_use attribute #6801

Conversation

turbocrime commented Dec 21, 2024 • edited Loading

ready for review

turbocrime commented Dec 22, 2024

turbocrime commented Dec 22, 2024 • edited Loading

turbocrime left a comment

Choose a reason for hiding this comment

turbocrime Dec 22, 2024

Choose a reason for hiding this comment

turbocrime Dec 22, 2024

Choose a reason for hiding this comment

turbocrime Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

turbocrime Dec 22, 2024

Choose a reason for hiding this comment

turbocrime commented Dec 31, 2024

sagudev commented Jan 1, 2025

turbocrime commented Jan 1, 2025 • edited Loading

jamienicol commented Jan 17, 2025 • edited Loading

turbocrime Jan 18, 2025

Choose a reason for hiding this comment

turbocrime commented Jan 18, 2025 • edited Loading

turbocrime commented Jan 18, 2025

turbocrime commented Jan 18, 2025 • edited Loading

[naga] parse and validate `@must_use` attribute #6801

[naga] parse and validate `@must_use` attribute #6801

turbocrime commented Dec 21, 2024 •

edited

Loading

turbocrime commented Dec 22, 2024 •

edited

Loading

turbocrime Dec 22, 2024 •

edited

Loading

turbocrime commented Jan 1, 2025 •

edited

Loading

jamienicol commented Jan 17, 2025 •

edited

Loading

turbocrime commented Jan 18, 2025 •

edited

Loading

turbocrime commented Jan 18, 2025 •

edited

Loading