Skip to content

Commit

Permalink
Keep track of named imports and add an error if importing non-exporte…
Browse files Browse the repository at this point in the history 
…d name.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=135411050
  • Loading branch information
@blickly
blickly committed Oct 6, 2016
1 parent 1b2bbc1 commit a398920
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 18 deletions.
71 changes: 58 additions & 13 deletions src/com/google/javascript/jscomp/ClosureRewriteModule.java
View file
Expand Up @@ -163,12 +163,17 @@ final class ClosureRewriteModule implements HotSwapCompilerPass {
+ "Either use short import syntax or" + "Either use short import syntax or"
+ " convert module to use goog.module.declareLegacyNamespace."); + " convert module to use goog.module.declareLegacyNamespace.");


static final DiagnosticType ILLEGAL_DESTRUCTURING_IMPORT = static final DiagnosticType ILLEGAL_DESTRUCTURING_DEFAULT_EXPORT =
DiagnosticType.error( DiagnosticType.error(
"JSC_ILLEGAL_DESTRUCTURING_IMPORT", "JSC_ILLEGAL_DESTRUCTURING_DEFAULT_EXPORT",
"Destructuring import cannot refer to a module with default export." "Destructuring import cannot refer to a module with default export."
+ " Please use standard (non-destructuring) import instead."); + " Please use standard (non-destructuring) import instead.");


static final DiagnosticType ILLEGAL_DESTRUCTURING_NOT_EXPORTED =
DiagnosticType.error(
"JSC_ILLEGAL_DESTRUCTURING_NOT_EXPORTED",
"Destructuring import reference to name \"{0}\" was not exported in module {1}");

private static final ImmutableSet<String> USE_STRICT_ONLY = ImmutableSet.of("use strict"); private static final ImmutableSet<String> USE_STRICT_ONLY = ImmutableSet.of("use strict");


private static final String MODULE_EXPORTS_PREFIX = "module$exports$"; private static final String MODULE_EXPORTS_PREFIX = "module$exports$";
Expand Down Expand Up @@ -224,6 +229,7 @@ private static final class ScriptDescription {
boolean hasCreatedExportObject; boolean hasCreatedExportObject;
Node defaultExportRhs; Node defaultExportRhs;
String defaultExportLocalName; String defaultExportLocalName;
Set<String> namedExports = new HashSet<>();


// The root of the module. The SCRIPT node (or for goog.loadModule, the body of the // The root of the module. The SCRIPT node (or for goog.loadModule, the body of the
// function) that contains the module contents. For recognizing top level names. Changes when // function) that contains the module contents. For recognizing top level names. Changes when
Expand Down Expand Up @@ -297,6 +303,12 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) {
} }
break; break;


case GETPROP:
if (isExportPropertyAssignment(n)) {
recordExportsPropertyAssignment(n);
}
break;

