Skip to content

Commit

Permalink
Automated g4 rollback of changelist 140645131.
Browse files Browse the repository at this point in the history 
*** 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
  • Loading branch information
@blickly
tbreisacher authored and blickly committed Dec 1, 2016
1 parent 5abfb93 commit 0ec55bc
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 104 deletions.
7 changes: 7 additions & 0 deletions src/com/google/javascript/jscomp/AbstractCompiler.java
View file
Expand Up @@ -178,6 +178,13 @@ static enum MostRecentTypechecker {
*/
abstract Iterable<TypeMismatch> getTypeMismatches();

/**
* Gets all types that are used implicitly as a
* matching structural interface type. These are
* recorded as TypeMismatchs only for convenience
*/
abstract Iterable<TypeMismatch> getImplicitInterfaceUses();

/**
* Used only by the new type inference
*/
Expand Down
6 changes: 6 additions & 0 deletions src/com/google/javascript/jscomp/AmbiguateProperties.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}

Expand Down
5 changes: 5 additions & 0 deletions src/com/google/javascript/jscomp/Compiler.java
View file
Expand Up @@ -1419,6 +1419,11 @@ Iterable<TypeMismatch> getTypeMismatches() {
return getTypeValidator().getMismatches();
}

@Override
Iterable<TypeMismatch> getImplicitInterfaceUses() {
return getTypeValidator().getImplicitStructuralInterfaceUses();
}

@Override
GlobalTypeInfo getSymbolTable() {
if (this.symbolTable == null) {
Expand Down
4 changes: 4 additions & 0 deletions src/com/google/javascript/jscomp/DisambiguateProperties.java
View file
Expand Up @@ -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
Expand Down
46 changes: 28 additions & 18 deletions src/com/google/javascript/jscomp/TypeValidator.java
View file
Expand Up @@ -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<TypeMismatch> mismatches = new ArrayList<>();
// the detection logic of this one is similar to this.mismatches
private final List<TypeMismatch> implicitStructuralInterfaceUses =
new ArrayList<>();

// User warnings
private static final String FOUND_REQUIRED =
Expand Down Expand Up @@ -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<TypeMismatch> 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
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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));
}
}

Expand All @@ -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;
}
Expand Down
97 changes: 46 additions & 51 deletions test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
View file
Expand Up @@ -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(
Expand Down
35 changes: 0 additions & 35 deletions test/com/google/javascript/jscomp/IntegrationTest.java
View file
Expand Up @@ -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();
Expand Down

0 comments on commit 0ec55bc

Please sign in to comment.