From d6f6b103e0334102e2887e6b467a3e3c035fdc85 Mon Sep 17 00:00:00 2001 From: dimvar Date: Thu, 23 Feb 2017 11:06:16 -0800 Subject: [PATCH] [NTI] When a class inherits a method from a grandparent interface instead of a parent one, be sure to instantiate generics correctly. Also, delete some unused code in DeclaredFunctionType. Fixes diamond-inheritance issue #2242 on github. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=148365005 --- .../javascript/jscomp/GlobalTypeInfo.java | 30 +++- .../jscomp/newtypes/DeclaredFunctionType.java | 45 ++---- .../jscomp/NewTypeInferenceTest.java | 145 ++++++++++++++++++ 3 files changed, 180 insertions(+), 40 deletions(-) diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfo.java b/src/com/google/javascript/jscomp/GlobalTypeInfo.java index 9c69a12123c..e85008e15d5 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfo.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfo.java @@ -516,15 +516,15 @@ public void process(Node externs, Node root) { this.compiler.setExternProperties(ImmutableSet.copyOf(this.externPropertyNames)); } - private Collection getPropDefsFromInterface( - NominalType nominalType, String pname) { + private Collection getPropDefsFromInterface(NominalType nominalType, String pname) { Preconditions.checkArgument(nominalType.isFinalized()); - Preconditions.checkArgument( - nominalType.isInterface() || nominalType.isBuiltinObject()); + Preconditions.checkArgument(nominalType.isInterface() || nominalType.isBuiltinObject()); if (nominalType.getPropDeclaredType(pname) == null) { return ImmutableSet.of(); } else if (propertyDefs.get(nominalType.getId(), pname) != null) { - return ImmutableSet.of(propertyDefs.get(nominalType.getId(), pname)); + PropertyDef propDef = propertyDefs.get(nominalType.getId(), pname); + return ImmutableSet.of( + nominalType.isGeneric() ? propDef.substituteNominalGenerics(nominalType) : propDef); } ImmutableSet.Builder result = ImmutableSet.builder(); for (NominalType interf : nominalType.getInstantiatedInterfaces()) { @@ -539,7 +539,8 @@ private PropertyDef getPropDefFromClass(NominalType nominalType, String pname) { Preconditions.checkArgument(nominalType.isClass()); if (propertyDefs.get(nominalType.getId(), pname) != null) { - return propertyDefs.get(nominalType.getId(), pname); + PropertyDef propDef = propertyDefs.get(nominalType.getId(), pname); + return nominalType.isGeneric() ? propDef.substituteNominalGenerics(nominalType) : propDef; } nominalType = nominalType.getInstantiatedSuperclass(); } @@ -749,8 +750,7 @@ private void checkSuperProperty( // If we are looking at a method definition, munging may be needed for (PropertyDef inheritedPropDef : inheritedPropDefs) { if (inheritedPropDef.methodType != null) { - propMethodTypesToProcess.put(pname, - inheritedPropDef.methodType.substituteNominalGenerics(superType)); + propMethodTypesToProcess.put(pname, inheritedPropDef.methodType); } } } @@ -2710,12 +2710,26 @@ private static class PropertyDef { this.methodScope = methodScope; } + PropertyDef substituteNominalGenerics(NominalType nt) { + Preconditions.checkArgument(nt.isGeneric(), nt); + if (this.methodType == null) { + return this; + } + return new PropertyDef( + this.defSite, this.methodType.substituteNominalGenerics(nt), this.methodScope); + } + void updateMethodType(DeclaredFunctionType updatedType) { this.methodType = updatedType; if (this.methodScope != null) { this.methodScope.setDeclaredType(updatedType); } } + + @Override + public String toString() { + return "PropertyDef(" + defSite + ", " + methodType + ")"; + } } @Override diff --git a/src/com/google/javascript/jscomp/newtypes/DeclaredFunctionType.java b/src/com/google/javascript/jscomp/newtypes/DeclaredFunctionType.java index 759dc2fd182..be434791139 100644 --- a/src/com/google/javascript/jscomp/newtypes/DeclaredFunctionType.java +++ b/src/com/google/javascript/jscomp/newtypes/DeclaredFunctionType.java @@ -18,7 +18,6 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -287,35 +286,23 @@ public DeclaredFunctionType substituteNominalGenerics(NominalType nt) { } Map typeMap = nt.getTypeMap(); Preconditions.checkState(!typeMap.isEmpty()); - Map reducedMap = typeMap; - boolean foundShadowedTypeParam = false; + // Before switching to unique generated names for type variables, a method's type variables + // could shadow type variables defined on the class. Check that it no longer happens. for (String typeParam : typeParameters) { - if (typeMap.containsKey(typeParam)) { - foundShadowedTypeParam = true; - break; - } - } - if (foundShadowedTypeParam) { - ImmutableMap.Builder builder = ImmutableMap.builder(); - for (Map.Entry entry : typeMap.entrySet()) { - if (!typeParameters.contains(entry.getKey())) { - builder.put(entry); - } - } - reducedMap = builder.build(); + Preconditions.checkState(!typeMap.containsKey(typeParam)); } FunctionTypeBuilder builder = new FunctionTypeBuilder(this.commonTypes); for (JSType reqFormal : requiredFormals) { - builder.addReqFormal(reqFormal == null ? null : reqFormal.substituteGenerics(reducedMap)); + builder.addReqFormal(reqFormal == null ? null : reqFormal.substituteGenerics(typeMap)); } for (JSType optFormal : optionalFormals) { - builder.addOptFormal(optFormal == null ? null : optFormal.substituteGenerics(reducedMap)); + builder.addOptFormal(optFormal == null ? null : optFormal.substituteGenerics(typeMap)); } if (restFormals != null) { - builder.addRestFormals(restFormals.substituteGenerics(reducedMap)); + builder.addRestFormals(restFormals.substituteGenerics(typeMap)); } if (returnType != null) { - builder.addRetType(returnType.substituteGenerics(reducedMap)); + builder.addRetType(returnType.substituteGenerics(typeMap)); } // Explicitly forget nominalType and receiverType. This method is used when // calculating the declared type of a method using the inherited types. @@ -324,8 +311,7 @@ public DeclaredFunctionType substituteNominalGenerics(NominalType nt) { return builder.buildDeclaration(); } - public static DeclaredFunctionType meet( - Collection toMeet) { + public static DeclaredFunctionType meet(Collection toMeet) { DeclaredFunctionType result = null; for (DeclaredFunctionType declType : toMeet) { if (result == null) { @@ -337,29 +323,24 @@ public static DeclaredFunctionType meet( return result; } - private static DeclaredFunctionType meet( - DeclaredFunctionType f1, DeclaredFunctionType f2) { + private static DeclaredFunctionType meet(DeclaredFunctionType f1, DeclaredFunctionType f2) { if (f1.equals(f2)) { return f1; } JSTypes commonTypes = f1.commonTypes; FunctionTypeBuilder builder = new FunctionTypeBuilder(f1.commonTypes); - int minRequiredArity = Math.min( - f1.requiredFormals.size(), f2.requiredFormals.size()); + int minRequiredArity = Math.min(f1.requiredFormals.size(), f2.requiredFormals.size()); for (int i = 0; i < minRequiredArity; i++) { - builder.addReqFormal(nullAcceptingJoin( - f1.getFormalType(i), f2.getFormalType(i))); + builder.addReqFormal(nullAcceptingJoin(f1.getFormalType(i), f2.getFormalType(i))); } int maxTotalArity = Math.max( f1.requiredFormals.size() + f1.optionalFormals.size(), f2.requiredFormals.size() + f2.optionalFormals.size()); for (int i = minRequiredArity; i < maxTotalArity; i++) { - builder.addOptFormal(nullAcceptingJoin( - f1.getFormalType(i), f2.getFormalType(i))); + builder.addOptFormal(nullAcceptingJoin(f1.getFormalType(i), f2.getFormalType(i))); } if (f1.restFormals != null || f2.restFormals != null) { - builder.addRestFormals( - nullAcceptingJoin(f1.restFormals, f2.restFormals)); + builder.addRestFormals(nullAcceptingJoin(f1.restFormals, f2.restFormals)); } JSType retType = nullAcceptingMeet(f1.returnType, f2.returnType); if (commonTypes.BOTTOM.equals(retType)) { diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index b0134283683..a65a06f8353 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -19167,4 +19167,149 @@ public void testDontSpecializeTopInInstanceofElseBranch() { "}"), NewTypeInference.MISTYPED_ASSIGN_RHS); } + + public void testMethodInheritedFromGrandparentType() { + typeCheck(LINE_JOINER.join( + "/**", + " * @interface", + " * @template T", + " */", + "function Foo() {}", + "/** @return {T} */", + "Foo.prototype.foo = function() {};", + "/**", + " * @interface", + " * @extends {Foo}", + " * @template T", + " */", + "function Bar() {}", + "/**", + " * @constructor", + " * @implements {Bar}", + " * @param {T} x", + " * @template T", + " */", + "function Baz(x) {", + " this.foo_ = x;", + "};", + "Baz.prototype.foo = function() { return this.foo_; };", + "function f(/** !Baz */ x) {", + " var /** number */ n = x.foo();", + "}")); + + typeCheck(LINE_JOINER.join( + "/**", + " * @interface", + " * @template T", + " */", + "function Foo() {}", + "/** @return {T} */", + "Foo.prototype.foo = function() {};", + "/**", + " * @interface", + " * @extends {Foo}", + " * @template T", + " */", + "function Bar() {}", + "/**", + " * @constructor", + " * @implements {Bar}", + " * @param {T} x", + " * @template T", + " */", + "function Baz(x) {", + " this.foo_ = x;", + "};", + "Baz.prototype.foo = function() { return this.foo_; };", + "function f(/** !Baz */ x) {", + " var /** string */ n = x.foo();", + "}"), + NewTypeInference.MISTYPED_ASSIGN_RHS); + + typeCheck(LINE_JOINER.join( + "/**", + " * @constructor", + " * @template T", + " */", + "function Foo() { this.prop_; }", + "/** @return {T} */", + "Foo.prototype.method = function() { return this.prop_; }", + "/**", + " * @constructor", + " * @extends {Foo}", + " * @template T", + " */", + "function Bar() {}", + "/**", + " * @constructor", + " * @extends {Bar}", + " * @template T", + " */", + "function Baz() {}", + "function f(/** !Baz */ x) {", + " var /** number */ n = x.method();", + "}")); + + typeCheck(LINE_JOINER.join( + "/**", + " * @constructor", + " * @template T", + " */", + "function Foo() { this.prop_; }", + "/** @return {T} */", + "Foo.prototype.method = function() { return this.prop_; }", + "/**", + " * @constructor", + " * @extends {Foo}", + " * @template T", + " */", + "function Bar() {}", + "/**", + " * @constructor", + " * @extends {Bar}", + " * @template T", + " */", + "function Baz() {}", + "function f(/** !Baz */ x) {", + " var /** string */ n = x.method();", + "}"), + NewTypeInference.MISTYPED_ASSIGN_RHS); + } + + public void testDiamondInheritance() { + typeCheck(LINE_JOINER.join( + "/**", + " * @interface", + " * @template T", + " */", + "function Foo() {}", + "/** @return {T} */", + "Foo.prototype.foo = function() {};", + "/**", + " * @interface", + " * @extends {Foo}", + " * @template T", + " */", + "function Bar() {}", + "/**", + " * @constructor", + " * @abstract", + " * @implements {Foo}", + " * @template T", + " */", + "function AbstractBaz() {}", + "/** @abstract */", + "AbstractBaz.prototype.foo = function() {};", + "/**", + " * @constructor", + " * @implements {Bar}", + " * @extends {AbstractBaz}", + " * @param {T} x", + " * @template T", + " */", + "function Baz(x) {", + " this.foo_ = x;", + "};", + "Baz.prototype.foo = function() { return this.foo_; };")); + } }