Skip to content

Commit

Permalink
Resolve function templates within the function body
Browse files Browse the repository at this point in the history 
Cleans up some ugliness in how JSTypeRegistry handles template types.  Rather than storing temporary state in the registry, this state is now moved out into a temporary StaticTypedScope implementation.  It does still require some special handling to look for names in this special type of scope, but the lifetime of this overlay is now much cleaner.

For type resolution within the body, the types are actually added into the registry at the proper scope root, and vars are added to the scope so that JSTypeRegistry#getLookupScope is able to find them correctly.  This has the chance to shadow outer names, but that shouldn't cause any problems.

Fixes github.com//issues/1924

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=202058228
  • Loading branch information
@shicks @brad4d
shicks authored and brad4d committed Jun 26, 2018
1 parent ce583a1 commit 55596f6
Show file tree
Hide file tree
Showing 6 changed files with 287 additions and 68 deletions.
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/AbstractScope.java
View file
Expand Up @@ -389,7 +389,7 @@ private S thisScope() {
}

/** Performs simple validity checks on when constructing a child scope. */
void checkChildScope(S parent) {
final void checkChildScope(S parent) {
checkNotNull(parent);
checkArgument(NodeUtil.createsScope(rootNode), rootNode);
checkArgument(
Expand All @@ -398,7 +398,7 @@ void checkChildScope(S parent) {
}

/** Performs simple validity checks on when constructing a root scope. */
void checkRootScope() {
final void checkRootScope() {
// TODO(tbreisacher): Can we tighten this to just NodeUtil.createsScope?
checkArgument(
NodeUtil.createsScope(rootNode) || rootNode.isScript() || rootNode.isRoot(), rootNode);
Expand Down
81 changes: 46 additions & 35 deletions src/com/google/javascript/jscomp/FunctionTypeBuilder.java
View file
Expand Up @@ -39,6 +39,7 @@
import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.JSTypeRegistry;
import com.google.javascript.rhino.jstype.ObjectType;
import com.google.javascript.rhino.jstype.StaticTypedScope;
import com.google.javascript.rhino.jstype.TemplateType;
import java.util.ArrayList;
import java.util.HashSet;
Expand Down Expand Up @@ -71,7 +72,7 @@ final class FunctionTypeBuilder {
private final CodingConvention codingConvention;
private final JSTypeRegistry typeRegistry;
private final Node errorRoot;
private final TypedScope scope;
private final TypedScope enclosingScope;

private FunctionContents contents = UnknownFunctionContents.get();

Expand All @@ -94,6 +95,7 @@ final class FunctionTypeBuilder {
// list.
private ImmutableList<TemplateType> classTemplateTypeNames = ImmutableList.of();
private TypedScope declarationScope = null;
private StaticTypedScope templateScope;

static final DiagnosticType EXTENDS_WITHOUT_TYPEDEF = DiagnosticType.warning(
"JSC_EXTENDS_WITHOUT_TYPEDEF",
Expand Down Expand Up @@ -226,13 +228,13 @@ public boolean apply(JSType type) {
FunctionTypeBuilder(String fnName, AbstractCompiler compiler,
Node errorRoot, TypedScope scope) {
checkNotNull(errorRoot);

this.fnName = nullToEmpty(fnName);
this.codingConvention = compiler.getCodingConvention();
this.typeRegistry = compiler.getTypeRegistry();
this.errorRoot = errorRoot;
this.compiler = compiler;
this.scope = scope;
this.enclosingScope = scope;
this.templateScope = scope;
}

/** Format the function name for use in warnings. */
Expand Down Expand Up @@ -343,7 +345,7 @@ FunctionTypeBuilder inferReturnType(
JSTypeExpression returnTypeExpr =
fromInlineDoc ? info.getType() : info.getReturnType();
if (returnTypeExpr != null) {
returnType = returnTypeExpr.evaluate(scope, typeRegistry);
returnType = returnTypeExpr.evaluate(templateScope, typeRegistry);
returnTypeInferred = false;
}
}
Expand Down Expand Up @@ -394,7 +396,6 @@ FunctionTypeBuilder inferInheritance(
if (nativeClassTemplateTypeNames != null
&& infoTemplateTypeNames.size() == nativeClassTemplateTypeNames.size()) {
classTemplateTypeNames = nativeClassTemplateTypeNames;
typeRegistry.setTemplateTypeNames(classTemplateTypeNames);
} else if (!infoTemplateTypeNames.isEmpty() && (isConstructor || isInterface)) {
// Otherwise, create new template type for
// the template values of the constructor/interface
Expand All @@ -405,15 +406,25 @@ FunctionTypeBuilder inferInheritance(
builder.add(typeRegistry.createTemplateType(typeParameter));
}
classTemplateTypeNames = builder.build();
typeRegistry.setTemplateTypeNames(classTemplateTypeNames);
}

if (!classTemplateTypeNames.isEmpty()) {
// Make a new templateScope for resolving types against.
templateScope = typeRegistry.createScopeWithTemplates(templateScope, classTemplateTypeNames);

// Register the template types on the function node.
Node functionNode = contents != null ? contents.getSourceNode() : null;
if (functionNode != null) {
typeRegistry.registerTemplateTypeNamesInScope(classTemplateTypeNames, functionNode);
}
}

// base type
if (info != null && info.hasBaseType()) {
if (isConstructor) {
ObjectType infoBaseType =
info.getBaseType().evaluate(scope, typeRegistry).toMaybeObjectType();
// TODO(sdh): ensure that JSDoc's baseType and AST's baseType are compatible if both are set
info.getBaseType().evaluate(templateScope, typeRegistry).toMaybeObjectType();
// TODO(sdh): ensure JSDoc's baseType and AST's baseType are compatible if both are set
baseType = infoBaseType;
} else {
reportWarning(EXTENDS_WITHOUT_TYPEDEF, formatFnName());
Expand All @@ -431,7 +442,7 @@ FunctionTypeBuilder inferInheritance(
implementedInterfaces = new ArrayList<>();
Set<JSType> baseInterfaces = new HashSet<>();
for (JSTypeExpression t : info.getImplementedInterfaces()) {
JSType maybeInterType = t.evaluate(scope, typeRegistry);
JSType maybeInterType = t.evaluate(templateScope, typeRegistry);

if (maybeInterType != null &&
maybeInterType.setValidator(new ImplementedTypeValidator())) {
Expand Down Expand Up @@ -463,7 +474,7 @@ FunctionTypeBuilder inferInheritance(
extendedInterfaces = new ArrayList<>();
if (info != null) {
for (JSTypeExpression t : info.getExtendedInterfaces()) {
JSType maybeInterfaceType = t.evaluate(scope, typeRegistry);
JSType maybeInterfaceType = t.evaluate(templateScope, typeRegistry);
if (maybeInterfaceType != null &&
maybeInterfaceType.setValidator(new ExtendedTypeValidator())) {
extendedInterfaces.add((ObjectType) maybeInterfaceType);
Expand Down Expand Up @@ -510,7 +521,7 @@ FunctionTypeBuilder inferThisType(JSDocInfo info) {
// undefined "this" value, but all the existing "@this" annotations
// don't declare restricted types.
JSType maybeThisType =
info.getThisType().evaluate(scope, typeRegistry).restrictByNotNullOrUndefined();
info.getThisType().evaluate(templateScope, typeRegistry).restrictByNotNullOrUndefined();
if (maybeThisType != null) {
thisType = maybeThisType;
}
Expand Down Expand Up @@ -573,11 +584,10 @@ FunctionTypeBuilder inferParameterTypes(@Nullable Node argsParent, @Nullable JSD
JSType parameterType = null;
if (info != null && info.hasParameterType(argumentName)) {
parameterType =
info.getParameterType(argumentName).evaluate(scope, typeRegistry);
info.getParameterType(argumentName).evaluate(templateScope, typeRegistry);
} else if (arg.getJSDocInfo() != null && arg.getJSDocInfo().hasType()) {
JSTypeExpression parameterTypeExpression = arg.getJSDocInfo().getType();
parameterType =
parameterTypeExpression.evaluate(scope, typeRegistry);
parameterType = parameterTypeExpression.evaluate(templateScope, typeRegistry);
isOptionalParam = parameterTypeExpression.isOptionalArg();
isVarArgs = parameterTypeExpression.isVarArgs();
} else if (oldParameterType != null &&
Expand Down Expand Up @@ -657,24 +667,17 @@ FunctionTypeBuilder inferTemplateTypeName(@Nullable JSDocInfo info, @Nullable JS
// of inherited ones from an overridden function.
if (info != null) {
ImmutableList.Builder<TemplateType> builder = ImmutableList.builder();
ImmutableList<String> infoTemplateTypeNames =
info.getTemplateTypeNames();
ImmutableMap<String, Node> infoTypeTransformations =
info.getTypeTransformations();
if (!infoTemplateTypeNames.isEmpty()) {
for (String key : infoTemplateTypeNames) {
builder.add(typeRegistry.createTemplateType(key));
}
ImmutableList<String> infoTemplateTypeNames = info.getTemplateTypeNames();
ImmutableMap<String, Node> infoTypeTransformations = info.getTypeTransformations();
for (String key : infoTemplateTypeNames) {
builder.add(typeRegistry.createTemplateType(key));
}
if (!infoTypeTransformations.isEmpty()) {
for (Entry<String, Node> entry : infoTypeTransformations.entrySet()) {
builder.add(typeRegistry.createTemplateTypeWithTransformation(
entry.getKey(), entry.getValue()));
}
for (Entry<String, Node> entry : infoTypeTransformations.entrySet()) {
builder.add(typeRegistry.createTemplateTypeWithTransformation(
entry.getKey(), entry.getValue()));
}
if (!infoTemplateTypeNames.isEmpty()
|| !infoTypeTransformations.isEmpty()) {
templateTypeNames = builder.build();
if (!infoTemplateTypeNames.isEmpty() || !infoTypeTransformations.isEmpty()) {
this.templateTypeNames = builder.build();
}
}

Expand All @@ -684,14 +687,24 @@ FunctionTypeBuilder inferTemplateTypeName(@Nullable JSDocInfo info, @Nullable JS
ownerType.getTemplateTypeMap().getTemplateKeys();
if (!ownerTypeKeys.isEmpty()) {
ImmutableList.Builder<TemplateType> builder = ImmutableList.builder();
// TODO(sdh): The order of these should be switched to avoid class templates shadowing
// method templates, but this currently loosens type checking of arrays more than we'd like.
// See http://github.com/google/closure-compiler/issues/2973
builder.addAll(templateTypeNames);
builder.addAll(ownerTypeKeys);
keys = builder.build();
}
}

if (!keys.isEmpty()) {
typeRegistry.setTemplateTypeNames(keys);
// Add any templates from JSDoc into our template scope.
templateScope = typeRegistry.createScopeWithTemplates(templateScope, keys);

// Register the template types on the function node.
Node functionNode = contents != null ? contents.getSourceNode() : null;
if (functionNode != null) {
typeRegistry.registerTemplateTypeNamesInScope(keys, functionNode);
}
}
return this;
}
Expand Down Expand Up @@ -817,8 +830,6 @@ FunctionType buildAndRegister() {
fnType.setImplicitMatch(true);
}

typeRegistry.clearTemplateTypeNames();

return fnType;
}

Expand Down Expand Up @@ -965,12 +976,12 @@ private TypedScope getScopeDeclaredIn() {
int dotIndex = fnName.indexOf('.');
if (dotIndex != -1) {
String rootVarName = fnName.substring(0, dotIndex);
TypedVar rootVar = scope.getVar(rootVarName);
TypedVar rootVar = enclosingScope.getVar(rootVarName);
if (rootVar != null) {
return rootVar.getScope();
}
}
return scope;
return enclosingScope;
}

/**
Expand Down
48 changes: 39 additions & 9 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -75,6 +75,7 @@
import com.google.javascript.rhino.jstype.NominalTypeBuilderOti;
import com.google.javascript.rhino.jstype.ObjectType;
import com.google.javascript.rhino.jstype.Property;
import com.google.javascript.rhino.jstype.StaticTypedScope;
import com.google.javascript.rhino.jstype.TemplateType;
import com.google.javascript.rhino.jstype.TemplateTypeMap;
import com.google.javascript.rhino.jstype.TemplateTypeMapReplacer;
Expand Down Expand Up @@ -540,6 +541,11 @@ private abstract class AbstractScopeBuilder implements NodeTraversal.Callback {
this.currentHoistScope = scope.getClosestHoistScope();
}

/** Returns the current compiler input. */
CompilerInput getCompilerInput() {
return compiler.getInput(inputId);
}

/** Traverse the scope root and build it. */
void build() {
NodeTraversal.traverse(compiler, currentScope.getRootNode(), this);
Expand Down Expand Up @@ -766,15 +772,11 @@ private JSType getDeclaredTypeInAnnotation(Node node, JSDocInfo info) {
}
}

if (!ownerTypeKeys.isEmpty()) {
typeRegistry.setTemplateTypeNames(ownerTypeKeys);
}

jsType = info.getType().evaluate(currentScope, typeRegistry);

if (!ownerTypeKeys.isEmpty()) {
typeRegistry.clearTemplateTypeNames();
}
StaticTypedScope templateScope =
!ownerTypeKeys.isEmpty()
? typeRegistry.createScopeWithTemplates(currentScope, ownerTypeKeys)
: currentScope;
jsType = info.getType().evaluate(templateScope, typeRegistry);
} else if (FunctionTypeBuilder.isFunctionTypeDeclaration(info)) {
String fnName = node.getQualifiedName();
jsType = createFunctionTypeFromNodes(null, fnName, info, node);
Expand Down Expand Up @@ -2301,6 +2303,34 @@ void declareArguments() {
}
}
}
// Also add template params to the scope so that JSTypeRegistry can find them (they
// were already registered by FunctionTypeBuilder).
JSDocInfo info = NodeUtil.getBestJSDocInfo(functionNode);
if (info != null) {
Iterable<String> templateNames =
Iterables.concat(info.getTemplateTypeNames(), info.getTypeTransformations().keySet());
if (!Iterables.isEmpty(templateNames)) {
CompilerInput input = getCompilerInput();
JSType voidType = typeRegistry.getNativeType(VOID_TYPE);
// Declare any template names in the function scope. This means that if someone shadows
// an outer variable FOO with a @template FOO and refers to FOO inside the method, we
// will treat it as undefined, rather than the correct type, which could lead to weird
// errors. Ideally we'd have a "don't use me" type that gives an error at use.
for (String name : templateNames) {
if (!currentScope.canDeclare(name)) {
validator.expectUndeclaredVariable(
NodeUtil.getSourceName(functionNode),
input,
functionNode,
functionNode.getParent(),
currentScope.getVar(name),
name,
voidType);
}
currentScope.declare(name, functionNode, voidType, input, /* inferred= */ false);
}
}
}
}
} // end declareArguments
} // end FunctionScopeBuilder
Expand Down

0 comments on commit 55596f6

Please sign in to comment.