Add support for ICU templates with placeholders in JSCompiler.

This CL fixes a JSCompiler bug where `declareIcuTemplate` messages that have multiple parts are not translated to other languages.

ICU messages created using `declareIcuTemplate` can get stored into the XMB
file as multiple parts if necessary to record example or original code text,
but those parts are lost and only the original text template string is recorded
when we replace the original declarations with protected versions early in
compilation. This means we don't have the full information needed to
re-calculate the message ID when doing the final message replacement.

As a result we had a bug where the wrong ID was calculated and the translation
could not be found. This change solves this problem by explicitly recording the
correctly calculated message ID in the protected message.

PiperOrigin-RevId: 700089624

Author: rahul-kamat

src/com/google/javascript/jscomp/ReplaceMessages.java

@@ -29,6 +29,8 @@
 import com.google.javascript.jscomp.JsMessage.Part;
 import com.google.javascript.jscomp.JsMessage.PlaceholderFormatException;
 import com.google.javascript.jscomp.JsMessage.StringPart;
+import com.google.javascript.jscomp.JsMessageVisitor.ExtractedIcuTemplateParts;
+import com.google.javascript.jscomp.JsMessageVisitor.IcuMessageTemplateString;
 import com.google.javascript.jscomp.JsMessageVisitor.MalformedException;
 import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback;
 import com.google.javascript.rhino.Node;
