Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion src/compiler/compile/Component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,20 @@ export default class Component {
scope = map.get(node);
}

if (node.type === 'VariableDeclarator' && node.init !== null && node.init.type === 'Identifier') {
const variable = component.var_lookup.get(node.init.name);
variable.aliased = true;
}

if (node.type === 'CallExpression') {
node.arguments.forEach(arg => {
if (arg.type === 'Identifier' && scope.find_owner(arg.name) === instance_scope) {
const variable = component.var_lookup.get(arg.name);
variable.aliased = true;
}
});
}

if (node.type === 'AssignmentExpression' || node.type === 'UpdateExpression') {
const assignee = node.type === 'AssignmentExpression' ? node.left : node.argument;
const names = extract_names(assignee);
Expand Down Expand Up @@ -1077,7 +1091,7 @@ export default class Component {
} else if (owner === instance_scope) {
const variable = var_lookup.get(name);

if (variable.reassigned || variable.mutated) hoistable = false;
if (variable.reassigned || variable.mutated || variable.aliased) hoistable = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right place for this. This is marking the function not hoistable if it references aliased variables, instead of marking the function not hoistable if itself is aliased.


if (name === fn_declaration.id.name) return;

Expand Down
1 change: 1 addition & 0 deletions src/compiler/interfaces.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ export interface Var {
hoistable?: boolean;
subscribable?: boolean;
is_reactive_dependency?: boolean;
aliased?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly does the aliased field mean? I see that it's another thing that can stop variables from being hoisted, but I don't understand what it's checking for.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was to have aliased mean whether the variable is passed around functions or given other names as to avoid hoisting functions/objects because they could've been modified through these other names.

We'd still have to track all forms of aliasing though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking all forms of aliasing doesn't sound like it would be possible. Is this another situation where we just need to document how the compiler works - like how only certain types of assignments trigger reactivity, rather than embarking on a never-ending journey to catch more cases?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, why not? We wouldn't need perfect tracking, just knowing whether the variable escapes a single name -- the possibility it's used elsewhere. The other option would be to disable function hoisting (like currently for objects), since the way it stands functions that shouldn't be hoisted, are.

}

export interface CssResult {
Expand Down
21 changes: 21 additions & 0 deletions test/js/samples/aliased-function/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/* generated by Svelte vX.Y.Z */
import { SvelteComponent, init, safe_not_equal } from "svelte/internal";

function instance($$self) {
function foo() {

}

const foo_alias = foo;
foo_alias.nasty_but_legal();
return [];
}

class Component extends SvelteComponent {
constructor(options) {
super();
init(this, options, instance, null, safe_not_equal, {});
}
}

export default Component;
5 changes: 5 additions & 0 deletions test/js/samples/aliased-function/input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
function foo() {}
const foo_alias = foo;
foo_alias.nasty_but_legal();
</script>
50 changes: 50 additions & 0 deletions test/js/samples/hoisted-function/expected.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* generated by Svelte vX.Y.Z */
import {
SvelteComponent,
detach,
element,
init,
insert,
noop,
safe_not_equal
} from "svelte/internal";

import { bar } from "./utils.js";

function create_fragment(ctx) {
let p;

return {
c() {
p = element("p");
p.textContent = `You'll be pleased to know the answer is ${/*foo*/ ctx[0]()}.`;
},
m(target, anchor) {
insert(target, p, anchor);
},
p: noop,
i: noop,
o: noop,
d(detaching) {
if (detaching) detach(p);
}
};
}

function instance($$self) {
function foo() {
return 42;
}

bar(foo);
return [foo];
}

class Component extends SvelteComponent {
constructor(options) {
super();
init(this, options, instance, create_fragment, safe_not_equal, {});
}
}

export default Component;
11 changes: 11 additions & 0 deletions test/js/samples/hoisted-function/input.svelte
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<script>
import { bar } from './utils.js';

function foo() {
return 42;
}

bar(foo);
</script>

<p>You'll be pleased to know the answer is {foo()}.</p>
3 changes: 3 additions & 0 deletions test/js/samples/hoisted-function/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export function bar(fn) {
console.log(`I'm safe but you'd never know, ${fn}!`);
}