diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfo.java b/src/com/google/javascript/jscomp/GlobalTypeInfo.java index 1f2450d4be8..e12a29506f6 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfo.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfo.java @@ -59,10 +59,11 @@ */ public class GlobalTypeInfo implements TypeIRegistry { - // An out-to-in list of the scopes, built during CollectNamedTypes - // This will be reversed at the end of GlobalTypeInfo to make sure - // that the scopes can be processed in-to-out in NewTypeInference. - private final List scopes = new ArrayList<>(); + // We collect function scopes during CollectNamedTypes, and put them in this list out-to-in + // (global scope first, then functions in global scope, etc). + // At the end of GlobalTypeInfo, we rearrange them in the order in which they will be + // processed during NewTypeInference. See GlobalTypeInfoCollector#reorderScopesForNTI. + private List scopes; private NTIScope globalScope; private final List mismatches; @@ -112,10 +113,8 @@ public Void apply(Node pnameNode) { } } - void initGlobalTypeInfo(Node root) { - this.globalScope = new NTIScope(root, null, ImmutableList.of(), this.getCommonTypes()); - this.globalScope.addUnknownTypeNames(this.unknownTypeNames); - this.scopes.add(this.globalScope); + void setGlobalScope(NTIScope globalScope) { + this.globalScope = globalScope; } void recordPropertyName(Node pnameNode) { @@ -162,10 +161,18 @@ List getScopes() { return this.scopes; } + void setScopes(List scopes) { + this.scopes = scopes; + } + NTIScope getGlobalScope() { return this.globalScope; } + Set getUnknownTypeNames() { + return this.unknownTypeNames; + } + JSTypes getCommonTypes() { return this.commonTypes; } diff --git a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java index 810eeb2f8cc..0c703c1bf5d 100644 --- a/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java +++ b/src/com/google/javascript/jscomp/GlobalTypeInfoCollector.java @@ -300,6 +300,8 @@ public class GlobalTypeInfoCollector implements CompilerPass { private final SimpleInference simpleInference; private final OrderedExterns orderedExterns; private RawNominalType window; + // This list is populated during CollectNamedTypes and is complete during ProcessScope. + private final List scopes; public GlobalTypeInfoCollector(AbstractCompiler compiler) { this.warnings = new WarningReporter(compiler); @@ -309,6 +311,7 @@ public GlobalTypeInfoCollector(AbstractCompiler compiler) { this.simpleInference = new SimpleInference(this.globalTypeInfo); this.convention = compiler.getCodingConvention(); this.orderedExterns = new OrderedExterns(); + this.scopes = new ArrayList<>(); } @Override @@ -319,23 +322,27 @@ public void process(Node externs, Node root) { this.compiler.setMostRecentTypechecker(MostRecentTypechecker.NTI); - this.globalTypeInfo.initGlobalTypeInfo(root); + NTIScope globalScope = new NTIScope(root, null, ImmutableList.of(), getCommonTypes()); + globalScope.addUnknownTypeNames(this.globalTypeInfo.getUnknownTypeNames()); + this.globalTypeInfo.setGlobalScope(globalScope); + this.scopes.add(globalScope); + // Processing of a scope is split into many separate phases, and it's not // straightforward to remember which phase does what. // (1) Find names of classes, interfaces, typedefs, enums, and namespaces // defined in the global scope. - CollectNamedTypes rootCnt = new CollectNamedTypes(getGlobalScope()); + CollectNamedTypes rootCnt = new CollectNamedTypes(globalScope); NodeTraversal.traverseEs6(this.compiler, externs, this.orderedExterns); rootCnt.collectNamedTypesInExterns(); defineObjectAndFunctionIfMissing(); NodeTraversal.traverseEs6(compiler, root, rootCnt); // (2) Determine the type represented by each typedef and each enum - getGlobalScope().resolveTypedefs(getTypeParser()); - getGlobalScope().resolveEnums(getTypeParser()); + globalScope.resolveTypedefs(getTypeParser()); + globalScope.resolveEnums(getTypeParser()); // (3) Repeat steps 1-2 for all the other scopes (outer-to-inner) - for (int i = 1; i < getScopes().size(); i++) { - NTIScope s = getScopes().get(i); + for (int i = 1; i < this.scopes.size(); i++) { + NTIScope s = this.scopes.get(i); CollectNamedTypes cnt = new CollectNamedTypes(s); NodeTraversal.traverseEs6(compiler, s.getBody(), cnt); s.resolveTypedefs(getTypeParser()); @@ -348,7 +355,7 @@ public void process(Node externs, Node root) { // (4) The bulk of the global-scope processing happens here: // - Create scopes for functions // - Declare properties on types - ProcessScope rootPs = new ProcessScope(getGlobalScope()); + ProcessScope rootPs = new ProcessScope(globalScope); if (externs != null) { NodeTraversal.traverseEs6(compiler, externs, rootPs); } @@ -357,8 +364,8 @@ public void process(Node externs, Node root) { rootPs.finishProcessingScope(); // (6) Repeat steps 4-5 for all the other scopes (outer-to-inner) - for (int i = 1; i < getScopes().size(); i++) { - NTIScope s = getScopes().get(i); + for (int i = 1; i < this.scopes.size(); i++) { + NTIScope s = this.scopes.get(i); ProcessScope ps = new ProcessScope(s); NodeTraversal.traverseEs6(compiler, s.getBody(), ps); ps.finishProcessingScope(); @@ -382,7 +389,7 @@ public void process(Node externs, Node root) { if (this.window != null) { // Copy properties from window to Window.prototype, because sometimes // people pass window around rather than using it directly. - Namespace winNs = getGlobalScope().getNamespace(WINDOW_INSTANCE); + Namespace winNs = globalScope.getNamespace(WINDOW_INSTANCE); if (winNs != null) { winNs.copyWindowProperties(getCommonTypes(), this.window); } @@ -396,14 +403,14 @@ public void process(Node externs, Node root) { globalThisType = getCommonTypes().getTopObject().withLoose(); } getCommonTypes().setGlobalThis(globalThisType); - getGlobalScope().setDeclaredType( + globalScope.setDeclaredType( (new FunctionTypeBuilder(getCommonTypes())). addReceiverType(globalThisType).buildDeclaration()); this.globalTypeInfo.setRawNominalTypes(nominaltypesByNode.values()); nominaltypesByNode = null; propertyDefs = null; - for (NTIScope s : getScopes()) { + for (NTIScope s : this.scopes) { s.freezeScope(); } this.simpleInference.setScopesAreFrozen(); @@ -439,13 +446,48 @@ public void visit(NodeTraversal t, Node n, Node parent) { this.warnings = null; this.funNameGen = null; - // If a scope s1 contains a scope s2, then s2 must be before s1 in scopes. - // The type inference relies on this fact to process deeper scopes before shallower scopes. - Collections.reverse(getScopes()); + reorderScopesForNTI(); this.compiler.setExternProperties(ImmutableSet.copyOf(getExternPropertyNames())); } + /** + * After ProcessScope, scopes are stored in a list; shallower scopes appear before deeper + * scopes (i.e., the top level is first). This method rearranges them in the order in which + * we will process them in NewTypeInference. The order in which scopes are processed in NTI is + * crucial for good type checking, because each function is only checked once; there is no + * global-fixpoint phase. + * + * Unannotated callbacks are processed at the end. This means that by the time we typecheck a + * callback, we have checked the surrounding scope, and we have inferred a good signature. + * + * For the rest of the scopes, we process deeper scopes before shallower scopes + * (the top level is processed last). Then, if a declared function f is called in the same + * scope S in which it is defined, we typecheck S after f, and we have a function summary for f. + * + * TODO(dimvar): We also want to use summaries when type checking method calls to unannotated + * methods. To do that, we will need to process all methods in the beginning, and then the + * remaining scopes. + */ + private void reorderScopesForNTI() { + List scopes = this.scopes; + ArrayList result = new ArrayList<>(scopes.size()); + ArrayList callbacks = new ArrayList<>(); + for (int i = scopes.size() - 1; i >= 0; i--) { + NTIScope s = scopes.get(i); + if (NodeUtil.isUnannotatedCallback(s.getRoot())) { + callbacks.add(s); + } else { + result.add(s); + } + } + // Outer unannotated callbacks are processed before inner unannotated callbacks, so that if + // a callback contains another callback, the outer one is analyzed first. + Collections.reverse(callbacks); + result.addAll(callbacks); + this.globalTypeInfo.setScopes(result); + } + private void setWindow(RawNominalType rawType) { this.window = rawType; } @@ -1217,7 +1259,7 @@ private void visitFunctionEarly(Node fn) { NTIScope fnScope = new NTIScope(fn, this.currentScope, collectFormals(fn, fnDoc), getCommonTypes()); if (!fn.isFromExterns()) { - getScopes().add(fnScope); + GlobalTypeInfoCollector.this.scopes.add(fnScope); } this.currentScope.addLocalFunDef(internalName, fnScope); maybeRecordNominalType(fn, nameNode, fnDoc, isRedeclaration); @@ -2527,7 +2569,7 @@ private DeclaredFunctionType getDeclaredFunctionTypeFromContext( } // The function literal is an argument at a call - if (parent.isCall() && declNode != parent.getFirstChild()) { + if (NodeUtil.isUnannotatedCallback(declNode)) { Node callee = parent.getFirstChild(); JSType calleeType = simpleInferExpr(callee, this.currentScope); FunctionType calleeFunType = calleeType == null ? null : calleeType.getFunType(); @@ -2861,10 +2903,6 @@ private JSTypes getCommonTypes() { return this.globalTypeInfo.getCommonTypes(); } - private List getScopes() { - return this.globalTypeInfo.getScopes(); - } - private JSTypeCreatorFromJSDoc getTypeParser() { return this.globalTypeInfo.getTypeParser(); } diff --git a/src/com/google/javascript/jscomp/NTIScope.java b/src/com/google/javascript/jscomp/NTIScope.java index dc712344ae6..988970c43c7 100644 --- a/src/com/google/javascript/jscomp/NTIScope.java +++ b/src/com/google/javascript/jscomp/NTIScope.java @@ -54,33 +54,46 @@ */ final class NTIScope implements DeclaredTypeRegistry, Serializable, TypeIEnv { + static enum VarKind { + DECLARED, + INFERRED + } + /** * Used for local variables. */ - static class TaggedType implements Serializable { + static class LocalVarInfo implements Serializable { // When we don't know the type of a local variable, this field is null, not ?. private final JSType type; - private final boolean isDeclared; + private final VarKind kind; + // Whether this variable is referenced in other scopes. + private final boolean escapes; - private TaggedType(JSType type, boolean isDeclared) { + private LocalVarInfo(JSType type, VarKind kind, boolean escapes) { this.type = type; - this.isDeclared = isDeclared; + this.kind = kind; + this.escapes = escapes; } - static TaggedType makeDeclared(JSType t) { - return new TaggedType(t, true); + static LocalVarInfo makeDeclared(JSType t) { + return new LocalVarInfo(t, VarKind.DECLARED, false); } - static TaggedType makeInferred(JSType t) { - return new TaggedType(t, false); + LocalVarInfo withEscaped() { + return new LocalVarInfo(this.type, this.kind, true); } JSType getInferredType() { - return !isDeclared ? type : null; + return this.kind == VarKind.INFERRED ? type : null; } JSType getDeclaredType() { - return isDeclared ? type : null; + return this.kind == VarKind.DECLARED ? type : null; + } + + @Override + public String toString() { + return "LocalVarInfo(" + this.type + "," + this.kind + "," + this.escapes + ")"; } } @@ -92,12 +105,10 @@ JSType getDeclaredType() { // Becomes true after freezeScope is run; so it's true during NTI. private boolean isFrozen = false; - private final Map locals = new LinkedHashMap<>(); + private final Map locals = new LinkedHashMap<>(); private final Map externs; private final Set constVars = new LinkedHashSet<>(); private final List formals; - // Variables that are defined in this scope and used in inner scopes. - private Set escapedVars = new LinkedHashSet<>(); // outerVars are the variables that appear free in this scope // and are defined in an outer scope. private final Set outerVars = new LinkedHashSet<>(); @@ -379,7 +390,8 @@ boolean isUndeclaredOuterVar(String name) { } boolean isEscapedVar(String name) { - return this.escapedVars.contains(name); + LocalVarInfo info = this.locals.get(name); + return info != null && info.escapes; } boolean hasThis() { @@ -499,21 +511,22 @@ void addDeclaredLocal(String name, JSType type, boolean isConstant, boolean isFr if (isFromExterns) { externs.put(name, type); } else { - locals.put(name, TaggedType.makeDeclared(type)); + LocalVarInfo info = this.locals.get(name); + this.locals.put( + name, new LocalVarInfo(type, VarKind.DECLARED, info != null && info.escapes)); } } void addInferredLocal(String name, JSType type) { checkArgument(!name.contains(".")); - locals.put(name, TaggedType.makeInferred(type)); + LocalVarInfo info = this.locals.get(name); + this.locals.put(name, new LocalVarInfo(type, VarKind.INFERRED, info != null && info.escapes)); } void clearInferredTypeOfVar(String name) { - if (locals.containsKey(name)) { - TaggedType localType = locals.get(name); - if (localType.getInferredType() != null) { - locals.put(name, TaggedType.makeInferred(null)); - } + LocalVarInfo info = this.locals.get(name); + if (info != null && info.getInferredType() != null) { + this.locals.put(name, new LocalVarInfo(null, VarKind.INFERRED, info.escapes)); } else if (!isDefinedLocally(name, false) && this.parent != null) { this.parent.clearInferredTypeOfVar(name); } @@ -525,7 +538,10 @@ static void mayRecordEscapedVar(NTIScope s, String name) { } while (s != null) { if (s.isDefinedLocally(name, false)) { - s.escapedVars.add(name); + LocalVarInfo info = s.locals.get(name); + if (info != null) { + s.locals.put(name, info.withEscaped()); + } return; } s = s.parent; @@ -585,16 +601,6 @@ void addNamespaceLit(QualifiedName qname, Node defSite) { addNamespace(qname, defSite, new NamespaceLit(this.commonTypes, qname.toString(), defSite)); } - void updateType(String name, JSType newDeclType) { - if (isDefinedLocally(name, false)) { - locals.put(name, TaggedType.makeDeclared(newDeclType)); - } else if (parent != null) { - parent.updateType(name, newDeclType); - } else { - throw new RuntimeException("Cannot update type of unknown variable: " + name); - } - } - void addOuterVar(String name) { outerVars.add(name); } @@ -809,16 +815,15 @@ void freezeScope() { if (externs.containsKey(name)) { externs.put(name, t); } else { - locals.put(name, TaggedType.makeDeclared(t)); + locals.put(name, LocalVarInfo.makeDeclared(t)); } } for (String typedefName : localTypedefs.keySet()) { - locals.put(typedefName, TaggedType.makeDeclared(this.commonTypes.UNDEFINED)); + locals.put(typedefName, LocalVarInfo.makeDeclared(this.commonTypes.UNDEFINED)); } copyOuterVarsTransitively(this); preservedNamespaces = localNamespaces; localNamespaces = ImmutableMap.of(); - escapedVars = ImmutableSet.of(); isFrozen = true; } diff --git a/src/com/google/javascript/jscomp/NewTypeInference.java b/src/com/google/javascript/jscomp/NewTypeInference.java index 6a7480afdfa..5e4c3538bb1 100644 --- a/src/com/google/javascript/jscomp/NewTypeInference.java +++ b/src/com/google/javascript/jscomp/NewTypeInference.java @@ -1395,20 +1395,51 @@ private TypeEnv analyzeVarDeclFwd(Node nameNode, TypeEnv inEnv) { } } } - JSType varType = rhsType; - if (rhs == null) { - varType = UNDEFINED; - } else if (declType != null) { - // If the declared type comes from a @const that was inferred during GTI, don't use here. - JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(nameNode); - if (jsdoc != null && (!jsdoc.hasConstAnnotation() || jsdoc.hasType())) { - varType = declType; - } - } + JSType varType = getInitialTypeOfVar(nameNode, declType, rhsType); maybeSetTypeI(nameNode, varType); return envPutType(outEnv, varName, varType); } + private JSType getInitialTypeOfVar(Node nameNode, JSType declType, JSType rhsType) { + String varName = nameNode.getString(); + JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(nameNode); + Node rhs = nameNode.getFirstChild(); + if (jsdoc != null && jsdoc.hasConstAnnotation()) { + // A constant without an explicit type annotation is still considered as having a declared + // type, and we have inferred that type in GlobalTypeInfoCollector. + // However, we want to type it using rhsType here, because the rhsType may be more precise + // due to flow-sensitive type checking. + return jsdoc.hasType() ? declType : (rhsType != null ? rhsType : UNKNOWN); + } else if (this.currentScope.isEscapedVar(varName)) { + // If a variable escapes (so, may be reassigned in a different scope), we type it + // more loosely to avoid false-positive warnings. + if (declType != null) { + return declType; + } else if (rhs == null || rhsType.isNullOrUndef()) { + // If the escaped variable is not declared and is uninitialized, or is initialized to null, + // type it as unknown. The usual pattern is that the variable will be initialized in a + // different scope and then used later in this scope, and we don't have a good way here + // of figuring out the intended type of the variable. + return UNKNOWN; + } else if (rhsType.isBoolean()) { + // If the escaped variable is initialized to true or false, type it as boolean instead of + // the more specialized type; its value may change in another scope. + return BOOLEAN; + } + return rhsType; + } else if (rhs == null) { + // If the variable doesn't escape and is not initialized, start it as undefined even if + // it is declared. This lets us catch uninitialized-variable errors. + return UNDEFINED; + } else if (declType != null) { + // If the unescaped variable is declared, use that type even if the variable is initialized + // to a more precise type. Useful because people can use the declared type to tell the + // compiler what the variable should be, and don't need to cast the initializer. + return declType; + } + return rhsType; + } + /** * Analyze the expression just to annotate its nodes with types, and give warnings; * we don't need the result. @@ -3667,7 +3698,12 @@ private TypeEnv collectTypesForEscapedVarsFwd(Node n, TypeEnv env) { checkArgument(n.isFunction() || (n.isName() && NodeUtil.isInvocationTarget(n))); String fnName = n.isFunction() ? symbolTable.getFunInternalName(n) : n.getString(); NTIScope innerScope = this.currentScope.getScope(fnName); - FunctionType summary = summaries.get(innerScope).getFunType(); + JSType summaryAsJstype = summaries.get(innerScope); + if (summaryAsJstype == null) { + checkState(NodeUtil.isUnannotatedCallback(n)); + return env; + } + FunctionType summary = summaryAsJstype.getFunType(); for (String freeVar : innerScope.getOuterVars()) { if (innerScope.getDeclaredTypeOf(freeVar) == null) { JSType outerType = envGetType(env, freeVar); @@ -3682,8 +3718,8 @@ private TypeEnv collectTypesForEscapedVarsFwd(Node n, TypeEnv env) { // so we don't warn for uninitialized variables. && (n.isName() || (n.isFunction() && !outerType.isUndefined())) && !JSType.haveCommonSubtype(outerType, innerType)) { - warnings.add(JSError.make(n, CROSS_SCOPE_GOTCHA, - freeVar, outerType.toString(), innerType.toString())); + warnings.add(JSError.make( + n, CROSS_SCOPE_GOTCHA, freeVar, outerType.toString(), innerType.toString())); } // If n is a callee node, we only want to keep the type in the callee. // If n is a function expression, we don't know if it will get called, so we take the diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 6797c5856f9..f26d1bb3c1d 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -3163,6 +3163,12 @@ static boolean isFunctionExpression(Node n) { && !NodeUtil.isMethodDeclaration(n); } + static boolean isUnannotatedCallback(Node n) { + JSDocInfo jsdoc = getBestJSDocInfo(n); + return n.isFunction() && n.getParent().isCall() && n != n.getParent().getFirstChild() + && jsdoc == null && !functionHasInlineJsdocs(n); + } + /** * @return Whether the node is both a function expression and the function is named. */ diff --git a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java index 6778f39c756..a41a17e6249 100644 --- a/test/com/google/javascript/jscomp/NewTypeInferenceTest.java +++ b/test/com/google/javascript/jscomp/NewTypeInferenceTest.java @@ -1233,7 +1233,7 @@ public void testFunctionsInsideFunctions() { NewTypeInference.INVALID_OPERAND_TYPE); } - public void testCrossScopeWarnings() { + public void testEscapedVariables() { typeCheck(LINE_JOINER.join( "function f() {", " x < 'str';", @@ -1247,8 +1247,7 @@ public void testCrossScopeWarnings() { "function f() {", " return x - 1;", "}", - "f()"), - NewTypeInference.CROSS_SCOPE_GOTCHA); + "f()")); typeCheck(LINE_JOINER.join( "function f(y) {", @@ -1473,6 +1472,52 @@ public void testCrossScopeWarnings() { NewTypeInference.CROSS_SCOPE_GOTCHA); } + public void testEscapedVariablesAvoidFalsePositives() { + // x typed as unknown. We don't know whether the assignment in the inner scope will happen. + typeCheck(LINE_JOINER.join( + "function f(y) {", + " var x;", + " y(function() { x = 123; });", + " var /** string */ s = x;", + "}")); + + typeCheck(LINE_JOINER.join( + "function f(y) {", + " var /** number */ x;", + " y(function() { x = 123; });", + " var /** string */ s = x;", + "}"), + NewTypeInference.MISTYPED_ASSIGN_RHS); + + // Don't type x as false and then say that the assertion always fails. + typeCheck(LINE_JOINER.join( + CLOSURE_BASE, + "function f(y) {", + " var x = false;", + " y(function() { x = true; });", + " goog.asserts.assert(x);", + "}")); + + // Don't warn about nullable dereference. + typeCheck(LINE_JOINER.join( + "function f(y) {", + " var x = null;", + " y(function() { x.prop = 123; });", + " var z = x.prop;", + "}")); + + // Type c a number, not as (number|null) which we inferred during @const inference. + typeCheck(LINE_JOINER.join( + "function f(/** ?number */ n, fun) {", + " if (n) {", + " /** @const */", + " var c = n;", + " fun(function() { var x = c; });", + " return c - 1;", + " }", + "}")); + } + public void testTrickyUnknownBehavior() { typeCheck(LINE_JOINER.join( "function f(/** function() */ x, cond) {", @@ -1901,8 +1946,7 @@ public void testLooseFunctions() { typeCheck(LINE_JOINER.join( "var foo;", "(function() { foo(); })();", - "foo();"), - NewTypeInference.NULLABLE_DEREFERENCE); + "foo();")); } public void testBackwardForwardPathologicalCase2() { @@ -14825,7 +14869,7 @@ public void testFunctionSubtypingWithReceiverTypes() { "function f(x) {}", "/** @constructor */", "function Foo() {}", - "f(/** @this{Foo} */ function () {});")); + "f(/** @this {Foo} */ function () {});")); typeCheck(LINE_JOINER.join( "/**",