Skip to content

Commit

Permalink
Declare externs on the global this when window is not present.
Browse files Browse the repository at this point in the history
This fix an issue where externs weren't assumed to be propeties on any object
when `window` is absent from the externs. This could cause the compiler to
improperly rename properties which are actually references to externs.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=195872147
  • Loading branch information
kevinoconnor7 authored and tjgq committed May 9, 2018
1 parent b79af55 commit 6df0ee1
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 25 deletions.
Expand Up @@ -31,6 +31,8 @@
*/ */
class DeclaredGlobalExternsOnWindow implements CompilerPass, NodeTraversal.Callback { class DeclaredGlobalExternsOnWindow implements CompilerPass, NodeTraversal.Callback {


private static final String WINDOW_NAME = "window";

private final AbstractCompiler compiler; private final AbstractCompiler compiler;
private final Set<Node> nodes = new LinkedHashSet<>(); private final Set<Node> nodes = new LinkedHashSet<>();


Expand All @@ -48,23 +50,21 @@ public void process(Node externs, Node root) {
} }


private void addWindowProperties() { private void addWindowProperties() {
if (!nodes.isEmpty() && windowInExterns) { if (!nodes.isEmpty()) {
for (Node node : nodes) { for (Node node : nodes) {
addExtern(node); addExtern(node, windowInExterns);
compiler.reportChangeToEnclosingScope(node); compiler.reportChangeToEnclosingScope(node);
} }
} }
} }


private static void addExtern(Node node) { private static void addExtern(Node node, boolean defineOnWindow) {
String name = node.getString(); String name = node.getString();
JSDocInfo oldJSDocInfo = NodeUtil.getBestJSDocInfo(node); JSDocInfo oldJSDocInfo = NodeUtil.getBestJSDocInfo(node);


// TODO(tbreisacher): Consider adding externs to 'this' instead of 'window', Node globalRef = defineOnWindow ? IR.name(WINDOW_NAME) : IR.thisNode();
// for environments where the global object is not called 'window.'
Node window = IR.name("window");
Node string = IR.string(name); Node string = IR.string(name);
Node getprop = IR.getprop(window, string); Node getprop = IR.getprop(globalRef, string);
Node newNode = getprop; Node newNode = getprop;


if (oldJSDocInfo != null) { if (oldJSDocInfo != null) {
Expand Down Expand Up @@ -131,7 +131,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
nodes.add(n.getFirstChild()); nodes.add(n.getFirstChild());
} else if (n.isVar()) { } else if (n.isVar()) {
for (Node c : n.children()) { for (Node c : n.children()) {
if (c.getString().equals("window")) { if (c.getString().equals(WINDOW_NAME)) {
windowInExterns = true; windowInExterns = true;
continue; continue;
} }
Expand Down
Expand Up @@ -43,9 +43,9 @@ public void testWindowProperty1a() {
testExternChanges("var window; var a;", "", "var window;var a;window.a"); testExternChanges("var window; var a;", "", "var window;var a;window.a");
} }


// No "var window;" so this is a no-op. // No "var window;" so use "this" instead.
public void testWindowProperty1b() { public void testWindowProperty1b() {
testExternChanges("var a", "", "var a"); testExternChanges("var a;", "", "var a;this.a");
} }


public void testWindowProperty2() { public void testWindowProperty2() {
Expand All @@ -57,9 +57,9 @@ public void testWindowProperty3a() {
"var window;function f(){}window.f;"); "var window;function f(){}window.f;");
} }


// No "var window;" so this is a no-op. // No "var window;" so use "this" instead.
public void testWindowProperty3b() { public void testWindowProperty3b() {
testExternChanges("function f() {}", "var b", "function f(){}"); testExternChanges("function f() {}", "var b", "function f(){}this.f");
} }


public void testWindowProperty4() { public void testWindowProperty4() {
Expand All @@ -73,9 +73,9 @@ public void testWindowProperty5a() {
"var window;var x=function(){};window.x;"); "var window;var x=function(){};window.x;");
} }


// No "var window;" so this is a no-op. // No "var window;" so use "this" instead.
public void testWindowProperty5b() { public void testWindowProperty5b() {
testExternChanges("var x = function f() {}", "var b", "var x=function f(){}"); testExternChanges("var x = function f() {};", "var b", "var x=function f(){};this.x");
} }


public void testWindowProperty5c() { public void testWindowProperty5c() {
Expand Down
11 changes: 5 additions & 6 deletions test/com/google/javascript/jscomp/IntegrationTest.java
Expand Up @@ -2013,11 +2013,11 @@ public void testClassWithGettersIsRemoved() {
// Investigate why are they kept when ran together with other passes. // Investigate why are they kept when ran together with other passes.
String expected = String expected =
LINE_JOINER.join( LINE_JOINER.join(
"('undefined'!=typeof window&&window===this?this:'undefined'!=typeof ", "('undefined'!=typeof window&&window===this?this:'undefined'!=typeof ",
"global&&null!=global?global:this).", "global&&null!=global?global:this).",
"c.defineProperties(function() {},", "Object.defineProperties(function() {},",
"{e:{a:!0,b:!0,d:function(){}},", // renamed from init "{d:{a:!0,b:!0,c:function(){}},", // renamed from init
"f:{a:!0,b:!0,g:function(){}}})"); // renamed from prop "e:{a:!0,b:!0,f:function(){}}})"); // renamed from prop


options.setLanguageIn(LanguageMode.ECMASCRIPT_2015); options.setLanguageIn(LanguageMode.ECMASCRIPT_2015);
options.setLanguageOut(LanguageMode.ECMASCRIPT5); options.setLanguageOut(LanguageMode.ECMASCRIPT5);
Expand Down Expand Up @@ -5141,7 +5141,6 @@ protected CompilerOptions createCompilerOptions() {
options.setDevMode(DevMode.EVERY_PASS); options.setDevMode(DevMode.EVERY_PASS);
options.setCodingConvention(new GoogleCodingConvention()); options.setCodingConvention(new GoogleCodingConvention());
options.setRenamePrefixNamespaceAssumeCrossModuleNames(true); options.setRenamePrefixNamespaceAssumeCrossModuleNames(true);
options.declaredGlobalExternsOnWindow = false;
return options; return options;
} }
} }
21 changes: 16 additions & 5 deletions test/com/google/javascript/jscomp/SymbolTableTest.java
Expand Up @@ -89,8 +89,9 @@ public void testGlobalVar() throws Exception {
} }


public void testGlobalThisReferences() throws Exception { public void testGlobalThisReferences() throws Exception {
SymbolTable table = SymbolTable table = createSymbolTable(
createSymbolTable("var x = this; function f() { return this + this + this; }"); "var x = this; function f() { return this + this + this; }",
/* externsCode= */ "");


Symbol global = getGlobalVar(table, "*global*"); Symbol global = getGlobalVar(table, "*global*");
assertNotNull(global); assertNotNull(global);
Expand All @@ -101,7 +102,7 @@ public void testGlobalThisReferences() throws Exception {


public void testGlobalThisReferences2() throws Exception { public void testGlobalThisReferences2() throws Exception {
// Make sure the global this is declared, even if it isn't referenced. // Make sure the global this is declared, even if it isn't referenced.
SymbolTable table = createSymbolTable(""); SymbolTable table = createSymbolTable("", /* externsCode= */ "");


Symbol global = getGlobalVar(table, "*global*"); Symbol global = getGlobalVar(table, "*global*");
assertNotNull(global); assertNotNull(global);
Expand All @@ -111,7 +112,8 @@ public void testGlobalThisReferences2() throws Exception {
} }


public void testGlobalThisReferences3() throws Exception { public void testGlobalThisReferences3() throws Exception {
SymbolTable table = createSymbolTable("this.foo = {}; this.foo.bar = {};"); SymbolTable table =
createSymbolTable("this.foo = {}; this.foo.bar = {};", /* externsCode= */ "");


Symbol global = getGlobalVar(table, "*global*"); Symbol global = getGlobalVar(table, "*global*");
assertNotNull(global); assertNotNull(global);
Expand Down Expand Up @@ -343,7 +345,7 @@ public void testGlobalVarInExterns() throws Exception {
SymbolTable table = createSymbolTable("customExternFn(1);"); SymbolTable table = createSymbolTable("customExternFn(1);");
Symbol fn = getGlobalVar(table, "customExternFn"); Symbol fn = getGlobalVar(table, "customExternFn");
List<Reference> refs = table.getReferenceList(fn); List<Reference> refs = table.getReferenceList(fn);
assertThat(refs).hasSize(2); assertThat(refs).hasSize(3);


SymbolScope scope = table.getEnclosingScope(refs.get(0).getNode()); SymbolScope scope = table.getEnclosingScope(refs.get(0).getNode());
assertTrue(scope.isGlobalScope()); assertTrue(scope.isGlobalScope());
Expand Down Expand Up @@ -1184,6 +1186,15 @@ private SymbolTable createSymbolTable(String input) {
return assertSymbolTableValid(compiler.buildKnownSymbolTable()); return assertSymbolTableValid(compiler.buildKnownSymbolTable());
} }


private SymbolTable createSymbolTable(String input, String externsCode) {
List<SourceFile> inputs = ImmutableList.of(SourceFile.fromCode("in1", input));
List<SourceFile> externs = ImmutableList.of(SourceFile.fromCode("externs1", externsCode));

Compiler compiler = new Compiler(new BlackHoleErrorManager());
compiler.compile(externs, inputs, options);
return assertSymbolTableValid(compiler.buildKnownSymbolTable());
}

/** /**
* Asserts that the symbol table meets some invariants. Returns the same table for easy chaining. * Asserts that the symbol table meets some invariants. Returns the same table for easy chaining.
*/ */
Expand Down

0 comments on commit 6df0ee1

Please sign in to comment.