Repository: casey/just
Pull request #2360 (2024-09-10 23:59:24)

Allow escaping arbitrary characters
Closes https://github.com/casey/just/issues/2359

Not sure the error reporting is ideal, and not sure if the tests are covering enough cases?

casey: What do you think about introducing a `State` enum, which can be declared inside the parse string function, like so:

```
enum State {
  Initial, // initial state
  Backslash, // after `\`
  Unicode, // after `\u`, but not opening `{`
  UnicodeValue { value: String }, // after `\u{`
}
```

Then you would do:

```
let mut state = State::Initial;
while let Some(c) = chars.next() {
  match state {
  }
}

if state != State::Initial {
  // return error
}
```

I think this might make the code a little simpler.

Also, I think we should see if the error messages from `u32::from_str_radix` are good enough to use without needing to define our own error type. If they're reasonable, then we could accept any characters between `\u{` and `}`, and let `u32::from_str_radix` complain if they're not hex, or if they overflow a u32.
casey: Looking at the error message from `std::num::ParseIntError`, I think it's actually not great, so I'm coming around to doing more checking ourselves and not delegating the error message.

I think we should have these errors:

- `UnicodeEscapeEmpty` for `\u{}`
- `UnicodeEscapeCharacter { character: char }` for `\u{X}` where `X` is anything other than `0-9a-fA-F`
- `UnicodeEscapeLength` for when there are more than six characters inside an escape sequence
- `UnicodeEscapeRange` when the u32 is not in range for a char, the error message should mention that `10FFFF` is the maximum valid codepoint
- `UnicodeEscapeUnterminated` when we don't encounter `}` before the end of the string

This should allow us to just `.unwrap` when calling `from_str_radix`, since it will be guaranteed to be a valid hex string that's in range for a u32.

Sorry for waffling!
casey: Compilation error message context is based on tokens, so since a string is one token, by default it points at the whole string. This could definitely be improved. The easiest solution would be to construct a new, fake token which only points at the sub-section of the string containing the invalid escape sequence, but that's probably best left to a follow-up PR.

Patch:
diff --git a/src/compile_error.rs b/src/compile_error.rs
index b2396219..94df9e24 100644
--- a/src/compile_error.rs
+++ b/src/compile_error.rs
@@ -259,6 +259,25 @@ impl Display for CompileError<'_> {
         ref expected,
         found,
       } => write!(f, "Expected {}, but found {found}", List::or(expected)),
+      UnicodeEscapeCharacter { character } => {
+        write!(f, "expected hex digit [0-9A-Fa-f] but found `{character}`")
+      }
+      UnicodeEscapeDelimiter { character } => write!(
+        f,
+        "expected unicode escape sequence delimiter `{{` but found `{character}`"
+      ),
+      UnicodeEscapeEmpty => write!(f, "unicode escape sequences must not be empty"),
+      UnicodeEscapeLength { hex } => write!(
+        f,
+        "unicode escape sequence starting with `\\u{{{hex}` longer than six hex digits"
+      ),
+      UnicodeEscapeRange { hex } => {
+        write!(
+          f,
+          "unicode escape sequence value `{hex}` greater than maximum valid code point `10FFFF`",
+        )
+      }
+      UnicodeEscapeUnterminated => write!(f, "unterminated unicode escape sequence"),
       UnknownAliasTarget { alias, target } => {
         write!(f, "Alias `{alias}` has an unknown target `{target}`")
       }
diff --git a/src/compile_error_kind.rs b/src/compile_error_kind.rs
index 73c3960e..c99cabac 100644
--- a/src/compile_error_kind.rs
+++ b/src/compile_error_kind.rs
@@ -120,6 +120,20 @@ pub(crate) enum CompileErrorKind<'src> {
     expected: Vec<TokenKind>,
     found: TokenKind,
   },
