Skip to content

Weirdly lenient parsing of unquoted strings #14

Description

@robofinch

I noticed that quartz_nbt accepts a lot more values in unquoted strings than the Minecraft spec; there's a comment in the part of the lexer handling unquoted strings that notes "we allow SNBT within SNBT strings", though I couldn't figure out where or why that could be important for Quartz. Quote-escaped SNBT can be put in quoted SNBT strings, so allowing SNBT in unquoted strings and not returning an error seems odd. (And converting an NBT string tag which happens to contain valid SNBT would add escapes for the quotes it needs to, as normal; putting SNBT into quoted SNBT strings is already something quartz_nbt can do.)

As an example, running

use quartz_nbt::snbt;

fn main() {
    println!("{}", snbt::parse(
        "{weird_          unquoted1234@[{\"\"}] {still:in the key}     unquoted string:1.40}"
    ).unwrap());
    println!("{}", snbt::parse(
        "{more;normal,example:of,an:issue}"
    ).unwrap());
}

outputs:

{'weird_          unquoted1234@[{""}] {still:in the key}     unquoted string':1.4D}
{"more;normal,example":of,an:issue}

The first example is parsed as a compound tag with a very long key for the double tag 1.4, and in the second example where more;normal has a semicolon instead of a possibly-intended colon, more;normal,example is parsed as the key for of.

Summary in advance:
I think this behavior should be removed if it's not necessary. The below is my attempt to think this through, just in case.

I think it'd be preferable to add an SnbtError variant (well, ParserErrorType variant) for an invalid character appearing in an unquoted string. The lexer could also be simplified by delimiting slurp_token by any character which isn't allowed in an unquoted string (namely anything but [0-9a-zA-Z] or _, -, ., +), and return an error if the first value of a string passed to slurp_token is invalid. If the delimiting character is something like { or :, then great, that'll be a valid token on the next call to next(). (And if the parser isn't expecting that token, it'll return an error.) Otherwise, slurp_token will return an error on its next call, so an invalid unquoted string is eventually caught; the fact it could take two calls to next() (instead of one) for, say, a non-ASCII character in an unquoted string to yield a lexer error is probably not important.

Maybe the goal is to require fewer quotes in SNBT, but any use cases for trying to successfully parse partially-invalid SNBT seem pretty limited. The SNBT is already a human-readable string that should be debuggable with the error messages provided by the SNBT parser. The exception is: if some tool out there is outputting invalid SNBT and quartz_nbt is trying to parse it, then of course quartz_nbt needs to handle such NBT (or the tool needs to be fixed). (As a trivial example, I don't usually see whitespace in one-line examples of SNBT, and it seems likely that Minecraft prefers SNBT to have no whitespace outside quoted strings. Whatever. Trimming input whitespace between tokens is the most reasonable option, otherwise long SNBT is unreadable. Sometimes people do actually use whitespace and newlines in SNBT. Therefore an SNBT parser should trim whitespace.)

Anyway, I could say more about potential solutions, but I've basically already coded one for the SNBT parser I'm making based off of quartz_nbt's parser, and could add something like it to quartz_nbt if this issue is considered a problem. The important thing is: I just can't figure out if you do intentionally want this, e.g. if anything is depending on that behavior from quartz_nbt. After all, NBT -> SNBT still yields valid data as far as I can tell, and this SNBT -> NBT parser still correctly accepts valid SNBT (and then accepts some invalid SNBT too). Ah, likewise for character escapes - formally, there's no support for escapes other than \', \", \\ in Minecraft's original version of SNBT. (There's also a new SNBT version in Java 1.21.5 that adds tons more behavior, including character escapes, but that's besides the point.)

TLDR:
Is this intentional or needed? Does Quartz or something else you've made use this behavior?

(I know I'm almost certainly overthinking this, but I just prefer to be thorough.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions