diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 8401e756b45..f3ae4f38212 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -1846,11 +1846,21 @@ void maybeDeclareQualifiedName(NodeTraversal t, JSDocInfo info, if (ownerType != null) { // Only declare this as an official property if it has not been // declared yet. - boolean isExtern = t.getInput() != null && t.getInput().isExtern(); - if ((!ownerType.hasOwnProperty(propName) || ownerType.isPropertyTypeInferred(propName)) - && ((isExtern && !ownerType.isNativeObjectType()) || !ownerType.isInstanceType())) { - // If the property is undeclared or inferred, declare it now. - ownerType.defineDeclaredProperty(propName, valueType, n); + if (!ownerType.hasOwnProperty(propName) || ownerType.isPropertyTypeInferred(propName)) { + // Define the property if any of the following are true: + // (1) it's a non-native extern type. Native types are excluded here because we don't + // want externs of the form "/** @type {!Object} */ var api = {}; api.foo;" to + // cause a property "foo" to be declared on Object. + // (2) it's a non-instance type. This primarily covers static properties on + // constructors (which are FunctionTypes, not InstanceTypes). + // (3) it's an assignment to 'this', which covers instance properties assigned in + // constructors or other methods. + boolean isNonNativeExtern = + t.getInput() != null && t.getInput().isExtern() && !ownerType.isNativeObjectType(); + if (isNonNativeExtern || !ownerType.isInstanceType() || ownerNode.isThis()) { + // If the property is undeclared or inferred, declare it now. + ownerType.defineDeclaredProperty(propName, valueType, n); + } } } @@ -2098,14 +2108,11 @@ private final class GlobalScopeBuilder extends AbstractScopeBuilder { * local variables. */ private final class LocalScopeBuilder extends AbstractScopeBuilder { - private final ObjectType thisTypeForProperties; - /** * @param scope The scope that we're building. */ private LocalScopeBuilder(TypedScope scope) { super(scope); - thisTypeForProperties = getThisTypeForCollectingProperties(); } /** @@ -2126,59 +2133,9 @@ private LocalScopeBuilder(TypedScope scope) { return; } - // Gather the properties declared in the function, - // if that function has a @this type that we can - // build properties on. - // TODO(nick): It's not clear to me why this is necessary; - // it appears to be papering over bugs in the main analyzer. - if (thisTypeForProperties != null && n.getParent().isExprResult()) { - if (n.isAssign()) { - maybeCollectMember(n.getFirstChild(), n, n.getLastChild()); - } else if (n.isGetProp()) { - maybeCollectMember(n, n, null); - } - } - super.visit(t, n, parent); } - private ObjectType getThisTypeForCollectingProperties() { - Node rootNode = currentHoistScope.getClosestContainerScope().getRootNode(); - if (rootNode.isFromExterns()) { - return null; - } - - JSType type = rootNode.getJSType(); - if (type == null || !type.isFunctionType()) { - return null; - } - - FunctionType fnType = type.toMaybeFunctionType(); - JSType fnThisType = fnType.getTypeOfThis(); - return fnThisType == null ? null : fnThisType.toObjectType(); - } - - private void maybeCollectMember(Node member, - Node nodeWithJsDocInfo, @Nullable Node value) { - JSDocInfo info = nodeWithJsDocInfo.getJSDocInfo(); - - // Do nothing if there is no JSDoc type info, or - // if the node is not a member expression, or - // if the member expression is not of the form: this.someProperty. - if (info == null || !member.isGetProp() || !member.getFirstChild().isThis()) { - return; - } - - JSType jsType = getDeclaredType(info, member, value); - Node name = member.getLastChild(); - if (jsType != null) { - thisTypeForProperties.defineDeclaredProperty( - name.getString(), - jsType, - member); - } - } - /** Handle bleeding functions and function parameters. */ private void handleFunctionInputs(Node fnNode) { // Handle bleeding functions. diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 41aff881afa..fc456329f3d 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -1143,15 +1143,20 @@ public void testStubsInExterns4() { } public void testPropertyInExterns1() { + // Declaring a property on a non-native extern type (e.g. 'Extern') declares it as a property + // on the instance type, but only in externs. testSame( externs( - "/** @constructor */ function Extern() {}" - + "/** @type {Extern} */ var extern;" - + "/** @return {number} */ extern.one;"), + lines( + "/** @constructor */ function Extern() {}", + "/** @type {Extern} */ var extern;", + "/** @return {number} */ extern.one;")), srcs( - "/** @constructor */ function Normal() {}" - + "/** @type {Normal} */ var normal;" - + "/** @return {number} */ normal.one;")); + lines( + "/** @constructor */ function Normal() {}", + "/** @type {Normal} */ var normal;", + "/** @return {number} */ normal.one;", + "var result = new Extern().one();"))); JSType e = globalScope.getVar("Extern").getType(); ObjectType externInstance = ((FunctionType) e).getInstanceType(); @@ -1159,6 +1164,7 @@ public void testPropertyInExterns1() { assertTrue(externInstance.isPropertyTypeDeclared("one")); assertEquals("function(): number", externInstance.getPropertyType("one").toString()); + assertEquals("number", globalScope.getVar("result").getType().toString()); JSType n = globalScope.getVar("Normal").getType(); ObjectType normalInstance = ((FunctionType) n).getInstanceType(); @@ -1166,12 +1172,26 @@ public void testPropertyInExterns1() { } public void testPropertyInExterns2() { + // Native extern types (such as Object) do not get stray properties declared, since this would + // cause problems with bad externs (such as `/** @type {!Object} */ var api = {}; api.foo;`, + // where we don't want to declare that all Objects have a "foo" property. Nevertheless, the + // specific qualified name (i.e. extern.one, in the example below) is still declared on the + // global scope, so referring to the "one" property specifically on "extern" is still checked + // as one would expect. testSame( - externs("/** @type {Object} */ var extern;" + "/** @return {number} */ extern.one;"), - srcs("/** @type {Object} */ var normal;" + "/** @return {number} */ normal.one;")); + externs( + lines( + "/** @type {Object} */ var extern;", // + "/** @return {number} */ extern.one;")), + srcs( + lines( + "/** @type {Object} */ var normal;", // + "/** @return {number} */ normal.one;", + "var result = extern.one();"))); JSType e = globalScope.getVar("extern").getType(); assertFalse(e.dereference().hasOwnProperty("one")); + assertEquals("number", globalScope.getVar("result").getType().toString()); JSType normal = globalScope.getVar("normal").getType(); assertFalse(normal.dereference().hasOwnProperty("one"));