Skip to content

Conversation

andyleiserson
Copy link
Contributor

Replace control characters when emitting diagnostics in naga.

The motivation here is that my fuzzing runs were making my terminals unhappy. In most cases, in a browser, they won't go to a terminal so it won't matter, but this seems like the right thing to do regardless.

Testing
Adds a unit test and a wgsl_errors test.

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

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

LGTM minus nits!

Comment on lines 148 to 153
let found_len = res[base + found_pos..].chars().next().unwrap().len_utf8();
res.to_mut().replace_range(
base + found_pos..base + found_pos + found_len,
REPLACEMENT_CHAR,
);
base += found_pos + REPLACEMENT_CHAR.len();
Copy link
Member

Choose a reason for hiding this comment

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

nitpick(non-blocking): I think it'd be easier to understand this code (including base += …) with base + found_pos factored out to a variable. Not a big deal, though.

}

pub(crate) fn replace_control_chars(s: &str) -> Cow<'_, str> {
const REPLACEMENT_CHAR: &str = "\u{FFFD}";
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: We should use std::char::REPLACEMENT_CHARACTER instead of using this ad-hoc constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to, but couldn't find a way to go from char to &str without a buffer.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, hmm. I suppose you could use a stack-allocated buffer and char::encode_utf8?

I'd be satisfied with merely a (doc) comment pointing to REPLACEMENT_CHARACTER expressing the type woe you're working around, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a debug assertion that the character in the string equals char::REPLACEMENT_CHARACTER.

Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Let's add a CHANGELOG entry. This isn't likely to be noticeable for most folks' shaders, but it's good to call out.

@andyleiserson andyleiserson merged commit 0cc8c11 into gfx-rs:trunk Aug 11, 2025
40 checks passed
@andyleiserson andyleiserson deleted the diag-escape branch August 11, 2025 19:16
@LukasKalbertodt
Copy link

I found this PR in the release notes and just wanted to drop this here: https://www.compart.com/en/unicode/block/U+2400
Unicode has special characters to display control characters. Might make sense to use those instead of FFFD for every control character.

@ErichDonGubler
Copy link
Member

@LukasKalbertodt: Filed #8295. Thanks!

sharmajai pushed a commit to sharmajai/wgpu that referenced this pull request Oct 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants