Skip to content

Commit

Permalink
Tighten strict missing properties for type "Object"
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=184187706
  • Loading branch information
@concavelenz @lauraharker
concavelenz authored and lauraharker committed Feb 1, 2018
1 parent e9152d8 commit 12331f1
Show file tree
Hide file tree
Showing 2 changed files with 174 additions and 84 deletions.
103 changes: 54 additions & 49 deletions src/com/google/javascript/jscomp/TypeCheck.java
View file
Expand Up @@ -1540,57 +1540,62 @@ private boolean isQNameAssignmentTarget(Node n) {
private void checkPropertyAccessHelper( private void checkPropertyAccessHelper(
JSType objectType, String propName, NodeTraversal t, Node n) { JSType objectType, String propName, NodeTraversal t, Node n) {
boolean isStruct = objectType.isStruct(); boolean isStruct = objectType.isStruct();
if (!reportMissingProperties
|| objectType.isEmptyType()
|| (!isStruct && allowLoosePropertyAccessOnNode(n))) {
return;
}
PropDefinitionKind kind = typeRegistry.canPropertyBeDefined(objectType, propName);
if (kind.equals(PropDefinitionKind.KNOWN)) {
return;
}
// If the property definition is known, but only loosely associated,
// only report a "strict error" which can be optional as code is migrated.
boolean isLooselyAssociated = kind.equals(PropDefinitionKind.LOOSE);
boolean isUnknownType = objectType.isUnknownType();
if (isLooselyAssociated && isUnknownType) {
// We still don't want to report this.
return;
}
boolean isObjectType = objectType.isEquivalentTo(getNativeType(OBJECT_TYPE));
boolean lowConfidence = isUnknownType || isObjectType;
boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct; boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct;
if (!objectType.isEmptyType() && reportMissingProperties // Traditionally, we would not report a warning for "loose" properties, but we want to be
&& (!allowLoosePropertyAccessOnNode(n) || isStruct)) { // able to be more strict, so introduce an optional warning.
PropDefinitionKind kind = typeRegistry.canPropertyBeDefined(objectType, propName); boolean strictReport = isLooselyAssociated || loosePropertyDeclaration;
if (!kind.equals(PropDefinitionKind.KNOWN)) { SuggestionPair pair = null;
// If the property definition is known, but only loosely associated, if (!lowConfidence) {
// only report a "strict error" which can be optional as code is migrated. pair = getClosestPropertySuggestion(objectType, propName);
boolean strict = kind.equals(PropDefinitionKind.LOOSE) || loosePropertyDeclaration; }
boolean lowConfidence = if (pair != null && pair.distance * 4 < propName.length()) {
objectType.isUnknownType() || objectType.isEquivalentTo(getNativeType(OBJECT_TYPE)); DiagnosticType reportType;
SuggestionPair pair = null; if (strictReport) {
if (lowConfidence) { reportType = STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION;
if (strict) { } else {
return; reportType = INEXISTENT_PROPERTY_WITH_SUGGESTION;
} }
} else { report(
pair = getClosestPropertySuggestion(objectType, propName); t,
// Traditionally, we would not report a warning for "loose" properties, but we want to be n.getLastChild(),
// able to be more strict, so introduce an optional warning. reportType,
} propName,
if (pair != null && pair.distance * 4 < propName.length()) { typeRegistry.getReadableTypeName(n.getFirstChild()),
DiagnosticType reportType; pair.suggestion);
if (strict) { } else {
reportType = STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION; DiagnosticType reportType;
} else { if (strictReport) {
reportType = INEXISTENT_PROPERTY_WITH_SUGGESTION; reportType = STRICT_INEXISTENT_PROPERTY;
} } else if (lowConfidence) {
report( reportType = POSSIBLE_INEXISTENT_PROPERTY;
t, } else {
n.getLastChild(), reportType = INEXISTENT_PROPERTY;
reportType,
propName,
typeRegistry.getReadableTypeName(n.getFirstChild()),
pair.suggestion);
} else {
DiagnosticType reportType;
if (lowConfidence) {
reportType = POSSIBLE_INEXISTENT_PROPERTY;
} else if (strict) {
reportType = STRICT_INEXISTENT_PROPERTY;
} else {
reportType = INEXISTENT_PROPERTY;
}
report(
t,
n.getLastChild(),
reportType,
propName,
typeRegistry.getReadableTypeName(n.getFirstChild()));
}
} }
report(
t,
n.getLastChild(),
reportType,
propName,
typeRegistry.getReadableTypeName(n.getFirstChild()));
} }
} }


