Skip to content

fix(wgsl-in): Handle Unterminated Block Comments - #9356

Merged
ErichDonGubler merged 4 commits into
gfx-rs:trunkfrom
BKDaugherty:brendon/8877/comment-parsing
Apr 2, 2026
Merged

fix(wgsl-in): Handle Unterminated Block Comments#9356
ErichDonGubler merged 4 commits into
gfx-rs:trunkfrom
BKDaugherty:brendon/8877/comment-parsing

Conversation

@BKDaugherty

@BKDaugherty BKDaugherty commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

Adds a new token to keep track of unterminated block comments in order to emit an error to the user highlighting the offending unterminated block comment.

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories

#8877 (comment)

When one pull request builds on another, please put "Depends on
#NNNN" towards the top of its description. This helps maintainers
notice that they shouldn't merge it until its ancestor has been
approved. Don't use draft PR status to indicate this.

Description
Describe what problem this is solving, and how it's solved.

Block comments that aren't terminated before this changeset were lexed as Token::End. This didn't give the parser enough information to report an error.

This PR introduces a new token so that this can be tracked, and reports errors when an unterminated block comment is found.

Testing
Explain how this change is tested.

This CTS test passes now. I also wrote tests.

[INFO  wgpu_xtask::cts] Running CTS
[INFO  wgpu_xtask::cts] Running webgpu:shader,validation,parse,comments:unterminated_block_comment:terminated=false
$ /Users/brendon/Dev/wgpu/target/debug/cts_runner ./tools/run_deno --verbose webgpu:shader,validation,parse,comments:unterminated_block_comment:terminated=false
[pass] webgpu:shader,validation,parse,comments:unterminated_block_comment:terminated=false (348ms). Log:
  - (in subcase: ) INFO: subcase ran

** Summary **
Passed  w/o warnings = 1 / 1 = 100.00%
Passed with warnings = 0 / 1 =   0.00%
Skipped              = 0 / 1 =   0.00%
Failed               = 0 / 1 =   0.00%

Squash or Rebase?

Squash.

If your pull request contains multiple commits, please indicate whether
they need to be squashed into a single commit before they're merged,
or if they're ready to rebase onto trunk as they stand. In the
latter case, please ensure that each commit passes all CI tests, so
that we can continue to bisect along trunk to isolate bugs.

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.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@BKDaugherty
BKDaugherty marked this pull request as ready for review April 1, 2026 18:24

@ErichDonGubler ErichDonGubler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just a couple of minor things that we can fix (if they're easy) before merging.

Comment thread naga/src/front/wgsl/error.rs Outdated
message: "unterminated block comment".to_string(),
labels: vec![(
span,
"must be closed with */".into(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Let's add backticks so the actual code being suggested is clear, like so:

Suggested change
"must be closed with */".into(),
"must be closed with `*/`".into(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed as suggested as a fixup! commit (510f538). LMK if this looks okay to you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah looks great! Thanks for doing that!

Comment thread cts_runner/test.lst
@ErichDonGubler ErichDonGubler self-assigned this Apr 2, 2026
@ErichDonGubler ErichDonGubler added area: correctness We're behaving incorrectly naga Shader Translator area: naga front-end Translation to Naga IR labels Apr 2, 2026
@ErichDonGubler ErichDonGubler changed the title fix(naga): Handle Unterminated Block Comments fix(wgsl-in): Handle Unterminated Block Comments Apr 2, 2026
@ErichDonGubler ErichDonGubler added lang: WGSL WebGPU Shading Language type: bug Something isn't working area: cts Issues stemming from the WebGPU Conformance Test Suite labels Apr 2, 2026
@ErichDonGubler

Copy link
Copy Markdown
Member

Not sure what's going on with the CHANGELOG check, but it's got something to do with Git. I bet if you rebase, it'll clear up.

BKDaugherty and others added 4 commits April 1, 2026 22:41
Adds a new token to keep track of unterminated block comments in order
to emit an error to the user highlighting the offending unterminated
block comment.
@BKDaugherty
BKDaugherty force-pushed the brendon/8877/comment-parsing branch from 510f538 to 584749b Compare April 2, 2026 05:41
@ErichDonGubler
ErichDonGubler merged commit 1080b4f into gfx-rs:trunk Apr 2, 2026
59 checks passed
@BKDaugherty

Copy link
Copy Markdown
Contributor Author

Thanks for the quick review and fixes @ErichDonGubler!

slyedoc pushed a commit to slyedoc/wgpu that referenced this pull request May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: correctness We're behaving incorrectly area: cts Issues stemming from the WebGPU Conformance Test Suite area: naga front-end Translation to Naga IR lang: WGSL WebGPU Shading Language naga Shader Translator type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants