Skip to content

Commit

Permalink
Tighten the strict missing property check.
Browse files Browse the repository at this point in the history 
This tightens the strict missing property check:
A warning is now emitted when assigning a property on a non-`struct` type when that property is not defined on that type.

The loose missing property checks would only emit warnings if the type of the resulting expression was "unknown" now they are emitted regardless of the type of the value.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=187514949
  • Loading branch information
@concavelenz
concavelenz authored and Tyler Breisacher committed Mar 3, 2018
1 parent 1b34cda commit c80da22
Show file tree
Hide file tree
Showing 14 changed files with 785 additions and 234 deletions.
3 changes: 2 additions & 1 deletion src/com/google/javascript/jscomp/DiagnosticGroups.java
View file
Expand Up @@ -405,7 +405,8 @@ public DiagnosticGroup forName(String name) {
public static final DiagnosticGroup STRICT_MISSING_PROPERTIES =
DiagnosticGroups.registerGroup("strictMissingProperties",
TypeCheck.STRICT_INEXISTENT_PROPERTY,
TypeCheck.STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION);
TypeCheck.STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION,
TypeCheck.STRICT_INEXISTENT_UNION_PROPERTY);

public static final DiagnosticGroup STRICT_PRIMITIVE_OPERATORS =
DiagnosticGroups.registerGroup("strictPrimitiveOperators",
Expand Down
96 changes: 73 additions & 23 deletions src/com/google/javascript/jscomp/TypeCheck.java
View file
Expand Up @@ -22,6 +22,7 @@
import static com.google.javascript.rhino.jstype.JSTypeNative.ARRAY_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.BOOLEAN_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.NULL_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.NULL_VOID;
import static com.google.javascript.rhino.jstype.JSTypeNative.NUMBER_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.OBJECT_FUNCTION_TYPE;
import static com.google.javascript.rhino.jstype.JSTypeNative.OBJECT_TYPE;
Expand Down Expand Up @@ -121,6 +122,11 @@ public final class TypeCheck implements NodeTraversal.Callback, CompilerPass {
"JSC_STRICT_INEXISTENT_PROPERTY",
"Property {0} never defined on {1}");

public static final DiagnosticType STRICT_INEXISTENT_UNION_PROPERTY =
DiagnosticType.disabled(
"JSC_STRICT_INEXISTENT_UNION_PROPERTY",
"Property {0} not defined on all member types of {1}");

static final DiagnosticType STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION =
DiagnosticType.disabled(
"JSC_STRICT_INEXISTENT_PROPERTY_WITH_SUGGESTION",
Expand Down Expand Up @@ -1042,9 +1048,23 @@ t, assign, getJSType(rvalue),
private void checkPropCreation(NodeTraversal t, Node lvalue) {
if (lvalue.isGetProp()) {
JSType objType = getJSType(lvalue.getFirstChild());
Node prop = lvalue.getLastChild();
if (objType.isStruct() && !objType.hasProperty(prop.getString())) {
report(t, prop, ILLEGAL_PROPERTY_CREATION);
if (!objType.isEmptyType() && !objType.isUnknownType()) {
Node prop = lvalue.getLastChild();
String propName = prop.getString();
PropDefinitionKind kind = typeRegistry.canPropertyBeDefined(objType, propName);
if (!kind.equals(PropDefinitionKind.KNOWN)) {
if (objType.isStruct()) {
report(t, prop, ILLEGAL_PROPERTY_CREATION);
} else {
// null checks are reported elsewhere
if (!objType.isNoType() && !objType.isUnknownType()
&& objType.isSubtype(getNativeType(NULL_VOID))) {
return;
}

reportMissingProperty(objType, propName, kind, t, lvalue, true);
}
}
}
}
}
Expand Down Expand Up @@ -1514,8 +1534,7 @@ private void visitGetProp(NodeTraversal t, Node n) {
* *cannot be defined*, whereas a java compiler would check if the
* property *can be undefined*.
*/
private void checkPropertyAccess(JSType childType, String propName,
NodeTraversal t, Node n) {
private void checkPropertyAccess(JSType childType, String propName, NodeTraversal t, Node n) {
// If the property type is unknown, check the object type to see if it
// can ever be defined. We explicitly exclude CHECKED_UNKNOWN (for
// properties where we've checked that it exists, or for properties on
Expand All @@ -1533,30 +1552,29 @@ private void checkPropertyAccess(JSType childType, String propName,
if (objectType instanceof EnumType) {
report(t, n, INEXISTENT_ENUM_ELEMENT, propName);
} else {
checkPropertyAccessHelper(objectType, propName, t, n);
checkPropertyAccessHelper(objectType, propName, t, n, false);
}
}

} else {
checkPropertyAccessHelper(childType, propName, t, n);
checkPropertyAccessHelper(childType, propName, t, n, false);
}
} else if (childType.isUnionType() && !isLValueGetProp(n)) {
checkPropertyAccessHelper(childType, propName, t, n, true);
}
}

private boolean allowLoosePropertyAccessOnNode(Node n) {
Node parent = n.getParent();
return NodeUtil.isPropertyTest(compiler, n)
// Stub property declaration
|| (n.isQualifiedName() && parent.isExprResult());
}

private boolean isQNameAssignmentTarget(Node n) {
boolean isLValueGetProp(Node n) {
Node parent = n.getParent();
return n.isQualifiedName() && parent.isAssign() && parent.getFirstChild() == n;
return (NodeUtil.isUpdateOperator(parent) || NodeUtil.isAssignmentOp(parent))
&& parent.getFirstChild() == n;
}

/**
* @param strictCheck Whether this is a check that is only performed when "strict missing
* properties" cheks are enabled.
*/
private void checkPropertyAccessHelper(
JSType objectType, String propName, NodeTraversal t, Node n) {
JSType objectType, String propName, NodeTraversal t, Node n, boolean strictCheck) {
boolean isStruct = objectType.isStruct();
if (!reportMissingProperties
|| objectType.isEmptyType()
Expand All @@ -1569,20 +1587,36 @@ private void checkPropertyAccessHelper(
}
// 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 isLooselyAssociated = kind.equals(PropDefinitionKind.LOOSE)
|| kind.equals(PropDefinitionKind.LOOSE_UNION);
boolean isUnknownType = objectType.isUnknownType();
if (isLooselyAssociated && isUnknownType) {
// We still don't want to report this.
return;
}
boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct;
// 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.
boolean strictReport = strictCheck || isLooselyAssociated || loosePropertyDeclaration;

reportMissingProperty(objectType, propName, kind, t, n, strictReport);
}

private void reportMissingProperty(
JSType objectType, String propName, PropDefinitionKind kind, NodeTraversal t, Node n,
boolean strictReport) {
checkState(n.isGetProp());
boolean isUnknownType = objectType.isUnknownType();
boolean isObjectType = objectType.isEquivalentTo(getNativeType(OBJECT_TYPE));
boolean lowConfidence = isUnknownType || isObjectType;
boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct;

boolean isKnownToUnionMember = kind.equals(PropDefinitionKind.LOOSE_UNION);

// boolean loosePropertyDeclaration = isQNameAssignmentTarget(n) && !isStruct;
// 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.
boolean strictReport = isLooselyAssociated || loosePropertyDeclaration;
SuggestionPair pair = null;
if (!lowConfidence) {
if (!lowConfidence && !isKnownToUnionMember) {
pair = getClosestPropertySuggestion(objectType, propName);
}
if (pair != null && pair.distance * 4 < propName.length()) {
Expand All @@ -1602,7 +1636,11 @@ private void checkPropertyAccessHelper(
} else {
DiagnosticType reportType;
if (strictReport) {
reportType = STRICT_INEXISTENT_PROPERTY;
if (isKnownToUnionMember) {
reportType = STRICT_INEXISTENT_UNION_PROPERTY;
} else {
reportType = STRICT_INEXISTENT_PROPERTY;
}
} else if (lowConfidence) {
reportType = POSSIBLE_INEXISTENT_PROPERTY;
} else {
Expand All @@ -1617,6 +1655,18 @@ private void checkPropertyAccessHelper(
}
}

private boolean allowLoosePropertyAccessOnNode(Node n) {
Node parent = n.getParent();
return NodeUtil.isPropertyTest(compiler, n)
// Stub property declaration
|| (n.isQualifiedName() && parent.isExprResult());
}

private boolean isQNameAssignmentTarget(Node n) {
Node parent = n.getParent();
return n.isQualifiedName() && parent.isAssign() && parent.getFirstChild() == n;
}

private static SuggestionPair getClosestPropertySuggestion(
JSType objectType, String propName) {
return null;
Expand Down
3 changes: 1 addition & 2 deletions src/com/google/javascript/jscomp/TypeValidator.java
View file
Expand Up @@ -168,8 +168,7 @@ class TypeValidator implements Serializable {
this.typeRegistry = compiler.getTypeRegistry();
this.allBitwisableValueTypes =
typeRegistry.createUnionType(STRING_TYPE, NUMBER_TYPE, BOOLEAN_TYPE, NULL_TYPE, VOID_TYPE);
this.nullOrUndefined = typeRegistry.createUnionType(
NULL_TYPE, VOID_TYPE);
this.nullOrUndefined = typeRegistry.getNativeType(JSTypeNative.NULL_VOID);
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/com/google/javascript/jscomp/js/es6/util/inherits.js
View file
Expand Up @@ -52,6 +52,7 @@
*
* @param {!Function} childCtor Child class.
* @param {!Function} parentCtor Parent class.
* @suppress {strictMissingProperties} 'superClass_' is not defined on Function
*/
$jscomp.inherits = function(childCtor, parentCtor) {
childCtor.prototype = $jscomp.objectCreate(parentCtor.prototype);
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/resources.json
View file

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion src/com/google/javascript/rhino/jstype/EnumElementType.java
View file
Expand Up @@ -79,7 +79,13 @@ public EnumType getEnumType() {
return enumType;
}

@Override public PropertyMap getPropertyMap() {
@Override
public HasPropertyKind getPropertyKind(String propertyName, boolean autobox) {
return primitiveType.getPropertyKind(propertyName, autobox);
}

@Override
public PropertyMap getPropertyMap() {
return primitiveObjectType == null
? PropertyMap.immutableEmptyMap()
: primitiveObjectType.getPropertyMap();
Expand Down
48 changes: 37 additions & 11 deletions src/com/google/javascript/rhino/jstype/JSType.java
View file
Expand Up @@ -40,8 +40,6 @@
package com.google.javascript.rhino.jstype;

import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.javascript.rhino.jstype.TernaryValue.FALSE;
import static com.google.javascript.rhino.jstype.TernaryValue.UNKNOWN;

import com.google.common.base.Predicate;
import com.google.common.collect.HashBasedTable;
Expand Down Expand Up @@ -148,12 +146,40 @@ public boolean hasDisplayName() {
return displayName != null && !displayName.isEmpty();
}

/** A tristate value returned from canPropertyBeDefined. */
public enum HasPropertyKind {
ABSENT, // The property is not known to be part of this type
KNOWN_PRESENT, // The properties is known to be defined on a type or its super types
MAYBE_PRESENT; // The property is loosely associated with a type, typically one of its subtypes

public static HasPropertyKind of(boolean has) {
return has ? KNOWN_PRESENT : ABSENT;
}
}

/**
* Checks whether the property is present on the object.
* @param pname The property name.
*/
public boolean hasProperty(String pname) {
return false;
public HasPropertyKind getPropertyKind(String pname) {
return getPropertyKind(pname, true);
}

/**
* Checks whether the property is present on the object.
* @param pname The property name.
* @param autobox Whether to check for the presents on an autoboxed type
*/
public HasPropertyKind getPropertyKind(String pname, boolean autobox) {
return HasPropertyKind.ABSENT;
}

/**
* Checks whether the property is present on the object.
* @param pname The property name.
*/
public final boolean hasProperty(String pname) {
return !getPropertyKind(pname, false).equals(HasPropertyKind.ABSENT);
}

public boolean isNoType() {
Expand Down Expand Up @@ -981,7 +1007,7 @@ public final ObjectType autoboxAndGetObject() {
* Algorithm (11.9.3, page 55&ndash;56) of the ECMA-262 specification.<p>
*/
public final boolean canTestForEqualityWith(JSType that) {
return testForEquality(that).equals(UNKNOWN);
return testForEquality(that).equals(TernaryValue.UNKNOWN);
}

/**
Expand All @@ -1006,7 +1032,7 @@ final TernaryValue testForEqualityHelper(JSType aType, JSType bType) {
bType.isNoResolvedType() ||
aType.isAllType() || aType.isUnknownType() ||
aType.isNoResolvedType()) {
return UNKNOWN;
return TernaryValue.UNKNOWN;
}

boolean aIsEmpty = aType.isEmptyType();
Expand All @@ -1015,7 +1041,7 @@ final TernaryValue testForEqualityHelper(JSType aType, JSType bType) {
if (aIsEmpty && bIsEmpty) {
return TernaryValue.TRUE;
} else {
return UNKNOWN;
return TernaryValue.UNKNOWN;
}
}

Expand Down Expand Up @@ -1049,14 +1075,14 @@ final TernaryValue testForEqualityHelper(JSType aType, JSType bType) {
// If this is a "Symbol" or that is "symbol" or "Symbol"
if (aType.isSymbol()) {
return bType.canCastTo(getNativeType(JSTypeNative.SYMBOL_VALUE_OR_OBJECT_TYPE))
? UNKNOWN
: FALSE;
? TernaryValue.UNKNOWN
: TernaryValue.FALSE;
}

if (bType.isSymbol()) {
return aType.canCastTo(getNativeType(JSTypeNative.SYMBOL_VALUE_OR_OBJECT_TYPE))
? UNKNOWN
: FALSE;
? TernaryValue.UNKNOWN
: TernaryValue.FALSE;
}

return null;
Expand Down
31 changes: 24 additions & 7 deletions src/com/google/javascript/rhino/jstype/JSTypeRegistry.java
View file
Expand Up @@ -906,8 +906,9 @@ public JSType getGreatestSubtypeWithProperty(
/** 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
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
LOOSE_UNION // The property is loosely associated with a union type
}

/**
Expand All @@ -918,12 +919,28 @@ public PropDefinitionKind canPropertyBeDefined(JSType type, String propertyName)
// We are stricter about "struct" types and only allow access to
// properties that to the best of our knowledge are available at creation
// time and specifically not properties only defined on subtypes.
return type.hasProperty(propertyName)
? PropDefinitionKind.KNOWN : PropDefinitionKind.UNKNOWN;

switch (type.getPropertyKind(propertyName)) {
case KNOWN_PRESENT:
return PropDefinitionKind.KNOWN;
case MAYBE_PRESENT:
// TODO(johnlenz): return LOOSE_UNION here.
return PropDefinitionKind.KNOWN;
case ABSENT:
return PropDefinitionKind.UNKNOWN;
}
} else {
if (!type.isEmptyType() && !type.isUnknownType()
&& type.hasProperty(propertyName)) {
return PropDefinitionKind.KNOWN;
if (!type.isEmptyType() && !type.isUnknownType()) {
switch (type.getPropertyKind(propertyName)) {
case KNOWN_PRESENT:
return PropDefinitionKind.KNOWN;
case MAYBE_PRESENT:
// TODO(johnlenz): return LOOSE_UNION here.
return PropDefinitionKind.KNOWN;
case ABSENT:
// check for loose properties below.
break;
}
}

if (typesIndexedByProperty.containsKey(propertyName)) {
Expand Down
16 changes: 13 additions & 3 deletions src/com/google/javascript/rhino/jstype/ObjectType.java
View file
Expand Up @@ -514,9 +514,19 @@ public JSType getPropertyType(String propertyName) {
}

@Override
public boolean hasProperty(String propertyName) {
public HasPropertyKind getPropertyKind(String propertyName, boolean autobox) {
// Unknown types have all properties.
return isEmptyType() || isUnknownType() || getSlot(propertyName) != null;
return HasPropertyKind.of(isEmptyType() || isUnknownType() || getSlot(propertyName) != null);
}

/**
* Checks whether the property whose name is given is present directly on
* the object. Returns false even if it is declared on a supertype.
*/
public HasPropertyKind getOwnPropertyKind(String propertyName) {
return getOwnSlot(propertyName) != null
? HasPropertyKind.KNOWN_PRESENT
: HasPropertyKind.ABSENT;
}

/**
Expand All @@ -525,7 +535,7 @@ public boolean hasProperty(String propertyName) {
*/
@Override
public boolean hasOwnProperty(String propertyName) {
return getOwnSlot(propertyName) != null;
return !getOwnPropertyKind(propertyName).equals(HasPropertyKind.ABSENT);
}

/**
Expand Down

0 comments on commit c80da22

Please sign in to comment.