Skip to content

Escape control chars in source when printing diagnostics #8049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions naga/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use alloc::{boxed::Box, string::String};
use alloc::{borrow::Cow, boxed::Box, string::String};
use core::{error::Error, fmt};

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -41,7 +41,7 @@ impl fmt::Display for ShaderError<crate::WithSpan<crate::valid::ValidationError>
use codespan_reporting::{files::SimpleFile, term};

let label = self.label.as_deref().unwrap_or_default();
let files = SimpleFile::new(label, &self.source);
let files = SimpleFile::new(label, replace_control_chars(&self.source));
let config = term::Config::default();

let writer = {
Expand Down Expand Up @@ -137,3 +137,29 @@ where
Some(&self.inner)
}
}

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.

let mut res = Cow::Borrowed(s);
let mut base = 0;

while let Some(found_pos) = res[base..].find(|c: char| c.is_control() && !c.is_whitespace()) {
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();
}
Comment on lines +148 to +153
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.


res
}

#[test]
fn test_replace_control_chars() {
// The UTF-8 encoding of \u{0080} is multiple bytes.
let input = "Foo\u{0080}Bar\u{0001}Baz\n";
let expected = "Foo\u{FFFD}Bar\u{FFFD}Baz\n";
assert_eq!(replace_control_chars(input), expected);
}
4 changes: 2 additions & 2 deletions naga/src/front/glsl/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use pp_rs::token::PreprocessorError;
use thiserror::Error;

use super::token::TokenValue;
use crate::SourceLocation;
use crate::{error::replace_control_chars, SourceLocation};
use crate::{error::ErrorWrite, proc::ConstantEvaluatorError, Span};

fn join_with_comma(list: &[ExpectedToken]) -> String {
Expand Down Expand Up @@ -171,7 +171,7 @@ impl ParseErrors {

pub fn emit_to_writer_with_path(&self, writer: &mut impl ErrorWrite, source: &str, path: &str) {
let path = path.to_string();
let files = SimpleFile::new(path, source);
let files = SimpleFile::new(path, replace_control_chars(source));
let config = term::Config::default();

for err in &self.errors {
Expand Down
8 changes: 6 additions & 2 deletions naga/src/front/spv/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ use codespan_reporting::files::SimpleFile;
use codespan_reporting::term;

use super::ModuleState;
use crate::{arena::Handle, error::ErrorWrite, front::atomic_upgrade};
use crate::{
arena::Handle,
error::{replace_control_chars, ErrorWrite},
front::atomic_upgrade,
};

#[derive(Clone, Debug, thiserror::Error)]
pub enum Error {
Expand Down Expand Up @@ -157,7 +161,7 @@ impl Error {

pub fn emit_to_writer_with_path(&self, writer: &mut impl ErrorWrite, source: &str, path: &str) {
let path = path.to_string();
let files = SimpleFile::new(path, source);
let files = SimpleFile::new(path, replace_control_chars(source));
let config = term::Config::default();
let diagnostic = Diagnostic::error().with_message(format!("{self:?}"));

Expand Down
5 changes: 3 additions & 2 deletions naga/src/front/wgsl/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use crate::common::wgsl::TryToWgsl;
use crate::diagnostic_filter::ConflictingDiagnosticRuleError;
use crate::error::replace_control_chars;
use crate::proc::{Alignment, ConstantEvaluatorError, ResolveError};
use crate::{Scalar, SourceLocation, Span};

Expand Down Expand Up @@ -79,7 +80,7 @@ impl ParseError {
P: AsRef<std::path::Path>,
{
let path = path.as_ref().display().to_string();
let files = SimpleFile::new(path, source);
let files = SimpleFile::new(path, replace_control_chars(source));
let config = term::Config::default();

cfg_if::cfg_if! {
Expand All @@ -105,7 +106,7 @@ impl ParseError {
P: AsRef<std::path::Path>,
{
let path = path.as_ref().display().to_string();
let files = SimpleFile::new(path, source);
let files = SimpleFile::new(path, replace_control_chars(source));
let config = term::Config::default();

let mut writer = crate::error::DiagnosticBuffer::new();
Expand Down
6 changes: 3 additions & 3 deletions naga/src/span.rs
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.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use alloc::{
};
use core::{error::Error, fmt, ops::Range};

use crate::{path_like::PathLike, Arena, Handle, UniqueArena};
use crate::{error::replace_control_chars, path_like::PathLike, Arena, Handle, UniqueArena};

/// A source code span, used for error reporting.
#[derive(Clone, Copy, Debug, PartialEq, Default)]
Expand Down Expand Up @@ -291,7 +291,7 @@ impl<E> WithSpan<E> {
use codespan_reporting::{files, term};

let path = path.to_string_lossy();
let files = files::SimpleFile::new(path, source);
let files = files::SimpleFile::new(path, replace_control_chars(source));
let config = term::Config::default();

cfg_if::cfg_if! {
Expand Down Expand Up @@ -323,7 +323,7 @@ impl<E> WithSpan<E> {
use codespan_reporting::{files, term};

let path = path.to_string_lossy();
let files = files::SimpleFile::new(path, source);
let files = files::SimpleFile::new(path, replace_control_chars(source));
let config = term::Config::default();

let mut writer = crate::error::DiagnosticBuffer::new();
Expand Down
15 changes: 15 additions & 0 deletions naga/tests/naga/wgsl_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3887,3 +3887,18 @@ fn max_type_size_array_of_structs() {
))
}
}

#[cfg(feature = "wgsl-in")]
#[test]
fn source_with_control_char() {
check(
"\x07",
"error: expected global item (`struct`, `const`, `var`, `alias`, `fn`, `diagnostic`, `enable`, `requires`, `;`) or the end of the file, found \"\\u{7}\"
┌─ wgsl:1:1
1 │ �
│ ^ expected global item (`struct`, `const`, `var`, `alias`, `fn`, `diagnostic`, `enable`, `requires`, `;`) or the end of the file

",
);
}
Loading