From 0ec55bc4f6774a8c639d3142da6fe177eeb7e517 Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Wed, 30 Nov 2016 17:15:23 -0800 Subject: [PATCH] Automated g4 rollback of changelist 140645131. *** Reason for rollback *** Breakages *** Original change description *** Record mismatch when casting to unrelated interface, to avoid wrong property disambiguation. *** ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=140673566 --- .../javascript/jscomp/AbstractCompiler.java | 7 ++ .../jscomp/AmbiguateProperties.java | 6 ++ .../google/javascript/jscomp/Compiler.java | 5 + .../jscomp/DisambiguateProperties.java | 4 + .../javascript/jscomp/TypeValidator.java | 46 +++++---- .../jscomp/DisambiguatePropertiesTest.java | 97 +++++++++---------- .../javascript/jscomp/IntegrationTest.java | 35 ------- 7 files changed, 96 insertions(+), 104 deletions(-) diff --git a/src/com/google/javascript/jscomp/AbstractCompiler.java b/src/com/google/javascript/jscomp/AbstractCompiler.java index 06c0ef9717f..9f02f620ab4 100644 --- a/src/com/google/javascript/jscomp/AbstractCompiler.java +++ b/src/com/google/javascript/jscomp/AbstractCompiler.java @@ -178,6 +178,13 @@ static enum MostRecentTypechecker { */ abstract Iterable getTypeMismatches(); + /** + * Gets all types that are used implicitly as a + * matching structural interface type. These are + * recorded as TypeMismatchs only for convenience + */ + abstract Iterable getImplicitInterfaceUses(); + /** * Used only by the new type inference */ diff --git a/src/com/google/javascript/jscomp/AmbiguateProperties.java b/src/com/google/javascript/jscomp/AmbiguateProperties.java index 3c32978066b..41ae6aa60d9 100644 --- a/src/com/google/javascript/jscomp/AmbiguateProperties.java +++ b/src/com/google/javascript/jscomp/AmbiguateProperties.java @@ -35,6 +35,7 @@ import com.google.javascript.rhino.jstype.JSTypeNative; import com.google.javascript.rhino.jstype.JSTypeRegistry; import com.google.javascript.rhino.jstype.ObjectType; + import java.util.ArrayList; import java.util.BitSet; import java.util.Comparator; @@ -147,6 +148,11 @@ public int compare(Property p1, Property p2) { addInvalidatingType(mis.typeB); } + for (TypeMismatch mis : compiler.getImplicitInterfaceUses()) { + addInvalidatingType(mis.typeA); + addInvalidatingType(mis.typeB); + } + externedNames = compiler.getExternProperties(); } diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index 7c72ee9bd2f..51e3c9a3a45 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -1419,6 +1419,11 @@ Iterable getTypeMismatches() { return getTypeValidator().getMismatches(); } + @Override + Iterable getImplicitInterfaceUses() { + return getTypeValidator().getImplicitStructuralInterfaceUses(); + } + @Override GlobalTypeInfo getSymbolTable() { if (this.symbolTable == null) { diff --git a/src/com/google/javascript/jscomp/DisambiguateProperties.java b/src/com/google/javascript/jscomp/DisambiguateProperties.java index 21cfdfc2efe..edf492d9ff0 100644 --- a/src/com/google/javascript/jscomp/DisambiguateProperties.java +++ b/src/com/google/javascript/jscomp/DisambiguateProperties.java @@ -359,6 +359,10 @@ public void process(Node externs, Node root) { recordInvalidatingType(mis.typeA, mis.src); recordInvalidatingType(mis.typeB, mis.src); } + for (TypeMismatch mis : compiler.getImplicitInterfaceUses()) { + recordInvalidatingType(mis.typeA, mis.src); + recordInvalidatingType(mis.typeB, mis.src); + } // Gather names of properties in externs; these properties can't be renamed. NodeTraversal.traverseEs6(compiler, externs, new FindExternProperties()); // Look at each unquoted property access and decide if that property will diff --git a/src/com/google/javascript/jscomp/TypeValidator.java b/src/com/google/javascript/jscomp/TypeValidator.java index ec9bd6f45f1..928b6f6ba01 100644 --- a/src/com/google/javascript/jscomp/TypeValidator.java +++ b/src/com/google/javascript/jscomp/TypeValidator.java @@ -75,6 +75,9 @@ class TypeValidator { // mismatches. For example, if we pass (Cake|null) where only Cake is // allowed, that doesn't mean we should invalidate all Cakes. private final List mismatches = new ArrayList<>(); + // the detection logic of this one is similar to this.mismatches + private final List implicitStructuralInterfaceUses = + new ArrayList<>(); // User warnings private static final String FOUND_REQUIRED = @@ -199,6 +202,15 @@ void setSubtypingMode(SubtypingMode mode) { this.subtypingMode = mode; } + /** + * all uses of implicitly implemented structural interfaces, + * captured during type validation and type checking + * (uses of explicitly @implemented structural interfaces are excluded) + */ + public Iterable getImplicitStructuralInterfaceUses() { + return implicitStructuralInterfaceUses; + } + // All non-private methods should have the form: // expectCondition(NodeTraversal t, Node n, ...); // If there is a mismatch, the {@code expect} method should issue @@ -354,7 +366,7 @@ void expectSwitchMatchesCase(NodeTraversal t, Node n, JSType switchType, && (caseType.autoboxesTo() == null || !caseType.autoboxesTo() .isSubtypeWithoutStructuralTyping(switchType))) { - recordImplicitInterfaceUses(caseType, switchType); + recordStructuralInterfaceUses(caseType, switchType); } } @@ -433,7 +445,7 @@ boolean expectCanAssignToPropertyOf(NodeTraversal t, Node n, JSType rightType, return false; } else if (!leftType.isNoType() && !rightType.isSubtypeWithoutStructuralTyping(leftType)){ - recordImplicitInterfaceUses(rightType, leftType); + recordStructuralInterfaceUses(rightType, leftType); } return true; } @@ -455,7 +467,7 @@ boolean expectCanAssignTo(NodeTraversal t, Node n, JSType rightType, mismatch(t, n, msg, rightType, leftType); return false; } else if (!rightType.isSubtypeWithoutStructuralTyping(leftType)) { - recordImplicitInterfaceUses(rightType, leftType); + recordStructuralInterfaceUses(rightType, leftType); } return true; } @@ -480,7 +492,7 @@ void expectArgumentMatchesParameter(NodeTraversal t, Node n, JSType argType, typeRegistry.getReadableTypeNameNoDeref(callNode.getFirstChild())), argType, paramType); } else if (!argType.isSubtypeWithoutStructuralTyping(paramType)){ - recordImplicitInterfaceUses(argType, paramType); + recordStructuralInterfaceUses(argType, paramType); } } @@ -526,15 +538,15 @@ void expectSuperType(NodeTraversal t, Node n, ObjectType superObject, * * @param t The node traversal. * @param n The node where warnings should point. - * @param targetType The type being cast to. - * @param sourceType The type being cast from. + * @param type The type being cast from. + * @param castType The type being cast to. */ - void expectCanCast(NodeTraversal t, Node n, JSType targetType, JSType sourceType) { - if (!sourceType.canCastTo(targetType)) { - registerMismatch(sourceType, targetType, report(t.makeError(n, INVALID_CAST, - sourceType.toString(), targetType.toString()))); - } else if (!sourceType.isSubtypeWithoutStructuralTyping(targetType)){ - recordImplicitInterfaceUses(sourceType, targetType); + void expectCanCast(NodeTraversal t, Node n, JSType castType, JSType type) { + if (!type.canCastTo(castType)) { + registerMismatch(type, castType, report(t.makeError(n, INVALID_CAST, + type.toString(), castType.toString()))); + } else if (!type.isSubtypeWithoutStructuralTyping(castType)){ + recordStructuralInterfaceUses(type, castType); } } @@ -761,15 +773,13 @@ private void mismatch(Node n, String msg, JSType found, JSType required) { } } - private void recordImplicitInterfaceUses(JSType found, JSType required) { - found = found.restrictByNotNullOrUndefined(); - required = required.restrictByNotNullOrUndefined(); + private void recordStructuralInterfaceUses(JSType found, JSType required) { boolean strictMismatch = !found.isSubtypeWithoutStructuralTyping(required) && !required.isSubtypeWithoutStructuralTyping(found); boolean mismatch = !found.isSubtype(required) && !required.isSubtype(found); - if (strictMismatch || mismatch) { - mismatches.add(new TypeMismatch(found, required, null)); + if (strictMismatch && !mismatch) { + implicitStructuralInterfaceUses.add(new TypeMismatch(found, required, null)); } } @@ -784,7 +794,7 @@ private void registerMismatch(JSType found, JSType required, JSError error) { !found.isSubtypeWithoutStructuralTyping(required) && !required.isSubtypeWithoutStructuralTyping(found); if (strictMismatch) { - mismatches.add(new TypeMismatch(found, required, error)); + implicitStructuralInterfaceUses.add(new TypeMismatch(found, required, error)); } return; } diff --git a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java index 559c014a503..22a51a68d9a 100644 --- a/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java +++ b/test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java @@ -1576,57 +1576,52 @@ public void testStructuralTypingWithDisambiguatePropertyRenaming1_2() { testSets(js, js, "{}"); } - // TODO(dimvar): Temporarily commenting these out to land a fix. - // These tests were passing by accident, they were not testing what they were supposed - // to. Disambiguation was backing off because of (I|null), not because of the - // implicit use of the record type. - -// public void testStructuralTypingWithDisambiguatePropertyRenaming1_3() { -// String js = LINE_JOINER.join( -// "/** @record */", -// "function I(){}", -// "/** @type {number} */", -// "I.prototype.x;", -// "", -// "/** @constructor @implements {I} */", -// "function Foo(){}", -// "/** @type {number} */", -// "Foo.prototype.x;", -// "", -// "/** @constructor */", -// "function Bar(){}", -// "/** @type {number} */", -// "Bar.prototype.x;", -// "", -// "function f(/** I */ i) { return i.x; }", -// "function g(/** {x:number} */ i) { return f(i); }", -// "g(new Bar());"); -// -// testSets(js, js, "{}"); -// } -// -// public void testStructuralTypingWithDisambiguatePropertyRenaming1_4() { -// String js = LINE_JOINER.join( -// "/** @record */", -// "function I(){}", -// "/** @type {number} */", -// "I.prototype.x;", -// "", -// "/** @constructor @implements {I} */", -// "function Foo(){}", -// "/** @type {number} */", -// "Foo.prototype.x;", -// "", -// "/** @constructor */", -// "function Bar(){}", -// "/** @type {number} */", -// "Bar.prototype.x;", -// "", -// "function f(/** !I */ i) { return i.x; }", -// "function g(/** {x:number} */ i) { return f(i); }", -// "g(new Bar());"); -// testSets(js, js, "{}"); -// } + public void testStructuralTypingWithDisambiguatePropertyRenaming1_3() { + String js = LINE_JOINER.join( + "/** @record */", + "function I(){}", + "/** @type {number} */", + "I.prototype.x;", + "", + "/** @constructor @implements {I} */", + "function Foo(){}", + "/** @type {number} */", + "Foo.prototype.x;", + "", + "/** @constructor */", + "function Bar(){}", + "/** @type {number} */", + "Bar.prototype.x;", + "", + "function f(/** I */ i) { return i.x; }", + "function g(/** {x:number} */ i) { return f(i); }", + "g(new Bar());"); + + testSets(js, js, "{}"); + } + + public void testStructuralTypingWithDisambiguatePropertyRenaming1_4() { + String js = LINE_JOINER.join( + "/** @record */", + "function I(){}", + "/** @type {number} */", + "I.prototype.x;", + "", + "/** @constructor @implements {I} */", + "function Foo(){}", + "/** @type {number} */", + "Foo.prototype.x;", + "", + "/** @constructor */", + "function Bar(){}", + "/** @type {number} */", + "Bar.prototype.x;", + "", + "function f(/** I */ i) { return i.x; }", + "function g(/** {x:number} */ i) { return f(i); }", + "g(new Bar());"); + testSets(js, js, "{}"); + } public void testStructuralTypingWithDisambiguatePropertyRenaming1_5() { String js = LINE_JOINER.join( diff --git a/test/com/google/javascript/jscomp/IntegrationTest.java b/test/com/google/javascript/jscomp/IntegrationTest.java index 070e1bef4a2..199b3f0424c 100644 --- a/test/com/google/javascript/jscomp/IntegrationTest.java +++ b/test/com/google/javascript/jscomp/IntegrationTest.java @@ -1195,41 +1195,6 @@ public void testDisambiguateProperties2() { "Bar.prototype.a=function(x){};")); } - public void testDisambiguatePropertiesClassCastedToUnrelatedInterface() { - CompilerOptions options = createCompilerOptions(); - options.setDisambiguateProperties(true); - options.setCheckTypes(true); - test(options, - LINE_JOINER.join( - "/** @interface */", - "function Foo() {}", - "Foo.prototype.prop1;", - "Foo.prototype.prop2;", - "/** @constructor */", - "function Bar() {", - " this.prop1 = 123;", - "}", - "var x = /** @type {Foo} */ (new Bar);", - "/** @constructor */", - "function Baz() {", - " this.prop1 = 123;", - "}"), - LINE_JOINER.join( - "/** @interface */", - "function Foo() {}", - "Foo.prototype.prop1;", - "Foo.prototype.prop2;", - "/** @constructor */", - "function Bar() {", - " this.prop1 = 123;", - "}", - "var x = new Bar;", - "/** @constructor */", - "function Baz() {", - " this.prop1 = 123;", - "}")); - } - public void testMarkPureCalls() { String testCode = "function foo() {} foo();"; CompilerOptions options = createCompilerOptions();