Expand Down
155 changes: 120 additions & 35 deletions test/com/google/javascript/jscomp/TypeCheckTest.java
View file
Expand Up @@ -2257,16 +2257,37 @@ public void testFunctionInference22() {
"/** @param {boolean} x */ var g = function(x) {};"); "/** @param {boolean} x */ var g = function(x) {};");
} }


public void testFunctionInference23() { public void testFunctionInference23a() {
// We want to make sure that 'prop' isn't declared on all objects. // We want to make sure that 'prop' isn't declared on all objects.

// This test is specifically checking loose property check behavior.
compiler.getOptions().setWarningLevel(
DiagnosticGroups.STRICT_MISSING_PROPERTIES, CheckLevel.OFF);

testTypes( testTypes(
"/** @type {!Function} */ var f = function() {\n" + lines(
" /** @type {number} */ this.prop = 3;\n" + "/** @type {!Function} */ var f = function() {",
"};" + " /** @type {number} */ this.prop = 3;",
"/**\n" + "};",
" * @param {Object} x\n" + "/**",
" * @return {string}\n" + " * @param {Object} x",
" */ var g = function(x) { return x.prop; };"); " * @return {string}",
" */ var g = function(x) { return x.prop; };"));
}

public void testFunctionInference23b() {
// We want to make sure that 'prop' isn't declared on all objects.

testTypes(
lines(
"/** @type {!Function} */ var f = function() {",
" /** @type {number} */ this.prop = 3;",
"};",
"/**",
" * @param {Object} x",
" * @return {string}",
" */ var g = function(x) { return x.prop; };"),
"Property prop never defined on Object");
} }


public void testInnerFunction1() { public void testInnerFunction1() {
Expand Down Expand Up @@ -9206,6 +9227,11 @@ public void testCast15() {
// while because of the cast, it could have any type (including // while because of the cast, it could have any type (including
// a union). // a union).
compiler.getOptions().setLanguageIn(CompilerOptions.LanguageMode.ECMASCRIPT_2015); compiler.getOptions().setLanguageIn(CompilerOptions.LanguageMode.ECMASCRIPT_2015);

// This test is specifically checking loose property check behavior.
compiler.getOptions().setWarningLevel(
DiagnosticGroups.STRICT_MISSING_PROPERTIES, CheckLevel.OFF);

testTypes( testTypes(
"for (var i = 0; i < 10; i++) {" + "for (var i = 0; i < 10; i++) {" +
"var x = /** @type {Object|number} */ ({foo: 3});" + "var x = /** @type {Object|number} */ ({foo: 3});" +
Expand All @@ -9216,6 +9242,26 @@ public void testCast15() {
"Property foo never defined on Array"); "Property foo never defined on Array");
} }


public void testCast15b() {
// This fixes a bug where a type cast on an object literal
// would cause a run-time cast exception if the node was visited
// more than once.
//
// Some code assumes that an object literal must have a object type,
// while because of the cast, it could have any type (including
// a union).
compiler.getOptions().setLanguageIn(CompilerOptions.LanguageMode.ECMASCRIPT_2015);
testTypes(
lines(
"for (var i = 0; i < 10; i++) {",
"var x = /** @type {{foo:number}}|number} */ ({foo: 3});",
"/** @param {number} x */ function f(x) {}",
"f(x.foo);",
"f([].foo);",
"}"),
"Property foo never defined on Array");
}

public void testCast16() { public void testCast16() {
// A type cast should not invalidate the checks on the members // A type cast should not invalidate the checks on the members
testTypes( testTypes(
Expand Down Expand Up @@ -9808,17 +9854,33 @@ public void testUnknownConstructorInstanceType3() {
testTypes("function g(f) { var x = new f(); x.a = 1; return x; }"); testTypes("function g(f) { var x = new f(); x.a = 1; return x; }");
} }


public void testUnknownPrototypeChain() { public void testUnknownPrototypeChain1() {
testTypes("/**\n" + testTypes(
"* @param {Object} co\n" + lines("/**",
" * @return {Object}\n" + "* @param {Object} co",
" */\n" + " * @return {Object}",
"function inst(co) {\n" + " */",
" /** @constructor */\n" + "function inst(co) {",
" var c = function() {};\n" + " /** @constructor */",
" c.prototype = co.prototype;\n" + " var c = function() {};",
" return new c;\n" + " c.prototype = co.prototype;",
"}"); " return new c;",
"}"),
"Property prototype never defined on Object");
}

public void testUnknownPrototypeChain2() {
testTypes(
lines("/**",
" * @param {Function} co",
" * @return {Object}",
" */",
"function inst(co) {",
" /** @constructor */",
" var c = function() {};",
" c.prototype = co.prototype;",
" return new c;",
"}"));
} }


public void testNamespacedConstructor() { public void testNamespacedConstructor() {
Expand Down Expand Up @@ -12003,7 +12065,8 @@ public void testMissingProperty30() {
" return {};" + " return {};" +
"}" + "}" +
"f().a = 3;" + "f().a = 3;" +
"/** @param {Object} y */ function g(y) { return y.a; }"); "/** @param {Object} y */ function g(y) { return y.a; }",
"Property a never defined on Object");
} }


public void testMissingProperty31a() { public void testMissingProperty31a() {
Expand Down Expand Up @@ -13594,10 +13657,15 @@ public void testInterfaceExtendsResolution() {
} }


public void testPropertyCanBeDefinedInObject() { public void testPropertyCanBeDefinedInObject() {
testTypes("/** @interface */ function I() {};" + // This test is specifically checking loose property check behavior.
"I.prototype.bar = function() {};" + compiler.getOptions().setWarningLevel(
"/** @type {Object} */ var foo;" + DiagnosticGroups.STRICT_MISSING_PROPERTIES, CheckLevel.OFF);
"foo.bar();"); testTypes(
lines(
"/** @interface */ function I() {};",
"I.prototype.bar = function() {};",
"/** @type {Object} */ var foo;",
"foo.bar();"));
} }


private void checkObjectType(ObjectType objectType, String propertyName, private void checkObjectType(ObjectType objectType, String propertyName,
Expand Down Expand Up @@ -14368,17 +14436,21 @@ public void testNonexistentPropertyAccessStructSubtype2() {
} }


public void testIssue1024a() { public void testIssue1024a() {
testTypes( // This test is specifically checking loose property check behavior.
"/** @param {Object} a */\n" + compiler.getOptions().setWarningLevel(
"function f(a) {\n" + DiagnosticGroups.STRICT_MISSING_PROPERTIES, CheckLevel.OFF);
" a.prototype = '__proto'\n" + testTypes(
"}\n" + lines(
"/** @param {Object} b\n" + "/** @param {Object} a */",
" * @return {!Object}\n" + "function f(a) {",
" */\n" + " a.prototype = '__proto'",
"function g(b) {\n" + "}",
" return b.prototype\n" + "/** @param {Object} b",
"}\n"); " * @return {!Object}",
" */",
"function g(b) {",
" return b.prototype",
"}"));
} }


public void testIssue1024b() { public void testIssue1024b() {
Expand Down Expand Up @@ -18165,6 +18237,19 @@ public void testb38182645() {
"required: null")); "required: null"));
} }


public void testMissingPropertiesWarningOnObject1() {
testTypes(lines(
"/** @constructor */",
"function Foo() {",
" this.prop = 123;",
"}",
"/** @param {!Object} x */",
"function f(x) {",
" return x.prop;",
"}"),
"Property prop never defined on Object");
}

private void testTypes(String js) { private void testTypes(String js) {
testTypes(js, (String) null); testTypes(js, (String) null);
} }
Expand Down

0 comments on commit 12331f1

Please sign in to comment.