Skip to content

Commit

Permalink
fix: better hoistability analysis (#2655)
Browse files Browse the repository at this point in the history 
Instead of collecting the types/values that are allowed, we collect the types/values that are _disallowed_ - this makes it possible to reference global values/types and still have them properly be declared as hoistable

#2640
  • Loading branch information
@dummdidumm
dummdidumm authored Jan 8, 2025
1 parent 931dd85 commit d5fbdfe
Show file tree
Hide file tree
Showing 4 changed files with 74 additions and 78 deletions.
6 changes: 4 additions & 2 deletions packages/svelte2tsx/src/svelte2tsx/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,11 +166,13 @@ export function svelte2tsx(
}

if (moduleScriptTag || scriptTag) {
const allowed = exportedNames.hoistableInterfaces.getAllowedValues();
for (const [start, end, globals] of rootSnippets) {
const hoist_to_module =
moduleScriptTag &&
(globals.size === 0 || [...globals.keys()].every((id) => allowed.has(id)));
(globals.size === 0 ||
[...globals.keys()].every((id) =>
exportedNames.hoistableInterfaces.isAllowedReference(id)
));

if (hoist_to_module) {
str.move(
Expand Down
122 changes: 46 additions & 76 deletions packages/svelte2tsx/src/svelte2tsx/nodes/HoistableInterfaces.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import MagicString from 'magic-string';
* Collects all imports and module-level declarations to then find out which interfaces/types are hoistable.
*/
export class HoistableInterfaces {
private import_value_set: Set<string> = new Set();
private import_type_set: Set<string> = new Set();
private module_types: Set<string> = new Set();
private disallowed_types = new Set<string>();
private disallowed_values = new Set<string>();
private interface_map: Map<
string,
{ type_deps: Set<string>; value_deps: Set<string>; node: ts.Node }
Expand All @@ -18,6 +19,7 @@ export class HoistableInterfaces {
value_deps: new Set<string>()
};

/** should be called before analyzeInstanceScriptNode */
analyzeModuleScriptNode(node: ts.Node) {
// Handle Import Declarations
if (ts.isImportDeclaration(node) && node.importClause) {
Expand All @@ -30,9 +32,7 @@ export class HoistableInterfaces {
node.importClause.namedBindings.elements.forEach((element) => {
const import_name = element.name.text;
if (is_type_only || element.isTypeOnly) {
this.import_type_set.add(import_name);
} else {
this.import_value_set.add(import_name);
this.module_types.add(import_name);
}
});
}
Expand All @@ -41,9 +41,7 @@ export class HoistableInterfaces {
if (node.importClause.name) {
const default_import = node.importClause.name.text;
if (is_type_only) {
this.import_type_set.add(default_import);
} else {
this.import_value_set.add(default_import);
this.module_types.add(default_import);
}
}

Expand All @@ -54,40 +52,17 @@ export class HoistableInterfaces {
) {
const namespace_import = node.importClause.namedBindings.name.text;
if (is_type_only) {
this.import_type_set.add(namespace_import);
} else {
this.import_value_set.add(namespace_import);
this.module_types.add(namespace_import);
}
}
}

// Handle top-level declarations
if (ts.isVariableStatement(node)) {
node.declarationList.declarations.forEach((declaration) => {
if (ts.isIdentifier(declaration.name)) {
this.import_value_set.add(declaration.name.text);
}
});
}

if (ts.isFunctionDeclaration(node) && node.name) {
this.import_value_set.add(node.name.text);
}

if (ts.isClassDeclaration(node) && node.name) {
this.import_value_set.add(node.name.text);
}

if (ts.isEnumDeclaration(node)) {
this.import_value_set.add(node.name.text);
}

if (ts.isTypeAliasDeclaration(node)) {
this.import_type_set.add(node.name.text);
this.module_types.add(node.name.text);
}

if (ts.isInterfaceDeclaration(node)) {
this.import_type_set.add(node.name.text);
this.module_types.add(node.name.text);
}
}

Expand All @@ -103,9 +78,7 @@ export class HoistableInterfaces {
node.importClause.namedBindings.elements.forEach((element) => {
const import_name = element.name.text;
if (is_type_only) {
this.import_type_set.add(import_name);
} else {
this.import_value_set.add(import_name);
this.module_types.add(import_name);
}
});
}
Expand All @@ -114,9 +87,7 @@ export class HoistableInterfaces {
if (node.importClause.name) {
const default_import = node.importClause.name.text;
if (is_type_only) {
this.import_type_set.add(default_import);
} else {
this.import_value_set.add(default_import);
this.module_types.add(default_import);
}
}

Expand All @@ -127,9 +98,7 @@ export class HoistableInterfaces {
) {
const namespace_import = node.importClause.namedBindings.name.text;
if (is_type_only) {
this.import_type_set.add(namespace_import);
} else {
this.import_value_set.add(namespace_import);
this.module_types.add(namespace_import);
}
}
}
Expand Down Expand Up @@ -167,9 +136,9 @@ export class HoistableInterfaces {
}
});

if (this.import_type_set.has(interface_name)) {
// shadowed; delete because we can't hoist
this.import_type_set.delete(interface_name);
if (this.module_types.has(interface_name)) {
// shadowed; we can't hoist
this.disallowed_types.add(interface_name);
} else {
this.interface_map.set(interface_name, {
type_deps: type_dependencies,
Expand All @@ -193,9 +162,9 @@ export class HoistableInterfaces {
generics
);

if (this.import_type_set.has(alias_name)) {
// shadowed; delete because we can't hoist
this.import_type_set.delete(alias_name);
if (this.module_types.has(alias_name)) {
// shadowed; we can't hoist
this.disallowed_types.add(alias_name);
} else {
this.interface_map.set(alias_name, {
type_deps: type_dependencies,
Expand All @@ -209,29 +178,21 @@ export class HoistableInterfaces {
if (ts.isVariableStatement(node)) {
node.declarationList.declarations.forEach((declaration) => {
if (ts.isIdentifier(declaration.name)) {
this.import_value_set.delete(declaration.name.text);
this.disallowed_values.add(declaration.name.text);
}
});
}

if (ts.isFunctionDeclaration(node) && node.name) {
this.import_value_set.delete(node.name.text);
this.disallowed_values.add(node.name.text);
}

if (ts.isClassDeclaration(node) && node.name) {
this.import_value_set.delete(node.name.text);
this.disallowed_values.add(node.name.text);
}

if (ts.isEnumDeclaration(node)) {
this.import_value_set.delete(node.name.text);
}

if (ts.isTypeAliasDeclaration(node)) {
this.import_type_set.delete(node.name.text);
}

if (ts.isInterfaceDeclaration(node)) {
this.import_type_set.delete(node.name.text);
this.disallowed_values.add(node.name.text);
}
}

Expand Down Expand Up @@ -281,13 +242,26 @@ export class HoistableInterfaces {
continue;
}

const can_hoist = [...deps.type_deps, ...deps.value_deps].every((dep) => {
return (
this.import_type_set.has(dep) ||
this.import_value_set.has(dep) ||
hoistable_interfaces.has(dep)
);
});
let can_hoist = true;

for (const dep of deps.type_deps) {
if (this.disallowed_types.has(dep)) {
this.disallowed_types.add(interface_name);
can_hoist = false;
break;
}
if (this.interface_map.has(dep) && !hoistable_interfaces.has(dep)) {
can_hoist = false;
}
}

for (const dep of deps.value_deps) {
if (this.disallowed_values.has(dep)) {
this.disallowed_types.add(interface_name);
can_hoist = false;
break;
}
}

if (can_hoist) {
hoistable_interfaces.set(interface_name, deps.node);
Expand All @@ -301,11 +275,7 @@ export class HoistableInterfaces {
...this.props_interface.type_deps,
...this.props_interface.value_deps
].every((dep) => {
return (
this.import_type_set.has(dep) ||
this.import_value_set.has(dep) ||
hoistable_interfaces.has(dep)
);
return !this.disallowed_types.has(dep) && !this.disallowed_values.has(dep);
});

if (can_hoist) {
Expand All @@ -328,7 +298,7 @@ export class HoistableInterfaces {
if (!this.props_interface.name) return;

for (const generic of generics) {
this.import_type_set.delete(generic);
this.disallowed_types.add(generic);
}

const hoistable = this.determineHoistableInterfaces();
Expand Down Expand Up @@ -362,8 +332,8 @@ export class HoistableInterfaces {
}
}

getAllowedValues() {
return this.import_value_set;
isAllowedReference(reference: string) {
return !this.disallowed_values.has(reference);
}

/**
Expand Down
11 changes: 11 additions & 0 deletions packages/svelte2tsx/test/svelte2tsx/samples/snippet-module-hoist-1.v5/expectedv2.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ import { imported } from './x';
{ svelteHTML.createElement("div", {});module; }
};return __sveltets_2_any(0)}; const hoistable7/*Ωignore_positionΩ*/ = ()/*Ωignore_startΩ*/: ReturnType<import('svelte').Snippet>/*Ωignore_endΩ*/ => { async ()/*Ωignore_positionΩ*/ => {
{ svelteHTML.createElement("div", {});imported; }
};return __sveltets_2_any(0)}; const hoistable8/*Ωignore_positionΩ*/ = ()/*Ωignore_startΩ*/: ReturnType<import('svelte').Snippet>/*Ωignore_endΩ*/ => { async ()/*Ωignore_positionΩ*/ => {
{ svelteHTML.createElement("div", {});global; }
};return __sveltets_2_any(0)}; const hoistable9/*Ωignore_positionΩ*/ = (props: HTMLAttributes<HTMLDivElement>)/*Ωignore_startΩ*/: ReturnType<import('svelte').Snippet>/*Ωignore_endΩ*/ => { async ()/*Ωignore_positionΩ*/ => { };return __sveltets_2_any(0)}; const hoistable10/*Ωignore_positionΩ*/ = (foo)/*Ωignore_startΩ*/: ReturnType<import('svelte').Snippet>/*Ωignore_endΩ*/ => { async ()/*Ωignore_positionΩ*/ => {
const bar = foo;
bar;
};return __sveltets_2_any(0)};function render() {
const not_hoistable/*Ωignore_positionΩ*/ = ()/*Ωignore_startΩ*/: ReturnType<import('svelte').Snippet>/*Ωignore_endΩ*/ => { async ()/*Ωignore_positionΩ*/ => {
{ svelteHTML.createElement("div", {});foo; }
Expand All @@ -40,6 +45,12 @@ async () => {












Expand Down
13 changes: 13 additions & 0 deletions packages/svelte2tsx/test/svelte2tsx/samples/snippet-module-hoist-1.v5/input.svelte
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,19 @@
<div>{imported}</div>
{/snippet}

{#snippet hoistable8()}
<div>{global}</div>
{/snippet}

{#snippet hoistable9(props: HTMLAttributes<HTMLDivElement>)}
Referencing global types
{/snippet}

{#snippet hoistable10(foo)}
{@const bar = foo}
{bar}
{/snippet}

{#snippet not_hoistable()}
<div>{foo}</div>
{/snippet}

0 comments on commit d5fbdfe

Please sign in to comment.