Skip to content

Commit

Permalink
Fix aliasing of typedefs
Browse files Browse the repository at this point in the history 
Adds a new AST annotation to keep track of any typedef types defined by a definition site. This is different from the type of the value, in that e.g.

```
/** @typedef {string} */
ns.MyString;
```

calling `getJSType()` on the `ns.MyString` qualified name node returns the undefined type, since `ns.MyString` has a runtime value of undefined, but the type *name* represents a string, which we store in `Node#getTypedefTypeProp`. This is necessary so that typedefs can be discovered on aliased namespaces: if `const alias = ns;` then `alias.MyString` will not be in the type registry, but we still need to be able to find it. This is now possible by actually adding the `MyString` property to the namespace type (it was omitted before, defined only in the registry), and then checking the definition node for the new annotation. This annotation is also transferred when a typedef is aliased directly.

This approach is sub-optimal, since it does not consolidate the handling of other kinds of type names, whose types are easily derived from their value types (specifically, constructors and enum objects). It would be nice if typedef objects could also store their typename types, but since most enums are undefined or objects, this would require additional proxying and leads to a mess figuring out where to add the new declarations and not conflict with the subset of typedefs that were already previously being declared. On the other hand, this approach could be consolidated to also include class and enum names, which we may pursue in the future.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=219725240
  • Loading branch information
@shicks @EatingW
shicks authored and EatingW committed Nov 2, 2018
1 parent 143d4a3 commit eb519bc
Show file tree
Hide file tree
Showing 5 changed files with 302 additions and 63 deletions.
85 changes: 70 additions & 15 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -63,6 +63,7 @@
import com.google.javascript.jscomp.NodeTraversal.AbstractScopedCallback;
import com.google.javascript.jscomp.NodeTraversal.AbstractShallowStatementCallback;
import com.google.javascript.rhino.ErrorReporter;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.InputId;
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;
Expand Down Expand Up @@ -224,19 +225,37 @@ void resolve() {
}
}

/** Stores the type and qualified name for a destructuring rvalue, which has no AST node */
/**
* Stores the type and qualified name for a destructuring rvalue, which has no actual AST node.
* The {@code qualifiedName} is therefore a detached Node cloned out of the source tree with extra
* getprops added onto it. Cloning should generally be pretty cheap (since it's always just a
* qualified name). Nevertheless, we may pull out a QualifiedName class abstraction at some point
* in the future, which would allow avoiding the clone in the first place. Using a Node here is
* necessary to avoid duplicating logic for non-destructured qualified name aliasing.
*/
private static class RValueInfo {
@Nullable final JSType type;
@Nullable final String qualifiedName;
@Nullable final Node qualifiedName;

private RValueInfo(JSType type, String qualifiedName) {
RValueInfo(JSType type, Node qualifiedName) {
this.type = type;
this.qualifiedName = qualifiedName;
}

private static RValueInfo empty() {
static RValueInfo empty() {
return new RValueInfo(null, null);
}

/** Returns a new RValueInfo whose qualified name has an extra property. */
RValueInfo forProperty(JSType type, String property) {
if (qualifiedName == null) {
return new RValueInfo(type, null);
} else if (qualifiedName.getParent() == null) {
return new RValueInfo(type, IR.getprop(qualifiedName, property));
}
// qualified name is already in the tree, so we need to clone it first.
return new RValueInfo(type, IR.getprop(qualifiedName.cloneTree(), property));
}
}

TypedScopeCreator(AbstractCompiler compiler) {
Expand Down Expand Up @@ -881,7 +900,8 @@ void defineVarChild(JSDocInfo declarationInfo, Node child, TypedScope scope) {
// for (const {x, y} of data) {
value != null
? new RValueInfo(
getDeclaredRValueType(/* lValue= */ null, value), value.getQualifiedName())
getDeclaredRValueType(/* lValue= */ null, value),
/* qualifiedName= */ value)
: new RValueInfo(unknownType, /* qualifiedName= */ null));
}
}
Expand All @@ -908,16 +928,14 @@ private RValueInfo inferTypeForDestructuredTarget(
RValueInfo rvalue = patternTypeSupplier.get();
JSType patternType = rvalue.type;
String propertyName = target.getStringKey().getString();
String qualifiedName =
rvalue.qualifiedName != null ? rvalue.qualifiedName + "." + propertyName : null;
if (patternType == null || patternType.isUnknownType()) {
return new RValueInfo(null, qualifiedName);
return rvalue.forProperty(null, propertyName);
}
if (patternType.hasProperty(propertyName)) {
JSType type = patternType.findPropertyType(propertyName);
return new RValueInfo(type, qualifiedName);
return rvalue.forProperty(type, propertyName);
}
return new RValueInfo(null, qualifiedName);
return rvalue.forProperty(null, propertyName);
}

