From af41c2c7bcbc4b228badfb4873b086bf52ca37a0 Mon Sep 17 00:00:00 2001 From: lharker Date: Tue, 18 Jun 2019 10:58:52 -0700 Subject: [PATCH] Make default nullability in the type system work identically for global and local types This is mostly important for moving module rewriting post-checks, as module types are local. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=253823662 --- .../javascript/jscomp/TypedScopeCreator.java | 82 ++++++++----------- .../rhino/jstype/JSTypeRegistry.java | 47 ++++++----- .../jscomp/TypeCheckNoTranspileTest.java | 31 ++++--- .../javascript/jscomp/TypeCheckTest.java | 6 +- .../jscomp/TypedScopeCreatorTest.java | 37 +++++++++ 5 files changed, 109 insertions(+), 94 deletions(-) diff --git a/src/com/google/javascript/jscomp/TypedScopeCreator.java b/src/com/google/javascript/jscomp/TypedScopeCreator.java index 3d13f57f437..fbf93d25636 100644 --- a/src/com/google/javascript/jscomp/TypedScopeCreator.java +++ b/src/com/google/javascript/jscomp/TypedScopeCreator.java @@ -61,8 +61,8 @@ import com.google.javascript.jscomp.CodingConvention.ObjectLiteralCast; import com.google.javascript.jscomp.CodingConvention.SubclassRelationship; import com.google.javascript.jscomp.FunctionTypeBuilder.AstFunctionContents; +import com.google.javascript.jscomp.NodeTraversal.AbstractPostOrderCallback; import com.google.javascript.jscomp.NodeTraversal.AbstractScopedCallback; -import com.google.javascript.jscomp.NodeTraversal.AbstractShallowStatementCallback; import com.google.javascript.jscomp.ProcessClosureProvidesAndRequires.ProvidedName; import com.google.javascript.jscomp.modules.Export; import com.google.javascript.jscomp.modules.Module; @@ -413,9 +413,6 @@ private TypedScope createScopeInternal(Node root, TypedScope typedParent) { externsRoot.setJSType(globalThis); jsRoot.setJSType(globalThis); - // Run a first-order analysis over the syntax tree. - new FirstOrderFunctionAnalyzer().process(root.getFirstChild(), root.getLastChild()); - // Find all the classes in the global scope. newScope = createInitialScope(root); } else { @@ -615,7 +612,7 @@ void patchGlobalScope(TypedScope globalScope, Node scriptRoot) { assignedVarNames.removeIf(var -> inScript.test(var.getScopeRoot())); functionsWithNonEmptyReturns.removeIf(inScript); - new FirstOrderFunctionAnalyzer().process(null, scriptRoot); + NodeTraversal.traverse(compiler, scriptRoot, new FirstOrderFunctionAnalyzer()); // TODO(bashir): Variable declaration is not the only side effect of last // global scope generation but here we only wipe that part off. @@ -657,10 +654,19 @@ void patchGlobalScope(TypedScope globalScope, Node scriptRoot) { TypedScope createInitialScope(Node root) { checkArgument(root.isRoot(), root); - NodeTraversal.traverse( - compiler, - root, - new IdentifyGlobalEnumsAndTypedefsAsNonNullable(typeRegistry, codingConvention)); + // Gather global information used in typed scope creation. Use a memoized scope creator because + // scope-building takes a nontrivial amount of time. + MemoizedScopeCreator scopeCreator = + new MemoizedScopeCreator(new Es6SyntacticScopeCreator(compiler)); + + new NodeTraversal(compiler, new FirstOrderFunctionAnalyzer(), scopeCreator) + .traverseRoots(root.getFirstChild(), root.getLastChild()); + + new NodeTraversal( + compiler, + new IdentifyEnumsAndTypedefsAsNonNullable(typeRegistry, codingConvention), + scopeCreator) + .traverse(root); TypedScope s = TypedScope.createGlobalScope(root); declareNativeFunctionType(s, ARRAY_FUNCTION_TYPE); @@ -733,17 +739,12 @@ void resolveTypes() { typeRegistry.resolveTypes(); } - /** - * Adds all globally-defined enums and typedefs to the registry's list of non-nullable types. - * - *

TODO(b/123710194): We should also make locally-defined enums and typedefs non-nullable. - */ - private static class IdentifyGlobalEnumsAndTypedefsAsNonNullable - extends AbstractShallowStatementCallback { + /** Adds all enums and typedefs to the registry's list of non-nullable types. */ + private static class IdentifyEnumsAndTypedefsAsNonNullable extends AbstractPostOrderCallback { private final JSTypeRegistry registry; private final CodingConvention codingConvention; - IdentifyGlobalEnumsAndTypedefsAsNonNullable( + IdentifyEnumsAndTypedefsAsNonNullable( JSTypeRegistry registry, CodingConvention codingConvention) { this.registry = registry; this.codingConvention = codingConvention; @@ -755,29 +756,22 @@ public void visit(NodeTraversal t, Node node, Node parent) { case LET: case CONST: case VAR: - // Note that this class expects to be invoked on the root node and does not traverse into - // functions. - Scope scope = t.getScope(); - if (!(scope.isGlobal() || scope.getClosestHoistScope().isGlobal())) { - return; - } - if (node.isVar() || scope.isGlobal()) { for (Node child = node.getFirstChild(); child != null; child = child.getNext()) { - // TODO(b/116853368): make this work for destructuring aliases as well. - identifyEnumOrTypedefDeclaration( - child, child.getFirstChild(), NodeUtil.getBestJSDocInfo(child)); + // TODO(b/116853368): make this work for destructuring aliases as well. + identifyEnumOrTypedefDeclaration( + t, child, child.getFirstChild(), NodeUtil.getBestJSDocInfo(child)); } - } + break; case EXPR_RESULT: Node firstChild = node.getFirstChild(); if (firstChild.isAssign()) { Node assign = firstChild; identifyEnumOrTypedefDeclaration( - assign.getFirstChild(), assign.getSecondChild(), assign.getJSDocInfo()); - } else { + t, assign.getFirstChild(), assign.getSecondChild(), assign.getJSDocInfo()); + } else if (firstChild.isGetProp()) { identifyEnumOrTypedefDeclaration( - firstChild, /* rvalue= */ null, firstChild.getJSDocInfo()); + t, firstChild, /* rvalue= */ null, firstChild.getJSDocInfo()); } break; default: @@ -786,19 +780,19 @@ public void visit(NodeTraversal t, Node node, Node parent) { } private void identifyEnumOrTypedefDeclaration( - Node nameNode, @Nullable Node rvalue, JSDocInfo info) { + NodeTraversal t, Node nameNode, @Nullable Node rvalue, JSDocInfo info) { if (!nameNode.isQualifiedName()) { return; } if (info != null && info.hasEnumParameterType()) { - registry.identifyNonNullableName(nameNode.getQualifiedName()); + registry.identifyNonNullableName(t.getScope(), nameNode.getQualifiedName()); } else if (info != null && info.hasTypedefType()) { - registry.identifyNonNullableName(nameNode.getQualifiedName()); + registry.identifyNonNullableName(t.getScope(), nameNode.getQualifiedName()); } else if (rvalue != null && rvalue.isQualifiedName() - && registry.isNonNullableName(rvalue.getQualifiedName()) + && registry.isNonNullableName(t.getScope(), rvalue.getQualifiedName()) && NodeUtil.isConstantDeclaration(codingConvention, info, nameNode)) { - registry.identifyNonNullableName(nameNode.getQualifiedName()); + registry.identifyNonNullableName(t.getScope(), nameNode.getQualifiedName()); } } } @@ -2335,7 +2329,7 @@ private void declareAliasTypeIfRvalueIsAliasable( // Propagate typedef type to typedef aliases. actualLvalueNode.setTypedefTypeProp(typedefType); if (lValueName != null) { - typeRegistry.identifyNonNullableName(lValueName); + typeRegistry.identifyNonNullableName(aliasDeclarationScope, lValueName); typeRegistry.declareType(aliasDeclarationScope, lValueName, typedefType); } return; @@ -2363,10 +2357,7 @@ private void declareAliasTypeIfRvalueIsAliasable( // Look for cases where the rValue is an Enum namespace typeRegistry.declareType( aliasDeclarationScope, lValueName, rValueType.toMaybeEnumType().getElementsType()); - if (isLValueRootedInGlobalScope(actualLvalueNode)) { - // TODO(b/123710194): Also make local aliases non-nullable - typeRegistry.identifyNonNullableName(lValueName); - } + typeRegistry.identifyNonNullableName(aliasDeclarationScope, lValueName); } } @@ -3433,15 +3424,6 @@ private JSType determineGetterType(FunctionType methodType) { *

The syntactic scopes created in this traversal are also stored for later use. */ private class FirstOrderFunctionAnalyzer extends AbstractScopedCallback { - - void process(Node externs, Node root) { - if (externs == null) { - NodeTraversal.traverse(compiler, root, this); - } else { - NodeTraversal.traverseRoots(compiler, this, externs, root); - } - } - @Override public void enterScope(NodeTraversal t) { Scope scope = t.getScope(); diff --git a/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java b/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java index 066093b56f2..b83e7d4566c 100644 --- a/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java +++ b/src/com/google/javascript/rhino/jstype/JSTypeRegistry.java @@ -78,7 +78,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; -import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -158,20 +157,22 @@ public class JSTypeRegistry implements Serializable { // when serializing and deserializing compiler state for multistage builds. private final Node nameTableGlobalRoot = new Node(Token.ROOT); - // NOTE(nicksantos): This is a terrible terrible hack. When type expressions - // are evaluated, we need to be able to decide whether that type name - // resolves to a nullable type or a non-nullable type. Object types are - // nullable, but enum types are not. + // NOTE(nicksantos): This is a terrible terrible hack. When type expressions are evaluated, we + // need to be able to decide whether that type name resolves to a nullable type or a non-nullable + // type. Object types are nullable, but enum types and typedefs are not. // - // Notice that it's not good enough to just declare enum types sooner. + // Notice that it's not good enough to just declare enum types and typedefs sooner. // For example, if we have - // /** @enum {MyObject} */ var MyEnum = ...; - // we won't be to declare "MyEnum" without evaluating the expression - // {MyObject}, and following those dependencies starts to lead us into - // undecidable territory. Instead, we "pre-declare" enum types and typedefs, - // so that the expression resolver can decide whether a given name is + // /** @enum {MyObject} */ var MyEnum = ...; + // we won't be to declare "MyEnum" without evaluating the expression {MyObject}, and following + // those dependencies starts to lead us into undecidable territory. Instead, we "pre-declare" enum + // types and typedefs, so that the expression resolver can decide whether a given name is // nullable or not. - private final Set nonNullableTypeNames = new LinkedHashSet<>(); + // Also, note that this solution is buggy and the type resolution still gets default nullability + // wrong in some cases. See b/116853368. Probably NamedType should be responsible for + // applying nullability to forward references instead of the type expression evaluation. + private final Multimap nonNullableTypeNames = + MultimapBuilder.hashKeys().hashSetValues().build(); // Types that have been "forward-declared." // If these types are not declared anywhere in the binary, we shouldn't @@ -490,7 +491,7 @@ private void initializeBuiltInTypes() { // Thenable is an @typedef JSType thenableType = createRecordType(ImmutableMap.of("then", unknownType)); - identifyNonNullableName("Thenable"); + identifyNonNullableName(null, "Thenable"); registerNativeType(JSTypeNative.THENABLE_TYPE, thenableType); // Create built-in Promise type, whose constructor takes one parameter. @@ -1900,20 +1901,18 @@ public NamedType createNamedType( return new NamedType(scope, this, reference, sourceName, lineno, charno); } - /** - * Identifies the name of a typedef or enum before we actually declare it. - */ - public void identifyNonNullableName(String name) { + /** Identifies the name of a typedef or enum before we actually declare it. */ + public void identifyNonNullableName(StaticScope scope, String name) { checkNotNull(name); - nonNullableTypeNames.add(name); + StaticScope lookupScope = getLookupScope(scope, name); + nonNullableTypeNames.put(getRootNodeForScope(lookupScope), name); } - /** - * Identifies the name of a typedef or enum before we actually declare it. - */ - public boolean isNonNullableName(String name) { + /** Identifies the name of a typedef or enum before we actually declare it. */ + public boolean isNonNullableName(StaticScope scope, String name) { checkNotNull(name); - return nonNullableTypeNames.contains(name); + scope = getLookupScope(scope, name); + return nonNullableTypeNames.containsEntry(getRootNodeForScope(scope), name); } public JSType evaluateTypeExpression(JSTypeExpression expr, StaticTypedScope scope) { @@ -2019,7 +2018,7 @@ private JSType createFromTypeNodesInternal( n.getLineno(), n.getCharno(), recordUnresolvedTypes); - if (namedType instanceof ObjectType && !nonNullableTypeNames.contains(n.getString())) { + if (namedType instanceof ObjectType && !isNonNullableName(scope, n.getString())) { Node typeList = n.getFirstChild(); boolean isForwardDeclared = namedType instanceof NamedType; if ((!namedType.isUnknownType() || isForwardDeclared) && typeList != null) { diff --git a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java index 7adea121fb1..d99f79045f3 100644 --- a/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckNoTranspileTest.java @@ -1027,7 +1027,6 @@ public void testGlobalEnumWithConst() { @Test public void testLocalEnumWithLet() { - // TODO(bradfordcsmith): Local enum types should be non-nullable just like the global ones. testTypes( lines( "{", @@ -1039,16 +1038,11 @@ public void testLocalEnumWithLet() { " * @return {number}", " */", " function f(x) {return x}", - "}"), - lines( - "inconsistent return type", - "found : (E|null)", - "required: number")); + "}")); } @Test public void testLocalEnumWithConst() { - // TODO(bradfordcsmith): Local enum types should be non-nullable just like the global ones. testTypes( lines( "{", @@ -1060,11 +1054,7 @@ public void testLocalEnumWithConst() { " * @return {number}", " */", " function f(x) {return x}", - "}"), - lines( - "inconsistent return type", - "found : (E|null)", - "required: number")); + "}")); } @Test @@ -6157,18 +6147,25 @@ public void testLocalEnumDoesNotInfluenceGlobalDefaultNullablity() { @Test public void testGlobalEnumDoesNotInfluenceLocalDefaultNullablity() { - // TODO(b/123710194): the local Foo should be nullable and this should not warn testTypes( lines( "/** @enum {number} */ const Foo = {A: 1};", "function f() {", " class Foo {};", " /** @type {Foo} */ let x = null;", - "}"), + "}")); + } + + @Test + public void testLocalEnumAliasDoesNotInfluenceGlobalDefaultNullablity() { + testTypes( lines( - "initializing variable", // - "found : null", - "required: Foo")); + "class Foo {};", + "/** @enum {number} */ const Bar = {A: 1};", + "function f() {", + " const Foo = Bar;", + "}", + "/** @type {Foo} */ let x = null;")); } @Test diff --git a/test/com/google/javascript/jscomp/TypeCheckTest.java b/test/com/google/javascript/jscomp/TypeCheckTest.java index 2a84236923c..e1589c0c55a 100644 --- a/test/com/google/javascript/jscomp/TypeCheckTest.java +++ b/test/com/google/javascript/jscomp/TypeCheckTest.java @@ -77,9 +77,9 @@ protected CompilerOptions getDefaultOptions() { @Test public void testInitialTypingScope() { - TypedScope s = new TypedScopeCreator(compiler, - CodingConventions.getDefault()).createInitialScope( - new Node(Token.ROOT)); + TypedScope s = + new TypedScopeCreator(compiler, CodingConventions.getDefault()) + .createInitialScope(new Node(Token.ROOT, new Node(Token.ROOT), new Node(Token.ROOT))); assertTypeEquals(getNativeArrayConstructorType(), s.getVar("Array").getType()); assertTypeEquals(getNativeBooleanObjectConstructorType(), s.getVar("Boolean").getType()); diff --git a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java index 92e27741fec..eef4367619f 100644 --- a/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/TypedScopeCreatorTest.java @@ -4825,6 +4825,43 @@ public void testGoogRequire_exportObjectLiteralWithLiteralValue() { assertNode(xNode).hasJSTypeThat().toStringIsEqualTo("{x: number}"); } + @Test + public void testModuleTypedef_isNonnullableWithinModule() { + testSame( + lines( + "goog.module('a.b'); var /** strType */ earlyRef; ", + "/** @typedef {string} */ let strType; ", + "var /** strType */ normalRef; MODULE: mod;")); + + TypedScope moduleScope = getLabeledStatement("MODULE").enclosingScope; + assertThat(moduleScope.getSlot("earlyRef")).hasJSTypeThat().isString(); + assertThat(moduleScope.getSlot("normalRef")).hasJSTypeThat().isString(); + } + + @Test + public void testModuleTypedef_isNonnullableWhenRequired() { + + testSame( + srcs( + lines( + "goog.module('root');", + "/** @typedef {string} */ let strType; ", + "exports.strType = strType;"), + lines( + "goog.module('a.b'); const {strType} = goog.require('root');", + "/** @param {strType} earlyRef */ function f(earlyRef) { EARLY_REF: earlyRef; }", + "var /** strType */ normalRef; NORMAL_REF: normalRef;"))); + + assertNode(getLabeledStatement("EARLY_REF").statementNode.getOnlyChild()) + .hasJSTypeThat() + // TODO(b/116853368): This should just be 'string', but the function declaration is + // hoisted so it hits bugs in nullability of forward references to types. + .toStringIsEqualTo("(null|string)"); + assertNode(getLabeledStatement("NORMAL_REF").statementNode.getOnlyChild()) + .hasJSTypeThat() + .isString(); + } + @Test public void testGoogRequire_namedExportImportedAsNamespace() { testSame(