[Variant] More efficient determination of String vs ShortString

[alamb]

this will make three times deciding between short vs. long string? one to make the input, another here, and a third to consume it? Any way we could make this more direct/efficient?

Originally posted by @scovich in #7653 (comment)

[alamb]

I think this is a pretty good first issue and is relatively small

[friendlymatthew]

take

[friendlymatthew]

Hi hi, I took a look at implementing a fix here to avoid superfluous checks for String vs. ShortString. If a user passes in a String directly, we do one check in the .into implementation to convert the String to a Variant. Then we do a match on the variant type, and we check the string length before appending the value.

The check inside .into seems unavoidable without changing the API of the function to be less generic.

The string length check right before appending also seems unavoidable because it's possible for users to construct Variant::ShortStrings directly, without doing input validation. This means you can have a short string variant which actually contains a string that is longer than 63 bytes.

I have a change here which splits the append_string function into append_string and append_short_string and moves the length check directly into append_value. Is this the sort of change you were thinking about for this issue, or is there some way we could avoid checking the string length more than once here?

[scovich]

The change looks great!

The string length check right before appending also seems unavoidable because it's possible for users to construct Variant::ShortString directly, without doing input validation.

Good catch. That is indeed rather annoying. I guess we could potentially mask off all but the lowest 6 bits of the string's length and write that. Then the behavior is always well-defined?

    fn append_short_string(&mut self, value: &str) {
        let value = &value[..value.len() & 0x3f];
        self.buffer.push(short_string_header(value.len()));
        self.buffer.extend_from_slice(value.as_bytes());
    }

... except that could truncate the bytes in the middle of a utf8 sequence, producing an invalid output. Ugh.

Alternatively, we could make Variant be a struct whose only (private) field is today's enum? That way, we could enforce always-valid Variant values via try_new. But it would make variants harder to work with, because user code could no longer match on the enum variants. Ugh.

[alamb]

The change looks great!

The string length check right before appending also seems unavoidable because it's possible for users to construct Variant::ShortString directly, without doing input validation.

Good catch. That is indeed rather annoying. I guess we could potentially mask off all but the lowest 6 bits of the string's length and write that. Then the behavior is always well-defined?

I think this would be surprising (to silently truncate the values) and would not recommend it

Alternatively, we could make Variant be a struct whose only (private) field is today's enum? That way, we could enforce always-valid Variant values via try_new. But it would make variants harder to work with, because user code could no longer match on the enum variants. Ugh.

Yeah I agree having match for Variant is really nice

Another idea would be to introduce a newtype similar to the VariantObject struct that wraps a &str and ensures lengths are valid:

enum Variant { 
...
  // short string doesn't contain a `&str` but rather a `ShortString
  ShortString(ShortString<'v>),
...
}

And then we would put validtaion on creating a ShortSstring

/// Wraps a string that is guaranteed to be a valid `Variant::ShortString`
struct ShortString<'a>(&'a str);

impl <'a> ShortString<'a> {
  fn try_new(value: &str) -> Result<Self> { 
    // check value.len and return an error if too long
    Self(value)
  }
}

[alamb]

I have a change here which splits the append_string function into append_string and append_short_string and moves the length check directly into append_value. Is this the sort of change you were thinking about for this issue, or is there some way we could avoid checking the string length more than once here?

I agree with @scovich this is the kind of change that was envisioned -- maybe you can get that up as a PR and we can merge it in and follow up with better validation for short string as another PR / issue