Skip to content

Commit

Permalink
Automated g4 rollback of changelist 195872147.
Browse files Browse the repository at this point in the history
*** 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=195893567
  • Loading branch information
kevinoconnor7 authored and tjgq committed May 9, 2018
1 parent 5557c75 commit 3b55507
Show file tree
Hide file tree
Showing 4 changed files with 25 additions and 35 deletions.
Expand Up @@ -31,8 +31,6 @@
*/ */
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 @@ -50,21 +48,23 @@ public void process(Node externs, Node root) {
} }


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


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


Node globalRef = defineOnWindow ? IR.name(WINDOW_NAME) : IR.thisNode(); // 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 string = IR.string(name); Node string = IR.string(name);
Node getprop = IR.getprop(globalRef, string); Node getprop = IR.getprop(window, 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_NAME)) { if (c.getString().equals("window")) {
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 use "this" instead. // No "var window;" so this is a no-op.
public void testWindowProperty1b() { public void testWindowProperty1b() {
testExternChanges("var a;", "", "var a;this.a"); testExternChanges("var a", "", "var 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 use "this" instead. // No "var window;" so this is a no-op.
public void testWindowProperty3b() { public void testWindowProperty3b() {
testExternChanges("function f() {}", "var b", "function f(){}this.f"); testExternChanges("function f() {}", "var b", "function 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 use "this" instead. // No "var window;" so this is a no-op.
public void testWindowProperty5b() { public void testWindowProperty5b() {
testExternChanges("var x = function f() {};", "var b", "var x=function f(){};this.x"); testExternChanges("var x = function f() {}", "var b", "var x=function f(){}");
} }


public void testWindowProperty5c() { public void testWindowProperty5c() {
Expand Down
11 changes: 6 additions & 5 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).",
"Object.defineProperties(function() {},", "c.defineProperties(function() {},",
"{d:{a:!0,b:!0,c:function(){}},", // renamed from init "{e:{a:!0,b:!0,d:function(){}},", // renamed from init
"e:{a:!0,b:!0,f:function(){}}})"); // renamed from prop "f:{a:!0,b:!0,g: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,6 +5141,7 @@ 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: 5 additions & 16 deletions test/com/google/javascript/jscomp/SymbolTableTest.java
Expand Up @@ -89,9 +89,8 @@ public void testGlobalVar() throws Exception {
} }


public void testGlobalThisReferences() throws Exception { public void testGlobalThisReferences() throws Exception {
SymbolTable table = createSymbolTable( SymbolTable table =
"var x = this; function f() { return this + this + this; }", createSymbolTable("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 @@ -102,7 +101,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("", /* externsCode= */ ""); SymbolTable table = createSymbolTable("");


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


public void testGlobalThisReferences3() throws Exception { public void testGlobalThisReferences3() throws Exception {
SymbolTable table = SymbolTable table = createSymbolTable("this.foo = {}; this.foo.bar = {};");
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 @@ -345,7 +343,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(3); assertThat(refs).hasSize(2);


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 @@ -1186,15 +1184,6 @@ 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 3b55507

Please sign in to comment.