Rollup silently removes undefined variables from final build

[nebrelbug]

• Rollup Version:1.19.4
• Operating System (or Browser):Windows 10
• Node Version:10.15.1

How Do We Reproduce?

Expected Behavior

Rollup should generate code including the randomstuff variable, or at least throw an error

Actual Behavior

https://rollupjs.org/repl/?version=1.19.4&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmltcG9ydCUyMG1heCUyMGZyb20lMjAnLiUyRm1hdGhzJyU1Q25leHBvcnQlMjBkZWZhdWx0JTIwZnVuY3Rpb24lMjAoYSUyQyUyMGIpJTIwJTdCJTVDbiUyMCUyMHJldHVybiUyMG1heChhJTJDJTIwYiUyQyUyMHJhbmRvbXN0dWZmKSU1Q24lN0QlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlMkMlN0IlMjJuYW1lJTIyJTNBJTIybWF0aHMuanMlMjIlMkMlMjJjb2RlJTIyJTNBJTIyZXhwb3J0JTIwZGVmYXVsdCUyMGZ1bmN0aW9uJTIwbWF4JTIwKGElMkMlMjBiKSUyMCU3QiU1Q24lMjAlMjByZXR1cm4lMjBNYXRoLm1heChhJTJDJTIwYiklNUNuJTdEJTVDbiUyMiUyQyUyMmlzRW50cnklMjIlM0FmYWxzZSU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJ1bWQlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==

[LongTengDao]

I sweated a cold sweat when I see this issue, until after test that, if ...rest of arguments be used in max, randomstuff won't be removed anymore.

So, does this cause any problem?

[lukastaegert]

The parameter is removed because it does not have an effect. Why would you want to keep it?

[nebrelbug]

@lukastaegert actually, I noticed the error when I was debugging tests of my npm package, rollup-closure-compile.

The tests used to check that if there was an undefined variable and the compilation level was ADVANCED, compilation would err. This test worked fine until I updated Rollup -- I'm guessing this is a fairly new behaviour?

Now that you explain, the behaviour actually makes a lot of sense. Can it be turned off by a certain flag?

[LongTengDao]

maybe set options.treeshake = false?

BTW: if you just want to check global variable leak, you can use acorn-globals as a single step.

[kzc]

How unknown globals should be handled by Rollup is an ongoing topic of debate.

Rollup presently silently drops such code even though it would be an error at runtime:

$ cat unknown.js 
function max(a, b) {
    return Math.max(a, b);
}
console.log(max(1, 2));
console.log(max(3, 4, String, Number, Int32Array));
console.log(max(5, 6, String, randomstuff, Int32Array));
let known = 123;
known ? "ok" : "fine";
unknown ? 2 : 3;

$ cat unknown.js | node
2
4
[stdin]:6
console.log(max(5, 6, String, randomstuff, Int32Array));
                              ^
ReferenceError: randomstuff is not defined

I think the user should be at least given the option to have Rollup warn about these cases.

Here's a proof of concept patch that warns when unknown globals are encountered. It is not gated by a new config or CLI option, but that could be added easily enough.

diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts
index 33ea314..bc26af2 100644
--- a/src/ast/nodes/Identifier.ts
+++ b/src/ast/nodes/Identifier.ts
@@ -15,6 +15,7 @@ import { ExpressionEntity } from './shared/Expression';
 import { ExpressionNode, NodeBase } from './shared/Node';
 import { PatternNode } from './shared/Pattern';
 import SpreadElement from './SpreadElement';
+import { isKnownGlobal } from '../../utils/identifierHelpers';
 
 export type IdentifierWithVariable = Identifier & { variable: Variable };
 
@@ -45,6 +46,17 @@ export default class Identifier extends NodeBase implements PatternNode {
 		) {
 			this.variable.consolidateInitializers();
 		}
