Accept ternary values picking between constants in NodeUtil.isStringL…

…iteralValue and goog.string.Const.from ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=195322294
google · May 3, 2018 · 496f84b · 496f84b
1 parent 5795aec
commit 496f84b
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 16 deletions.
diff --git a/src/com/google/javascript/jscomp/ConformanceRules.java b/src/com/google/javascript/jscomp/ConformanceRules.java
@@ -33,8 +33,6 @@
 import com.google.javascript.jscomp.CheckConformance.InvalidRequirementSpec;
 import com.google.javascript.jscomp.CheckConformance.Rule;
 import com.google.javascript.jscomp.CodingConvention.AssertionFunctionSpec;
-import com.google.javascript.jscomp.ConformanceRules.AbstractRule;
-import com.google.javascript.jscomp.ConformanceRules.ConformanceResult;
 import com.google.javascript.jscomp.Requirement.Severity;
 import com.google.javascript.jscomp.Requirement.Type;
 import com.google.javascript.jscomp.parsing.JsDocInfoParser;
@@ -626,7 +624,7 @@ private boolean isCandidatePropUse(Node propAccess, Property prop) {
           }
           if (NodeUtil.isLhsOfAssign(propAccess)
               && (NodeUtil.isLiteralValue(propAccess.getNext(), false /* includeFunctions */)
-                  || NodeUtil.isStringLiteralValue(propAccess.getNext()))) {
+                  || NodeUtil.isSomeCompileTimeConstStringValue(propAccess.getNext()))) {
             return false;
           }
           return true;
