Skip to content

Commit

Permalink
Introduce optional strict property checks.
Browse files Browse the repository at this point in the history
Add the "strictMissingProperties" diagnostic group.  While the current "missingProperties" warnings report when a property can't be found on the type or any of its subtypes, when the "strictMissingProperties" diagnostic group is enabled, the type check will report references to properties that are not declared on that type.

As the subtype property references are typed as "unknown" this encourages patterns that less frequently into pits of cascading unknowns.

The historic "missingProperties" check was implemented they way it was in no small part because of the awkwardness of typing the DOM API (knowning which subclass of Node/Element you are expecting, etc) and it is expected that this will continue to be the case here that it is awkward.

Also here, we hack the suppression guard so that "checkTypes" and "missingProperties" include the "strictMissingProperties" warnings when they are used for suppressions.

Note this CL lays a solid foundation for these checks but there are two areas of follow up:
property assignments and properties of unions types require additional unit tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=184007224
  • Loading branch information
concavelenz authored and lauraharker committed Feb 1, 2018
1 parent 85089d7 commit f14ef90
Show file tree
Hide file tree
Showing 5 changed files with 412 additions and 122 deletions.
10 changes: 9 additions & 1 deletion src/com/google/javascript/jscomp/DiagnosticGroups.java
Expand Up @@ -56,7 +56,9 @@ public class DiagnosticGroups {
"newCheckTypes", "newCheckTypes",
"newCheckTypesCompatibility", "newCheckTypesCompatibility",
"newCheckTypesExtraChecks", "newCheckTypesExtraChecks",
"missingSourcesWarnings"); "missingSourcesWarnings",
// TODO(johnlenz): "strictMissingProperties" is here until it has a shake down cruise.
"strictMissingProperties");


public DiagnosticGroups() {} public DiagnosticGroups() {}


Expand Down Expand Up @@ -138,6 +140,7 @@ public DiagnosticGroup forName(String name) {
+ "missingSourcesWarnings, " + "missingSourcesWarnings, "
+ "reportUnknownTypes, " + "reportUnknownTypes, "
+ "suspiciousCode, " + "suspiciousCode, "
+ "strictMissingProperties, "
+ "strictModuleDepCheck, " + "strictModuleDepCheck, "
+ "typeInvalidation, " + "typeInvalidation, "
+ "undefinedNames, " + "undefinedNames, "
Expand Down Expand Up @@ -395,6 +398,11 @@ public DiagnosticGroup forName(String name) {
DiagnosticGroups.registerGroup("oldReportUnknownTypes", // undocumented DiagnosticGroups.registerGroup("oldReportUnknownTypes", // undocumented
TypeCheck.UNKNOWN_EXPR_TYPE); TypeCheck.UNKNOWN_EXPR_TYPE);


public static final DiagnosticGroup STRICT_MISSING_PROPERTIES =
DiagnosticGroups.registerGroup("strictMissingProperties",
TypeCheck.STRICT_INEXISTENT_PROPERTY,
TypeCheck.STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION);

public static final DiagnosticGroup REPORT_UNKNOWN_TYPES = public static final DiagnosticGroup REPORT_UNKNOWN_TYPES =
DiagnosticGroups.registerGroup("reportUnknownTypes", DiagnosticGroups.registerGroup("reportUnknownTypes",
TypeCheck.UNKNOWN_EXPR_TYPE, TypeCheck.UNKNOWN_EXPR_TYPE,
Expand Down
20 changes: 20 additions & 0 deletions src/com/google/javascript/jscomp/SuppressDocWarningsGuard.java
Expand Up @@ -60,6 +60,26 @@ class SuppressDocWarningsGuard extends WarningsGuard {
suppressors.put( suppressors.put(
"missingRequire", "missingRequire",
new DiagnosticGroupWarningsGuard(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.OFF)); new DiagnosticGroupWarningsGuard(DiagnosticGroups.STRICT_MISSING_REQUIRE, CheckLevel.OFF));

// Hack: Allow "@suppress {missingProperties}" to mean
// "@suppress {strictmissingProperties}".
// TODO(johnlenz): Delete this when it is enabled with missingProperties
suppressors.put(
"missingProperties",
new DiagnosticGroupWarningsGuard(
new DiagnosticGroup(
DiagnosticGroups.MISSING_PROPERTIES,
DiagnosticGroups.STRICT_MISSING_PROPERTIES), CheckLevel.OFF));

// Hack: Allow "@suppress {checkTypes}" to include
// "strictmissingProperties".
// TODO(johnlenz): Delete this when it is enabled with missingProperties
suppressors.put(
"checkTypes",
new DiagnosticGroupWarningsGuard(
new DiagnosticGroup(
DiagnosticGroups.CHECK_TYPES,
DiagnosticGroups.STRICT_MISSING_PROPERTIES), CheckLevel.OFF));
} }


@Override @Override
Expand Down
92 changes: 70 additions & 22 deletions src/com/google/javascript/jscomp/TypeCheck.java
Expand Up @@ -46,6 +46,7 @@
import com.google.javascript.rhino.jstype.JSType.SubtypingMode; import com.google.javascript.rhino.jstype.JSType.SubtypingMode;
import com.google.javascript.rhino.jstype.JSTypeNative; import com.google.javascript.rhino.jstype.JSTypeNative;
import com.google.javascript.rhino.jstype.JSTypeRegistry; import com.google.javascript.rhino.jstype.JSTypeRegistry;
import com.google.javascript.rhino.jstype.JSTypeRegistry.PropDefinitionKind;
import com.google.javascript.rhino.jstype.NamedType; import com.google.javascript.rhino.jstype.NamedType;
import com.google.javascript.rhino.jstype.ObjectType; import com.google.javascript.rhino.jstype.ObjectType;
import com.google.javascript.rhino.jstype.Property; import com.google.javascript.rhino.jstype.Property;
Expand Down Expand Up @@ -114,6 +115,16 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
"JSC_INEXISTENT_PROPERTY_WITH_SUGGESTION", "JSC_INEXISTENT_PROPERTY_WITH_SUGGESTION",
"Property {0} never defined on {1}. Did you mean {2}?"); "Property {0} never defined on {1}. Did you mean {2}?");


