diff --git a/src/com/google/javascript/jscomp/ConformanceRules.java b/src/com/google/javascript/jscomp/ConformanceRules.java index a813597813b..ff7e826a2eb 100644 --- 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 index adf092c11ad..0d41c373888 100644 --- 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) { *
    *
  1. The argument is a constant variable assigned from a string literal, or *
  2. The argument is an expression that is a string literal, or + *
  3. The argument is a ternary expression choosing between string literals, or *
  4. The argument is a concatenation of the above. *
* * @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 index 017fe6829f0..dfc07ecadca 100644 --- 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 index d7e4e66e378..942d00afecd 100644 --- 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 index 934af4f57b3..79647e9aecb 100644 --- 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 index ac0441c3164..34d1be2a415 100644 --- 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");