Skip to content

Commit

Permalink
Fix bugs around goog.modules in TypedScopeCreator
Browse files Browse the repository at this point in the history 
 - An inferred parent namespace of a goog.provide caused a NPE
 - Typechecking failed to treat "exports = Foo;" and "exports = {Foo}"
   as constant declarations sometimes, causing 'unknown type' errors.
 - Root namespaces of legacy goog.modules were getting declared in the module scope, not
   the global scope.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=247530269
  • Loading branch information
@lauraharker @EatingW
lauraharker authored and EatingW committed May 10, 2019
1 parent 328f655 commit 6f1704e
Show file tree
Hide file tree
Showing 2 changed files with 189 additions and 15 deletions.
52 changes: 37 additions & 15 deletions src/com/google/javascript/jscomp/TypedScopeCreator.java
View file
Expand Up @@ -2119,6 +2119,10 @@ private TypedScope getLValueRootScope(Node n) {
return currentScope;

default:
if (isGoogModuleExports(root)) {
// Ensure that 'exports = class {}' in a goog.module returns the module scope.
return currentScope;
}
TypedVar var = currentScope.getVar(root.getString());
if (var != null) {
return var.getScope();
Expand Down Expand Up @@ -2227,9 +2231,6 @@ private void declareAliasTypeIfRvalueIsAliasable(
@Nullable QualifiedName rValue,
@Nullable JSType rValueType,
TypedScope rValueLookupScope) {
if (!lValue.isQualifiedName()) {
return;
}
declareAliasTypeIfRvalueIsAliasable(
lValue.getQualifiedName(), lValue, rValue, rValueType, rValueLookupScope, currentScope);
}
Expand All @@ -2243,11 +2244,13 @@ private void declareAliasTypeIfRvalueIsAliasable(
* 1) there's no GETPROP node representing the lvalue and 2) the type is declared in the global
* scope, not the current module-local scope.
*
* @param lValueName the fully qualified lValue name, if any. If null, all this method will do
* is propagate the @typedef Node annotation to actualLvalueNode.
* @param aliasDeclarationScope The scope in which to declare the alias name. In most cases,
* this should just be the {@link #currentScope}.
*/
private void declareAliasTypeIfRvalueIsAliasable(
String lValueName,
@Nullable String lValueName,
@Nullable Node actualLvalueNode,
@Nullable QualifiedName rValue,
@Nullable JSType rValueType,
Expand All @@ -2268,12 +2271,18 @@ private void declareAliasTypeIfRvalueIsAliasable(
if (typedefType != null) {
// Propagate typedef type to typedef aliases.
actualLvalueNode.setTypedefTypeProp(typedefType);
typeRegistry.identifyNonNullableName(lValueName);
typeRegistry.declareType(aliasDeclarationScope, lValueName, typedefType);
if (lValueName != null) {
typeRegistry.identifyNonNullableName(lValueName);
typeRegistry.declareType(aliasDeclarationScope, lValueName, typedefType);
}
return;
}
}

if (lValueName == null) {
return;
}

// Check if the provided rValueType indicates that we should declare this type
// Note that we only look for enums and constructors/interfaces here: this step cannot work
// for @typedefs. The 'type' of the TypedVar representing a @typedef'd name is the None type,
Expand All @@ -2290,16 +2299,26 @@ private void declareAliasTypeIfRvalueIsAliasable(
if (rValueType != null && rValueType.isEnumType()) {
// Look for cases where the rValue is an Enum namespace
typeRegistry.declareType(
currentScope, lValueName, rValueType.toMaybeEnumType().getElementsType());
aliasDeclarationScope, lValueName, rValueType.toMaybeEnumType().getElementsType());
if (isLValueRootedInGlobalScope(actualLvalueNode)) {
// TODO(b/123710194): Also make local aliases non-nullable
typeRegistry.identifyNonNullableName(lValueName);
}
}
}

/** Whether this lvalue is either `exports`, `exports.x`, or a string key in `exports = {x}`. */
boolean isGoogModuleExports(Node lValue) {
return currentScope.isModuleScope() && undeclaredNamesForClosure.contains(lValue);
if (module == null) {
return false;
}
if (undeclaredNamesForClosure.contains(lValue)) {
return true;
}
return lValue.isStringKey()
&& lValue.getParent().isObjectLit()
&& lValue.getGrandparent().isAssign()
&& undeclaredNamesForClosure.contains(lValue.getParent().getPrevious());
}

/** Returns the AST node associated with the definition, if any. */
Expand Down Expand Up @@ -2707,13 +2726,16 @@ private boolean isQualifiedNameInferred(
return false;
}

/** Given a `goog.provide()` call and implicit ProvidedName, declares the name in the scope. */
/**
* Given a `goog.provide()` or legacy `goog.module()` call and implicit ProvidedName, declares
* the name in the global scope.
*/
void declareProvidedNs(Node provideCall, ProvidedName providedName) {
// Redefine this name if we haven't already added a provide definition.
// Note: in some cases, this will cause a redefinition error.
ObjectType anonymousObjectType = typeRegistry.createAnonymousObjectType(null);
new SlotDefiner()
.inScope(currentScope)
.inScope(currentScope.getGlobalScope())
.allowLaterTypeInference(false)
.forVariableName(providedName.getNamespace())
.forDeclarationNode(provideCall)
Expand All @@ -2722,11 +2744,11 @@ void declareProvidedNs(Node provideCall, ProvidedName providedName) {

QualifiedName namespace = QualifiedName.of(providedName.getNamespace());
if (!namespace.isSimple()) {
ObjectType ownerType =
currentScope.lookupQualifiedName(namespace.getOwner()).toMaybeObjectType();
if (ownerType != null) {
ownerType.defineDeclaredProperty(
namespace.getComponent(), anonymousObjectType, provideCall);
JSType ownerType = currentScope.lookupQualifiedName(namespace.getOwner());
if (ownerType != null && ownerType.isObjectType()) {
ownerType
.toMaybeObjectType()
.defineDeclaredProperty(namespace.getComponent(), anonymousObjectType, provideCall);
}
}
}
Expand Down
152 changes: 152 additions & 0 deletions test/com/google/javascript/jscomp/TypedScopeCreatorTest.java
View file
Expand Up @@ -4485,6 +4485,24 @@ public void testGoogModule_declaresExportsVariableImplicitly() {
assertThat(exports).hasJSTypeThat().isLiteralObject();
}

@Test
public void testGoogModule_exportsDeclaresTypeInModuleScope() {
testSame("goog.module('mod.a'); exports = class {}; MOD_A: 0;");

assertType(registry.getGlobalType("exports")).isNull();
assertType(registry.getTypeForScope(getLabeledStatement("MOD_A").enclosingScope, "exports"))
.isNotNull();
}

@Test
public void testGoogModule_namedExportDeclaresTypeInModuleScope() {
testSame("goog.module('mod.a'); exports.B = class {}; MOD_A: 0;");

assertType(registry.getGlobalType("exports.B")).isNull();
assertType(registry.getTypeForScope(getLabeledStatement("MOD_A").enclosingScope, "exports.B"))
.isNotNull();
}

@Test
public void testGoogRequireDefaultExport_getsInferredType() {
// Do some indirection when assigning `exports = f()` so that we don't find the type of
Expand Down Expand Up @@ -4915,6 +4933,23 @@ public void testRequire_requireLoadModuleInRegularModule_inferredNamedExport() {
assertNode(xNode).hasJSTypeThat().isUnknown();
}

@Test
public void testRequire_requireLoadModule_defaultExportOfLocalClass() {
testSame(
srcs(
lines(
"goog.loadModule(function(exports) {",
" goog.module('a');",
" class X {}",
" exports = X;",
" return exports;",
"});"),
lines(
"goog.module('b');", //
"const Y = goog.require('a');",
"var /** !Y */ x;")));
}

@Test
public void testGoogProvide_singleNameWithConstJSDocIsDeclared() {
processClosurePrimitives = true;
Expand Down Expand Up @@ -5060,6 +5095,16 @@ public void testProvide_longNamespace_declaresProperties() {
assertThat(abcdeVar).hasJSTypeThat().hasDeclaredProperty("f");
}

@Test
public void testGoogProvide_parentNamespaceIsInferred() {
processClosurePrimitives = true;
testSame(srcs(CLOSURE_GLOBALS, "goog.provide('a.b.c'); a.b = {};"));

assertThat(globalScope.getVar("a")).hasJSTypeThat().withTypeOfProp("b").isLiteralObject();
assertThat(globalScope.getVar("a.b")).isNull(); // Only declared qnames get TypedVars.
assertThat(globalScope.getVar("a.b.c")).isNotInferred();
}

@Test
public void testGoogProvide_extendsExternsNamespace() {
processClosurePrimitives = true;
Expand Down Expand Up @@ -5117,6 +5162,50 @@ public void testLegacyGoogLoadModule_accessibleWithGoogRequire_exportingConstruc
.toStringIsEqualTo("exports");
}

@Test
public void testLegacyGoogModule_declaresParentNamespacesGlobally() {
processClosurePrimitives = true;
testSame(
srcs(
CLOSURE_GLOBALS,
"goog.module('a.b.c'); goog.module.declareLegacyNamespace(); MOD: 0;"));

assertScope(globalScope).declares("a").directly();
assertScope(globalScope).declares("a.b").directly();
assertScope(globalScope).declares("a.b.c").directly();
assertScope(getLabeledStatement("MOD").enclosingScope).declares("a").onSomeParent();
}

@Test
public void testGoogRequire_multipleRequiresInModule() {
// TODO(b/132208293): we should not get this error.
testSame(
srcs(
lines(
"goog.module('mod.A');", //
"/** @interface */",
"exports = class {};"),
lines(
"goog.module('mod.B');", //
"/** @interface */",
"exports = class {};"),
lines(
"goog.module('mod.C');",
"const A = goog.require('mod.A');",
"const B = goog.require('mod.B');",
"/** @implements {A} @implements {B} */",
"class C {};",
"MOD_C: 0;")),
warning(FunctionTypeBuilder.SAME_INTERFACE_MULTIPLE_IMPLEMENTS));

TypedScope modCScope = getLabeledStatement("MOD_C").enclosingScope;
FunctionType aCtor = modCScope.getVar("A").getType().toMaybeFunctionType();
FunctionType cCtor = modCScope.getVar("C").getType().toMaybeFunctionType();

// TODO(b/132208293): This list should include B.
assertThat(cCtor.getAllImplementedInterfaces()).containsExactly(aCtor.getInstanceType());
}

@Test
public void testLegacyGoogModule_accessibleWithGoogRequire_exportingConstructor() {
testSame(
Expand Down Expand Up @@ -5191,6 +5280,53 @@ public void testLegacyGoogModule_withNamedExport_extendedByGoogProvide() {
assertType(modType).withTypeOfProp("B").toStringIsEqualTo("(typeof mod.B)");
}

@Test
public void testLegacyGoogModule_withDefaultClassExport_createsGlobalType() {
processClosurePrimitives = true;
testSame(
srcs(
CLOSURE_GLOBALS,
lines(
"goog.module('mod.MyClass');",
"goog.module.declareLegacyNamespace();",
"class MyClass {}",
"exports = MyClass;")));

assertType(registry.getGlobalType("mod.MyClass")).toStringIsEqualTo("MyClass");
}

@Test
public void testLegacyGoogModule_withDefaultEnumExport_createsGlobalType() {
processClosurePrimitives = true;
testSame(
srcs(
CLOSURE_GLOBALS,
lines(
"goog.module('mod.MyEnum');",
"goog.module.declareLegacyNamespace();",
"/** @enum {string} */",
"const MyEnum = {A: 'a', B: 'b'};",
"exports = MyEnum;")));

assertType(registry.getGlobalType("mod.MyEnum")).toStringIsEqualTo("MyEnum<string>");
}

@Test
public void testLegacyGoogModule_withDefaultTypedefExport_createsGlobalType() {
processClosurePrimitives = true;
testSame(
srcs(
CLOSURE_GLOBALS,
lines(
"goog.module('mod.MyTypedef');",
"goog.module.declareLegacyNamespace();",
"/** @typedef {number} */",
"let MyTypedef;",
"exports = MyTypedef;")));

assertType(registry.getGlobalType("mod.MyTypedef")).isNumber();
}

@Test
public void testLegacyGoogModule_withDefaultExport_extendedByGoogProvide() {
testSame(
Expand Down Expand Up @@ -5248,6 +5384,22 @@ public void testGoogModuleRequiringGoogProvide_class() {
.toStringIsEqualTo("a.b.Foo");
}

@Test
public void testTypedef_namedExportInObjectLit() {
testSame(
srcs(
CLOSURE_GLOBALS,
lines(
"goog.module('a');", //
"/** @typedef {number} */",
"let Foo;",
"exports = {Foo};"),
lines(
"goog.module('b');", //
"const {Foo} = goog.require('a');",
"var /** !Foo */ x;")));
}

@Test
public void testGoogModuleRequiringGoogProvide_classWithDestructuring() {
testSame(
Expand Down

0 comments on commit 6f1704e

Please sign in to comment.