From e5a5d891d29e46de0c077d74d7b033551a83f211 Mon Sep 17 00:00:00 2001
From: Rich Harris <richard.a.harris@gmail.com>
Date: Fri, 23 Nov 2018 16:21:09 -0500
Subject: [PATCH] unified approach to hoisted functions

---
 src/compile/Component.ts                      |   7 +-
 src/compile/nodes/Action.ts                   |   3 +
 src/compile/nodes/EventHandler.ts             |  26 +--
 src/compile/nodes/shared/Expression.ts        | 149 ++++++++++++++----
 src/compile/render-dom/Block.ts               |   4 +-
 src/compile/render-dom/index.ts               |  10 +-
 .../render-dom/wrappers/Element/index.ts      |  93 ++++-------
 .../wrappers/InlineComponent/index.ts         |  30 ++--
 src/compile/render-dom/wrappers/Window.ts     |  49 +-----
 src/utils/deindent.ts                         |  18 ++-
 10 files changed, 193 insertions(+), 196 deletions(-)

diff --git a/src/compile/Component.ts b/src/compile/Component.ts
index 919cfb1171..8a9169a564 100644
--- a/src/compile/Component.ts
+++ b/src/compile/Component.ts
@@ -54,7 +54,8 @@ export default class Component {
 	writable_declarations: Set<string> = new Set();
 	initialised_declarations: Set<string> = new Set();
 	exports: Array<{ name: string, as: string }> = [];
-	event_handlers: Array<{ name: string, body: string }> = [];
+	partly_hoisted: string[] = [];
+	fully_hoisted: string[] = [];
 
 	code: MagicString;
 
@@ -192,7 +193,9 @@ export default class Component {
 				return { name, alias };
 			});
 
-		const sharedPath = options.shared || 'svelte/internal.js';
+		const sharedPath = typeof options.shared === 'string'
+			? options.shared
+			: 'svelte/internal.js';
 
 		const module = wrapModule(result, format, name, options, banner, sharedPath, importedHelpers, this.imports, this.source);
 
