Skip to content

Commit

Permalink
Make NodeUtil.isObjectLitKey return false for object pattern keys
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@lauraharker @brad4d
lauraharker authored and brad4d committed Dec 21, 2018
1 parent cd556c2 commit 041fa19
Show file tree
Hide file tree
Showing 14 changed files with 59 additions and 46 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/AggressiveInlineAliases.java
View file
Expand Up @@ -686,7 +686,7 @@ private void rewriteAliasProp(Node value, int depth, Set<AstChange> 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();
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/AnalyzePrototypeProperties.java
View file
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/CheckGlobalThis.java
View file
Expand Up @@ -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()
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/CollapseProperties.java
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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++) {
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/DefinitionsRemover.java
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/ExpressionDecomposer.java
View file
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/FindExportableNodes.java
View file
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/GlobalNamespace.java
View file
Expand Up @@ -225,7 +225,7 @@ void scanNewNodes(Set<AstChange> 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);
Expand Down Expand Up @@ -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;
Expand Down
31 changes: 12 additions & 19 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -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).
*
* <p>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:
Expand All @@ -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).
*
* <p>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);
}

/**
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/Normalize.java
View file
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/SourceMap.java
View file
Expand Up @@ -81,7 +81,7 @@ public static enum DetailLevel implements Predicate<Node> {
|| node.isFunction()
|| node.isName()
|| NodeUtil.isGet(node)
|| NodeUtil.isObjectLitKey(node)
|| NodeUtil.mayBeObjectLitKey(node)
|| (node.isString() && NodeUtil.isGet(node.getParent()))
|| node.isTaggedTemplateLit();
}
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/SuppressDocWarningsGuard.java
View file
Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -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
Expand Down
44 changes: 32 additions & 12 deletions test/com/google/javascript/jscomp/NodeUtilTest.java
View file
Expand Up @@ -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);
}

Expand Down

0 comments on commit 041fa19

Please sign in to comment.