public static final DiagnosticType STRICT_INEXISTENT_PROPERTY =
DiagnosticType.disabled(
"JSC_STRICT_INEXISTENT_PROPERTY",
"Property {0} never defined on {1}");

static final DiagnosticType STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION =
DiagnosticType.disabled(
"JSC_STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION",
"Property {0} never defined on {1}. Did you mean {2}?");

protected static final DiagnosticType NOT_A_CONSTRUCTOR = protected static final DiagnosticType NOT_A_CONSTRUCTOR =
DiagnosticType.warning( DiagnosticType.warning(
"JSC_NOT_A_CONSTRUCTOR", "JSC_NOT_A_CONSTRUCTOR",
Expand Down Expand Up @@ -1515,31 +1526,68 @@ private void checkPropertyAccess(JSType childType, String propName,
private boolean allowLoosePropertyAccessOnNode(Node n) { private boolean allowLoosePropertyAccessOnNode(Node n) {
Node parent = n.getParent(); Node parent = n.getParent();
return NodeUtil.isPropertyTest(compiler, n) return NodeUtil.isPropertyTest(compiler, n)
// Property declaration // Stub property declaration
|| (n.isQualifiedName() && parent.isExprResult()) || (n.isQualifiedName() && parent.isExprResult());
// Property creation
|| (n.isQualifiedName() && parent.isAssign() && parent.getFirstChild() == n);
} }


private void checkPropertyAccessHelper(JSType objectType, String propName, private boolean isQNameAssignmentTarget(Node n) {
NodeTraversal t, Node n) { Node parent = n.getParent();
return n.isQualifiedName() && parent.isAssign() && parent.getFirstChild() == n;
}

private void checkPropertyAccessHelper(
JSType objectType, String propName, NodeTraversal t, Node n) {
boolean isStruct = objectType.isStruct();
boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct;
if (!objectType.isEmptyType() && reportMissingProperties if (!objectType.isEmptyType() && reportMissingProperties
&& (!allowLoosePropertyAccessOnNode(n) || objectType.isStruct()) && (!allowLoosePropertyAccessOnNode(n) || isStruct)) {
&& !typeRegistry.canPropertyBeDefined(objectType, propName)) { PropDefinitionKind kind = typeRegistry.canPropertyBeDefined(objectType, propName);
boolean lowConfidence = if (!kind.equals(PropDefinitionKind.KNOWN)) {
objectType.isUnknownType() || objectType.isEquivalentTo(getNativeType(OBJECT_TYPE)); // If the property definition is known, but only loosely associated,
SuggestionPair pair = null; // only report a "strict error" which can be optional as code is migrated.
if (!lowConfidence) { boolean strict = kind.equals(PropDefinitionKind.LOOSE) || loosePropertyDeclaration;
pair = getClosestPropertySuggestion(objectType, propName); boolean lowConfidence =
} objectType.isUnknownType() || objectType.isEquivalentTo(getNativeType(OBJECT_TYPE));
if (pair != null && pair.distance * 4 < propName.length()) { SuggestionPair pair = null;
report(t, n.getLastChild(), INEXISTENT_PROPERTY_WITH_SUGGESTION, propName, if (lowConfidence) {
typeRegistry.getReadableTypeName(n.getFirstChild()), pair.suggestion); if (strict) {
} else { return;
DiagnosticType reportType = }
lowConfidence ? POSSIBLE_INEXISTENT_PROPERTY : INEXISTENT_PROPERTY; } else {
report(t, n.getLastChild(), reportType, propName, pair = getClosestPropertySuggestion(objectType, propName);
typeRegistry.getReadableTypeName(n.getFirstChild())); // Traditionally, we would not report a warning for "loose" properties, but we want to be
// able to be more strict, so introduce an optional warning.
}
if (pair != null && pair.distance * 4 < propName.length()) {
DiagnosticType reportType;
if (strict) {
reportType = STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION;
} else {
reportType = INEXISTENT_PROPERTY_WITH_SUGGESTION;
}
report(
t,
n.getLastChild(),
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()));
}
} }
} }
} }
Expand Down
25 changes: 19 additions & 6 deletions src/com/google/javascript/rhino/jstype/JSTypeRegistry.java
Expand Up @@ -840,20 +840,29 @@ public JSType getGreatestSubtypeWithProperty(
return getNativeType(NO_TYPE); return getNativeType(NO_TYPE);
} }


/** A tristate value returned from canPropertyBeDefined. */
public enum PropDefinitionKind {
UNKNOWN, // The property is not known to be part of this type
KNOWN, // The properties is known to be defined on a type or its super types
LOOSE // The property is loosely associated with a type, typically one of its subtypes
}

/** /**
* Returns whether the given property can possibly be set on the given type. * Returns whether the given property can possibly be set on the given type.
*/ */
public boolean canPropertyBeDefined(JSType type, String propertyName) { public PropDefinitionKind canPropertyBeDefined(JSType type, String propertyName) {
if (type.isStruct()) { if (type.isStruct()) {
// We are stricter about "struct" types and only allow access to // We are stricter about "struct" types and only allow access to
// properties that to the best of our knowledge are available at creation // properties that to the best of our knowledge are available at creation
// time and specifically not properties only defined on subtypes. // time and specifically not properties only defined on subtypes.
return type.hasProperty(propertyName); return type.hasProperty(propertyName)
? PropDefinitionKind.KNOWN : PropDefinitionKind.UNKNOWN;
} else { } else {
if (!type.isEmptyType() && !type.isUnknownType() if (!type.isEmptyType() && !type.isUnknownType()
&& type.hasProperty(propertyName)) { && type.hasProperty(propertyName)) {
return true; return PropDefinitionKind.KNOWN;
} }

if (typesIndexedByProperty.containsKey(propertyName)) { if (typesIndexedByProperty.containsKey(propertyName)) {
for (JSType alt : for (JSType alt :
typesIndexedByProperty.get(propertyName).getAlternates()) { typesIndexedByProperty.get(propertyName).getAlternates()) {
Expand All @@ -866,10 +875,11 @@ public boolean canPropertyBeDefined(JSType type, String propertyName) {
continue; continue;
} }


return true; return PropDefinitionKind.LOOSE;
} }
} }
} }

if (type.toMaybeRecordType() != null) { if (type.toMaybeRecordType() != null) {
RecordType rec = type.toMaybeRecordType(); RecordType rec = type.toMaybeRecordType();
boolean mayBeInUnion = false; boolean mayBeInUnion = false;
Expand All @@ -879,10 +889,13 @@ public boolean canPropertyBeDefined(JSType type, String propertyName) {
break; break;
} }
} }
return mayBeInUnion && this.droppedPropertiesOfUnions.contains(propertyName);
if (mayBeInUnion && this.droppedPropertiesOfUnions.contains(propertyName)) {
return PropDefinitionKind.LOOSE;
}
} }
} }
return false; return PropDefinitionKind.UNKNOWN;
} }


/** /**
Expand Down

0 comments on commit f14ef90

Please sign in to comment.