From c93fcd25d028dedd3794efef4801abedd1e32c07 Mon Sep 17 00:00:00 2001
From: Brendan Kenny <bckenny@gmail.com>
Date: Fri, 23 Aug 2019 17:28:28 -0700
Subject: [PATCH] i18n: throw on excess placeholder replacement values (#9580)

---
 lighthouse-core/audits/third-party-summary.js |  1 -
 lighthouse-core/lib/i18n/i18n.js              | 11 ++++++++
 lighthouse-core/lib/lh-error.js               |  3 ++-
 lighthouse-core/test/lib/i18n/i18n-test.js    | 25 +++++++++++++++----
 lighthouse-core/test/results/sample_v2.json   |  1 -
 5 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/lighthouse-core/audits/third-party-summary.js b/lighthouse-core/audits/third-party-summary.js
index 9aac6184e12c..53e7a4f3c50f 100644
--- a/lighthouse-core/audits/third-party-summary.js
+++ b/lighthouse-core/audits/third-party-summary.js
@@ -165,7 +165,6 @@ class ThirdPartySummary extends Audit {
     return {
       score: Number(summary.wastedMs <= PASS_THRESHOLD_IN_MS),
       displayValue: str_(UIStrings.displayValue, {
-        itemCount: results.length,
         timeInMs: summary.wastedMs,
       }),
       details: Audit.makeTableDetails(headings, results, summary),
diff --git a/lighthouse-core/lib/i18n/i18n.js b/lighthouse-core/lib/i18n/i18n.js
index 2d176e7bbbab..12ce647acbe8 100644
--- a/lighthouse-core/lib/i18n/i18n.js
+++ b/lighthouse-core/lib/i18n/i18n.js
@@ -227,6 +227,17 @@ function _processParsedElements(icuMessage, argumentElements, values = {}) {
     }
   }
 
+  // Throw an error if a value is provided but has no placeholder in the message.
+  for (const valueId of Object.keys(values)) {
+    // errorCode is a special case always allowed to help ease of LHError use.
+    if (valueId === 'errorCode') continue;
+
+    if (!argumentElements.find(el => el.id === valueId)) {
+      throw new Error(`Provided value "${valueId}" does not match any placeholder in ` +
+        `ICU message "${icuMessage}"`);
+    }
+  }
+
   return values;
 }
 
diff --git a/lighthouse-core/lib/lh-error.js b/lighthouse-core/lib/lh-error.js
index f1809ff15105..51153e4a9ab6 100644
--- a/lighthouse-core/lib/lh-error.js
+++ b/lighthouse-core/lib/lh-error.js
@@ -100,7 +100,8 @@ class LighthouseError extends Error {
     super(errorDefinition.code);
     this.name = 'LHError';
     this.code = errorDefinition.code;
-    // Insert the i18n reference with errorCode and all additional ICU replacement properties.
+    // Add additional properties to be ICU replacements in the error string.
+    // `code` is always added as `errorCode` so callers don't need to specify the code multiple times.
     this.friendlyMessage = str_(errorDefinition.message, {errorCode: this.code, ...properties});
     this.lhrRuntimeError = !!errorDefinition.lhrRuntimeError;
     if (properties) Object.assign(this, properties);
diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js
index 74b078ff1911..5d45b032362e 100644
--- a/lighthouse-core/test/lib/i18n/i18n-test.js
+++ b/lighthouse-core/test/lib/i18n/i18n-test.js
@@ -44,14 +44,19 @@ describe('i18n', () => {
 
   describe('#replaceIcuMessageInstanceIds', () => {
     it('replaces the references in the LHR', () => {
-      const templateID = 'lighthouse-core/test/lib/i18n/fake-file.js | daString';
-      const reference = templateID + ' # 0';
-      const lhr = {audits: {'fake-audit': {title: reference}}};
+      const fakeFile = path.join(__dirname, 'fake-file-number-2.js');
+      const UIStrings = {aString: 'different {x}!'};
+      const formatter = i18n.createMessageInstanceIdFn(fakeFile, UIStrings);
+
+      const title = formatter(UIStrings.aString, {x: 1});
+      const lhr = {audits: {'fake-audit': {title}}};
 
       const icuMessagePaths = i18n.replaceIcuMessageInstanceIds(lhr, 'en-US');
-      expect(lhr.audits['fake-audit'].title).toBe('use me!');
+      expect(lhr.audits['fake-audit'].title).toBe('different 1!');
+
+      const expectedPathId = 'lighthouse-core/test/lib/i18n/fake-file-number-2.js | aString';
       expect(icuMessagePaths).toEqual({
-        [templateID]: [{path: 'audits[fake-audit].title', values: {x: 1}}]});
+        [expectedPathId]: [{path: 'audits[fake-audit].title', values: {x: 1}}]});
     });
   });
 
@@ -191,5 +196,15 @@ describe('i18n', () => {
         // eslint-disable-next-line max-len
         .toThrow(`ICU Message "Hello {timeInMs, number, seconds} World" contains a numeric reference ("timeInMs") but provided value was not a number`);
     });
+
+    it('throws an error if a value is provided that has no placeholder in the message', () => {
+      const helloStr = str_(UIStrings.helloTimeInMsWorld, {
+        timeInMs: 55,
+        sirNotAppearingInThisString: 66,
+      });
+      expect(_ => i18n.getFormatted(helloStr, 'en-US'))
+        // eslint-disable-next-line max-len
+        .toThrow(`Provided value "sirNotAppearingInThisString" does not match any placeholder in ICU message "Hello {timeInMs, number, seconds} World"`);
+    });
   });
 });
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index 0b99e087ab0c..c5138e345409 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/lighthouse-core/test/results/sample_v2.json
@@ -5606,7 +5606,6 @@
       "lighthouse-core/audits/third-party-summary.js | displayValue": [
         {
           "values": {
-            "itemCount": 1,
             "timeInMs": 22.918000000000006
           },
           "path": "audits[third-party-summary].displayValue"