case STRING_KEY: case STRING_KEY:
// Short objects must be converted first, so that we can rewrite module-global names. // Short objects must be converted first, so that we can rewrite module-global names.
if (currentScript.isModule) { if (currentScript.isModule) {
Expand Down Expand Up @@ -759,16 +771,15 @@ private void rewriteShortObjectKey(Node n) {


private void maybeRecordExportDeclaration(NodeTraversal t, Node n) { private void maybeRecordExportDeclaration(NodeTraversal t, Node n) {
if (!currentScript.isModule if (!currentScript.isModule
|| currentScript.declareLegacyNamespace
|| !n.getString().equals("exports") || !n.getString().equals("exports")
|| !isAssignTarget(n)) { || !isAssignTarget(n)) {
return; return;
} }


Preconditions.checkState(currentScript.defaultExportLocalName == null); Preconditions.checkState(currentScript.defaultExportLocalName == null);
currentScript.defaultExportRhs = n.getNext(); Node exportRhs = n.getNext();
if (currentScript.defaultExportRhs.isName() && !currentScript.declareLegacyNamespace) { if (exportRhs.isName() && !currentScript.declareLegacyNamespace) {
String exportedName = currentScript.defaultExportRhs.getString(); String exportedName = exportRhs.getString();
Var var = t.getScope().getVar(exportedName); Var var = t.getScope().getVar(exportedName);
// If rhs is the short name from an import, then we can't do the uninlining. // If rhs is the short name from an import, then we can't do the uninlining.
if (var != null if (var != null
Expand All @@ -777,8 +788,16 @@ private void maybeRecordExportDeclaration(NodeTraversal t, Node n) {
currentScript.defaultExportLocalName = exportedName; currentScript.defaultExportLocalName = exportedName;
recordNameToInline(exportedName, currentScript.getBinaryNamespace()); recordNameToInline(exportedName, currentScript.getBinaryNamespace());
} }
} else if (exportRhs.isObjectLit()) {
for (Node key = exportRhs.getFirstChild(); key != null; key = key.getNext()) {
if (key.isStringKey()) {
String exportName = key.getString();
currentScript.namedExports.add(exportName);
}
}
} }


currentScript.defaultExportRhs = exportRhs;
currentScript.willCreateExportsObject = true; currentScript.willCreateExportsObject = true;
return; return;
} }
Expand Down Expand Up @@ -866,7 +885,7 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
recordNameToInline(aliasName, exportedNamespace); recordNameToInline(aliasName, exportedNamespace);
} else if (lhs.isDestructuringLhs() && lhs.getFirstChild().isObjectPattern()) { } else if (lhs.isDestructuringLhs() && lhs.getFirstChild().isObjectPattern()) {
// `const {Foo}` case // `const {Foo}` case
maybeWarnForInvalidDestructuring(t, lhs, legacyNamespace); maybeWarnForInvalidDestructuring(t, lhs.getParent(), legacyNamespace);
for (Node importSpec : lhs.getFirstChild().children()) { for (Node importSpec : lhs.getFirstChild().children()) {
String importedProperty = importSpec.getString(); String importedProperty = importSpec.getString();
String aliasName = String aliasName =
Expand Down Expand Up @@ -917,16 +936,27 @@ && isTopLevel(t, statementNode, ScopeType.EXEC_CONTEXT)) {
} }
} }


// These restrictions are in place to make it easier to migrate goog.modules to ES6 modules,
// by structuring the imports/exports in a consistent way.
private void maybeWarnForInvalidDestructuring( private void maybeWarnForInvalidDestructuring(
NodeTraversal t, Node importNode, String importedNamespace) { NodeTraversal t, Node importNode, String importedNamespace) {
// This restriction is in place to make it easier to migrate goog.modules to ES6 modules, Preconditions.checkArgument(importNode.getFirstChild().isDestructuringLhs());
// by structuring the imports/exports in a consistent way.
ScriptDescription importedModule = ScriptDescription importedModule =
rewriteState.scriptDescriptionsByGoogModuleNamespace.get(importedNamespace); rewriteState.scriptDescriptionsByGoogModuleNamespace.get(importedNamespace);
if (importedModule != null if (importedModule == null) {
&& importedModule.defaultExportRhs != null // Don't know enough to give a good warning here.
&& !importedModule.defaultExportRhs.isObjectLit()) { return;
t.report(importNode, ILLEGAL_DESTRUCTURING_IMPORT); }
if (importedModule.defaultExportRhs != null && !importedModule.defaultExportRhs.isObjectLit()) {
t.report(importNode, ILLEGAL_DESTRUCTURING_DEFAULT_EXPORT);
return;
}
Node objPattern = importNode.getFirstFirstChild();
for (Node key = objPattern.getFirstChild(); key != null; key = key.getNext()) {
String exportName = key.getString();
if (!importedModule.namedExports.contains(exportName)) {
t.report(importNode, ILLEGAL_DESTRUCTURING_NOT_EXPORTED, exportName, importedNamespace);
}
} }
} }


Expand Down Expand Up @@ -957,6 +987,21 @@ private void updateGoogModuleGetCall(Node call) {
compiler.reportCodeChange(); compiler.reportCodeChange();
} }


private void recordExportsPropertyAssignment(Node getpropNode) {
if (!currentScript.isModule) {
return;
}

Node parent = getpropNode.getParent();
Preconditions.checkState(parent.isAssign() || parent.isExprResult(), parent);

Node exportsNameNode = getpropNode.getFirstChild();
Preconditions.checkState(exportsNameNode.getString().equals("exports"));

String exportName = getpropNode.getLastChild().getString();
currentScript.namedExports.add(exportName);
}

private void updateExportsPropertyAssignment(Node getpropNode) { private void updateExportsPropertyAssignment(Node getpropNode) {
if (!currentScript.isModule) { if (!currentScript.isModule) {
return; return;
Expand Down
76 changes: 71 additions & 5 deletions test/com/google/javascript/jscomp/ClosureRewriteModuleTest.java
View file
Expand Up @@ -17,7 +17,8 @@


import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_MODULE; import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_MODULE;
import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_NAMESPACE; import static com.google.javascript.jscomp.ClosureRewriteModule.DUPLICATE_NAMESPACE;
import static com.google.javascript.jscomp.ClosureRewriteModule.ILLEGAL_DESTRUCTURING_IMPORT; import static com.google.javascript.jscomp.ClosureRewriteModule.ILLEGAL_DESTRUCTURING_DEFAULT_EXPORT;
import static com.google.javascript.jscomp.ClosureRewriteModule.ILLEGAL_DESTRUCTURING_NOT_EXPORTED;
import static com.google.javascript.jscomp.ClosureRewriteModule.IMPORT_INLINING_SHADOWS_VAR; import static com.google.javascript.jscomp.ClosureRewriteModule.IMPORT_INLINING_SHADOWS_VAR;
import static com.google.javascript.jscomp.ClosureRewriteModule.INVALID_EXPORT_COMPUTED_PROPERTY; import static com.google.javascript.jscomp.ClosureRewriteModule.INVALID_EXPORT_COMPUTED_PROPERTY;
import static com.google.javascript.jscomp.ClosureRewriteModule.INVALID_FORWARD_DECLARE_NAMESPACE; import static com.google.javascript.jscomp.ClosureRewriteModule.INVALID_FORWARD_DECLARE_NAMESPACE;
Expand Down Expand Up @@ -227,6 +228,57 @@ public void testDestructuringImports() {
"/** @type {module$exports$ns$b.Foo} */", "/** @type {module$exports$ns$b.Foo} */",
"var module$contents$ns$a_f = new module$exports$ns$b.Foo;")}); "var module$contents$ns$a_f = new module$exports$ns$b.Foo;")});


