Skip to content

Commit

Permalink
Record mismatch when casting to unrelated interface, to avoid wrong p…
Browse files Browse the repository at this point in the history 
…roperty

disambiguation. Don't back off for null/undefined and for different instantiations of the same generic type.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140748384
  • Loading branch information
@dimvar @blickly
dimvar authored and blickly committed Dec 2, 2016
1 parent bfb32f0 commit 4262859
Show file tree
Hide file tree
Showing 4 changed files with 146 additions and 72 deletions.
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/AbstractCompiler.java
View file
Expand Up @@ -180,7 +180,7 @@ static enum MostRecentTypechecker {

/**
* Gets all types that are used implicitly as a
* matching structural interface type. These are
* matching interface type. These are
* recorded as TypeMismatchs only for convenience
*/
abstract Iterable<TypeMismatch> getImplicitInterfaceUses();
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/Compiler.java
View file
Expand Up @@ -1421,7 +1421,7 @@ Iterable<TypeMismatch> getTypeMismatches() {

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

@Override
Expand Down
61 changes: 37 additions & 24 deletions src/com/google/javascript/jscomp/TypeValidator.java
View file
Expand Up @@ -76,8 +76,7 @@ class TypeValidator {
// 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<>();
private final List<TypeMismatch> implicitInterfaceUses = new ArrayList<>();

// User warnings
private static final String FOUND_REQUIRED =
Expand Down Expand Up @@ -203,12 +202,12 @@ void setSubtypingMode(SubtypingMode mode) {
}

/**
* all uses of implicitly implemented structural interfaces,
* all uses of implicitly implemented interfaces,
* captured during type validation and type checking
* (uses of explicitly @implemented structural interfaces are excluded)
*/
public Iterable<TypeMismatch> getImplicitStructuralInterfaceUses() {
return implicitStructuralInterfaceUses;
public Iterable<TypeMismatch> getImplicitInterfaceUses() {
return implicitInterfaceUses;
}

// All non-private methods should have the form:
Expand Down Expand Up @@ -366,7 +365,7 @@ void expectSwitchMatchesCase(NodeTraversal t, Node n, JSType switchType,
&& (caseType.autoboxesTo() == null
|| !caseType.autoboxesTo()
.isSubtypeWithoutStructuralTyping(switchType))) {
recordStructuralInterfaceUses(caseType, switchType);
recordImplicitInterfaceUses(n, caseType, switchType);
}
}

Expand Down Expand Up @@ -445,7 +444,7 @@ boolean expectCanAssignToPropertyOf(NodeTraversal t, Node n, JSType rightType,
return false;
} else if (!leftType.isNoType()
&& !rightType.isSubtypeWithoutStructuralTyping(leftType)){
recordStructuralInterfaceUses(rightType, leftType);
recordImplicitInterfaceUses(n, rightType, leftType);
}
return true;
}
Expand All @@ -467,7 +466,7 @@ boolean expectCanAssignTo(NodeTraversal t, Node n, JSType rightType,
mismatch(t, n, msg, rightType, leftType);
return false;
} else if (!rightType.isSubtypeWithoutStructuralTyping(leftType)) {
recordStructuralInterfaceUses(rightType, leftType);
recordImplicitInterfaceUses(n, rightType, leftType);
}
return true;
}
Expand All @@ -492,7 +491,7 @@ void expectArgumentMatchesParameter(NodeTraversal t, Node n, JSType argType,
typeRegistry.getReadableTypeNameNoDeref(callNode.getFirstChild())),
argType, paramType);
} else if (!argType.isSubtypeWithoutStructuralTyping(paramType)){
recordStructuralInterfaceUses(argType, paramType);
recordImplicitInterfaceUses(n, argType, paramType);
}
}

Expand Down Expand Up @@ -538,15 +537,15 @@ void expectSuperType(NodeTraversal t, Node n, ObjectType superObject,
*
* @param t The node traversal.
* @param n The node where warnings should point.
* @param type The type being cast from.
* @param castType The type being cast to.
* @param targetType The type being cast to.
* @param sourceType The type being cast from.
*/
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);
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(n, sourceType, targetType);
}
}