+  UnicodeEscapeCharacter {
+    character: char,
+  },
+  UnicodeEscapeDelimiter {
+    character: char,
+  },
+  UnicodeEscapeEmpty,
+  UnicodeEscapeLength {
+    hex: String,
+  },
+  UnicodeEscapeRange {
+    hex: String,
+  },
+  UnicodeEscapeUnterminated,
   UnknownAliasTarget {
     alias: &'src str,
     target: &'src str,
diff --git a/src/parser.rs b/src/parser.rs
index b896cd0b..da2ff76f 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -676,31 +676,7 @@ impl<'run, 'src> Parser<'run, 'src> {
     };
 
     let cooked = if kind.processes_escape_sequences() {
-      let mut cooked = String::new();
-      let mut escape = false;
-      for c in unindented.chars() {
-        if escape {
-          match c {
-            'n' => cooked.push('\n'),
-            'r' => cooked.push('\r'),
-            't' => cooked.push('\t'),
-            '\\' => cooked.push('\\'),
-            '\n' => {}
-            '"' => cooked.push('"'),
-            other => {
-              return Err(
-                token.error(CompileErrorKind::InvalidEscapeSequence { character: other }),
-              );
-            }
-          }
-          escape = false;
-        } else if c == '\\' {
-          escape = true;
-        } else {
-          cooked.push(c);
-        }
-      }
-      cooked
+      Self::cook_string(token, &unindented)?
     } else {
       unindented
     };
@@ -724,6 +700,88 @@ impl<'run, 'src> Parser<'run, 'src> {
     ))
   }
 
