ICU4J: Fix parser bug where backup() was counting in code units rather than code points

Author: catamorphism

Diff for: icu4j/main/core/src/main/java/com/ibm/icu/message2/InputSource.java

@@ -62,10 +62,22 @@ int readCodePoint() {
         return c;
     }
 
-    // Backup a number of characters.
+    // Backup a number of code points.
     void backup(int amount) {
+        int amountBackedUp = 0;
         // TODO: validate
-        cursor -= amount;
+        while (amountBackedUp < amount) {
+            backupOneCodePoint();
+            amountBackedUp++;
+        }
+    }
+
+    void backupOneCodePoint() {
+        if (Character.isLowSurrogate(buffer.charAt(cursor - 1))) {
+            cursor -= 2;
+        } else {
+            cursor--;
+        }
     }
 
     int getPosition() {

Diff for: icu4j/main/core/src/main/java/com/ibm/icu/message2/MFParser.java

@@ -53,12 +53,12 @@ private MFDataModel.Message parseImpl() throws MFParseException {
             cp = input.readCodePoint();
             cp = input.peekChar();
             if (cp == '{') { // `{{`, complex body without declarations
-                input.backup(1); // let complexBody deal with the wrapping {{ and }}
+                input.backupOneCodePoint(); // let complexBody deal with the wrapping {{ and }}
                 MFDataModel.Pattern pattern = getQuotedPattern();
                 skipOptionalWhitespaces();
                 result = new MFDataModel.PatternMessage(new ArrayList<>(), pattern);
             } else { // placeholder
-                input.backup(1); // We want the '{' present, to detect the part as placeholder.
+                input.backupOneCodePoint(); // We want the '{' present, to detect the part as placeholder.
                 MFDataModel.Pattern pattern = getPattern();
                 result = new MFDataModel.PatternMessage(new ArrayList<>(), pattern);
             }
@@ -129,7 +129,7 @@ private String getText() {
                     if (StringUtils.isContentChar(cp) || StringUtils.isWhitespace(cp)) {
                         result.appendCodePoint(cp);
                     } else {
-                        input.backup(1);
+                        input.backupOneCodePoint();
                         return result.toString();
                     }
             }
@@ -361,37 +361,48 @@ private MFDataModel.Attribute getAttribute() throws MFParseException {
         return null;
     }
 
-    // abnf: reserved-body = *([s] 1*(reserved-char / reserved-escape / quoted))
-    // abnf: reserved-escape = backslash ( backslash / "{" / "|" / "}" )
+    // abnf: reserved-body = reserved-body-part *([s] reserved-body-part)
     private String getReservedBody() throws MFParseException {
         int spaceCount = skipWhitespaces();
         StringBuilder result = new StringBuilder();
+        input.dump();
+        String firstPart = getReservedBodyPart();
+        result.append(firstPart);
         while (true) {
-            int cp = input.readCodePoint();
-            if (StringUtils.isReservedChar(cp)) {
-                result.appendCodePoint(cp);
-            } else if (cp == '\\') {
-                cp = input.readCodePoint();
-                checkCondition(
-                        cp == '{' || cp == '|' || cp == '}',
-                        "Invalid escape sequence. Only \\{, \\| and \\} are valid here.");
-                result.append(cp);
-            } else if (cp == '|') {
-                input.backup(1);
-                MFDataModel.Literal quoted = getQuotedLiteral();
-                result.append(quoted.value);
-            } else if (cp == EOF) {
-                return result.toString();
-            } else {
-                if (result.length() == 0) {
-                    input.backup(spaceCount + 1);
-                    return "";
-                } else {
-                    input.backup(1);
-                    return result.toString();
-                }
+            spaceCount = skipWhitespaces();
+            String nextPart = getReservedBodyPart();
+            if (nextPart.length() == 0) {
+                input.backup(spaceCount);
+                break;
             }
+            result.append(nextPart);
+        }
+        if (result.length() == 0) {
+            input.backup(spaceCount);
+        }
+        return result.toString();
+    }
+
+    // abnf: reserved-body-part = reserved-char / escaped-char / quoted-literal
+    private String getReservedBodyPart() throws MFParseException {
+        StringBuilder result = new StringBuilder();
+        int cp = input.readCodePoint();
+        if (StringUtils.isReservedChar(cp)) {
+            result.appendCodePoint(cp);
+        } else if (cp == '\\') {
+            cp = input.readCodePoint();
+            checkCondition(
+                           cp == '{' || cp == '|' || cp == '}',
+                           "Invalid escape sequence. Only \\{, \\| and \\} are valid here.");
+            result.append(cp);
+        } else if (cp == '|') {
+            input.backup(1);
+            MFDataModel.Literal quoted = getQuotedLiteral();
+            result.append(quoted.value);
+        } else {
+            input.backup(1);
         }
+        return result.toString();
     }
 
     // abnf: identifier = [namespace ":"] name
@@ -408,7 +419,7 @@ private String getIdentifier() throws MFParseException {
             checkCondition(name != null, "Expected name after namespace '" + namespace + "'");
             return namespace + ":" + name;
         } else {
-            input.backup(1);
+            input.backupOneCodePoint();
         }
         return namespace;
     }
@@ -479,7 +490,7 @@ private MFDataModel.Literal getLiteral() throws MFParseException {
                 MFDataModel.Literal ql = getQuotedLiteral();
                 return ql;
             default: // unquoted
-                input.backup(1);
+                input.backupOneCodePoint();
                 MFDataModel.Literal unql = getUnQuotedLiteral();
                 return unql;
         }
@@ -561,7 +572,7 @@ private int skipWhitespaces() {
                 return skipCount;
             }
             if (!StringUtils.isWhitespace(cp)) {
-                input.backup(1);
+                input.backupOneCodePoint();
                 return skipCount;
             }
             skipCount++;

Diff for: icu4j/main/core/src/test/java/com/ibm/icu/dev/test/message2/CoreTest.java

@@ -32,6 +32,7 @@ public class CoreTest extends CoreTestFmwk {
                                                 "more-functions.json",
                                                 "more-functions-multiline.json",
                                                 "reserved-syntax.json",
+                                                "reserved-syntax-2.json",
                                                 "resolution-errors.json",
                                                 "runtime-errors.json",
                                                 "spec/data-model-errors.json",

Diff for: testdata/message2/reserved-syntax.json

@@ -50,7 +50,8 @@
     { "src" : ".input{ $n ~ }{{{$n}}}", "ignoreJava": "ICU4J doesn't error out on reserved annotations" },
     { "src" : ".i {1} {{}}", "ignoreJava": "ICU4J doesn't error out on reserved annotations", "expErrors": [{ "type": "unsupported-statement" }] },
     { "src" : ".l $y = {|bar|} {{}}", "ignoreJava": "ICU4J doesn't error out on reserved annotations", "expErrors": [{ "type": "unsupported-statement" }] },
-    { "src" : ".򱋿󠆿 . |@| {$󛒁񓝖 & \\{ . @𯼼򋼡= $򵀀񓞈}{{\\\\}}", "ignoreJava": "ICU4J doesn't error out on reserved annotations", "expErrors": [{ "type": "unsupported-statement" }] }
+    { "src" : ".򱋿󠆿 . |@| {$󛒁񓝖 & \\{ . @𯼼򋼡= $򵀀񓞈}{{\\\\}}", "ignoreJava": "ICU4J doesn't error out on reserved annotations", "expErrors": [{ "type": "unsupported-statement" }] },
+    { "src" : "\\\\{   ?   󗟋   𫓜|@}   |}", "ignoreJava": "ICU4J doesn't error out on reserved annotations" }
   ]
 }