Skip to content

Commit

Permalink
Rollback "Resolve function templates within the function body"
Browse files Browse the repository at this point in the history 
Broke internal google tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=202064733
  • Loading branch information
@brad4d
ariaxu authored and brad4d committed Jun 26, 2018
1 parent c19c762 commit 74a8e02
Show file tree
Hide file tree
Showing 6 changed files with 68 additions and 287 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. */ /** Performs simple validity checks on when constructing a child scope. */
final void checkChildScope(S parent) { void checkChildScope(S parent) {
checkNotNull(parent); checkNotNull(parent);
checkArgument(NodeUtil.createsScope(rootNode), rootNode); checkArgument(NodeUtil.createsScope(rootNode), rootNode);
checkArgument( checkArgument(
Expand All @@ -398,7 +398,7 @@ final void checkChildScope(S parent) {
} }


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


private FunctionContents contents = UnknownFunctionContents.get(); private FunctionContents contents = UnknownFunctionContents.get();


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


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

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


/** Format the function name for use in warnings. */ /** Format the function name for use in warnings. */
Expand Down Expand Up @@ -345,7 +343,7 @@ FunctionTypeBuilder inferReturnType(
JSTypeExpression returnTypeExpr = JSTypeExpression returnTypeExpr =
fromInlineDoc ? info.getType() : info.getReturnType(); fromInlineDoc ? info.getType() : info.getReturnType();
if (returnTypeExpr != null) { if (returnTypeExpr != null) {
returnType = returnTypeExpr.evaluate(templateScope, typeRegistry); returnType = returnTypeExpr.evaluate(scope, typeRegistry);
returnTypeInferred = false; returnTypeInferred = false;
} }
} }
Expand Down Expand Up @@ -396,6 +394,7 @@ FunctionTypeBuilder inferInheritance(
if (nativeClassTemplateTypeNames != null if (nativeClassTemplateTypeNames != null
&& infoTemplateTypeNames.size() == nativeClassTemplateTypeNames.size()) { && infoTemplateTypeNames.size() == nativeClassTemplateTypeNames.size()) {
classTemplateTypeNames = nativeClassTemplateTypeNames; classTemplateTypeNames = nativeClassTemplateTypeNames;
typeRegistry.setTemplateTypeNames(classTemplateTypeNames);
} else if (!infoTemplateTypeNames.isEmpty() && (isConstructor || isInterface)) { } else if (!infoTemplateTypeNames.isEmpty() && (isConstructor || isInterface)) {
// Otherwise, create new template type for // Otherwise, create new template type for
// the template values of the constructor/interface // the template values of the constructor/interface
Expand All @@ -406,25 +405,15 @@ FunctionTypeBuilder inferInheritance(
builder.add(typeRegistry.createTemplateType(typeParameter)); builder.add(typeRegistry.createTemplateType(typeParameter));
} }
classTemplateTypeNames = builder.build(); 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 // base type
if (info != null && info.hasBaseType()) { if (info != null && info.hasBaseType()) {
if (isConstructor) { if (isConstructor) {
ObjectType infoBaseType = ObjectType infoBaseType =
info.getBaseType().evaluate(templateScope, typeRegistry).toMaybeObjectType(); info.getBaseType().evaluate(scope, typeRegistry).toMaybeObjectType();
// TODO(sdh): ensure JSDoc's baseType and AST's baseType are compatible if both are set // TODO(sdh): ensure that JSDoc's baseType and AST's baseType are compatible if both are set
baseType = infoBaseType; baseType = infoBaseType;
} else { } else {
reportWarning(EXTENDS_WITHOUT_TYPEDEF, formatFnName()); reportWarning(EXTENDS_WITHOUT_TYPEDEF, formatFnName());
Expand All @@ -442,7 +431,7 @@ FunctionTypeBuilder inferInheritance(
implementedInterfaces = new ArrayList<>(); implementedInterfaces = new ArrayList<>();
Set<JSType> baseInterfaces = new HashSet<>(); Set<JSType> baseInterfaces = new HashSet<>();
for (JSTypeExpression t : info.getImplementedInterfaces()) { for (JSTypeExpression t : info.getImplementedInterfaces()) {
JSType maybeInterType = t.evaluate(templateScope, typeRegistry); JSType maybeInterType = t.evaluate(scope, typeRegistry);


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


Expand All @@ -687,24 +684,14 @@ FunctionTypeBuilder inferTemplateTypeName(@Nullable JSDocInfo info, @Nullable JS
ownerType.getTemplateTypeMap().getTemplateKeys(); ownerType.getTemplateTypeMap().getTemplateKeys();
if (!ownerTypeKeys.isEmpty()) { if (!ownerTypeKeys.isEmpty()) {
ImmutableList.Builder<TemplateType> builder = ImmutableList.builder(); 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(templateTypeNames);
builder.addAll(ownerTypeKeys); builder.addAll(ownerTypeKeys);
keys = builder.build(); keys = builder.build();
} }
} }


if (!keys.isEmpty()) { if (!keys.isEmpty()) {
// Add any templates from JSDoc into our template scope. typeRegistry.setTemplateTypeNames(keys);
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; return this;
} }
Expand Down Expand Up @@ -830,6 +817,8 @@ FunctionType buildAndRegister() {
fnType.setImplicitMatch(true); fnType.setImplicitMatch(true);
} }


typeRegistry.clearTemplateTypeNames();

return fnType; return fnType;
} }


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


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


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

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


StaticTypedScope templateScope = if (!ownerTypeKeys.isEmpty()) {
!ownerTypeKeys.isEmpty() typeRegistry.setTemplateTypeNames(ownerTypeKeys);
? typeRegistry.createScopeWithTemplates(currentScope, ownerTypeKeys) }
: currentScope;
jsType = info.getType().evaluate(templateScope, typeRegistry); jsType = info.getType().evaluate(currentScope, typeRegistry);

if (!ownerTypeKeys.isEmpty()) {
typeRegistry.clearTemplateTypeNames();
}
} else if (FunctionTypeBuilder.isFunctionTypeDeclaration(info)) { } else if (FunctionTypeBuilder.isFunctionTypeDeclaration(info)) {
String fnName = node.getQualifiedName(); String fnName = node.getQualifiedName();
jsType = createFunctionTypeFromNodes(null, fnName, info, node); jsType = createFunctionTypeFromNodes(null, fnName, info, node);
Expand Down Expand Up @@ -2303,34 +2301,6 @@ 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 declareArguments
} // end FunctionScopeBuilder } // end FunctionScopeBuilder
Expand Down

0 comments on commit 74a8e02

Please sign in to comment.