// TODO(blickly): We don't want any module$contents variables for this definition of Foo
testEs6(
new String[] {
"goog.module('modA'); class Foo {} exports = {Foo};",
LINE_JOINER.join(
"goog.module('modB');",
"",
"var {Foo} = goog.require('modA');",
"",
"/** @type {Foo} */",
"var f = new Foo;")
},
new String[] {
LINE_JOINER.join(
"class module$contents$modA_Foo {}",
"/** @const */ var module$exports$modA = {",
" /** @const */ Foo: module$contents$modA_Foo",
"};"),
LINE_JOINER.join(
"/** @const */ var module$exports$modB = {}",
"/** @type {module$exports$modA.Foo} */",
"var module$contents$modB_f = new module$exports$modA.Foo;")});

testEs6(
new String[] {
LINE_JOINER.join(
"goog.module('modA');",
"goog.module.declareLegacyNamespace();",
"",
"class Foo {}",
"exports = {Foo};"),
LINE_JOINER.join(
"goog.module('modB');",
"",
"var {Foo} = goog.require('modA');",
"",
"/** @type {Foo} */",
"var f = new Foo;")
},
new String[] {
LINE_JOINER.join(
"goog.provide('modA');",
"class module$contents$modA_Foo {}",
"/** @const */ modA = {",
" /** @const */ Foo: module$contents$modA_Foo",
"};"),
LINE_JOINER.join(
"/** @const */ var module$exports$modB = {}",
"/** @type {modA.Foo} */",
"var module$contents$modB_f = new modA.Foo;")});

} }


public void testIllegalDestructuringImports() { public void testIllegalDestructuringImports() {
Expand All @@ -244,7 +296,7 @@ public void testIllegalDestructuringImports() {
" method();", " method();",
"}") "}")
}, },
ILLEGAL_DESTRUCTURING_IMPORT); ILLEGAL_DESTRUCTURING_DEFAULT_EXPORT);


testErrorEs6( testErrorEs6(
new String[] { new String[] {
Expand All @@ -258,7 +310,21 @@ public void testIllegalDestructuringImports() {
" method();", " method();",
"}") "}")
}, },
ILLEGAL_DESTRUCTURING_IMPORT); ILLEGAL_DESTRUCTURING_DEFAULT_EXPORT);

testErrorEs6(
new String[] {
LINE_JOINER.join(
"goog.module('p.A');",
"",
"/** @constructor */ exports.Foo = class {};",
"/** @constructor */ exports.Bar = class {};"),
LINE_JOINER.join(
"goog.module('p.C');",
"",
"var {Baz} = goog.require('p.A');")
},
ILLEGAL_DESTRUCTURING_NOT_EXPORTED);


// TODO(blickly): We should warn for this as well, but it's harder to detect. // TODO(blickly): We should warn for this as well, but it's harder to detect.
testEs6( testEs6(
Expand Down Expand Up @@ -1683,8 +1749,8 @@ public void testRewriteGoogModuleAliases6() {
}, },
new String[] { new String[] {
LINE_JOINER.join( LINE_JOINER.join(
"/** @const */ var module$exports$base = {};", "/** @const */ var module$exports$base = {};",
"/** @constructor */ module$exports$base.Foo = function() {};"), "/** @constructor */ module$exports$base.Foo = function() {};"),
"/** @const */ var module$exports$FooWrapper = module$exports$base.Foo;", "/** @const */ var module$exports$FooWrapper = module$exports$base.Foo;",
}); });
} }
Expand Down

0 comments on commit a398920

Please sign in to comment.