Expand Down Expand Up @@ -773,13 +772,27 @@ private void mismatch(Node n, String msg, JSType found, JSType required) {
}
}

private void recordStructuralInterfaceUses(JSType found, JSType required) {
private JSType removeNullUndefinedAndTemplates(JSType t) {
JSType result = t.restrictByNotNullOrUndefined();
if (result.isTemplatizedType()) {
return result.toMaybeTemplatizedType().getReferencedType();
}
return result;
}

private void recordImplicitInterfaceUses(Node src, JSType sourceType, JSType targetType) {
sourceType = removeNullUndefinedAndTemplates(sourceType);
targetType = removeNullUndefinedAndTemplates(targetType);
boolean strictMismatch =
!found.isSubtypeWithoutStructuralTyping(required)
&& !required.isSubtypeWithoutStructuralTyping(found);
boolean mismatch = !found.isSubtype(required) && !required.isSubtype(found);
if (strictMismatch && !mismatch) {
implicitStructuralInterfaceUses.add(new TypeMismatch(found, required, null));
!sourceType.isSubtypeWithoutStructuralTyping(targetType)
&& !targetType.isSubtypeWithoutStructuralTyping(sourceType);
boolean mismatch = !sourceType.isSubtype(targetType) && !targetType.isSubtype(sourceType);
if (strictMismatch || mismatch) {
// We don't report a type error, but we still need to construct a JSError,
// for people who enable the invalidation diagnostics in DisambiguateProperties.
String msg = "Implicit use of type " + sourceType + " as " + targetType;
JSError err = JSError.make(src, TYPE_MISMATCH_WARNING, msg);
implicitInterfaceUses.add(new TypeMismatch(sourceType, targetType, err));
}
}

Expand All @@ -794,7 +807,7 @@ private void registerMismatch(JSType found, JSType required, JSError error) {
!found.isSubtypeWithoutStructuralTyping(required)
&& !required.isSubtypeWithoutStructuralTyping(found);
if (strictMismatch) {
implicitStructuralInterfaceUses.add(new TypeMismatch(found, required, error));
implicitInterfaceUses.add(new TypeMismatch(found, required, error));
}
return;
}
Expand Down
153 changes: 107 additions & 46 deletions test/com/google/javascript/jscomp/DisambiguatePropertiesTest.java
View file
Expand Up @@ -1576,52 +1576,57 @@ public void testStructuralTypingWithDisambiguatePropertyRenaming1_2() {
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, "{}");
}
// 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. This is fixed by cl/140660035.

// 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 Expand Up @@ -1680,6 +1685,62 @@ public void testStructuralTypingWithDisambiguatePropertyRenaming1_7() throws Exc
testSets(js, js, "{}");
}

public void testDisambiguatePropertiesClassCastedToUnrelatedInterface() {
String js = 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;",
"}");

testSets(js, js, "{}");
}

public void testDontInvalidateForGenericsMismatch() {
testSets(
LINE_JOINER.join(
"/**",
" * @constructor",
" * @template T",
" */",
"function Foo() {",
" this.prop = 123;",
"}",
"/** @param {!Foo<number>} x */",
"function f(x) {",
" return (/** @type {!Foo<string>} */ (x)).prop;",
"}",
"/** @constructor */",
"function Bar() {",
" this.prop = 123;",
"}"),
LINE_JOINER.join(
"/**",
" * @constructor",
" * @template T",
" */",
"function Foo() {",
" this.Foo$prop = 123;",
"}",
"/** @param {!Foo<number>} x */",
"function f(x) {",
" return (/** @type {!Foo<string>} */ (x)).Foo$prop;",
"}",
"/** @constructor */",
"function Bar() {",
" this.Bar$prop = 123;",
"}"),
"{prop=[[Bar], [Foo]]}");
}

public void testStructuralTypingWithDisambiguatePropertyRenaming2() {
String js = LINE_JOINER.join(
"/** @record */",
Expand Down

0 comments on commit 4262859

Please sign in to comment.