From 5ea88cb6aa1e5a98eef34c80852936be3f73f9d8 Mon Sep 17 00:00:00 2001 From: dimvar Date: Fri, 10 Nov 2017 09:24:49 -0800 Subject: [PATCH] [NTI] Handle namespace definitions on object-literal keys. Also, minor unrelated cleanup in GTI (attach some types to the AST instead of using maps in symbol table). Fixes https://github.com/google/closure-compiler/issues/2702. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175298833 --- .../javascript/jscomp/GlobalTypeInfo.java | 21 ---- .../jscomp/GlobalTypeInfoCollector.java | 111 ++++++++++-------- .../javascript/jscomp/NewTypeInference.java | 8 +- .../javascript/jscomp/SimpleInference.java | 2 +- .../jscomp/NewTypeInferenceTest.java | 33 +++++- 5 files changed, 96 insertions(+), 79 deletions(-) diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfo.java b/src/com/google/javascript/jscomp/GlobalTypeInfo.java index 3111c552afc..1f2450d4be8 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfo.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfo.java @@ -70,9 +70,6 @@ public class GlobalTypeInfo implements TypeIRegistry { private final JSTypeCreatorFromJSDoc typeParser; private final Map anonFunNames = new LinkedHashMap<>(); private final UniqueNameGenerator varNameGen; - // TODO(dimvar): Eventually attach these to nodes, like the current types. - private final Map castTypes = new LinkedHashMap<>(); - private final Map declaredObjLitProps = new LinkedHashMap<>(); private final JSTypes commonTypes; private final Set unknownTypeNames; @@ -133,14 +130,6 @@ Set getExternPropertyNames() { return this.externPropertyNames; } - Map getCastTypes() { - return this.castTypes; - } - - Map getDeclaredObjLitProps() { - return this.declaredObjLitProps; - } - public JSTypeCreatorFromJSDoc getTypeParser() { return this.typeParser; } @@ -161,16 +150,6 @@ List getImplicitInterfaceUses() { return this.implicitInterfaceUses; } - JSType getCastType(Node n) { - JSType t = castTypes.get(n); - checkNotNull(t); - return t; - } - - JSType getPropDeclaredType(Node n) { - return declaredObjLitProps.get(n); - } - Collection getAllPropertyNames() { return this.allPropertyNames; } diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java index 1cfc06f484a..e58a4f2509a 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java @@ -1122,6 +1122,12 @@ private void visitObjlitNamespace(QualifiedName qname, Node defSite) { QualifiedName propQname = new QualifiedName(propNode.getString()); if (isAliasedNamespaceDefinition(propNode)) { visitAliasedNamespace(QualifiedName.join(qname, propQname), propNode); + } else { + JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(propNode); + Node propVal = propNode.getLastChild(); + if (jsdoc != null && jsdoc.hasConstAnnotation() && propVal.isObjectLit()) { + visitObjlitNamespace(QualifiedName.join(qname, propQname), propNode); + } } } } @@ -1526,7 +1532,7 @@ void processLendsToNamespace( Namespace borrowerNamespace = currentScope.getNamespace(lendsQname); for (Node prop : objlit.children()) { String pname = NodeUtil.getObjectLitKeyName(prop); - JSType propDeclType = getDeclaredObjLitProps().get(prop); + JSType propDeclType = (JSType) prop.getTypeI(); if (propDeclType != null) { borrowerNamespace.addProperty(pname, prop, propDeclType, false); } else { @@ -1618,11 +1624,13 @@ public void visit(NodeTraversal t, Node n, Node parent) { break; } case CAST: - getCastTypes().put(n, - getDeclaredTypeOfNode(n.getJSDocInfo(), this.currentScope)); + n.setTypeI(getDeclaredTypeOfNode(n.getJSDocInfo(), this.currentScope)); break; case OBJECTLIT: - visitObjectLit(n, parent); + if (!NodeUtil.isObjectLitKey(parent)) { + Node lval = NodeUtil.getBestLValue(n); + visitObjectLit(n, QualifiedName.fromNode(lval)); + } break; case CALL: visitCall(n); @@ -1703,38 +1711,46 @@ private void visitVar(Node nameNode, Node parent) { } } - private void visitObjectLit(Node objLitNode, Node parent) { + private void visitObjectLit(Node objLitNode, QualifiedName lvalueQname) { + Node parent = objLitNode.getParent(); JSDocInfo jsdoc = objLitNode.getJSDocInfo(); if (jsdoc != null && jsdoc.getLendsName() != null) { lendsObjlits.add(objLitNode); } Node maybeLvalue = parent.isAssign() ? parent.getFirstChild() : parent; - if (NodeUtil.isNamespaceDecl(maybeLvalue) && currentScope.isNamespace(maybeLvalue)) { + if (currentScope.isNamespace(lvalueQname)) { for (Node prop : objLitNode.children()) { - Node keyNode = NodeUtil.getObjectLitKeyNode(prop); - if (keyNode != null) { - recordPropertyName(keyNode); - } if (!prop.isComputedProp()) { - visitNamespacePropertyDeclaration(prop, maybeLvalue, prop.getString()); + visitNamespacePropertyDeclaration(prop, lvalueQname, prop.getString()); } } } else if (!NodeUtil.isEnumDecl(maybeLvalue) && !NodeUtil.isPrototypeAssignment(maybeLvalue)) { + // For object literals that are not "special" (namespaces, enums, etc), + // record their declared properties so we can typecheck them later. for (Node prop : objLitNode.children()) { - Node keyNode = NodeUtil.getObjectLitKeyNode(prop); - if (keyNode != null) { - recordPropertyName(keyNode); - } JSDocInfo propJsdoc = prop.getJSDocInfo(); if (propJsdoc != null) { - getDeclaredObjLitProps().put(prop, getDeclaredTypeOfNode(propJsdoc, currentScope)); + JSType propType = getDeclaredTypeOfNode(propJsdoc, currentScope); + prop.setTypeI(propType); } if (isAnnotatedAsConst(prop)) { warnings.add(JSError.make(prop, MISPLACED_CONST_ANNOTATION)); } } } + // Record property names and recur to nested object literals. + for (Node prop : objLitNode.children()) { + Node keyNode = NodeUtil.getObjectLitKeyNode(prop); + if (keyNode != null) { + recordPropertyName(keyNode); + } + if (prop.hasChildren() && prop.getFirstChild().isObjectLit()) { + QualifiedName nestedQname = lvalueQname == null || keyNode == null + ? null : QualifiedName.join(lvalueQname, new QualifiedName(keyNode.getString())); + visitObjectLit(prop.getFirstChild(), nestedQname); + } + } } private void visitCall(Node call) { @@ -2063,66 +2079,67 @@ private void visitNamespacePropertyDeclaration(Node getProp) { } Node recv = getProp.getFirstChild(); String pname = getProp.getLastChild().getString(); - visitNamespacePropertyDeclaration(getProp, recv, pname); + visitNamespacePropertyDeclaration(getProp, QualifiedName.fromNode(recv), pname); } - private void visitNamespacePropertyDeclaration(Node declNode, Node recv, String pname) { - checkArgument( - declNode.isGetProp() || NodeUtil.isObjLitProperty(declNode), - declNode); - QualifiedName recvQname = QualifiedName.fromNode(recv); - checkArgument(currentScope.isNamespace(recvQname)); - if (declNode.isGetterDef()) { + private void visitNamespacePropertyDeclaration( + Node defSite, QualifiedName nsQname, String pname) { + checkArgument(defSite.isGetProp() || NodeUtil.isObjLitProperty(defSite), defSite); + checkArgument(currentScope.isNamespace(nsQname)); + if (defSite.isGetterDef()) { pname = getCommonTypes().createGetterPropName(pname); - } else if (declNode.isSetterDef()) { + } else if (defSite.isSetterDef()) { pname = getCommonTypes().createSetterPropName(pname); } - // Named types have already been crawled in CollectNamedTypes - if (declNode.isStringKey() && currentScope.isNamespace(declNode.getFirstChild())) { - return; + if (defSite.isStringKey()) { + QualifiedName propQname = new QualifiedName(defSite.getString()); + // Namespaces have already been crawled in CollectNamedTypes + if (currentScope.isNamespace(QualifiedName.join(nsQname, propQname))) { + return; + } } - EnumType et = currentScope.getEnum(QualifiedName.fromNode(recv)); + EnumType et = currentScope.getEnum(nsQname); // If there is a reassignment to one of the enum's members, don't consider // that a definition of a new property. if (et != null && et.enumLiteralHasKey(pname)) { return; } - Namespace ns = currentScope.getNamespace(recvQname); - JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(declNode); - PropertyType pt = getPropTypeHelper(jsdoc, declNode, null); + Namespace ns = currentScope.getNamespace(nsQname); + JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(defSite); + PropertyType pt = getPropTypeHelper(jsdoc, defSite, null); JSType propDeclType = pt.declType; JSType propInferredFunType = pt.inferredFunType; - boolean isConst = isConst(declNode); + boolean isConst = isConst(defSite); if (propDeclType != null || isConst) { JSType previousPropType = ns.getPropDeclaredType(pname); - declNode.putBooleanProp(Node.ANALYZED_DURING_GTI, true); + defSite.putBooleanProp(Node.ANALYZED_DURING_GTI, true); if (ns.hasSubnamespace(new QualifiedName(pname)) || (ns.hasStaticProp(pname) && previousPropType != null && !suppressDupPropWarning(jsdoc, propDeclType, previousPropType))) { warnings.add(JSError.make( - declNode, REDECLARED_PROPERTY, pname, "namespace " + ns)); - declNode.getParent().putBooleanProp(Node.ANALYZED_DURING_GTI, true); + defSite, REDECLARED_PROPERTY, pname, "namespace " + ns)); + defSite.getParent().putBooleanProp(Node.ANALYZED_DURING_GTI, true); return; } if (propDeclType == null) { - propDeclType = mayInferFromRhsIfConst(declNode); + propDeclType = mayInferFromRhsIfConst(defSite); } - ns.addProperty(pname, declNode, propDeclType, isConst); - if (declNode.isGetProp() && isConst) { - declNode.putBooleanProp(Node.CONSTANT_PROPERTY_DEF, true); + ns.addProperty(pname, defSite, propDeclType, isConst); + if (defSite.isGetProp() && isConst) { + defSite.putBooleanProp(Node.CONSTANT_PROPERTY_DEF, true); } } else if (propInferredFunType != null) { - ns.addUndeclaredProperty(pname, declNode, propInferredFunType, false); + ns.addUndeclaredProperty(pname, defSite, propInferredFunType, false); } else { // Try to infer the prop type, but don't say that the prop is declared. - Node initializer = NodeUtil.getRValueOfLValue(declNode); + Node initializer = NodeUtil.getRValueOfLValue(defSite); JSType t = initializer == null ? null : simpleInferExpr(initializer, this.currentScope); if (t == null) { t = getCommonTypes().UNKNOWN; } - ns.addUndeclaredProperty(pname, declNode, t, false); + ns.addUndeclaredProperty(pname, defSite, t, false); } } @@ -2864,14 +2881,6 @@ private UniqueNameGenerator getVarNameGen() { return this.globalTypeInfo.getVarNameGen(); } - private Map getDeclaredObjLitProps() { - return this.globalTypeInfo.getDeclaredObjLitProps(); - } - - private Map getCastTypes() { - return this.globalTypeInfo.getCastTypes(); - } - private void recordPropertyName(Node pnameNode) { this.globalTypeInfo.recordPropertyName(pnameNode); } diff --git a/src/com/google/javascript/jscomp/NewTypeInference.java b/src/com/google/javascript/jscomp/NewTypeInference.java index 71a31abfaf6..6a7480afdfa 100644 --- a/src/com/google/javascript/jscomp/NewTypeInference.java +++ b/src/com/google/javascript/jscomp/NewTypeInference.java @@ -2546,7 +2546,7 @@ private EnvTypePair analyzeCastFwd(Node expr, TypeEnv inEnv, JSType specializedT Node insideCast = expr.getFirstChild(); EnvTypePair pair = analyzeExprFwd(insideCast, inEnv, this.commonTypes.UNKNOWN, newSpecType); JSType fromType = pair.type; - JSType toType = symbolTable.getCastType(expr); + JSType toType = (JSType) expr.getTypeI(); if (!fromType.isInterfaceInstance() && !toType.isInterfaceInstance() && !JSType.haveCommonSubtype(fromType, toType) @@ -3035,7 +3035,7 @@ private EnvTypePair analyzeObjLitFwd( continue; } QualifiedName qname = new QualifiedName(pnameNode.getString()); - JSType jsdocType = symbolTable.getPropDeclaredType(prop); + JSType jsdocType = (JSType) prop.getTypeI(); JSType reqPtype; JSType specPtype; if (jsdocType != null) { @@ -3918,7 +3918,7 @@ private EnvTypePair analyzeExprBwd( return analyzeArrayLitBwd(expr, outEnv); case CAST: { EnvTypePair pair = analyzeExprBwd(expr.getFirstChild(), outEnv); - pair.type = symbolTable.getCastType(expr); + pair.type = (JSType) expr.getTypeI(); return pair; } case TEMPLATELIT: @@ -4275,7 +4275,7 @@ private EnvTypePair analyzeObjLitBwd( env = analyzeExprBwd(prop, env).env; } else { QualifiedName pname = new QualifiedName(NodeUtil.getObjectLitKeyName(prop)); - JSType jsdocType = symbolTable.getPropDeclaredType(prop); + JSType jsdocType = (JSType) prop.getTypeI(); JSType reqPtype; if (jsdocType != null) { reqPtype = jsdocType; diff --git a/src/com/google/javascript/jscomp/SimpleInference.java b/src/com/google/javascript/jscomp/SimpleInference.java index 28e1f5a5e8c..fdca489f8f2 100644 --- a/src/com/google/javascript/jscomp/SimpleInference.java +++ b/src/com/google/javascript/jscomp/SimpleInference.java @@ -166,7 +166,7 @@ private JSType inferExprRecur(Node n, NTIScope scope) { case REGEXP: return this.commonTypes.getRegexpType(); case CAST: - return this.gti.getCastTypes().get(n); + return (JSType) n.getTypeI(); case ARRAYLIT: { if (!n.hasChildren()) { return this.commonTypes.getArrayInstance(); diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index 7f5c66f1abf..185da949fa2 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -7289,8 +7289,7 @@ public void testUnifyObjects() { " var y = cond ? {prop: 'str'} : {prop: 5};", " f({prop: x}, y);", "}", - "g({}, true);"), - NewTypeInference.INVALID_ARGUMENT_TYPE); + "g({}, true);")); typeCheck(LINE_JOINER.join( "function g(x, cond) {", @@ -12983,6 +12982,36 @@ public void testNamespacesWithNonEmptyObjectLiteral() { " obj.ns = { /** @const */ PROP: 5 };", "}"), GlobalTypeInfoCollector.MISPLACED_CONST_ANNOTATION); + + typeCheck(LINE_JOINER.join( + "/** @const */", + "var ns = { /** @const */ prop: {} };", + "/** @constructor */", + "ns.prop.Foo = function() {};", + "var /** !ns.prop.Foo */ x = new ns.prop.Foo();")); + + typeCheck(LINE_JOINER.join( + "/** @const */", + "var ns = { /** @const */ prop: {} };", + "/** @constructor */", + "ns.prop.Foo = function() {", + " /** @type {number} */ this.a = 123; ", + "};", + "var /** string */ s = (new ns.prop.Foo()).a;"), + NewTypeInference.MISTYPED_ASSIGN_RHS); + + typeCheck(LINE_JOINER.join( + "/** @const */", + "var ns = {};", + "/** @const */", + "ns.Constants = {", + " /** @const */ Async: {", + " FEATURE_ID:'feature_id'", + " }", + "};", + "/** @const {number} */", + "var x = ns.Constants.Async.FEATURE_ID;"), + NewTypeInference.MISTYPED_ASSIGN_RHS); } public void testNamespaceRedeclaredProps() {