@@ -1653,7 +1651,7 @@ protected ConformanceResult checkConformance(NodeTraversal t, Node n) {
         }
         Node prop = NodeUtil.getFirstPropMatchingKey(attrs, tagAttr[1]);
         if (prop != null) {
-          if (NodeUtil.isStringLiteralValue(prop)) {
+          if (NodeUtil.isSomeCompileTimeConstStringValue(prop)) {
             // Ignore string literal values.
             continue;
           }

diff --git a/src/com/google/javascript/jscomp/ConstParamCheck.java b/src/com/google/javascript/jscomp/ConstParamCheck.java
@@ -93,7 +93,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
 
       if (name.matchesQualifiedName(CONST_FUNCTION_NAME)
           || name.matchesQualifiedName(CONST_FUNCTION_NAME_COLLAPSED)) {
-        if (!isCompileTimeConstant(traversal.getScope(), argument)) {
+        if (!isSafeValue(traversal.getScope(), argument)) {
           compiler.report(traversal.makeError(argument, CONST_NOT_STRING_LITERAL_ERROR));
         }
       }
@@ -108,19 +108,20 @@ public void visit(NodeTraversal traversal, Node node, Node parent) {
    * <ol>
    *   <li>The argument is a constant variable assigned from a string literal, or
    *   <li>The argument is an expression that is a string literal, or
+   *   <li>The argument is a ternary expression choosing between string literals, or
    *   <li>The argument is a concatenation of the above.
    * </ol>
    *
    * @param scope The scope chain to use in name lookups.
    * @param argument The node of function argument to check.
    */
-  private boolean isCompileTimeConstant(Scope scope, Node argument) {
-    if (argument.isString() || (argument.isTemplateLit() && argument.hasOneChild())) {
+  private boolean isSafeValue(Scope scope, Node argument) {
+    if (NodeUtil.isSomeCompileTimeConstStringValue(argument)) {
       return true;
     } else if (argument.isAdd()) {
       Node left = argument.getFirstChild();
       Node right = argument.getLastChild();
-      return isCompileTimeConstant(scope, left) && isCompileTimeConstant(scope, right);
+      return isSafeValue(scope, left) && isSafeValue(scope, right);
     } else if (argument.isName()) {
       String name = argument.getString();
       Var var = scope.getVar(name);
@@ -131,7 +132,7 @@ private boolean isCompileTimeConstant(Scope scope, Node argument) {
       if (initialValue == null) {
         return false;
       }
-      return isCompileTimeConstant(var.getScope(), initialValue);
+      return isSafeValue(var.getScope(), initialValue);
     }
     return false;
   }

diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java
@@ -740,17 +740,23 @@ public static boolean isLiteralValue(Node n, boolean includeFunctions) {
   }
 
   /**
-   * Returns true iff the value associated with the node is a JS string literal,
-   * or a concatenation thereof.
+   * Returns true iff the value associated with the node is a JS string literal, a concatenation
+   * thereof or a ternary operator choosing between string literals.
    */
-  static boolean isStringLiteralValue(Node node) {
-    if (node.isString()) {
+  static boolean isSomeCompileTimeConstStringValue(Node node) {
+    // TODO(bangert): Support constants, using a Scope argument. See ConstParamCheck
+    if (node.isString() || (node.isTemplateLit() && node.hasOneChild())) {
       return true;
     } else if (node.isAdd()) {
       checkState(node.hasTwoChildren(), node);
       Node left = node.getFirstChild();
       Node right = node.getLastChild();
-      return isStringLiteralValue(left) && isStringLiteralValue(right);
+      return isSomeCompileTimeConstStringValue(left) && isSomeCompileTimeConstStringValue(right);
+    } else if (node.isHook()) {
+      // Ternary operator a ? b : c
+      Node left = node.getSecondChild();
+      Node right = node.getLastChild();
+      return isSomeCompileTimeConstStringValue(left) && isSomeCompileTimeConstStringValue(right);
     }
     return false;
   }

diff --git a/src/com/google/javascript/jscomp/TemplateAstMatcher.java b/src/com/google/javascript/jscomp/TemplateAstMatcher.java
@@ -284,6 +284,7 @@ private boolean isTemplateParameterNode(Node n) {
     return (n.getToken() == TEMPLATE_TYPE_PARAM);
   }
 
+  /** Matches parameters (in the refasterJS template) whose names start with 'string_literal_'. */
   private boolean isTemplateParameterStringLiteralNode(Node n) {
     return (n.getToken() == TEMPLATE_STRING_LITERAL);
   }
@@ -341,7 +342,8 @@ private boolean matchesNodeShape(Node template, Node ast) {
         return false;
       }
     } else if (isTemplateParameterStringLiteralNode(template)) {
-      return NodeUtil.isStringLiteralValue(ast);
+      // Matches parameters (in the refasterJS template) whose names start with 'string_literal_'.
+      return NodeUtil.isSomeCompileTimeConstStringValue(ast);
     } else if (template.isCall()) {
       // Loosely match CALL nodes. isEquivalentToShallow checks free calls against non-free calls,
       // but the template should ignore that distinction.
@@ -451,7 +453,7 @@ private boolean matchesNode(Node template, Node ast) {
         return ast.isEquivalentTo(previousMatch);
       }
 
-      if (NodeUtil.isStringLiteralValue(ast)) {
+      if (NodeUtil.isSomeCompileTimeConstStringValue(ast)) {
         paramNodeMatches.set(paramIndex, ast);
         return true;
       }

diff --git a/test/com/google/javascript/jscomp/ConstParamCheckTest.java b/test/com/google/javascript/jscomp/ConstParamCheckTest.java
@@ -123,6 +123,27 @@ public void testNotStringLiteralArgumentOnCollapsedProperties() {
     testError("goog$string$Const$from(null);", ConstParamCheck.CONST_NOT_STRING_LITERAL_ERROR);
   }
 
+  public void testStringLiteralTernaryArgument() {
+    testSame(CLOSURE_DEFS + "var a = false;" + "goog.string.Const.from(a ? 'foo' : 'bar');");
+  }
+
+  public void testStringLiteralComplexExpression() {
+    testSame(
+        CLOSURE_DEFS
+            + "const domain = 'example.org';"
+            + "goog.string.Const.from("
+            + "'http' + (Math.random() ? 's' : '') + ':' +  domain + '/ponies/');");
+  }
+
+  public void testStringLiteralTernaryArgumentNonConstant() {
+    testError(
+        CLOSURE_DEFS
+            + "var a = false;"
+            + "var f = function() { return 'foo'; };"
+            + "goog.string.Const.from(a ? f() : 'bar');",
+        ConstParamCheck.CONST_NOT_STRING_LITERAL_ERROR);
+  }
+
   // Tests for string literal constant arguments.
 
   public void testStringLiteralConstantArgument() {

diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java
@@ -218,6 +218,40 @@ private void assertNotLiteral(Node n) {
     assertFalse(NodeUtil.isImmutableValue(n));
   }
 
+  public void testIsStringLiteralStringLiteral() {
+    assertTrue(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("'b'"))); // String
+  }
+
+  public void testIsStringLiteralTemplateNoSubst() {
+    assertTrue(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("`b`"))); // Template
+  }
+
+  public void testIsStringLiteralTemplateWithSubstitution() {
+    // Template with substitution
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("`foo${bar}`")));
+  }
+
+  public void testIsStringLiteralVariable() {
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("b"))); // Variable
+  }
+
+  public void testIsStringLiteralConcatLiterals() {
+    assertTrue(
+        NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("'b' + 'c'"))); // String concatenation
+  }
+
+  public void testIsStringLiteralConcatLiteralVariable() {
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("b + 'c'")));
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("'b' + c")));
+  }
+
+  public void testIsStringLiteralTernary() {
+    assertTrue(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("a ? 'b' : 'c'")));
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("a ? b : 'c'")));
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("a ? 'b' : c")));
+    assertFalse(NodeUtil.isSomeCompileTimeConstStringValue(parseExpr("a ? b : c")));
+  }
+
   public void testGetBooleanValue() {
     assertPureBooleanTrue("true");
     assertPureBooleanTrue("10");