Skip to content

Commit

Permalink
[NTI] Be more explicit about what property definitions we support on …
Browse files Browse the repository at this point in the history 
…prototype methods.

Also, remove PropertySetter.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=171913238
  • Loading branch information
@dimvar
dimvar authored and Tyler Breisacher committed Oct 13, 2017
1 parent 24965e9 commit 33bcadb
Show file tree
Hide file tree
Showing 6 changed files with 149 additions and 57 deletions.
94 changes: 48 additions & 46 deletions src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java
View file
Expand Up @@ -293,7 +293,10 @@ public class GlobalTypeInfoCollector implements CompilerPass {
private DefaultNameGenerator funNameGen;
// Only for original definitions, not for aliased constructors
private Map<Node, RawNominalType> 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<RawNominalType, String, PropertyDef> propertyDefs =
HashBasedTable.create();
private final Set<RawNominalType> inProgressFreezes = new LinkedHashSet<>();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
}

Expand All @@ -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<Node, String> getAnonFunNames() {
return this.globalTypeInfo.getAnonFunNames();
}
Expand Down
4 changes: 4 additions & 0 deletions src/com/google/javascript/jscomp/newtypes/NominalType.java
View file
Expand Up @@ -345,6 +345,10 @@ public Set<String> 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;
Expand Down
9 changes: 7 additions & 2 deletions src/com/google/javascript/jscomp/newtypes/Property.java
View file
Expand Up @@ -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) {
Expand Down
10 changes: 10 additions & 0 deletions src/com/google/javascript/jscomp/newtypes/RawNominalType.java
View file
Expand Up @@ -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);
Expand Down
88 changes: 79 additions & 9 deletions test/com/google/javascript/jscomp/NewTypeInferenceTest.java
View file
Expand Up @@ -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() {};",
Expand All @@ -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() {};",
Expand All @@ -19501,7 +19498,7 @@ public void testAbstractMethodCallsWithGoogInerhits() {

typeCheck(
LINE_JOINER.join(
closureDefs,
CLOSURE_BASE,
"/** @constructor @abstract */ var A = function() {};",
"/** @abstract */",
"A.prototype.foo = function() {};",
Expand All @@ -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() {};",
Expand All @@ -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() {};",
Expand All @@ -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() {};",
Expand Down Expand Up @@ -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() {
Expand Down
1 change: 1 addition & 0 deletions test/com/google/javascript/jscomp/NewTypeInferenceTestBase.java
View file
Expand Up @@ -56,6 +56,7 @@ boolean checkTranspiled() {
LINE_JOINER.join(
"/** @const */",
"var goog = {};",
"goog.inherits = function(child, parent){};",
"/** @return {void} */",
"goog.nullFunction = function() {};",
"/** @type {!Function} */",
Expand Down

0 comments on commit 33bcadb

Please sign in to comment.