From 33bcadb099b2a413936b0245d98b324d7ddd9ad8 Mon Sep 17 00:00:00 2001 From: dimvar Date: Wed, 11 Oct 2017 20:39:54 -0700 Subject: [PATCH] [NTI] Be more explicit about what property definitions we support on prototype methods. Also, remove PropertySetter. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=171913238 --- .../jscomp/GlobalTypeInfoCollector.java | 94 ++++++++++--------- .../jscomp/newtypes/NominalType.java | 4 + .../javascript/jscomp/newtypes/Property.java | 9 +- .../jscomp/newtypes/RawNominalType.java | 10 ++ .../jscomp/NewTypeInferenceTest.java | 88 +++++++++++++++-- .../jscomp/NewTypeInferenceTestBase.java | 1 + 6 files changed, 149 insertions(+), 57 deletions(-) diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java index 0daeb969099..d3f261cf972 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java @@ -293,7 +293,10 @@ public class GlobalTypeInfoCollector implements CompilerPass { private DefaultNameGenerator funNameGen; // Only for original definitions, not for aliased constructors private Map nominaltypesByNode = new LinkedHashMap<>(); - // Keyed on RawNominalTypes and property names + // propertyDefs collects places in the AST where properties are defined. + // For properties that are methods, PropertyDef includes some extra information. + // We use propertyDefs to handle inheritance issues, e.g., invalid property overrides. + // Keyed on RawNominalTypes and property names. private HashBasedTable propertyDefs = HashBasedTable.create(); private final Set inProgressFreezes = new LinkedHashSet<>(); @@ -715,8 +718,9 @@ private void checkSuperProperty( return; } PropertyDef localPropDef = propertyDefs.get(current, pname); - JSType localPropType = localPropDef == null - ? null : current.getInstancePropDeclaredType(pname); + // If the property is inherited, we want to drop the result of getInstancePropDeclaredType. + JSType localPropType = + localPropDef == null ? null : current.getInstancePropDeclaredType(pname); if (localPropDef != null && superType.isClass() && localPropType != null && localPropType.getFunType() != null @@ -2155,18 +2159,11 @@ private void visitOtherPropertyDeclaration(Node getProp) { getProp.getParent().putBooleanProp(Node.ANALYZED_DURING_GTI, true); return; } - JSType recvType = simpleInferExprType(recv); - // Might still be worth recording a property, e.g. on a function. - PropertyDef def = findPropertyDef(recv); - if (def != null) { - JSType type = - getProp.getNext() != null - ? simpleInferExprType(getProp.getNext()) - : getCommonTypes().UNKNOWN; - if (type != null) { - def.addProperty(recv.getNext().getString(), type); - } + if (isPrototypeProperty(getProp.getFirstChild())) { + mayAddPropertyToPrototypeMethod(getProp); + return; } + JSType recvType = simpleInferExprType(recv); if (recvType == null) { return; } @@ -2202,18 +2199,46 @@ private void visitOtherPropertyDeclaration(Node getProp) { } } - /** Given a qualified name node, find a corresponding PropertyDef in propertyDefs. */ - PropertyDef findPropertyDef(Node n) { - if (!isPrototypeProperty(n)) { - return null; + /** + * Handle property declarations on prototype methods, such as: + * Foo.prototype.f = function() {}; + * Foo.prototype.f.numprop = 123; + * We need to handle these definitions because some frameworks define such properties. + * Note that these declarations are still not as general as declarations on namespaces, e.g., + * we don't handle definitions of new types on prototype methods. + */ + void mayAddPropertyToPrototypeMethod(Node getProp) { + Node protoProp = getProp.getFirstChild(); + if (!isPrototypeProperty(protoProp)) { + return; } - RawNominalType ownerType = - currentScope.getNominalType(QualifiedName.fromNode(n.getFirstFirstChild())); + String protoPropName = protoProp.getLastChild().getString(); + QualifiedName rawtypeQname = QualifiedName.fromNode(protoProp.getFirstFirstChild()); + RawNominalType ownerType = this.currentScope.getNominalType(rawtypeQname); if (ownerType == null) { - return null; + return; + } + PropertyDef def = propertyDefs.get(ownerType, protoPropName); + if (def == null || def.methodType == null) { + return; + } + Node rhs = NodeUtil.getRValueOfLValue(getProp); + JSType newPropType = rhs == null ? null : simpleInferExprType(rhs); + if (newPropType == null) { + newPropType = getCommonTypes().UNKNOWN; + } + if (newPropType.isConstructor() || newPropType.isInterfaceDefinition()) { + // A prototype property is not a namespace, so don't allow defining types on it. + return; + } + String newPropName = getProp.getLastChild().getString(); + JSType protoPropType = ownerType.getProtoPropDeclaredType(protoPropName); + if (protoPropType == null) { + protoPropType = getCommonTypes().fromFunctionType(def.methodType.toFunctionType()); } - String propertyName = n.getLastChild().getString(); - return propertyDefs.get(ownerType, propertyName); + ownerType.updateProtoProperty( + protoPropName, + protoPropType.withProperty(new QualifiedName(newPropName), newPropType)); } boolean mayWarnAboutNoInit(Node constExpr) { @@ -2806,7 +2831,6 @@ private void mayAddPropToPrototype( if (propDeclType == null) { propDeclType = mayInferFromRhsIfConst(defSite); } - def.setter = new PropertySetter(rawType, pname, propDeclType, isConst); rawType.addProtoProperty(pname, defSite, propDeclType, isConst); if (defSite.isGetProp()) { // Don't bother saving for @lends defSite.putBooleanProp(Node.ANALYZED_DURING_GTI, true); @@ -2993,7 +3017,6 @@ private static class PropertyDef { final Node defSite; // The getProp/objectLitKey of the property definition DeclaredFunctionType methodType; // null for non-method property decls final NTIScope methodScope; // null for decls without function on the RHS - PropertySetter setter; // optional extra information for updating the rawtype PropertyDef( Node defSite, DeclaredFunctionType methodType, NTIScope methodScope) { @@ -3011,7 +3034,6 @@ PropertyDef substituteNominalGenerics(NominalType nt) { } PropertyDef def = new PropertyDef( this.defSite, this.methodType.substituteNominalGenerics(nt), this.methodScope); - def.setter = this.setter; return def; } @@ -3022,32 +3044,12 @@ void updateMethodType(DeclaredFunctionType updatedType) { } } - void addProperty(String name, JSType type) { - if (this.setter != null) { - setter.type = setter.type.withProperty(new QualifiedName(name), type); - setter.rawType.addProtoProperty(setter.name, defSite, setter.type, setter.isConstant); - } - } - @Override public String toString() { return "PropertyDef(" + defSite + ", " + methodType + ")"; } } - private static class PropertySetter { - final RawNominalType rawType; - final String name; - JSType type; - final boolean isConstant; - PropertySetter(RawNominalType rawType, String name, JSType type, boolean isConstant) { - this.rawType = rawType; - this.name = name; - this.type = type; - this.isConstant = isConstant; - } - } - private Map getAnonFunNames() { return this.globalTypeInfo.getAnonFunNames(); } diff --git a/src/com/google/javascript/jscomp/newtypes/NominalType.java b/src/com/google/javascript/jscomp/newtypes/NominalType.java index f0e379c1ea4..7bfce9515b8 100644 --- a/src/com/google/javascript/jscomp/newtypes/NominalType.java +++ b/src/com/google/javascript/jscomp/newtypes/NominalType.java @@ -345,6 +345,10 @@ public Set getAllNonInheritedInstanceProps() { return this.rawType.getAllNonInheritedInstanceProps(); } + /** + * Use with caution during GlobalTypeInfo; if some types are not known/resolved, + * the instantiation may be wrong. + */ public NominalType getInstantiatedSuperclass() { if (this.rawType.getSuperClass() == null) { return null; diff --git a/src/com/google/javascript/jscomp/newtypes/Property.java b/src/com/google/javascript/jscomp/newtypes/Property.java index 1bc56a27d93..67bfca92bd7 100644 --- a/src/com/google/javascript/jscomp/newtypes/Property.java +++ b/src/com/google/javascript/jscomp/newtypes/Property.java @@ -103,12 +103,17 @@ JSType getDeclaredType() { Property withOptional() { return isOptional() ? this - : new Property(defSite, inferredType, declaredType, Attribute.OPTIONAL); + : new Property(this.defSite, this.inferredType, this.declaredType, Attribute.OPTIONAL); } Property withRequired() { return isRequired() ? this - : new Property(defSite, inferredType, declaredType, Attribute.REQUIRED); + : new Property(this.defSite, this.inferredType, this.declaredType, Attribute.REQUIRED); + } + + Property withNewType(JSType newType) { + return new Property( + this.defSite, newType, this.declaredType == null ? null : newType, this.attribute); } private static Attribute meetAttributes(Attribute a1, Attribute a2) { diff --git a/src/com/google/javascript/jscomp/newtypes/RawNominalType.java b/src/com/google/javascript/jscomp/newtypes/RawNominalType.java index 918dbbb1089..1ea427f09df 100644 --- a/src/com/google/javascript/jscomp/newtypes/RawNominalType.java +++ b/src/com/google/javascript/jscomp/newtypes/RawNominalType.java @@ -625,6 +625,16 @@ public void addProtoProperty(String pname, Node defSite, JSType type, boolean is this.protoProps = this.protoProps.with(pname, newProp); } + /** + * Update the type of an existing prototype property. We use this when properties are defined + * on prototype methods. + */ + public void updateProtoProperty(String pname, JSType type) { + checkState(this.protoProps.containsKey(pname)); + Property newProp = this.protoProps.get(pname).withNewType(type); + this.protoProps = this.protoProps.with(pname, newProp); + } + /** Add a new undeclared prototype property to this class */ public void addUndeclaredProtoProperty(String pname, Node defSite, JSType inferredType) { checkState(!this.isFrozen); diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index a06519ced11..99080407da3 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -19469,13 +19469,10 @@ public void testAbstractMethodCalls() { } public void testAbstractMethodCallsWithGoogInerhits() { - String closureDefs = LINE_JOINER.join( - "/** @const */ var goog = {};", - "goog.inherits = function(child, parent){};"); // Converted from Closure style "goog.base" super call typeCheck( LINE_JOINER.join( - closureDefs, + CLOSURE_BASE, "/** @const */ var ns = {};", "/** @constructor @abstract */ ns.A = function() {};", "/** @abstract */ ns.A.prototype.foo = function() {};", @@ -19488,7 +19485,7 @@ public void testAbstractMethodCallsWithGoogInerhits() { typeCheck( LINE_JOINER.join( - closureDefs, + CLOSURE_BASE, "/** @const */ var ns = {};", "/** @constructor @abstract */ ns.A = function() {};", "/** @abstract */ ns.A.prototype.foo = function() {};", @@ -19501,7 +19498,7 @@ public void testAbstractMethodCallsWithGoogInerhits() { typeCheck( LINE_JOINER.join( - closureDefs, + CLOSURE_BASE, "/** @constructor @abstract */ var A = function() {};", "/** @abstract */", "A.prototype.foo = function() {};", @@ -19512,7 +19509,7 @@ public void testAbstractMethodCallsWithGoogInerhits() { typeCheck( LINE_JOINER.join( - closureDefs, + CLOSURE_BASE, "/** @struct @constructor @abstract */ var A = function() {};", "/** @abstract */ A.prototype.foo = function() {};", "/** @struct @constructor @extends {A} */ var B = function() {};", @@ -19526,7 +19523,7 @@ public void testAbstractMethodCallsWithGoogInerhits() { typeCheck( LINE_JOINER.join( - closureDefs, + CLOSURE_BASE, "/** @struct @constructor */ var A = function() {};", "A.prototype.foo = function() {};", "/** @struct @constructor @extends {A} */ var B = function() {};", @@ -19539,7 +19536,7 @@ public void testAbstractMethodCallsWithGoogInerhits() { typeCheck( LINE_JOINER.join( - closureDefs, + CLOSURE_BASE, "/** @constructor @abstract */ function A() {};", "/** @abstract */ A.prototype.foo = function() {};", "/** @constructor @extends {A} */ function B() {};", @@ -21855,6 +21852,79 @@ public void testPropertiesOnMethods() { "Foo.prototype.bar = function() {};", "Foo.prototype.bar.baz;", "var qux = new Foo().bar.baz;")); + + // We register bar#p even though we can't infer the type of x. + typeCheck(LINE_JOINER.join( + "function f(x) {", + " /** @constructor */", + " function Foo() {}", + " Foo.prototype.bar = function() {};", + " Foo.prototype.bar.p = x;", + " var y = (new Foo).bar.p;", + "}")); + + typeCheck(LINE_JOINER.join( + "/** @constructor */", + "function Foo() {}", + "Foo.prototype.bar = function() {};", + "Foo.prototype.bar.p1 = 123;", + "Foo.prototype.bar.p2 = 'asdf';", + "var /** number */ n = (new Foo).bar.p2;"), + NewTypeInference.MISTYPED_ASSIGN_RHS); + + // Property bar#p isn't inherited by Baz. A framework may copy such properties explicitly + // from the superclass to the subclass. + typeCheck(LINE_JOINER.join( + "/** @constructor */", + "function Foo() {}", + "Foo.prototype.bar = function() {};", + "Foo.prototype.bar.p = 123;", + "/** @constructor @extends {Foo} */", + "function Baz() {}", + "Baz.prototype.bar = function() {};", + "var /** string */ s = (new Baz).bar.p;"), + NewTypeInference.INEXISTENT_PROPERTY); + + // The properties declared on prototype methods are not declared (the type annotation is + // ignored). Not necessarily a good decision, but documenting with a test. + typeCheck( + LINE_JOINER.join( + "/** @constructor */ function Foo() {}", + "/** @param {number} x */", + "Foo.prototype.bar = function(x) {};", + "/** @type {number} */", + "Foo.prototype.bar.baz = 42;", + "Foo.prototype.bar.baz = '';")); + + // We only record the properties on methods, not on non-method prototype properties. + typeCheck( + LINE_JOINER.join( + "/** @constructor */ function Foo() {}", + "Foo.prototype.bar = {};", + "Foo.prototype.bar.baz = 42;", + "var x = (new Foo).bar.baz;"), + NewTypeInference.INEXISTENT_PROPERTY); + + typeCheck(LINE_JOINER.join( + "/** @constructor */", + "function Foo() {}", + "Foo.prototype.bar = function() {};", + "/** @constructor */", + "Foo.prototype.bar.Baz = function() {};", + "var /** !Foo.prototype.bar.Baz */ x = new (new Foo).bar.Baz();"), + GlobalTypeInfoCollector.UNRECOGNIZED_TYPE_NAME, + NewTypeInference.INEXISTENT_PROPERTY); + + // Don't add a stray property to all functions + typeCheck( + LINE_JOINER.join( + "/** @constructor */ function Foo() {}", + "/** @param {number} x */", + "Foo.prototype.bar = function(x) {};", + "/** @type {number} */", + "Foo.prototype.bar.baz = 42;", + "var /** string */ s = (function() {}).baz;"), + NewTypeInference.INEXISTENT_PROPERTY); } public void testAliasedNamespaceWithDotInTheName() { diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTestBase.java b/test/com/google/javascript/jscomp/NewTypeInferenceTestBase.java index ee6c4077536..e078744f565 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTestBase.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTestBase.java @@ -56,6 +56,7 @@ boolean checkTranspiled() { LINE_JOINER.join( "/** @const */", "var goog = {};", + "goog.inherits = function(child, parent){};", "/** @return {void} */", "goog.nullFunction = function() {};", "/** @type {!Function} */",