Fix: no-implicit-coercion false positive with String() (fixes #14623) (#14641)

mdjermanovic · web-flow · commit 1f048cb0eec6 · 2021-06-04T18:27:34.000+02:00
diff --git a/lib/rules/no-implicit-coercion.js b/lib/rules/no-implicit-coercion.js
@@ -109,6 +109,20 @@ function getNonNumericOperand(node) {
     return null;
 }
 
+/**
+ * Checks whether an expression evaluates to a string.
+ * @param {ASTNode} node node that represents the expression to check.
+ * @returns {boolean} Whether or not the expression evaluates to a string.
+ */
+function isStringType(node) {
+    return astUtils.isStringLiteral(node) ||
+        (
+            node.type === "CallExpression" &&
+            node.callee.type === "Identifier" &&
+            node.callee.name === "String"
+        );
+}
+
 /**
  * Checks whether a node is an empty string literal or not.
  * @param {ASTNode} node The node to check.
@@ -126,8 +140,8 @@ function isEmptyString(node) {
  */
 function isConcatWithEmptyString(node) {
     return node.operator === "+" && (
-        (isEmptyString(node.left) && !astUtils.isStringLiteral(node.right)) ||
-        (isEmptyString(node.right) && !astUtils.isStringLiteral(node.left))
+        (isEmptyString(node.left) && !isStringType(node.right)) ||
+        (isEmptyString(node.right) && !isStringType(node.left))
     );
 }
 
@@ -332,6 +346,11 @@ module.exports = {
                     return;
                 }
 
+                // if the expression is already a string, then this isn't a coercion
+                if (isStringType(node.expressions[0])) {
+                    return;
+                }
+
                 const code = sourceCode.getText(node.expressions[0]);
                 const recommendation = `String(${code})`;
 
diff --git a/tests/lib/rules/no-implicit-coercion.js b/tests/lib/rules/no-implicit-coercion.js
@@ -95,7 +95,16 @@ ruleTester.run("no-implicit-coercion", rule, {
         { code: "`${foo}`", parserOptions: { ecmaVersion: 6 } },
         { code: "`${foo}`", options: [{ }], parserOptions: { ecmaVersion: 6 } },
         { code: "`${foo}`", options: [{ disallowTemplateShorthand: false }], parserOptions: { ecmaVersion: 6 } },
-        "+42"
+        "+42",
+
+        // https://github.com/eslint/eslint/issues/14623
+        "'' + String(foo)",
+        "String(foo) + ''",
+        { code: "`` + String(foo)", parserOptions: { ecmaVersion: 6 } },
+        { code: "String(foo) + ``", parserOptions: { ecmaVersion: 6 } },
+        { code: "`${'foo'}`", options: [{ disallowTemplateShorthand: true }], parserOptions: { ecmaVersion: 6 } },
+        { code: "`${`foo`}`", options: [{ disallowTemplateShorthand: true }], parserOptions: { ecmaVersion: 6 } },
+        { code: "`${String(foo)}`", options: [{ disallowTemplateShorthand: true }], parserOptions: { ecmaVersion: 6 } }
     ],
     invalid: [
         {