+		if (
+			this.variable
+			&& !this.variable.module
+			&& !this.variable.init
+			&& !isKnownGlobal(this.variable.name)
+		) {
+			this.context.warn({
+				code: "UNKNOWN_GLOBAL",
+				message: "Unknown global: " + this.variable.name
+			}, this.start);
+		}
 	}
 
 	declare(kind: string, init: ExpressionEntity) {
diff --git a/src/utils/identifierHelpers.ts b/src/utils/identifierHelpers.ts
index 38463f0..f9c9108 100644
--- a/src/utils/identifierHelpers.ts
+++ b/src/utils/identifierHelpers.ts
@@ -19,6 +19,19 @@ export function isLegal(str: string): boolean {
 	return !illegalCharacters.test(str);
 }
 
+const knownGlobals = Object.create(null);
+builtins.forEach(word => knownGlobals[word] = true);
+
+// Add other known globals as the need arises.
+// TODO: Should have a way to customize this list
+// as browser targets would not want NodeJS globals.
+'BigInt Buffer clearTimeout console define exports global module process require setTimeout self window'
+	.split(' ').forEach(word => knownGlobals[word] = true);
+
+export function isKnownGlobal(str: string): boolean {
+	return knownGlobals[str];
+}
+
 export function makeLegal(str: string): string {
 	str = str.replace(/-(\w)/g, (_, letter) => letter.toUpperCase()).replace(illegalCharacters, '_');

The patched Rollup with warnings enabled:

$ dist/bin/rollup unknown.js -f es

unknown.js → stdout...
function max(a, b) {
    return Math.max(a, b);
}
console.log(max(1, 2));
console.log(max(3, 4));
console.log(max(5, 6));
(!) Unknown global: randomstuff
unknown.js: (6:30)
4: console.log(max(1, 2));
5: console.log(max(3, 4, String, Number, Int32Array));
6: console.log(max(5, 6, String, randomstuff, Int32Array));
                                 ^
7: let known = 123;
8: known ? "ok" : "fine";
(!) Unknown global: unknown
unknown.js: (9:0)
7: let known = 123;
8: known ? "ok" : "fine";
9: unknown ? 2 : 3;
   ^
created stdout in 59ms

When warnings are disabled, the patched Rollup behaves the same way it used to:

$ dist/bin/rollup unknown.js -f es --silent
function max(a, b) {
    return Math.max(a, b);
}
console.log(max(1, 2));
console.log(max(3, 4));
console.log(max(5, 6));

$ dist/bin/rollup unknown.js -f es --silent | node
2
4
6

Edit: patch was revised.

[kzc]

Here's an alternative proof of concept patch adding a new treeshake option unknownGlobalSideEffects, which is disabled by default. Unlike the previous patch above, it does not emit warnings. If unknownGlobalSideEffects is enabled it will retain code involving unknown globals. I prefer this solution to the previous one as it's strictly an opt in and has no change to default rollup behavior.

diff --git a/src/Graph.ts b/src/Graph.ts
index 871994d..5de1a88 100644
--- a/src/Graph.ts
+++ b/src/Graph.ts
@@ -118,13 +118,16 @@ export default class Graph {
 							(options.treeshake as TreeshakingOptions).propertyReadSideEffects !== false,
 						pureExternalModules: (options.treeshake as TreeshakingOptions).pureExternalModules,
 						tryCatchDeoptimization:
-							(options.treeshake as TreeshakingOptions).tryCatchDeoptimization !== false
+							(options.treeshake as TreeshakingOptions).tryCatchDeoptimization !== false,
+						unknownGlobalSideEffects:
+							!!(options.treeshake as TreeshakingOptions).unknownGlobalSideEffects,
 				  }
 				: {
 						annotations: true,
 						moduleSideEffects: true,
 						propertyReadSideEffects: true,
-						tryCatchDeoptimization: true
+						tryCatchDeoptimization: true,
+						unknownGlobalSideEffects: false,
 				  };
 			if (typeof this.treeshakingOptions.pureExternalModules !== 'undefined') {
 				this.warnDeprecation(
diff --git a/src/Module.ts b/src/Module.ts
index 63302db..a762875 100644
--- a/src/Module.ts
+++ b/src/Module.ts
@@ -110,6 +110,7 @@ export interface AstContext {
 	traceVariable: (name: string) => Variable | null;
 	treeshake: boolean;
 	tryCatchDeoptimization: boolean;
+	unknownGlobalSideEffects: boolean;
 	usesTopLevelAwait: boolean;
 	warn: (warning: RollupWarning, pos: number) => void;
 	warnDeprecation: (deprecation: string | RollupWarning, activeDeprecation: boolean) => void;
@@ -603,6 +604,8 @@ export default class Module {
 			treeshake: !!this.graph.treeshakingOptions,
 			tryCatchDeoptimization: (!this.graph.treeshakingOptions ||
 				this.graph.treeshakingOptions.tryCatchDeoptimization) as boolean,
+			unknownGlobalSideEffects: (!this.graph.treeshakingOptions ||
+				this.graph.treeshakingOptions.unknownGlobalSideEffects) as boolean,
 			usesTopLevelAwait: false,
 			warn: this.warn.bind(this),
 			warnDeprecation: this.graph.warnDeprecation.bind(this.graph)
diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts
index 33ea314..3b1d37d 100644
--- a/src/ast/nodes/Identifier.ts
+++ b/src/ast/nodes/Identifier.ts
@@ -15,6 +15,7 @@ import { ExpressionEntity } from './shared/Expression';
 import { ExpressionNode, NodeBase } from './shared/Node';
 import { PatternNode } from './shared/Pattern';
 import SpreadElement from './SpreadElement';
+import { isKnownGlobal } from '../../utils/identifierHelpers';
 
 export type IdentifierWithVariable = Identifier & { variable: Variable };
 
@@ -24,6 +25,7 @@ export default class Identifier extends NodeBase implements PatternNode {
 
 	variable: Variable | null = null;
 	private bound = false;
+	private unknownGlobal = false;
 
 	addExportedVariables(variables: Variable[]): void {
 		if (this.variable !== null && this.variable.exportName) {
@@ -45,6 +47,14 @@ export default class Identifier extends NodeBase implements PatternNode {
 		) {
 			this.variable.consolidateInitializers();
 		}
+		if (
+			this.variable
+			&& !this.variable.module
+			&& !this.variable.init
+			&& !isKnownGlobal(this.variable.name)
+		) {
+			this.unknownGlobal = true;
+		}
 	}
 
 	declare(kind: string, init: ExpressionEntity) {
@@ -106,6 +116,10 @@ export default class Identifier extends NodeBase implements PatternNode {
 		return UNKNOWN_EXPRESSION;
 	}
 
+	hasEffects(options: ExecutionPathOptions): boolean {
+		return this.context.unknownGlobalSideEffects && this.unknownGlobal;
+	}
+
 	hasEffectsWhenAccessedAtPath(path: ObjectPath, options: ExecutionPathOptions): boolean {
 		return this.variable !== null && this.variable.hasEffectsWhenAccessedAtPath(path, options);
 	}
diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts
index 41d9b5c..3b7cb02 100644
--- a/src/rollup/types.d.ts
+++ b/src/rollup/types.d.ts
@@ -384,6 +384,7 @@ export interface TreeshakingOptions {
 	/** @deprecated Use `moduleSideEffects` instead */
 	pureExternalModules?: PureModulesOption;
 	tryCatchDeoptimization?: boolean;
+	unknownGlobalSideEffects?: boolean;
 }
 
 export type GetManualChunk = (id: string) => string | null | undefined;
diff --git a/src/utils/identifierHelpers.ts b/src/utils/identifierHelpers.ts
index 38463f0..f9c9108 100644
--- a/src/utils/identifierHelpers.ts
+++ b/src/utils/identifierHelpers.ts
@@ -19,6 +19,19 @@ export function isLegal(str: string): boolean {
 	return !illegalCharacters.test(str);
 }
 
+const knownGlobals = Object.create(null);
+builtins.forEach(word => knownGlobals[word] = true);
+
+// Add other known globals as the need arises.
+// TODO: Should have a way to customize this list
+// as browser targets would not want NodeJS globals.
+'BigInt Buffer clearTimeout console define exports global module process require setTimeout self window'
+	.split(' ').forEach(word => knownGlobals[word] = true);
+
+export function isKnownGlobal(str: string): boolean {
+	return knownGlobals[str];
+}
+
 export function makeLegal(str: string): string {
 	str = str.replace(/-(\w)/g, (_, letter) => letter.toUpperCase()).replace(illegalCharacters, '_');

Patched rollup with treeshaking disabled showing the input code:

$ dist/bin/rollup unknown.js -f es --silent --no-treeshake
function max(a, b) {
    return Math.max(a, b);
}
console.log(max(1, 2));
console.log(max(3, 4, String, Number, Int32Array));
console.log(max(5, 6, String, randomstuff, Int32Array));
let known = 123;
known ? "ok" : "fine";
unknown ? 2 : 3;

Patched rollup with default treeshaking behaving exactly as it did in the past:

$ dist/bin/rollup unknown.js -f es --silent
function max(a, b) {
    return Math.max(a, b);
}
console.log(max(1, 2));
console.log(max(3, 4));
console.log(max(5, 6));

Patched rollup with unknown global side effects explicitly enabled:

$ dist/bin/rollup unknown.js -f es --silent --treeshake.unknownGlobalSideEffects
function max(a, b) {
    return Math.max(a, b);
}
console.log(max(1, 2));
console.log(max(3, 4));
console.log(max(5, 6, String, randomstuff));
unknown ? 2 : 3;

Edit: patch was revised.

[lukastaegert]

I think I will add someting along @kzc's suggestion, maybe even enabled by default. I know I originally had a stance against it but then again, people usually care about functionality first (even though here this is usually just bugs in the code) and optimization later. Maybe at some point we can add presets to the treeshaking option like a preset: "recommended", which would include this option + side-effect-free getters as a selection of things that will work well for most code-bases.

[lukastaegert]

Fix at #3068