Skip to content

Commit

Permalink
Automated g4 rollback of changelist 195893567.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Clean rollforward now that clutz has been patched to handle this behavior.

*** Original change description ***

Automated g4 rollback of changelist 195872147.

*** Reason for rollback ***

Breaks some typescript builds

*** Original change description ***

Declare externs on the global `this` when window is not present.

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=196035069
  • Loading branch information
kevinoconnor7 authored and tjgq committed May 10, 2018
1 parent 8616f17 commit f3bc96d
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 {

private static final String WINDOW_NAME = "window";

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

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

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

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

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

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

public void testWindowProperty2() {
Expand All @@ -57,9 +57,9 @@ public void testWindowProperty3a() {
"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() {
testExternChanges("function f() {}", "var b", "function f(){}");
testExternChanges("function f() {}", "var b", "function f(){}this.f");
}

public void testWindowProperty4() {
Expand All @@ -73,9 +73,9 @@ public void testWindowProperty5a() {
"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() {
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() {
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.
String expected =
LINE_JOINER.join(
"('undefined'!=typeof window&&window===this?this:'undefined'!=typeof ",
"global&&null!=global?global:this).",
"c.defineProperties(function() {},",
"{e:{a:!0,b:!0,d:function(){}},", // renamed from init
"f:{a:!0,b:!0,g:function(){}}})"); // renamed from prop
"('undefined'!=typeof window&&window===this?this:'undefined'!=typeof ",
"global&&null!=global?global:this).",
"Object.defineProperties(function() {},",
"{d:{a:!0,b:!0,c:function(){}},", // renamed from init
"e:{a:!0,b:!0,f:function(){}}})"); // renamed from prop

options.setLanguageIn(LanguageMode.ECMASCRIPT_2015);
options.setLanguageOut(LanguageMode.ECMASCRIPT5);
Expand Down Expand Up @@ -5141,7 +5141,6 @@ protected CompilerOptions createCompilerOptions() {
options.setDevMode(DevMode.EVERY_PASS);
options.setCodingConvention(new GoogleCodingConvention());
options.setRenamePrefixNamespaceAssumeCrossModuleNames(true);
options.declaredGlobalExternsOnWindow = false;
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 {
SymbolTable table =
createSymbolTable("var x = this; function f() { return this + this + this; }");
SymbolTable table = createSymbolTable(
"var x = this; function f() { return this + this + this; }",
/* externsCode= */ "");

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

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

SymbolScope scope = table.getEnclosingScope(refs.get(0).getNode());
assertTrue(scope.isGlobalScope());
Expand Down Expand Up @@ -1184,6 +1186,15 @@ private SymbolTable createSymbolTable(String input) {
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.
*/
Expand Down

0 comments on commit f3bc96d

Please sign in to comment.