diff --git a/src/compile/nodes/Action.ts b/src/compile/nodes/Action.ts
index 79b8f7bdcb..87565a0151 100644
--- a/src/compile/nodes/Action.ts
+++ b/src/compile/nodes/Action.ts
@@ -5,6 +5,7 @@ export default class Action extends Node {
 	type: 'Action';
 	name: string;
 	expression: Expression;
+	usesContext: boolean;
 
 	constructor(component, parent, scope, info) {
 		super(component, parent, scope, info);
@@ -14,5 +15,7 @@ export default class Action extends Node {
 		this.expression = info.expression
 			? new Expression(component, this, scope, info.expression)
 			: null;
+
+		this.usesContext = this.expression && this.expression.usesContext;
 	}
 }
\ No newline at end of file
diff --git a/src/compile/nodes/EventHandler.ts b/src/compile/nodes/EventHandler.ts
index ff75bf5309..46f8f7ab5d 100644
--- a/src/compile/nodes/EventHandler.ts
+++ b/src/compile/nodes/EventHandler.ts
@@ -27,32 +27,10 @@ export default class EventHandler extends Node {
 		this.modifiers = new Set(info.modifiers);
 
 		if (info.expression) {
-			this.expression = new Expression(component, parent, template_scope, info.expression, true);
+			this.expression = new Expression(component, this, template_scope, info.expression, true);
 			this.snippet = this.expression.snippet;
 
-			let { scope, map } = createScopes(info.expression);
-
-			walk(info.expression, {
-				enter: (node, parent) => {
-					if (map.has(node)) {
-						scope = map.get(node);
-					}
-
-					if (node.type === 'AssignmentExpression') {
-						const { name } = flattenReference(node.left);
-
-						if (!scope.has(name)) {
-							component.instrument(node, parent, name, true);
-						}
-					}
-				},
-
-				leave(node) {
-					if (map.has(node)) {
-						scope = scope.parent;
-					}
-				}
-			});
+			this.usesContext = this.expression.usesContext;
 		} else {
 			component.init_uses_self = true;
 			this.snippet = `e => @bubble($$self, e)`
diff --git a/src/compile/nodes/shared/Expression.ts b/src/compile/nodes/shared/Expression.ts
index 5b43be6c33..b7e973d988 100644
--- a/src/compile/nodes/shared/Expression.ts
+++ b/src/compile/nodes/shared/Expression.ts
@@ -6,6 +6,8 @@ import { createScopes } from '../../../utils/annotateWithScopes';
 import { Node } from '../../../interfaces';
 import addToSet from '../../../utils/addToSet';
 import globalWhitelist from '../../../utils/globalWhitelist';
+import deindent from '../../../utils/deindent';
+import Wrapper from '../../render-dom/wrappers/shared/Wrapper';
 
 const binaryOperators: Record<string, number> = {
 	'**': 15,
@@ -63,12 +65,11 @@ export default class Expression {
 	dependencies: Set<string>;
 	contextual_dependencies: Set<string>;
 
+	declarations: string[] = [];
 	usesContext = false;
 	usesEvent = false;
 
-	thisReferences: Array<{ start: number, end: number }>;
-
-	constructor(component, parent, scope, info, isEventHandler?: boolean) {
+	constructor(component: Component, owner: Wrapper, scope, info, isEventHandler?: boolean) {
 		// TODO revert to direct property access in prod?
 		Object.defineProperties(this, {
 			component: {
@@ -77,19 +78,24 @@ export default class Expression {
 		});
 
 		this.node = info;
-		this.thisReferences = [];
 
 		this.snippet = `[✂${info.start}-${info.end}✂]`;
 
-		const dependencies = new Set();
-		const contextual_dependencies = new Set();
+		const expression_dependencies = new Set();
+		const expression_contextual_dependencies = new Set();
+
+		let dependencies = expression_dependencies;
+		let contextual_dependencies = expression_contextual_dependencies;
 
+		const { declarations } = this;
 		const { code } = component;
 
 		let { map, scope: currentScope } = createScopes(info);
 
 		const expression = this;
-		const isSynthetic = parent.isSynthetic;
+		const isSynthetic = owner.isSynthetic;
+
+		let function_expression;
 
 		walk(info, {
 			enter(node: any, parent: any, key: string) {
@@ -101,24 +107,6 @@ export default class Expression {
 
 				if (map.has(node)) {
 					currentScope = map.get(node);
-					return;
-				}
-
-				if (node.type === 'ThisExpression') {
-					expression.thisReferences.push(node);
-				}
-
-				if (node.type === 'CallExpression') {
-					if (node.callee.type === 'Identifier') {
-						const dependencies_for_invocation = component.findDependenciesForFunctionCall(node.callee.name);
-						if (dependencies_for_invocation) {
-							addToSet(dependencies, dependencies_for_invocation);
-						} else {
-							dependencies.add('$$BAIL$$');
-						}
-					} else {
-						dependencies.add('$$BAIL$$');
-					}
 				}
 
 				if (isReference(node, parent)) {
@@ -127,9 +115,7 @@ export default class Expression {
 					if (currentScope.has(name)) return;
 					if (globalWhitelist.has(name) && component.declarations.indexOf(name) === -1) return;
 
-					expression.usesContext = true;
-
-					if (!isSynthetic && !isEventHandler) {
+					if (!isSynthetic && !function_expression) {
 						// <option> value attribute could be synthetic — avoid double editing
 						code.prependRight(node.start, key === 'key' && parent.shorthand
 							? `${name}: ctx.`
@@ -137,6 +123,8 @@ export default class Expression {
 					}
 
 					if (scope.names.has(name)) {
+						expression.usesContext = true;
+
 						contextual_dependencies.add(name);
 
 						scope.dependenciesForName.get(name).forEach(dependency => {
@@ -156,10 +144,97 @@ export default class Expression {
 
 					this.skip();
 				}
+
+				if (function_expression) {
+					if (node.type === 'AssignmentExpression') {
+						// TODO handle destructuring assignments
+						const { name } = flattenReference(node.left);
+
+						code.prependRight(node.start, `($$make_dirty('${name}'), `);
+						code.appendLeft(node.end, ')');
+					}
+				} else {
+					if (node.type === 'AssignmentExpression') {
+						// TODO should this be a warning/error? `<p>{foo = 1}</p>`
+					}
+
+					if (node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') {
+						function_expression = node;
+						dependencies = new Set();
+						contextual_dependencies = new Set();
+					}
+
+					if (node.type === 'CallExpression') {
+						if (node.callee.type === 'Identifier') {
+							const dependencies_for_invocation = component.findDependenciesForFunctionCall(node.callee.name);
+							if (dependencies_for_invocation) {
+								addToSet(dependencies, dependencies_for_invocation);
+							} else {
+								dependencies.add('$$BAIL$$');
+							}
+						} else {
+							dependencies.add('$$BAIL$$');
+						}
+					}
+				}
 			},
 
-			leave(node: Node, parent: Node) {
+			leave(node: Node) {
 				if (map.has(node)) currentScope = currentScope.parent;
+
+				if (node === function_expression) {
+					const name = component.getUniqueName(get_function_name(node, owner));
+
+					const args = contextual_dependencies.size > 0
+						? [`{ ${[...contextual_dependencies].join(', ')} }`]
+						: [];
+
+					let original_params;
+
+					if (node.params.length > 0) {
+						original_params = code.slice(node.params[0].start, node.params[node.params.length - 1].end);
+						args.push(original_params);
+					}
+
+					let body = code.slice(node.body.start, node.body.end).trim();
+					if (node.body.type !== 'BlockStatement') {
+						body = `{\n\treturn ${body};\n}`;
+					}
+
+					const fn = deindent`
+						function ${name}(${args.join(', ')}) ${body}
+					`;
+
+					if (dependencies.size === 0 && contextual_dependencies.size === 0) {
+						// we can hoist this out of the component completely
+						component.fully_hoisted.push(fn);
+						code.overwrite(node.start, node.end, name);
+					}
+
+					else if (contextual_dependencies.size === 0) {
+						// function can be hoisted inside the component init
+						component.partly_hoisted.push(fn);
+						component.declarations.push(name);
+						code.overwrite(node.start, node.end, `ctx.${name}`);
+					}
+
+					else {
+						// we need a combo block/init recipe
+						component.partly_hoisted.push(fn);
+						component.declarations.push(name);
+						code.overwrite(node.start, node.end, name);
+
+						declarations.push(deindent`
+							function ${name}(${original_params ? '...args' : ''}) {
+								return ctx.${name}(ctx${original_params ? ', ...args' : ''});
+							}
+						`);
+					}
+
+					function_expression = null;
+					dependencies = expression_dependencies;
+					contextual_dependencies = expression_contextual_dependencies;
+				}
 			}
 		});
 
@@ -170,12 +245,16 @@ export default class Expression {
 	getPrecedence() {
 		return this.node.type in precedence ? precedence[this.node.type](this.node) : 0;
 	}
+}
 
-	overwriteThis(name) {
-		this.thisReferences.forEach(ref => {
-			this.component.code.overwrite(ref.start, ref.end, name, {
-				storeName: true
-			});
-		});
+function get_function_name(node, parent) {
+	if (parent.type === 'EventHandler') {
+		return `${parent.name}_handler`;
 	}
+
+	if (parent.type === 'Action') {
+		return `${parent.name}_function`;
+	}
+
+	return 'func';
 }
\ No newline at end of file
diff --git a/src/compile/render-dom/Block.ts b/src/compile/render-dom/Block.ts
index 85e7850ef2..d036fc16da 100644
--- a/src/compile/render-dom/Block.ts
+++ b/src/compile/render-dom/Block.ts
@@ -295,8 +295,8 @@ export default class Block {
 				properties.addBlock(`p: @noop,`);
 			} else {
 				properties.addBlock(deindent`
-					${dev ? 'p: function update' : 'p'}(changed, ${this.maintainContext ? '_ctx' : 'ctx'}) {
-						${this.maintainContext && `ctx = _ctx;`}
+					${dev ? 'p: function update' : 'p'}(changed, ${this.maintainContext ? 'new_ctx' : 'ctx'}) {
+						${this.maintainContext && `ctx = new_ctx;`}
 						${this.builders.update}
 					},
 				`);
diff --git a/src/compile/render-dom/index.ts b/src/compile/render-dom/index.ts
index 98cf6bc8ea..804dcae279 100644
--- a/src/compile/render-dom/index.ts
+++ b/src/compile/render-dom/index.ts
@@ -107,10 +107,6 @@ export default function dom(
 	} else {
 		const refs = Array.from(component.refs);
 
-		const declarations = component.declarations.concat(
-			component.event_handlers.map(handler => handler.name)
-		);
-
 		const superclass = component.alias(options.dev ? '$$ComponentDev' : '$$Component');
 
 		if (options.dev && !options.hydratable) {
@@ -137,11 +133,11 @@ export default function dom(
 
 					${component.javascript || component.exports.map(x => `let ${x.name};`)}
 
-					${component.event_handlers.map(handler => handler.body)}
+					${component.partly_hoisted.length > 0 && component.partly_hoisted.join('\n\n')}
 
 					return [
 						// TODO only what's needed by the template
-						() => ({ ${declarations.join(', ')} }),
+						() => ({ ${component.declarations.join(', ')} }),
 						props => {
 							// TODO only do this for export let|var
 							${(component.exports.map(name =>
@@ -209,6 +205,8 @@ export default function dom(
 		});
 
 		builder.addBlock(deindent`
+			${component.fully_hoisted.length > 0 && component.fully_hoisted.join('\n\n')}
+
 			class ${name} extends ${superclass} {
 				${body.join('\n\n')}
 			}
diff --git a/src/compile/render-dom/wrappers/Element/index.ts b/src/compile/render-dom/wrappers/Element/index.ts
index e6b3a06d72..38257aba0f 100644
--- a/src/compile/render-dom/wrappers/Element/index.ts
+++ b/src/compile/render-dom/wrappers/Element/index.ts
@@ -292,7 +292,8 @@ export default class ElementWrapper extends Wrapper {
 
 		const eventHandlerOrBindingUsesContext = (
 			this.bindings.some(binding => binding.node.usesContext) ||
-			this.node.handlers.some(handler => handler.usesContext)
+			this.node.handlers.some(handler => handler.usesContext) ||
+			this.node.actions.some(action => action.usesContext)
 		);
 
 		if (hasHoistedEventHandlerOrBinding) {
@@ -438,6 +439,7 @@ export default class ElementWrapper extends Wrapper {
 
 		groups.forEach(group => {
 			const handler = block.getUniqueName(`${this.var}_${group.events.join('_')}_handler`);
+			this.renderer.component.declarations.push(handler);
 
 			const needsLock = group.bindings.some(binding => binding.needsLock);
 
@@ -485,37 +487,31 @@ export default class ElementWrapper extends Wrapper {
 					}
 				`);
 
-				this.renderer.component.event_handlers.push({
-					name: handler,
-					body: deindent`
-						function ${handler}({ ${deps.join(', ')} }) {
-							${
-								animation_frame && deindent`
-									cancelAnimationFrame(${animation_frame});
-									if (!${this.var}.paused) ${animation_frame} = requestAnimationFrame(${handler});`
-							}
-							${mutations.length > 0 && mutations}
-							${Array.from(dependencies).map(dep => `$$make_dirty('${dep}');`)}
+				this.renderer.component.partly_hoisted.push(deindent`
+					function ${handler}({ ${deps.join(', ')} }) {
+						${
+							animation_frame && deindent`
+								cancelAnimationFrame(${animation_frame});
+								if (!${this.var}.paused) ${animation_frame} = requestAnimationFrame(${handler});`
 						}
-					`
-				});
+						${mutations.length > 0 && mutations}
+						${Array.from(dependencies).map(dep => `$$make_dirty('${dep}');`)}
+					}
+				`);
 
 				callee = handler;
 			} else {
-				this.renderer.component.event_handlers.push({
-					name: handler,
-					body: deindent`
-						function ${handler}() {
-							${
-								animation_frame && deindent`
-									cancelAnimationFrame(${animation_frame});
-									if (!${this.var}.paused) ${animation_frame} = requestAnimationFrame(${handler});`
-							}
-							${mutations.length > 0 && mutations}
-							${Array.from(dependencies).map(dep => `$$make_dirty('${dep}');`)}
+				this.renderer.component.partly_hoisted.push(deindent`
+					function ${handler}() {
+						${
+							animation_frame && deindent`
+								cancelAnimationFrame(${animation_frame});
+								if (!${this.var}.paused) ${animation_frame} = requestAnimationFrame(${handler});`
 						}
-					`
-				});
+						${mutations.length > 0 && mutations}
+						${Array.from(dependencies).map(dep => `$$make_dirty('${dep}');`)}
+					}
+				`);
 
 				callee = `ctx.${handler}`;
 			}
@@ -664,40 +660,7 @@ export default class ElementWrapper extends Wrapper {
 				if (handler.modifiers.has('preventDefault')) modifiers.push('event.preventDefault();');
 				if (handler.modifiers.has('stopPropagation')) modifiers.push('event.stopPropagation();');
 
-				let name;
-
-				if (handler.expression && handler.expression.contextual_dependencies.size > 0) {
-					name = handler_name;
-
-					const deps = Array.from(handler.expression.contextual_dependencies);
-
-					component.event_handlers.push({
-						name: handler_name,
-						body: deindent`
-							function ${handler_name}(event, { ${deps.join(', ')} }) {
-								(${handler.snippet})(event);
-							}
-						`
-					});
-
-					block.builders.init.addBlock(deindent`
-						function ${name}(event) {
-							ctx.${handler_name}.call(this, event, ctx);
-						}
-					`);
-				} else {
-					name = `ctx.${handler_name}`;
-
-					component.event_handlers.push({
-						name: handler_name,
-						body: deindent`
-							function ${handler_name}(event) {
-								${modifiers}
-								(${handler.snippet})(event);
-							}
-						`
-					});
-				}
+				let name = handler.expression.snippet;
 
 				const opts = ['passive', 'once', 'capture'].filter(mod => handler.modifiers.has(mod));
 				if (opts.length) {
@@ -722,6 +685,10 @@ export default class ElementWrapper extends Wrapper {
 					);
 				}
 			}
+
+			handler.expression.declarations.forEach(declaration => {
+				block.builders.init.addBlock(declaration);
+			});
 		});
 	}
 
@@ -855,6 +822,10 @@ export default class ElementWrapper extends Wrapper {
 			if (expression) {
 				snippet = expression.snippet;
 				dependencies = expression.dependencies;
+
+				expression.declarations.forEach(declaration => {
+					block.builders.init.addBlock(declaration);
+				});
 			}
 
 			const name = block.getUniqueName(
diff --git a/src/compile/render-dom/wrappers/InlineComponent/index.ts b/src/compile/render-dom/wrappers/InlineComponent/index.ts
index b72c69e59d..b56a23a32e 100644
--- a/src/compile/render-dom/wrappers/InlineComponent/index.ts
+++ b/src/compile/render-dom/wrappers/InlineComponent/index.ts
@@ -191,6 +191,7 @@ export default class InlineComponentWrapper extends Wrapper {
 
 		const munged_bindings = this.node.bindings.map(binding => {
 			const name = component.getUniqueName(`${this.var}_${binding.name}_binding`);
+			component.declarations.push(name);
 
 			const contextual_dependencies = Array.from(binding.expression.contextual_dependencies);
 			const dependencies = Array.from(binding.expression.dependencies);
@@ -224,7 +225,7 @@ export default class InlineComponentWrapper extends Wrapper {
 				}
 			`;
 
-			component.event_handlers.push({ name, body });
+			component.partly_hoisted.push(body);
 
 			return contextual_dependencies.length > 0
 				? `${this.var}.$$bind('${binding.name}', ${name});`
@@ -261,20 +262,18 @@ export default class InlineComponentWrapper extends Wrapper {
 			const handler_name = component.getUniqueName(
 				`${handler.name.replace(/[^a-zA-Z0-9_$]/g, '_')}_handler`
 			);
+			component.declarations.push(handler_name);
 
 			if (handler.expression && handler.expression.contextual_dependencies.size > 0) {
 				block.maintainContext = true; // TODO is there a better place to put this?
 
 				const deps = Array.from(handler.expression.contextual_dependencies);
 
-				component.event_handlers.push({
-					name: handler_name,
-					body: deindent`
-						function ${handler_name}(event, { ${deps.join(', ')} }) {
-							(${snippet})(event);
-						}
-					`
-				});
+				component.partly_hoisted.push(deindent`
+					function ${handler_name}(event, { ${deps.join(', ')} }) {
+						(${snippet})(event);
+					}
+				`);
 
 				block.builders.init.addBlock(deindent`
 					function ${handler.name}(event) {
@@ -285,14 +284,11 @@ export default class InlineComponentWrapper extends Wrapper {
 				return `${name}.$on("${handler.name}", ${handler_name})`;
 			}
 
-			component.event_handlers.push({
-				name: handler_name,
-				body: deindent`
-					function ${handler_name}(event) {
-						(${snippet})(event);
-					}
-				`
-			});
+			component.partly_hoisted.push(deindent`
+				function ${handler_name}(event) {
+					(${snippet})(event);
+				}
+			`);
 
 			return `${name}.$on("${handler.name}", ctx.${handler_name})`;
 		});
diff --git a/src/compile/render-dom/wrappers/Window.ts b/src/compile/render-dom/wrappers/Window.ts
index 89e1f03bd0..3432f8079c 100644
--- a/src/compile/render-dom/wrappers/Window.ts
+++ b/src/compile/render-dom/wrappers/Window.ts
@@ -40,50 +40,15 @@ export default class WindowWrapper extends Wrapper {
 		const bindings: Record<string, string> = {};
 
 		this.node.handlers.forEach(handler => {
-			// TODO verify that it's a valid callee (i.e. built-in or declared method)
-			component.addSourcemapLocations(handler.expression);
+			const { snippet } = handler.expression;
 
-			const isCustomEvent = false; // TODO!!!
-
-			let usesState = handler.expression.dependencies.size > 0;
-
-			const handler_name = block.getUniqueName(`onwindow${handler.name}`);
-			const handler_body = deindent`
-				${usesState && `var ctx = #component.get();`}
-				${handler.snippet}
-			`;
-
-			if (isCustomEvent) {
-				// TODO dry this out
-				block.addVariable(handler_name);
-
-				block.builders.hydrate.addBlock(deindent`
-					${handler_name} = ctx.${handler.name}.call(#component, window, function(event) {
-						(${handler_body})(event);
-					});
-				`);
-
-				block.builders.destroy.addLine(deindent`
-					${handler_name}.destroy();
-				`);
-			} else {
-				component.event_handlers.push({
-					name: handler_name,
-					body: deindent`
-						function ${handler_name}(event) {
-							(${handler.snippet})(event);
-						}
-					`
-				});
-
-				block.builders.init.addLine(
-					`window.addEventListener("${handler.name}", ctx.${handler_name});`
-				);
+			block.builders.init.addLine(
+				`window.addEventListener("${handler.name}", ${snippet});`
+			);
 
-				block.builders.destroy.addLine(
-					`window.removeEventListener("${handler.name}", ctx.${handler_name});`
-				);
-			}
+			block.builders.destroy.addLine(
+				`window.removeEventListener("${handler.name}", ${snippet});`
+			);
 		});
 
 		this.node.bindings.forEach(binding => {
diff --git a/src/utils/deindent.ts b/src/utils/deindent.ts
index 9fbcf10953..fbd794ca9f 100644
--- a/src/utils/deindent.ts
+++ b/src/utils/deindent.ts
@@ -9,7 +9,7 @@ export default function deindent(
 
 	let result = strings[0].replace(start, '').replace(pattern, '');
 
-	let trailingIndentation = getTrailingIndentation(result);
+	let current_indentation = get_current_indentation(result);
 
 	for (let i = 1; i < strings.length; i += 1) {
 		let expression = values[i - 1];
@@ -22,7 +22,7 @@ export default function deindent(
 		if (expression || expression === '') {
 			const value = String(expression).replace(
 				/\n/g,
-				`\n${trailingIndentation}`
+				`\n${current_indentation}`
 			);
 			result += value + string;
 		} else {
@@ -31,14 +31,18 @@ export default function deindent(
 			result = result.slice(0, c) + string;
 		}
 
-		trailingIndentation = getTrailingIndentation(result);
+		current_indentation = get_current_indentation(result);
 	}
 
 	return result.trim().replace(/\t+$/gm, '');
 }
 
-function getTrailingIndentation(str: string) {
-	let i = str.length;
-	while (str[i - 1] === ' ' || str[i - 1] === '\t') i -= 1;
-	return str.slice(i, str.length);
+function get_current_indentation(str: string) {
+	let a = str.length;
+	while (a > 0 && str[a - 1] !== '\n') a -= 1;
+
+	let b = a;
+	while (b < str.length && /\s/.test(str[b])) b += 1;
+
+	return str.slice(a, b);
 }