From 974f1352c644dbb81f2abbd88919c71a3a30486d Mon Sep 17 00:00:00 2001 From: yitingwang Date: Mon, 17 Jul 2017 15:41:35 -0700 Subject: [PATCH] Add support for computed property names and shorthand method names for object literals in NTI. For computed property names, a property would only be added to the object's list of properties during type checking if it is a simple string, number, or boolean without any computation involved. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=162281584 --- .../javascript/jscomp/CodeGenerator.java | 7 +- .../jscomp/GlobalTypeInfoCollector.java | 22 ++++--- .../javascript/jscomp/NewTypeInference.java | 29 +++++++-- .../google/javascript/jscomp/NodeUtil.java | 29 ++++++++- .../jscomp/NewTypeInferenceTest.java | 65 +++++++++++++++++++ 5 files changed, 131 insertions(+), 21 deletions(-) diff --git a/src/com/google/javascript/jscomp/CodeGenerator.java b/src/com/google/javascript/jscomp/CodeGenerator.java index ea2dabcc141..824bb18c92f 100644 --- a/src/com/google/javascript/jscomp/CodeGenerator.java +++ b/src/com/google/javascript/jscomp/CodeGenerator.java @@ -983,12 +983,7 @@ protected void add(Node n, Context context) { cc.listSeparator(); } - checkState( - c.isComputedProp() - || c.isGetterDef() - || c.isSetterDef() - || c.isStringKey() - || c.isMemberFunctionDef()); + checkState(NodeUtil.isObjLitProperty(c)); add(c); } add("}"); diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java index 76fdc492c2d..231f0c06070 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java @@ -1529,13 +1529,21 @@ private void visitObjectLit(Node objLitNode, Node parent) { Node maybeLvalue = parent.isAssign() ? parent.getFirstChild() : parent; if (NodeUtil.isNamespaceDecl(maybeLvalue) && currentScope.isNamespace(maybeLvalue)) { for (Node prop : objLitNode.children()) { - recordPropertyName(prop); - visitNamespacePropertyDeclaration(prop, maybeLvalue, prop.getString()); + Node keyNode = NodeUtil.getObjectLitKeyNode(prop); + if (keyNode != null) { + recordPropertyName(keyNode); + } + if (!prop.isComputedProp()) { + visitNamespacePropertyDeclaration(prop, maybeLvalue, prop.getString()); + } } } else if (!NodeUtil.isEnumDecl(maybeLvalue) && !NodeUtil.isPrototypeAssignment(maybeLvalue)) { for (Node prop : objLitNode.children()) { - recordPropertyName(prop); + Node keyNode = NodeUtil.getObjectLitKeyNode(prop); + if (keyNode != null) { + recordPropertyName(keyNode); + } JSDocInfo propJsdoc = prop.getJSDocInfo(); if (propJsdoc != null) { getDeclaredObjLitProps().put(prop, getDeclaredTypeOfNode(propJsdoc, currentScope)); @@ -1806,10 +1814,7 @@ private void visitNamespacePropertyDeclaration(Node getProp) { private void visitNamespacePropertyDeclaration(Node declNode, Node recv, String pname) { checkArgument( - declNode.isGetProp() - || declNode.isStringKey() - || declNode.isGetterDef() - || declNode.isSetterDef(), + declNode.isGetProp() || NodeUtil.isObjLitProperty(declNode), declNode); checkArgument(currentScope.isNamespace(recv)); if (declNode.isGetterDef()) { @@ -2682,7 +2687,8 @@ private static Node fromDefsiteToName(Node defSite) { return defSite.getLastChild(); } if (defSite.isName() || defSite.isStringKey() - || defSite.isGetterDef() || defSite.isSetterDef()) { + || defSite.isGetterDef() || defSite.isSetterDef() + || defSite.isMemberFunctionDef()) { return defSite; } throw new RuntimeException("Unknown defsite: " + defSite.getToken()); diff --git a/src/com/google/javascript/jscomp/NewTypeInference.java b/src/com/google/javascript/jscomp/NewTypeInference.java index 1a43f2e1e37..38f8f7caa32 100644 --- a/src/com/google/javascript/jscomp/NewTypeInference.java +++ b/src/com/google/javascript/jscomp/NewTypeInference.java @@ -1532,6 +1532,14 @@ private EnvTypePair analyzeExprFwd( resultPair = analyzeNameFwd(expr, inEnv, requiredType, specializedType); } break; + case MEMBER_FUNCTION_DEF: + resultPair = analyzeExprFwd(expr.getFirstChild(), inEnv, requiredType, specializedType); + break; + case COMPUTED_PROP: + resultPair = analyzeExprFwd(expr.getFirstChild(), inEnv, requiredType, specializedType); + resultPair = analyzeExprFwd( + expr.getSecondChild(), resultPair.env, requiredType, specializedType); + break; default: throw new RuntimeException("Unhandled expression type: " + expr.getToken()); } @@ -2766,13 +2774,13 @@ private EnvTypePair analyzeObjLitFwd( } else if (isDict && !prop.isQuotedString()) { warnings.add(JSError.make(prop, ILLEGAL_OBJLIT_KEY, "dict")); } - String pname = NodeUtil.getObjectLitKeyName(prop); // We can't assign to a getter to change its value. // We can't do a prop access on a setter. // So, we don't associate pname with a getter/setter. // We add a property with a name that's weird enough to hopefully avoid // an accidental clash. if (prop.isGetterDef() || prop.isSetterDef()) { + String pname = NodeUtil.getObjectLitKeyName(prop); EnvTypePair pair = analyzeExprFwd(prop.getFirstChild(), env); FunctionType funType = pair.type.getFunType(); checkNotNull(funType); @@ -2788,7 +2796,14 @@ private EnvTypePair analyzeObjLitFwd( result = result.withProperty(new QualifiedName(specialPropName), propType); env = pair.env; } else { - QualifiedName qname = new QualifiedName(pname); + Node pnameNode = NodeUtil.getObjectLitKeyNode(prop); + if (pnameNode == null) { + // pnameNode is null when prop is a computed prop does not have a String node key. + // Just type-check the prop, then move on to the next property. + env = analyzeExprFwd(prop, env).env; + continue; + } + QualifiedName qname = new QualifiedName(pnameNode.getString()); JSType jsdocType = symbolTable.getPropDeclaredType(prop); JSType reqPtype; JSType specPtype; @@ -3665,6 +3680,11 @@ private EnvTypePair analyzeExprBwd( } else { return analyzeNameBwd(expr, outEnv, requiredType); } + case MEMBER_FUNCTION_DEF: + return analyzeExprBwd(expr.getFirstChild(), outEnv, requiredType); + case COMPUTED_PROP: + TypeEnv env = analyzeExprBwd(expr.getSecondChild(), outEnv).env; + return analyzeExprBwd(expr.getFirstChild(), env); default: throw new RuntimeException( "BWD: Unhandled expression type: " @@ -3992,11 +4012,12 @@ private EnvTypePair analyzeObjLitBwd( for (Node prop = objLit.getLastChild(); prop != null; prop = prop.getPrevious()) { - QualifiedName pname = - new QualifiedName(NodeUtil.getObjectLitKeyName(prop)); if (prop.isGetterDef() || prop.isSetterDef()) { env = analyzeExprBwd(prop.getFirstChild(), env).env; + } else if (prop.isComputedProp() && !prop.getFirstChild().isString()){ + env = analyzeExprBwd(prop, env).env; } else { + QualifiedName pname = new QualifiedName(NodeUtil.getObjectLitKeyName(prop)); JSType jsdocType = symbolTable.getPropDeclaredType(prop); JSType reqPtype; if (jsdocType != null) { diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index d7b6e35bfa9..539536a6f32 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -3146,12 +3146,27 @@ static boolean isObjectLitKey(Node node) { * @param key A node */ static String getObjectLitKeyName(Node key) { + Node keyNode = getObjectLitKeyNode(key); + if (keyNode != null) { + return keyNode.getString(); + } + throw new IllegalStateException("Unexpected node type: " + key); + } + + /** + * Get the Node that defines the name of an object literal key. + * + * @param key A node + */ + static Node getObjectLitKeyNode(Node key) { switch (key.getToken()) { case STRING_KEY: case GETTER_DEF: case SETTER_DEF: case MEMBER_FUNCTION_DEF: - return key.getString(); + return key; + case COMPUTED_PROP: + return key.getFirstChild().isString() ? key.getFirstChild() : null; default: break; } @@ -4400,8 +4415,7 @@ static boolean evaluatesToLocalValue(Node value, Predicate locals) { return true; case OBJECTLIT: for (Node key : value.children()) { - Preconditions.checkState( - key.isGetterDef() || key.isSetterDef() || key.isStringKey() || key.isComputedProp(), + Preconditions.checkState(isObjLitProperty(key), "Unexpected obj literal key:", key); @@ -5111,4 +5125,13 @@ public void visit(NodeTraversal t, Node n, Node parent) {} NodeTraversal t = new NodeTraversal(compiler, finder, scopeCreator); t.traverseAtScope(scope); } + + /** Returns true if the node is a property of an object literal. */ + public static boolean isObjLitProperty(Node node) { + return node.isStringKey() + || node.isGetterDef() + || node.isSetterDef() + || node.isMemberFunctionDef() + || node.isComputedProp(); + } } diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index 374e431d333..f50f6d0c1fc 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -2479,6 +2479,71 @@ public void testObjectLiteralShorthandProperty() { NewTypeInference.MISTYPED_ASSIGN_RHS); } + public void testComputedProp() { + typeCheck(LINE_JOINER.join( + "var i = 1;", + "var obj = {", + " ['var' + i]: i,", + "};")); + + // Computed prop type checks within + typeCheck(LINE_JOINER.join( + "var i = null;", + "var obj = {", + " ['var' + i++]: i", + "};"), + NewTypeInference.INVALID_OPERAND_TYPE); + + // Computed prop does not exist as obj prop + typeCheck(LINE_JOINER.join( + "var i = 1;", + "var obj = {", + " ['var' + i]: i", + "};", + "var x = obj.var1"), + NewTypeInference.INEXISTENT_PROPERTY); + + // But if it is a plain string then safe to add it as a property. + typeCheck(LINE_JOINER.join( + "var obj = {", + " ['static']: 1", + "};", + "var /** number */ x = obj.static")); + + // Does not yet have support for qualified names + typeCheck(LINE_JOINER.join( + "/** @const */ FOO = 'bar';", + "var obj = {", + " [FOO]: 1", + "};", + "var x = obj.bar"), + NewTypeInference.INEXISTENT_PROPERTY); + + // Test recordPropertyName calls in GTICollector + typeCheck(LINE_JOINER.join( + "/** @const */", + "var ns = { ['a']: 123 };", + "var obj = { ['b']: 234 };", + "function f(x) {", + " return x.a + x.b;", + "}")); + } + + public void testMemberFunctionDef() { + typeCheck(LINE_JOINER.join( + "var obj = {", + " method (/** number */ n) {}", + "};", + "obj.method(1);")); + + typeCheck(LINE_JOINER.join( + "var obj = {", + " method (/** string */ n) {}", + "};", + "obj.method(1);"), + NewTypeInference.INVALID_ARGUMENT_TYPE); + } + public void testDeclaredVersusInferredTypeForVar() { typeCheck("var /** ? */ x = 'str'; x - 123;");