void defineDestructuringPatternInVarDeclaration(
Expand Down Expand Up @@ -1872,7 +1890,7 @@ && shouldUseFunctionLiteralType(
if (NodeUtil.isConstantDeclaration(compiler.getCodingConvention(), info, lValue)) {
if (rValue != null) {
JSType rValueType = getDeclaredRValueType(lValue, rValue);
maybeDeclareAliasType(lValue, rValue.getQualifiedName(), rValueType);
maybeDeclareAliasType(lValue, rValue, rValueType);
if (rValueType != null) {
return rValueType;
}
Expand Down Expand Up @@ -1901,30 +1919,57 @@ && shouldUseFunctionLiteralType(
*
* @param rvalueName the rvalue's qualified name if it exists, null otherwise
*/
private void maybeDeclareAliasType(Node lValue, String rvalueName, JSType rValueType) {
private void maybeDeclareAliasType(
Node lValue, @Nullable Node rValue, @Nullable JSType rValueType) {
// NOTE: this allows some strange patterns such allowing instance properties
// to be aliases of constructors, and then creating a local alias of that to be
// used as a type name. Consider restricting this.

if (!lValue.isQualifiedName() || (rvalueName == null)) {
if (!lValue.isQualifiedName() || (rValue == null)) {
return;
}

Node definitionNode = getDefinitionNode(rValue);
if (definitionNode != null) {
JSType typedefType = definitionNode.getTypedefTypeProp();
if (typedefType != null) {
// Propagate typedef type to typedef aliases.
lValue.setTypedefTypeProp(typedefType);
String qName = lValue.getQualifiedName();
typeRegistry.identifyNonNullableName(qName);
typeRegistry.declareType(currentScope, qName, typedefType);
return;
}
}

// Treat @const-annotated aliases like @constructor/@interface if RHS has instance type
if (rValueType != null
&& rValueType.isFunctionType()
&& rValueType.toMaybeFunctionType().hasInstanceType()) {
FunctionType functionType = rValueType.toMaybeFunctionType();
typeRegistry.declareType(
currentScope, lValue.getQualifiedName(), functionType.getInstanceType());
} else {
} else if (rValue.isQualifiedName()) {
// Also infer a type name for aliased @typedef
JSType rhsNamedType = typeRegistry.getType(currentScope, rvalueName);
JSType rhsNamedType = typeRegistry.getType(currentScope, rValue.getQualifiedName());
if (rhsNamedType != null) {
typeRegistry.declareType(currentScope, lValue.getQualifiedName(), rhsNamedType);
}
}
}

/** Returns the AST node associated with the definition, if any. */
private Node getDefinitionNode(Node qnameNode) {
if (!qnameNode.isGetProp()) {
TypedVar var = currentScope.getVar(qnameNode.getQualifiedName());
return var != null ? var.getNameNode() : null;
}
ObjectType parent = ObjectType.cast(lookupQualifiedName(qnameNode.getFirstChild()));
return parent != null
? parent.getPropertyDefSite(qnameNode.getLastChild().getString())
: null;
}

/**
* Check for common idioms of a typed R-value assigned to a const L-value.
*
Expand Down Expand Up @@ -2460,6 +2505,8 @@ void checkForTypedef(Node candidate, JSDocInfo info) {
JSType realType = info.getTypedefType().evaluate(currentScope, typeRegistry);
if (realType == null) {
report(JSError.make(candidate, MALFORMED_TYPEDEF, typedef));
} else {
candidate.setTypedefTypeProp(realType);
}

typeRegistry.overwriteDeclaredType(currentScope, typedef, realType);
Expand All @@ -2471,6 +2518,14 @@ void checkForTypedef(Node candidate, JSDocInfo info) {
.withType(getNativeType(NO_TYPE))
.allowLaterTypeInference(false)
.defineSlot();
Node definitionNode = getDefinitionNode(candidate.getFirstChild());
if (definitionNode != null) {
ObjectType parent = ObjectType.cast(definitionNode.getJSType());
if (parent != null) {
JSType type = getNativeType(NO_TYPE);
parent.defineDeclaredProperty(candidate.getLastChild().getString(), type, candidate);
}
}
}
}
} // end AbstractScopeBuilder
Expand Down
17 changes: 16 additions & 1 deletion src/com/google/javascript/rhino/Node.java
View file
Expand Up @@ -166,8 +166,10 @@ public class Node implements Serializable {
MODULE_EXPORT = 97, // Mark a property as a module export so that collase properties
// can act on it.
IS_SHORTHAND_PROPERTY = 98, // Indicates that a property {x:x} was originally parsed as {x}.
ES6_MODULE = 99; // Indicates that a SCRIPT node is or was an ES6 module. Remains set
ES6_MODULE = 99, // Indicates that a SCRIPT node is or was an ES6 module. Remains set
// after the module is rewritten.
TYPEDEF_TYPE = 100; // Record the type associated with a @typedef to enable looking up typedef
// in the AST possible without saving the type scope.

private static final String propToString(byte propType) {
switch (propType) {
Expand Down Expand Up @@ -236,6 +238,8 @@ private static final String propToString(byte propType) {
return "is_shorthand_property";
case ES6_MODULE:
return "es6_module";
case TYPEDEF_TYPE:
return "typedef_type";
default:
throw new IllegalStateException("unexpected prop id " + propType);
}
Expand Down Expand Up @@ -2541,6 +2545,17 @@ public final boolean isDeleted() {
return getBooleanProp(DELETED);
}

/** If this node represents a typedef declaration, the associated JSType */
public final void setTypedefTypeProp(JSType type) {
putProp(TYPEDEF_TYPE, type);
}

/** If this node represents a typedef declaration, the associated JSType */
public final JSType getTypedefTypeProp() {
return (JSType) getProp(TYPEDEF_TYPE);
}

/** @param unused Whether a parameter was function to be unused. Set by RemoveUnusedVars */
public final void setUnusedParameter(boolean unused) {
putBooleanProp(IS_UNUSED_PARAMETER, unused);
}
Expand Down
75 changes: 38 additions & 37 deletions src/com/google/javascript/rhino/jstype/NamedType.java
View file
Expand Up @@ -285,67 +285,68 @@ private boolean resolveViaRegistry(ErrorReporter reporter) {
* as properties. The scope must have been fully parsed and a symbol table constructed.
*/
private void resolveViaProperties(ErrorReporter reporter) {
JSType value = lookupViaProperties();
// last component of the chain
if (value != null && value.isFunctionType() &&
(value.isConstructor() || value.isInterface())) {
FunctionType functionType = value.toMaybeFunctionType();
setReferencedAndResolvedType(functionType.getInstanceType(), reporter);
} else if (value != null && value.isNoObjectType()) {
setReferencedAndResolvedType(
registry.getNativeObjectType(
JSTypeNative.NO_OBJECT_TYPE), reporter);
} else if (value instanceof EnumType) {
setReferencedAndResolvedType(
((EnumType) value).getElementsType(), reporter);
} else {
// We've been running into issues where people forward-declare
// non-named types. (This is legitimate...our dependency management
// code doubles as our forward-declaration code.)
//
// So if the type does resolve to an actual value, but it's not named,
// then don't respect the forward declaration.
handleUnresolvedType(reporter, value == null || value.isUnknownType());
}
}

/**
* Resolves a type by looking up its first component in the scope, and
* subsequent components as properties. The scope must have been fully
* parsed and a symbol table constructed.
* @return The type of the symbol, or null if the type could not be found.
*/
private JSType lookupViaProperties() {
String[] componentNames = reference.split("\\.", -1);
if (componentNames[0].length() == 0) {
return null;
handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true);
return;
}

StaticTypedSlot slot = resolutionScope.getSlot(componentNames[0]);
if (slot == null) {
return null;
handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true);
return;
}

// If the first component has a type of 'Unknown', then any type
// names using it should be regarded as silently 'Unknown' rather than be
// noisy about it.
JSType slotType = slot.getType();
if (slotType == null || slotType.isAllType() || slotType.isNoType()) {
return null;
handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true);
return;
}

// resolving component by component
for (int i = 1; i < componentNames.length; i++) {
ObjectType parentObj = ObjectType.cast(slotType);
if (parentObj == null) {
return null;
handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true);
return;
}
if (componentNames[i].length() == 0) {
return null;
handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true);
return;
} else if (i == componentNames.length - 1) {
// Look for a typedefTypeProp on the definition node of the last component.
Node def = parentObj.getPropertyDefSite(componentNames[i]);
JSType typedefType = def != null ? def.getTypedefTypeProp() : null;
if (typedefType != null) {
setReferencedAndResolvedType(typedefType, reporter);
return;
}
}
slotType = parentObj.getPropertyType(componentNames[i]);
}
return slotType;

// Translate "constructor" types to "instance" types.
if (slotType == null) {
handleUnresolvedType(reporter, /* ignoreForwardReferencedTypes= */ true);
} else if (slotType.isFunctionType() && (slotType.isConstructor() || slotType.isInterface())) {
setReferencedAndResolvedType(slotType.toMaybeFunctionType().getInstanceType(), reporter);
} else if (slotType.isNoObjectType()) {
setReferencedAndResolvedType(
registry.getNativeObjectType(JSTypeNative.NO_OBJECT_TYPE), reporter);
} else if (slotType instanceof EnumType) {
setReferencedAndResolvedType(((EnumType) slotType).getElementsType(), reporter);
} else {
// We've been running into issues where people forward-declare
// non-named types. (This is legitimate...our dependency management
// code doubles as our forward-declaration code.)
//
// So if the type does resolve to an actual value, but it's not named,
// then don't respect the forward declaration.
handleUnresolvedType(reporter, slotType.isUnknownType());
}
}

private void setReferencedAndResolvedType(
Expand Down

0 comments on commit eb519bc

Please sign in to comment.