[Marcono1234]

Purpose

Deduplicate the JsonReader code for JSON string reading and skipping to make sure it behaves identical

Supersedes / follow-up for #3138

Description

Instead of having separate implementations for reading and skipping of JSON strings, which risks that the behavior of these methods mismatch (as described in #3138), the reading method has been refactored to take a new internal JsonStringConsumer interface, which has two implementations:

• JsonStringBuilder: consumes the value using a StringBuilder and builds a String
• JsonStringIgnorer: ignores the value

As part of that the Unicode surrogate char validation has been refactored:
It now runs during consumption of the JSON string instead of on the final constructed String. This is necessary to implement it efficiently when skipping a JSON string.
Additionally escaped + unescaped (and vice versa) surrogates are no longer allowed (see related #3116 (comment)). That makes the implementation a bit easier, and it is probably rather unlikely that such escaped + unescaped pairs occur in real usage (?).

[Marcono1234]

Any feedback, also regarding naming of the new methods and types is welcome!

[Marcono1234]

This roughly matches the previous behavior (at least for quoted strings):

gson/gson/src/main/java/com/google/gson/stream/JsonReader.java

Lines 1238 to 1239 in 845664b

	int estimatedLength = (len + 1) * 2;
	builder = new StringBuilder(Math.max(estimatedLength, 16));

gson/gson/src/main/java/com/google/gson/stream/JsonReader.java

Lines 1253 to 1254 in 845664b

	int estimatedLength = (p - start) * 2;
	builder = new StringBuilder(Math.max(estimatedLength, 16));

[Marcono1234]

Removed this because it looks like this was redundant? Unquoted strings are only supported in lenient mode so this check always succeeded.

[Marcono1234]

Changed this to extend Reader instead of FilterReader. Previously this only overrode FilterReader#read(char[], int, int) but any other Reader methods still accessed the underlying the reader directly without chunking.

Probably was not an issue before because JsonReader only calls read(char[], int, int) currently, but that is an implementation detail, making this test rather brittle.

[Marcono1234]

Note: This test probably duplicates some of the other tests, but it is more structured and its generic implementation allows using it for nextString, nextName and skipValue.

But for simplicity I decided to keep the existing duplicate tests nonetheless. Maybe they also cover corner cases not covered here.

[Marcono1234]

(closing and reopening to retrigger CI; was previously failing due to network issues)

[Marcono1234]

As side note: This splitting of the string literal ("\uD834" + "\\u1234") is needed due to JDK-8394548 where javac erroneously rejects the string literal otherwise with "illegal escape character".

[eamonnmcmanus]

This is great, thanks! I was a little concerned that #3138 would make skipping string values more expensive (by constructing the string anyway).

[eamonnmcmanus]

We have three if-statements here for every input character, which are only relevant for the (I think) unusual case where the input contains surrogate pairs. I think we could restructure a little bit so that we have only two ifs in the non-surrogate case:

// This convoluted logic reduces the number of ifs in the usual non-surrogate case.
if (Character.isSurrogate(c)) {
  if (Character.isLowSurrogate(c)) {
    if (expectingLowSurrogate) {
      expectingLowSurrogate = false;
    } else {
      throw syntaxError("Unpaired low surrogate char");
    }
  } else if (expectingLowSurrogate) {
    throw syntaxError("Expected low surrogate char");
  } else {
    expectingLowSurrogate = true;
  }
} else if (expectingLowSurrogate) {
  throw syntaxError("Expected low surrogate char");
}

But also, if we are disallowing surrogate pairs where one of the pair is a regular char and the other is \uNNNN, then perhaps we could dispense with expectingLowSurrogate and just grab the low surrogate as soon as we see the high one.

[Marcono1234]

The implementation of Character#isLowSurrogate and Character#isHighSurrogate just performs an int range check, so I guess it does not have a big impact on performance. Though I can understand that when possible it would be beneficial to avoid redundant checks.

Your proposed change reduces 3 ifs to 2 ifs but on the other hand duplicates the if (expectingLowSurrogate) { throw syntaxError("Expected low surrogate char"); }, making the code more verbose.
I am not that convinced by that proposed change, but if you insist on it I can change it nonetheless.

---

perhaps we could dispense with expectingLowSurrogate and just grab the low surrogate as soon as we see the high one

Ideally yes, but the code has to account for the low surrogate not being in the buffer. Something like this might work:

if (Character.isSurrogate(c)) {
  if (Character.isLowSurrogate(c)) {
    throw syntaxError("Unpaired low surrogate char");
  }

  // Handling high surrogate, try to read subsequent low surrogate

  if (p == l) {
    consumer.append(buffer, start, p - start);
    pos = p;
    if (!fillBuffer(1)) {
      throw syntaxError("Unterminated string");
    }
    p = pos;
    l = limit;
  }

  c = buffer[p++];
  if (Character.isLowSurrogate(c)) {
    // Valid surrogate pair was found, continue with next char
    continue;
  }
  throw syntaxError("Expected low surrogate char");
}

But this is quite verbose and error-prone due to having to update all the local variables, so I don't think it is much better than the current proposed code.

[eamonnmcmanus]

I believe the logic here introduces a new constraint that was not present before, namely that a surrogate pair must be spelled either as two chars or as two \uNNNN escapes, but not a mix of the two. I think that is a reasonable interpretation of RFC 8259, but it is probably worth spelling out in comments, and mentioning in the release notes when the time comes.

[Marcono1234]

Have extended the documentation for this internal readEscapeCharacter method and the public method setStrictness in 79c333d.

Is it ok like this?