From 041fa19de2c0cd89d6960ea0500e4996e0e5c581 Mon Sep 17 00:00:00 2001 From: lharker Date: Thu, 20 Dec 2018 16:03:35 -0800 Subject: [PATCH] Make NodeUtil.isObjectLitKey return false for object pattern keys This migrates callers of NodeUtil.isObjectLitKey to NodeUtil.isMaybeObjectLitKey, which still returns true for object pattern keys, in case callers are relying on the current behavior. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=226409005 --- .../jscomp/AggressiveInlineAliases.java | 2 +- .../jscomp/AnalyzePrototypeProperties.java | 2 +- .../javascript/jscomp/CheckGlobalThis.java | 2 +- .../javascript/jscomp/CollapseProperties.java | 4 +- .../javascript/jscomp/DefinitionsRemover.java | 4 +- .../jscomp/ExpressionDecomposer.java | 2 +- .../jscomp/FindExportableNodes.java | 2 +- .../javascript/jscomp/GlobalNamespace.java | 4 +- .../google/javascript/jscomp/NodeUtil.java | 31 +++++-------- .../google/javascript/jscomp/Normalize.java | 2 +- .../google/javascript/jscomp/SourceMap.java | 2 +- .../jscomp/SuppressDocWarningsGuard.java | 2 +- .../javascript/jscomp/TypedScopeCreator.java | 2 +- .../javascript/jscomp/NodeUtilTest.java | 44 ++++++++++++++----- 14 files changed, 59 insertions(+), 46 deletions(-) diff --git a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java index d6fc5e0f363..b1efd5b9009 100644 --- a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java +++ b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java @@ -686,7 +686,7 @@ private void rewriteAliasProp(Node value, int depth, Set newNodes, Na for (int i = 0; i <= depth; i++) { if (target.isGetProp()) { target = target.getFirstChild(); - } else if (NodeUtil.isObjectLitKeyInObjectLit(target)) { + } else if (NodeUtil.isObjectLitKey(target)) { // Object literal key definitions are a little trickier, as we // need to find the assignment target Node gparent = target.getGrandparent(); diff --git a/src/com/google/javascript/jscomp/AnalyzePrototypeProperties.java b/src/com/google/javascript/jscomp/AnalyzePrototypeProperties.java index cec556a975f..d642a9a1cab 100644 --- a/src/com/google/javascript/jscomp/AnalyzePrototypeProperties.java +++ b/src/com/google/javascript/jscomp/AnalyzePrototypeProperties.java @@ -411,7 +411,7 @@ private boolean isAssignRValue(Node n, Node parent) { private String getPrototypePropertyNameFromRValue(Node rValue) { Node lValue = NodeUtil.getBestLValue(rValue); if (lValue == null - || !((NodeUtil.isObjectLitKey(lValue) && !lValue.isQuotedString()) + || !((NodeUtil.mayBeObjectLitKey(lValue) && !lValue.isQuotedString()) || NodeUtil.isExprAssign(lValue.getGrandparent()))) { return null; } diff --git a/src/com/google/javascript/jscomp/CheckGlobalThis.java b/src/com/google/javascript/jscomp/CheckGlobalThis.java index 71b0525d44e..bb12af32001 100644 --- a/src/com/google/javascript/jscomp/CheckGlobalThis.java +++ b/src/com/google/javascript/jscomp/CheckGlobalThis.java @@ -118,7 +118,7 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { // Don't traverse functions that are getting lent to a prototype. Node grandparent = parent.getParent(); - if (NodeUtil.isObjectLitKey(parent)) { + if (NodeUtil.mayBeObjectLitKey(parent)) { JSDocInfo maybeLends = grandparent.getJSDocInfo(); if (maybeLends != null && maybeLends.hasLendsName() diff --git a/src/com/google/javascript/jscomp/CollapseProperties.java b/src/com/google/javascript/jscomp/CollapseProperties.java index ac7b81e9235..ad042a2cf08 100644 --- a/src/com/google/javascript/jscomp/CollapseProperties.java +++ b/src/com/google/javascript/jscomp/CollapseProperties.java @@ -301,7 +301,7 @@ private void flattenReferencesTo(Name n, String alias) { // 2) References inside a complex assign. (a = x.y = 0). These are // called TWIN references, because they show up twice in the // reference list. Only collapse the set, not the alias. - if (!NodeUtil.isObjectLitKey(r.node) && (r.getTwin() == null || r.isSet())) { + if (!NodeUtil.mayBeObjectLitKey(r.node) && (r.getTwin() == null || r.isSet())) { flattenNameRef(alias, r.node, rParent, originalName); } else if (r.node.isStringKey() && r.node.getParent().isObjectPattern()) { Node newNode = IR.name(alias).srcref(r.node); @@ -375,7 +375,7 @@ private void flattenNameRefAtDepth(String alias, Node n, int depth, // proceeding. In the OBJLIT case, we don't need to do anything. Token nType = n.getToken(); boolean isQName = nType == Token.NAME || nType == Token.GETPROP; - boolean isObjKey = NodeUtil.isObjectLitKey(n); + boolean isObjKey = NodeUtil.mayBeObjectLitKey(n); checkState(isObjKey || isQName); if (isQName) { for (int i = 1; i < depth && n.hasChildren(); i++) { diff --git a/src/com/google/javascript/jscomp/DefinitionsRemover.java b/src/com/google/javascript/jscomp/DefinitionsRemover.java index 43392e348e5..2d7f19df05c 100644 --- a/src/com/google/javascript/jscomp/DefinitionsRemover.java +++ b/src/com/google/javascript/jscomp/DefinitionsRemover.java @@ -63,7 +63,7 @@ static Definition getDefinition(Node n, boolean isExtern) { return new MemberFunctionDefinition(n, isExtern); } else if (parent.isAssign() && parent.getFirstChild() == n) { return new AssignmentDefinition(parent, isExtern); - } else if (NodeUtil.isObjectLitKey(n)) { + } else if (NodeUtil.mayBeObjectLitKey(n)) { return new ObjectLiteralPropertyDefinition(n, n.getFirstChild(), isExtern); } else if (NodeUtil.getEnclosingType(n, Token.PARAM_LIST) != null && n.isName()) { Node paramList = NodeUtil.getEnclosingType(n, Token.PARAM_LIST); @@ -110,7 +110,7 @@ static boolean isDefinitionNode(Node n) { return true; } else if (parent.isAssign() && parent.getFirstChild() == n) { return true; - } else if (NodeUtil.isObjectLitKey(n)) { + } else if (NodeUtil.mayBeObjectLitKey(n)) { return true; } else if (parent.isParamList()) { return true; diff --git a/src/com/google/javascript/jscomp/ExpressionDecomposer.java b/src/com/google/javascript/jscomp/ExpressionDecomposer.java index f64d76d399d..e54929d0b47 100644 --- a/src/com/google/javascript/jscomp/ExpressionDecomposer.java +++ b/src/com/google/javascript/jscomp/ExpressionDecomposer.java @@ -311,7 +311,7 @@ private void decomposeSubExpressions(Node n, Node stopNode, DecompositionState s } // Never try to decompose an object literal key. - checkState(!NodeUtil.isObjectLitKey(n), n); + checkState(!NodeUtil.mayBeObjectLitKey(n), n); // Decompose the children in reverse evaluation order. This simplifies // determining if the any of the children following have side-effects. diff --git a/src/com/google/javascript/jscomp/FindExportableNodes.java b/src/com/google/javascript/jscomp/FindExportableNodes.java index 36d6af2f220..57e0f387505 100644 --- a/src/com/google/javascript/jscomp/FindExportableNodes.java +++ b/src/com/google/javascript/jscomp/FindExportableNodes.java @@ -191,7 +191,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { // If we cleanup the code base in the future, we can warn again. else if (!(n.isGetProp() && parent.isExprResult())) { // Don't produce extra warnings for functions values of object literals - if (!n.isFunction() || !NodeUtil.isObjectLitKey(parent)) { + if (!n.isFunction() || !NodeUtil.mayBeObjectLitKey(parent)) { if (allowLocalExports) { compiler.report(t.makeError(n, EXPORT_ANNOTATION_NOT_ALLOWED)); } else { diff --git a/src/com/google/javascript/jscomp/GlobalNamespace.java b/src/com/google/javascript/jscomp/GlobalNamespace.java index 0c0d05bbeca..37a8ab001fe 100644 --- a/src/com/google/javascript/jscomp/GlobalNamespace.java +++ b/src/com/google/javascript/jscomp/GlobalNamespace.java @@ -225,7 +225,7 @@ void scanNewNodes(Set newNodes) { BuildGlobalNamespace builder = new BuildGlobalNamespace(); for (AstChange info : newNodes) { - if (!info.node.isQualifiedName() && !NodeUtil.isObjectLitKey(info.node)) { + if (!info.node.isQualifiedName() && !NodeUtil.mayBeObjectLitKey(info.node)) { continue; } scanFromNode(builder, info.module, info.scope, info.node); @@ -988,7 +988,7 @@ boolean maybeHandlePrototypePrefix(JSModule module, Scope scope, } } - if (parent != null && NodeUtil.isObjectLitKey(n)) { + if (parent != null && NodeUtil.mayBeObjectLitKey(n)) { // Object literal keys have no prefix that's referenced directly per // key, so we're done. return true; diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index d00880d6342..289939d7e67 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -3640,15 +3640,14 @@ private static boolean isLhsByDestructuringHelper(Node n) { } /** - * Determines whether a node represents an object literal key (e.g. key1 in {key1: value1, key2: - * value2}). Computed properties are excluded here (see b/111621528). - * - *

TODO(lharker): rename this to isMaybeObjectLitKey, since it also returns true for object - * pattern keys. + * Determines whether a node represents a possible object literal key (e.g. key1 in {key1: value1, + * key2: value2}). Computed properties are excluded here (see b/111621528). This method does not + * check whether the node is actually in an object literal! it also returns true for object + * pattern keys, and member functions/getters in ES6 classes. * * @param node A node */ - static boolean isObjectLitKey(Node node) { + static boolean mayBeObjectLitKey(Node node) { switch (node.getToken()) { case STRING_KEY: case GETTER_DEF: @@ -3664,16 +3663,10 @@ static boolean isObjectLitKey(Node node) { * Determines whether a node represents an object literal key (e.g. key1 in {key1: value1, key2: * value2}) and is in an object literal. Computed properties are excluded here (see b/111621528). * - *

TODO(lharker): rename isObjectLitKey to isMaybeObjectLitKey, since that method also returns - * true for object pattern keys, and rename this to isObjectLitKey - * * @param node A node */ - static boolean isObjectLitKeyInObjectLit(Node node) { - if (!node.getParent().isObjectLit()) { - return false; - } - return isObjectLitKey(node); + static boolean isObjectLitKey(Node node) { + return node.getParent().isObjectLit() && mayBeObjectLitKey(node); } /** @@ -4832,7 +4825,7 @@ static boolean isConstantByConvention( Node parent = node.getParent(); if (parent.isGetProp() && node == parent.getLastChild()) { return convention.isConstantKey(node.getString()); - } else if (isObjectLitKey(node)) { + } else if (mayBeObjectLitKey(node)) { return convention.isConstantKey(node.getString()); } else if (node.isName()) { return convention.isConstant(node.getString()); @@ -5279,7 +5272,7 @@ public static Node getBestJSDocInfoNode(Node n) { return getBestJSDocInfoNode(parent); } else if (parent.isAssign()) { return getBestJSDocInfoNode(parent); - } else if (isObjectLitKey(parent) || parent.isComputedProp()) { + } else if (mayBeObjectLitKey(parent) || parent.isComputedProp()) { return parent; } else if ((parent.isFunction() || parent.isClass()) && n == parent.getFirstChild()) { // n is the NAME node of the function/class. @@ -5307,7 +5300,7 @@ public static Node getBestLValue(Node n) { return parent; } else if (parent.isAssign()) { return parent.getFirstChild(); - } else if (isObjectLitKey(parent) || parent.isComputedProp()) { + } else if (mayBeObjectLitKey(parent) || parent.isComputedProp()) { return parent; } else if ((parent.isHook() && parent.getFirstChild() != n) || parent.isOr() @@ -5360,7 +5353,7 @@ static Node getBestLValueOwner(@Nullable Node lValue) { if (lValue == null || lValue.getParent() == null) { return null; } - if (isObjectLitKey(lValue) || lValue.isComputedProp()) { + if (mayBeObjectLitKey(lValue) || lValue.isComputedProp()) { return getBestLValue(lValue.getParent()); } else if (isGet(lValue)) { return lValue.getFirstChild(); @@ -5385,7 +5378,7 @@ static String getBestLValueName(@Nullable Node lValue) { } // TODO(sdh): Tighten this to simply require !lValue.isQuotedString() // Could get rid of the isJSIdentifier check, but may need to fix depot. - if (isObjectLitKey(lValue)) { + if (mayBeObjectLitKey(lValue)) { Node owner = getBestLValue(lValue.getParent()); if (owner != null) { String ownerName = getBestLValueName(owner); diff --git a/src/com/google/javascript/jscomp/Normalize.java b/src/com/google/javascript/jscomp/Normalize.java index 48bc5878c16..325c8d39727 100644 --- a/src/com/google/javascript/jscomp/Normalize.java +++ b/src/com/google/javascript/jscomp/Normalize.java @@ -419,7 +419,7 @@ private void annotateConstantsByConvention(Node n, Node parent) { // There are only two cases where a string token // may be a variable reference: The right side of a GETPROP // or an OBJECTLIT key. - boolean isObjLitKey = NodeUtil.isObjectLitKey(n); + boolean isObjLitKey = NodeUtil.mayBeObjectLitKey(n); boolean isProperty = isObjLitKey || (parent.isGetProp() && parent.getLastChild() == n); if (n.isName() || isProperty) { boolean isMarkedConstant = n.getBooleanProp(Node.IS_CONSTANT_NAME); diff --git a/src/com/google/javascript/jscomp/SourceMap.java b/src/com/google/javascript/jscomp/SourceMap.java index 7ac84211c0a..5e8e59e7aa9 100644 --- a/src/com/google/javascript/jscomp/SourceMap.java +++ b/src/com/google/javascript/jscomp/SourceMap.java @@ -81,7 +81,7 @@ public static enum DetailLevel implements Predicate { || node.isFunction() || node.isName() || NodeUtil.isGet(node) - || NodeUtil.isObjectLitKey(node) + || NodeUtil.mayBeObjectLitKey(node) || (node.isString() && NodeUtil.isGet(node.getParent())) || node.isTaggedTemplateLit(); } diff --git a/src/com/google/javascript/jscomp/SuppressDocWarningsGuard.java b/src/com/google/javascript/jscomp/SuppressDocWarningsGuard.java index 92b9d73c8da..2686c9760d8 100644 --- a/src/com/google/javascript/jscomp/SuppressDocWarningsGuard.java +++ b/src/com/google/javascript/jscomp/SuppressDocWarningsGuard.java @@ -103,7 +103,7 @@ public CheckLevel level(JSError error) { } else if (NodeUtil.isNameDeclaration(current) || (current.isAssign() && current.getParent().isExprResult()) || (current.isGetProp() && current.getParent().isExprResult()) - || NodeUtil.isObjectLitKey(current) + || NodeUtil.mayBeObjectLitKey(current) || current.isComputedProp()) { info = NodeUtil.getBestJSDocInfo(current); } diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index c49c349d65d..8272768d7d6 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -1060,7 +1060,7 @@ private boolean shouldUseFunctionLiteralType( if (info != null) { return true; } - if (lValue != null && NodeUtil.isObjectLitKey(lValue)) { + if (lValue != null && NodeUtil.mayBeObjectLitKey(lValue)) { return false; } // TODO(johnlenz): consider unifying global and local behavior diff --git a/test/com/google/javascript/jscomp/NodeUtilTest.java b/test/com/google/javascript/jscomp/NodeUtilTest.java index 86c02507051..6afb1afdca4 100644 --- a/test/com/google/javascript/jscomp/NodeUtilTest.java +++ b/test/com/google/javascript/jscomp/NodeUtilTest.java @@ -436,23 +436,43 @@ public void testGetArrayStringValue() { } @Test - public void testIsObjectLiteralKey1() { - assertIsObjectLiteralKey(parseExpr("({})"), false); - assertIsObjectLiteralKey(parseExpr("a"), false); - assertIsObjectLiteralKey(parseExpr("'a'"), false); - assertIsObjectLiteralKey(parseExpr("1"), false); - assertIsObjectLiteralKey(parseExpr("({a: 1})").getFirstChild(), true); - assertIsObjectLiteralKey(parseExpr("({1: 1})").getFirstChild(), true); - assertIsObjectLiteralKey(parseExpr("({get a(){}})").getFirstChild(), true); - assertIsObjectLiteralKey(parseExpr("({set a(b){}})").getFirstChild(), true); - } - + public void testMayBeObjectLitKey() { + assertMayBeObjectLitKey(parseExpr("({})"), false); + assertMayBeObjectLitKey(parseExpr("a"), false); + assertMayBeObjectLitKey(parseExpr("'a'"), false); + assertMayBeObjectLitKey(parseExpr("1"), false); + assertMayBeObjectLitKey(parseExpr("({a: 1})").getFirstChild(), true); + assertMayBeObjectLitKey(parseExpr("({1: 1})").getFirstChild(), true); + assertMayBeObjectLitKey(parseExpr("({get a(){}})").getFirstChild(), true); + assertMayBeObjectLitKey(parseExpr("({set a(b){}})").getFirstChild(), true); + // returns false for computed properties (b/111621528) + assertMayBeObjectLitKey(parseExpr("({['a']: 1})").getFirstChild(), false); + // returns true for non-object-literal keys + assertMayBeObjectLitKey(parseExpr("({a} = {})").getFirstFirstChild(), true); + assertMayBeObjectLitKey(parseExpr("(class { a() {} })").getLastChild().getFirstChild(), true); + } + + @Test + public void testIsObjectLitKey() { + assertIsObjectLitKey(parseExpr("({a: 1})").getFirstChild(), true); + // returns false for computed properties (b/111621528) + assertMayBeObjectLitKey(parseExpr("({['a']: 1})").getFirstChild(), false); + // returns false for object patterns + assertIsObjectLitKey(parseExpr("({a} = {})").getFirstFirstChild(), false); + assertIsObjectLitKey(parseExpr("(class { a() {} })").getLastChild().getFirstChild(), false); + } + + /** Returns the parsed expression (e.g. returns a NAME given 'a') */ private Node parseExpr(String js) { Node root = parse(js); return root.getFirstFirstChild(); } - private void assertIsObjectLiteralKey(Node node, boolean expected) { + private void assertMayBeObjectLitKey(Node node, boolean expected) { + assertThat(NodeUtil.mayBeObjectLitKey(node)).isEqualTo(expected); + } + + private void assertIsObjectLitKey(Node node, boolean expected) { assertThat(NodeUtil.isObjectLitKey(node)).isEqualTo(expected); }