Incorrect scope alteration of function by redeclaring as 'var'

[AshleyScirra]

Edit: this is probably actually just as simple as Closure not recognising "self" as an alias for "window" - see below

Tested with Closure compiler v20200614

in.js:

"use strict";

self.global1 = "foo";

{
	function foo(p) { self["external1"](p) }
	let local = self["external2"]();
	
	console.log(foo(local));
	console.log(foo(local));
}

Command:
java -jar ./closure-compiler.jar --js in.js --js_output_file out.js --compilation_level ADVANCED --language_in ECMASCRIPT_2015 --language_out ECMASCRIPT_2015 --strict_mode_input --formatting PRETTY_PRINT

This produces out.js:

'use strict';
self.a = "foo";
{
  var a = function(c) {  // <-- ERROR
    self.external1(c);
  };
  let b = self.external2();
  console.log(a(b));
  console.log(a(b));
}

Notice that:

• self.global1 was renamed to self.a
• function foo was redeclared as var a
• var a outside a function overwrites the global variable, breaking code relying on self.global1

Note that some brief testing shows that inside the scope block, function foo is scoped to that block only. However a var declaration is global, even inside the scope block (presumably because it's not a function). Therefore by redeclaring a function with var inside a scope block, its scope is unintentionally expanded to global, where it trashes global variables that happen to have the same name.

[AshleyScirra]

I just realised this doesn't reproduce if you use window instead of self. So I guess Closure doesn't recognise that in a browser context self is an alias for window?

[concavelenz]

Thanks for the report. There are a couple of things going on here.

1. Generally, the compiler treats property and variables as being in separate namespaces when renaming. You can avoid this the conflict by using an "output_wrapper" (--isolate_mode=IIFE). It makes a best effort to avoid the conflict with "GatherRawExports" which recognizes window and top but not self currently.

It should be updated to recognize both self and globalThis.

2. "Normalize" was originally written for ES3 code, tries to avoid block scoped function whose behavior was not consistent between browsers and rewrote block scoped function declarations as var declarations . ES5 changed that and specifically banned block scoped functions in strict mode. Assuming strict mode behavior is useful for the compiler, and to foster adopting strict mode, we continued to rewrite block scoped function declarations when the compiler warnings were ignored or suppressed.

With ES2015 and later the behavior is simply wrong, and we should remove the normalization and the behavior is well defined but we might need to do that in a couple of steps to shake out optimizations that are intentionally or not assuming there are no block scoped function declarations.

Potentially, we just rewrite them as "let x = ..." to avoid the scope busting, if we don't do that in global scope (global function declarations appear on the global "this" but "let", "const" and "class" do not.

3. We need to make sure that the "MoveFunctionDeclarations" doesn't also break it, but that is less important

[concavelenz]

This should now be fixed now that b3627df has been committed and will be part of the next release if we don't need to roll it back for some reason.