@@ -276,6 +278,20 @@ public boolean unescapeHtmlEntities() {
   private Node createMsgPropertiesNode(JsMessage message, MsgOptions msgOptions) {
     QuotedKeyObjectLitBuilder msgPropsBuilder = new QuotedKeyObjectLitBuilder();
     msgPropsBuilder.addString("key", message.getKey());
+    if (msgOptions.isIcuTemplate() && !message.canonicalPlaceholderNames().isEmpty()) {
+      // ICU messages created using `declareIcuTemplate` can get stored into the XMB file as
+      // multiple parts if necessary to record example or original code text.
+      // `icu_placeholder_names` stores these parts of the ICU message, which allows us to
+      // correctly calculate the message ID in the protected message.
+      final Node namesArrayLit = astFactory.createArraylit();
+      for (String name : message.canonicalPlaceholderNames()) {
+        namesArrayLit.addChildToBack(astFactory.createString(name));
+      }
+      // Example:
+      // declareIcuTemplate('blah blah {PH1} blah {PH2}', ... );
+      // icu_placeholder_names: ['PH1', 'PH2']
+      msgPropsBuilder.addNode("icu_placeholder_names", namesArrayLit);
+    }
     String altId = message.getAlternateId();
     if (altId != null) {
       msgPropsBuilder.addString("alt_id", altId);
@@ -882,12 +898,41 @@ private ProtectedJsMessage(
       checkState(propertiesNode.isObjectLit(), propertiesNode);
       String msgKey = null;
       String meaning = null;
+      Set<String> icuPlaceholderNames = new LinkedHashSet<>();
+      String messageText = null;
+      Node messageTextNode = null;
       for (Node strKey = propertiesNode.getFirstChild();
           strKey != null;
           strKey = strKey.getNext()) {
         checkState(strKey.isStringKey(), strKey);
         String key = strKey.getString();
         Node valueNode = strKey.getOnlyChild();
+        if (key.equals("icu_placeholder_names")) {
+          checkState(valueNode.isArrayLit(), "icu_placeholder_names must be an array");
+          // If the message is an ICU template and `icu_placeholder_names` is present, then there
+          // are placeholders in the message. These placeholders will be replaced at runtime, but
+          // it is important that we keep track of these placeholders because it means that the
+          // ICU template CANNOT be treated as a single string part, because having placeholders
+          // means that the message has multiple parts.
+          // When a message is a `declareIcuTemplate` with multiple parts, we generate a `msg id`
+          // in the XMB, which is sent to the Translation Console so that the translators can
+          // translate this. We generate the deterministic `msg id` using an algorithm that takes
+          // into account how many parts the message has.
+          // Now during JSCompiler compilation process, we protect the message by wrapping it in a
+          // `__jscomp_define_msg__` (for safety because we don't want any of our optimization
+          // passes to change the message).
+          // Later in this method, we generate an ID (using `idGenerator.generateId()`) and use
+          // this to lookup a message in the translated XTB file. As I mentioned earlier, the
+          // algorithm for generating an ID needs to know the correct parts of the message, so we
+          // fail to generate the same ID we did when we added the message to the XMB.
+          // This `icu_placeholder_names` field is necessary to help us figure out the correct
+          // parts of the message, in order to generate the correct message ID that matches the ID
+          // we generated when we added the message to the XMB (which is the same ID in the XTB).
+          for (Node valueNodeChild : valueNode.children()) {
+            icuPlaceholderNames.add(valueNodeChild.getString());
+          }
+          continue;
+        }
         checkState(valueNode.isStringLit(), valueNode);
         String value = valueNode.getString();
         switch (key) {
@@ -903,17 +948,13 @@ private ProtectedJsMessage(
             jsMessageBuilder.setAlternateId(value);
             break;
           case "msg_text":
-            try {
-              // NOTE: If the text is for an ICU template, then it will not contain any
-              // placeholders ("{$placeholderName}"), so it will be treated as a single string
-              // part.
-              jsMessageBuilder.appendParts(JsMessageVisitor.parseJsMessageTextIntoParts(value));
-            } catch (PlaceholderFormatException unused) {
-              // Somehow we stored the protected message text incorrectly, which should never
-              // happen.
-              throw new IllegalStateException(
-                  valueNode.getLocation() + ": Placeholder incorrectly formatted: >" + value + "<");
-            }
+            // This may be an ICU template that also has the `icu_placeholder_names` property, which
+            // means we need to append multiple parts of the message to `jsMessageBuilder`. For now,
+            // we will save the message text and current node, and we'll parse it once we know if
+            // this is an ICU template with multiple parts (after this loop to run through all the
+            // properties is finished).
+            messageText = value;
+            messageTextNode = valueNode;
             break;
           case "isIcuTemplate":
             isIcuTemplate = true;
@@ -930,6 +971,39 @@ private ProtectedJsMessage(
             throw new IllegalStateException("unknown protected message key: " + strKey);
         }
       }
+
+      try {
+        if (!icuPlaceholderNames.isEmpty()) {
+          checkState(
+              isIcuTemplate,
+              "Found icu_placeholder_names for a message that is not an ICU template.");
+          // This is an ICU template with placeholders ("{$placeholderName}"). We cannot treat this
+          // as a single string part, because it has multiple parts. Otherwise, we will generate the
+          // wrong message id and we will not be able to find the correct translated message in the
+          // XTB file (because when the XMB message was created during message extraction, we
+          // treated this ICU template as having multiple parts).
+          final IcuMessageTemplateString icuMessageTemplateString =
+              new IcuMessageTemplateString(messageText);
+          final ExtractedIcuTemplateParts extractedIcuTemplateParts =
+              icuMessageTemplateString.extractParts(icuPlaceholderNames);
+
+          // Append the parts of the ICU template to the jsMessageBuilder.
+          jsMessageBuilder.appendParts(extractedIcuTemplateParts.extractedParts);
+        } else {
+          // This message is a single string part. It may be an ICU template without placeholders,
+          // or it may be a normal `goog.getMsg()` message.
+          jsMessageBuilder.appendParts(JsMessageVisitor.parseJsMessageTextIntoParts(messageText));
+        }
+      } catch (PlaceholderFormatException unused) {
+        // Somehow we stored the protected message text incorrectly, which should never
+        // happen 🙏
+        throw new IllegalStateException(
+            messageTextNode.getLocation()
+                + ": Placeholder incorrectly formatted: >"
+                + messageText
+                + "<");
+      }
+
       final String externalMessageId = JsMessageVisitor.getExternalMessageId(msgKey);
       if (externalMessageId != null) {
         // MSG_EXTERNAL_12345 = ...

test/com/google/javascript/jscomp/ReplaceMessagesTest.java

@@ -31,7 +31,6 @@
 import java.util.List;
 import java.util.Map;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -328,8 +327,10 @@ public void testReplaceExternalMessage() {
   }
 
   @Test
-  @Ignore // TODO(b/378574591): fix the bug this test exposes
   public void testReplaceIcuTemplateMessageWithBundleAndJsPlaceholders() {
+    // This unit test contains an ICU template with placeholders ("{EMAIL}"). We cannot treat this
+    // message as a single string part, because it has multiple parts. Otherwise, we will generate
+    // the wrong message id and this unit test will fail.
     useTestIdGenerator = true;
     strictReplacement = true;
 
@@ -370,16 +371,11 @@ public void testReplaceIcuTemplateMessageWithBundleAndJsPlaceholders() {
         lines(
             "const {declareIcuTemplate} = goog.require('goog.i18n.messages');",
             "",
-            // TODO(b/378574591): The information gathered here isn't enough to correctly generate
-            // the message ID for lookup. Since the "example" information is dropped, we won't
-            // know that we need to split the message text into parts, treating "EMAIL" as a
-            // placeholder. As a result, we calculate the message ID using just a single string
-            // part, so we get a different message ID than we got during extraction, and we
-            // won't be able to find the message.
             "const MSG_SHOW_EMAIL =",
             "    __jscomp_define_msg__(",
             "        {",
             "          \"key\":    \"MSG_SHOW_EMAIL\",",
+            "          \"icu_placeholder_names\": [\"EMAIL\"],",
             "          \"msg_text\": \"Email: {EMAIL}\",",
             "          \"isIcuTemplate\": \"\"",
             "        });"),
@@ -430,6 +426,50 @@ public void testReplaceIcuTemplateMessageWithoutJsPlaceholders() {
             "const MSG_SHOW_EMAIL = 'Retpoŝtadreso: {EMAIL}';"));
   }
 
+  @Test
+  public void testReplaceIcuTemplateMessageWithJsPlaceholders() {
+    // Make sure ICU messages with multiple parts are handled correctly.
+    // We cannot treat this ICU message as a single string part, because it has two placeholders
+    // (EMAIL1 and EMAIL2) that cause the message to be split into multiple parts.
+    registerMessage(
+        getTestMessageBuilder("MSG_SHOW_EMAIL")
+            .appendStringPart("Retpoŝtadreso: ")
+            .appendCanonicalPlaceholderReference("EMAIL_1")
+            .appendStringPart(" aŭ ")
+            .appendCanonicalPlaceholderReference("EMAIL_2")
+            .build());
+
+    multiPhaseTest(
+        lines(
+            "const {declareIcuTemplate} = goog.require('goog.i18n.messages');",
+            "",
+            "const MSG_SHOW_EMAIL =",
+            "    declareIcuTemplate(",
+            "        'Email Options: {EMAIL_1} or {EMAIL_2}',",
+            "        {",
+            "          description: 'Labeled email address',",
+            "          example: {",
+            "            'EMAIL_1': '[email protected]',",
+            "            'EMAIL_2': '[email protected]'",
+            "           }",
+            "        });"),
+        lines(
+            "const {declareIcuTemplate} = goog.require('goog.i18n.messages');",
+            "",
+            "const MSG_SHOW_EMAIL =",
+            "    __jscomp_define_msg__(",
+            "        {",
+            "          \"key\":    \"MSG_SHOW_EMAIL\",",
+            "          \"icu_placeholder_names\": [\"EMAIL_1\", \"EMAIL_2\"],",
+            "          \"msg_text\": \"Email Options: {EMAIL_1} or {EMAIL_2}\",",
+            "          \"isIcuTemplate\": \"\"",
+            "        });"),
+        lines(
+            "const {declareIcuTemplate} = goog.require('goog.i18n.messages');",
+            "",
+            "const MSG_SHOW_EMAIL = 'Retpoŝtadreso: {EMAIL_1} aŭ {EMAIL_2}';"));
+  }
+
   @Test
   public void testMissingIcuTemplateMessage() {
     // We don't registerMessage() here, so there are no messages in the bundle used by this test.
@@ -446,8 +486,12 @@ public void testMissingIcuTemplateMessage() {
             // The example text is dropped, since it is only used for XMB extraction.
             // However, it does cause the JsMessage read from the JS code to have a placeholder
             // in it.
-            // One purpose of this test is to make sure the message template is properly put back
-            // together.
+            // We add this placeholder in the "icu_placeholder_names" field to keep track of how the
+            // message has multiple parts, which is necessary for the message ID to be generated
+            // correctly.
+            // The purpose of this test is to:
+            // 1. make sure the message template is properly put back together.
+            // 2. make sure the "icu_placeholder_names" field is populated with `EMAIL`
             "          example: {",
             "            'EMAIL': '[email protected]'",
             "           }",
@@ -459,6 +503,7 @@ public void testMissingIcuTemplateMessage() {
             "    __jscomp_define_msg__(",
             "        {",
             "          \"key\":    \"MSG_SHOW_EMAIL\",",
+            "          \"icu_placeholder_names\": [\"EMAIL\"],",
             "          \"msg_text\": \"Email: {EMAIL}\",",
             "          \"isIcuTemplate\": \"\"",
             "        });"),