+  // Transform escape sequences in from string literal `token` with content `text`
+  fn cook_string(token: Token<'src>, text: &str) -> CompileResult<'src, String> {
+    #[derive(PartialEq, Eq)]
+    enum State {
+      Initial,
+      Backslash,
+      Unicode,
+      UnicodeValue { hex: String },
+    }
+
+    let mut cooked = String::new();
+
+    let mut state = State::Initial;
+
+    for c in text.chars() {
+      match state {
+        State::Initial => {
+          if c == '\\' {
+            state = State::Backslash;
+          } else {
+            cooked.push(c);
+          }
+        }
+        State::Backslash if c == 'u' => {
+          state = State::Unicode;
+        }
+        State::Backslash => {
+          match c {
+            'n' => cooked.push('\n'),
+            'r' => cooked.push('\r'),
+            't' => cooked.push('\t'),
+            '\\' => cooked.push('\\'),
+            '\n' => {}
+            '"' => cooked.push('"'),
+            character => {
+              return Err(token.error(CompileErrorKind::InvalidEscapeSequence { character }))
+            }
+          }
+          state = State::Initial;
+        }
+        State::Unicode => match c {
+          '{' => {
+            state = State::UnicodeValue { hex: String::new() };
+          }
+          character => {
+            return Err(token.error(CompileErrorKind::UnicodeEscapeDelimiter { character }));
+          }
+        },
+        State::UnicodeValue { ref mut hex } => match c {
+          '}' => {
+            if hex.is_empty() {
+              return Err(token.error(CompileErrorKind::UnicodeEscapeEmpty));
+            }
+
+            let codepoint = u32::from_str_radix(hex, 16).unwrap();
+
+            cooked.push(char::from_u32(codepoint).ok_or_else(|| {
+              token.error(CompileErrorKind::UnicodeEscapeRange { hex: hex.clone() })
+            })?);
+
+            state = State::Initial;
+          }
+          '0'..='9' | 'A'..='F' | 'a'..='f' => {
+            hex.push(c);
+            if hex.len() > 6 {
+              return Err(token.error(CompileErrorKind::UnicodeEscapeLength { hex: hex.clone() }));
+            }
+          }
+          _ => {
+            return Err(token.error(CompileErrorKind::UnicodeEscapeCharacter { character: c }));
+          }
+        },
+      }
+    }
+
+    if state != State::Initial {
+      return Err(token.error(CompileErrorKind::UnicodeEscapeUnterminated));
+    }
+
+    Ok(cooked)
+  }
+
   /// Parse a string literal, e.g. `"FOO"`
   fn parse_string_literal(&mut self) -> CompileResult<'src, StringLiteral<'src>> {
     let (_token, string_literal) = self.parse_string_literal_token()?;


Test patch:
diff --git a/tests/string.rs b/tests/string.rs
index 1803877a..12bcbac0 100644
--- a/tests/string.rs
+++ b/tests/string.rs
@@ -391,3 +391,156 @@ test! {
   ",
   status:   EXIT_FAILURE,
 }
+
+#[test]
+fn valid_unicode_escape() {
+  Test::new()
+    .justfile(r#"x := "\u{1f916}\u{1F916}""#)
+    .args(["--evaluate", "x"])
+    .stdout("🤖🤖")
+    .run();
+}
+
+#[test]
+fn unicode_escapes_with_all_hex_digits() {
+  Test::new()
+    .justfile(r#"x := "\u{012345}\u{6789a}\u{bcdef}\u{ABCDE}\u{F}""#)
+    .args(["--evaluate", "x"])
+    .stdout("\u{012345}\u{6789a}\u{bcdef}\u{ABCDE}\u{F}")
+    .run();
+}
+
+#[test]
+fn maximum_valid_unicode_escape() {
+  Test::new()
+    .justfile(r#"x := "\u{10FFFF}""#)
+    .args(["--evaluate", "x"])
+    .stdout("\u{10FFFF}")
+    .run();
+}
+
+#[test]
+fn unicode_escape_no_braces() {
+  Test::new()
+    .justfile("x := \"\\u1234\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: expected unicode escape sequence delimiter `{` but found `1`
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u1234"
+  │      ^^^^^^^^
+"#,
+    )
+    .run();
+}
+
+#[test]
+fn unicode_escape_empty() {
+  Test::new()
+    .justfile("x := \"\\u{}\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: unicode escape sequences must not be empty
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u{}"
+  │      ^^^^^^
+"#,
+    )
+    .run();
+}
+
+#[test]
+fn unicode_escape_requires_immediate_opening_brace() {
+  Test::new()
+    .justfile("x := \"\\u {1f916}\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: expected unicode escape sequence delimiter `{` but found ` `
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u {1f916}"
+  │      ^^^^^^^^^^^^
+"#,
+    )
+    .run();
+}
+
+#[test]
+fn unicode_escape_non_hex() {
+  Test::new()
+    .justfile("x := \"\\u{foo}\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: expected hex digit [0-9A-Fa-f] but found `o`
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u{foo}"
+  │      ^^^^^^^^^
+"#,
+    )
+    .run();
+}
+
+#[test]
+fn unicode_escape_invalid_character() {
+  Test::new()
+    .justfile("x := \"\\u{BadBad}\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: unicode escape sequence value `BadBad` greater than maximum valid code point `10FFFF`
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u{BadBad}"
+  │      ^^^^^^^^^^^^
+"#,
+    )
+    .run();
+}
+
+#[test]
+fn unicode_escape_too_long() {
+  Test::new()
+    .justfile("x := \"\\u{FFFFFFFFFF}\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: unicode escape sequence starting with `\u{FFFFFFF` longer than six hex digits
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u{FFFFFFFFFF}"
+  │      ^^^^^^^^^^^^^^^^
+"#,
+    )
+    .run();
+}
+
+#[test]
+fn unicode_escape_unterminated() {
+  Test::new()
+    .justfile("x := \"\\u{1f917\"")
+    .args(["--evaluate", "x"])
+    .status(1)
+    .stderr(
+      r#"
+error: unterminated unicode escape sequence
+ ——▶ justfile:1:6
+  │
+1 │ x := "\u{1f917"
+  │      ^^^^^^^^^^
+"#,
+    )
+    .run();
+}