ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/1-parse/remove_typescript_nodes.js (JavaScript)

Code before:
	},
	ExportAllDeclaration(node) {
		if (node.exportKind === 'type') return b.empty;
		return node;
	},
	PropertyDefinition(node) {
		if (node.accessor) {
			e.typescript_invalid_feature(
				node,
				'accessor fields (related TSC proposal is not stage 4 yet)'
			);
		}
	},
	TSAsExpression(node, context) {
		return context.visit(node.expression);
	},
	TSSatisfiesExpression(node, context) {
		return context.visit(node.expression);
	},
	TSNonNullExpression(node, context) {
		return context.visit(node.expression);
	},
	TSTypeAnnotation() {
		return b.empty;
	},
	TSInterfaceDeclaration() {
		return b.empty;
	},
	TSTypeAliasDeclaration() {
		return b.empty;
	},
	TSTypeParameterDeclaration() {
		return b.empty;
	},
	TSTypeParameterInstantiation() {
		return b.empty;
	},
	TSEnumDeclaration(node) {
		e.typescript_invalid_feature(node, 'enums');
	},
	TSParameterProperty(node, context) {
		if (node.accessibility && context.path.at(-2)?.kind === 'constructor') {
			e.typescript_invalid_feature(node, 'accessibility modifiers on constructor parameters');
		}
		return node.parameter;
	},
	Identifier(node) {
		if (node.typeAnnotation) {
			return {
				...node,
				typeAnnotation: null

Review comment (suggestion):
mhhhhm can we really just return `b.empty`? I worry that it would result in a `;` in the output at an invalid place. Then again, esrap seems to handle it fine / just ignore it? Either way, I would feel better if the fix was to instead go up to the parent, and set the `typeAnnotation` property to `null` on it

Code after:
	},
	ExportAllDeclaration(node) {
		if (node.exportKind === 'type') return b.empty;
		return node;
	},
	PropertyDefinition(node) {
		if (node.accessor) {
			e.typescript_invalid_feature(
				node,
				'accessor fields (related TSC proposal is not stage 4 yet)'
			);
		}
	},
	TSAsExpression(node, context) {
		return context.visit(node.expression);
	},
	TSSatisfiesExpression(node, context) {
		return context.visit(node.expression);
	},
	TSNonNullExpression(node, context) {
		return context.visit(node.expression);
	},
	TSTypeAnnotation() {
		// This isn't correct, strictly speaking, and could result in invalid ASTs (like an empty statement within function parameters),
		// but esrap, our printing tool, just ignores these AST nodes at invalid positions, so it's fine
		return b.empty;
	},
	TSInterfaceDeclaration() {
		return b.empty;
	},
	TSTypeAliasDeclaration() {
		return b.empty;
	},
	TSTypeParameterDeclaration() {
		return b.empty;
	},
	TSTypeParameterInstantiation() {
		return b.empty;
	},
	TSEnumDeclaration(node) {
		e.typescript_invalid_feature(node, 'enums');
	},
	TSParameterProperty(node, context) {
		if (node.accessibility && context.path.at(-2)?.kind === 'constructor') {
			e.typescript_invalid_feature(node, 'accessibility modifiers on constructor parameters');
		}
		return node.parameter;
	},
	FunctionExpression: remove_this_param,
	FunctionDeclaration: remove_this_param,
	TSDeclareFunction() {
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:

	if ((flags & MAYBE_DIRTY) !== 0) {
		var dependencies = reaction.deps;
		var is_unowned = (flags & UNOWNED) !== 0;

		if (dependencies !== null) {
			var i;

			if ((flags & DISCONNECTED) !== 0) {
				for (i = 0; i < dependencies.length; i++) {
					(dependencies[i].reactions ??= []).push(reaction);
				}

				reaction.f ^= DISCONNECTED;
			}

			for (i = 0; i < dependencies.length; i++) {
				var dependency = dependencies[i];

				if (check_dirtiness(/** @type {Derived} */ (dependency))) {
					update_derived(/** @type {Derived} */ (dependency));
				}

				// If we are working with an unowned signal as part of an effect (due to !current_skip_reaction)
				// and the version hasn't changed, we still need to check that this reaction
				// if linked to the dependency source – otherwise future updates will not be caught.
				if (
					is_unowned &&
					current_effect !== null &&
					!current_skip_reaction &&
					!dependency?.reactions?.includes(reaction)
				) {
					(dependency.reactions ??= []).push(reaction);
				}

				if (dependency.version > reaction.version) {
					return true;
				}
			}
		}

		// Unowned signals should never be marked as clean.
		if (!is_unowned) {
			set_signal_status(reaction, CLEAN);
		}
	}

	return false;
}

/**

Review comment (suggestion):
```suggestion
				// is linked to the dependency source – otherwise future updates will not be caught.
```

Code after:

	if ((flags & MAYBE_DIRTY) !== 0) {
		var dependencies = reaction.deps;
		var is_unowned = (flags & UNOWNED) !== 0;

		if (dependencies !== null) {
			var i;

			if ((flags & DISCONNECTED) !== 0) {
				for (i = 0; i < dependencies.length; i++) {
					(dependencies[i].reactions ??= []).push(reaction);
				}

				reaction.f ^= DISCONNECTED;
			}

			for (i = 0; i < dependencies.length; i++) {
				var dependency = dependencies[i];

				if (check_dirtiness(/** @type {Derived} */ (dependency))) {
					update_derived(/** @type {Derived} */ (dependency));
				}

				// If we are working with an unowned signal as part of an effect (due to !current_skip_reaction)
				// and the version hasn't changed, we still need to check that this reaction
				// is linked to the dependency source – otherwise future updates will not be caught.
				if (
					is_unowned &&
					current_effect !== null &&
					!current_skip_reaction &&
					!dependency?.reactions?.includes(reaction)
				) {
					(dependency.reactions ??= []).push(reaction);
				}

				if (dependency.version > reaction.version) {
					return true;
				}
			}
		}

		// Unowned signals should never be marked as clean.
		if (!is_unowned) {
			set_signal_status(reaction, CLEAN);
		}
	}

	return false;
}

/**
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js (JavaScript)

Code before:
/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, Statement, Super } from 'estree' */
/** @import { AST, SvelteNode } from '#compiler' */
/** @import { ComponentClientTransformState } from '../../types' */
import { walk } from 'zimmerframe';
import { object } from '../../../../../utils/ast.js';
import * as b from '../../../../../utils/builders.js';
import { sanitize_template_string } from '../../../../../utils/sanitize_template_string.js';
import { regex_is_valid_identifier } from '../../../../patterns.js';
import { create_derived } from '../../utils.js';
import is_reference from 'is-reference';
import { locator } from '../../../../../state.js';

/**
 * @param {Array<AST.Text | AST.ExpressionTag>} values
 */
export function get_states_and_call(values) {
	let states = 0;
	let calls = 0;
	for (let i = 0; i < values.length; i++) {
		const node = values[i];

		if (node.type === 'ExpressionTag') {
			if (node.metadata.expression.has_call) {
				calls++;
			}
			if (node.metadata.expression.has_state) {
				states++;
			}
		}
	}

	return { states, calls };
}

/**
 * @param {Array<AST.Text | AST.ExpressionTag>} values
 * @param {(node: SvelteNode, state: any) => any} visit
 * @param {ComponentClientTransformState} state
 */
export function build_template_literal(values, visit, state) {
	/** @type {Expression[]} */

Review comment (suggestion):
(and in the corresponding call sites, too)
```suggestion
export function get_states_and_calls(values) {
```

Code after:
/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, Statement, Super } from 'estree' */
/** @import { AST, SvelteNode } from '#compiler' */
/** @import { ComponentClientTransformState } from '../../types' */
import { walk } from 'zimmerframe';
import { object } from '../../../../../utils/ast.js';
import * as b from '../../../../../utils/builders.js';
import { sanitize_template_string } from '../../../../../utils/sanitize_template_string.js';
import { regex_is_valid_identifier } from '../../../../patterns.js';
import { create_derived } from '../../utils.js';
import is_reference from 'is-reference';
import { locator } from '../../../../../state.js';

/**
 * @param {Array<AST.Text | AST.ExpressionTag>} values
 */
export function get_states_and_calls(values) {
	let states = 0;
	let calls = 0;
	for (let i = 0; i < values.length; i++) {
		const node = values[i];

		if (node.type === 'ExpressionTag') {
			if (node.metadata.expression.has_call) {
				calls++;
			}
			if (node.metadata.expression.has_state) {
				states++;
			}
		}
	}

	return { states, calls };
}

/**
 * @param {Array<AST.Text | AST.ExpressionTag>} values
 * @param {(node: SvelteNode, state: any) => any} visit
 * @param {ComponentClientTransformState} state
 */
export function build_template_literal(values, visit, state) {
	/** @type {Expression[]} */
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dev/equality.js (JavaScript)

Code before:
			if (test) {
				w.state_proxy_equality_mismatch('array.includes(...)');
			}
		}

		return has;
	};

	// @ts-expect-error
	Array.__svelte_cleanup = () => {
		array_prototype.indexOf = indexOf;
		array_prototype.lastIndexOf = lastIndexOf;
		array_prototype.includes = includes;
	};
}

/**
 * @param {any} a
 * @param {any} b
 * @param {boolean} equal
 * @returns {boolean}
 */
export function strict_equals(a, b, equal = true) {
	try {
		if ((a === b) !== (get_proxied_value(a) === get_proxied_value(b))) {
			w.state_proxy_equality_mismatch(equal ? '===' : '!==');
		}
	} catch {}

	return (a === b) === equal;
}

/**
 * @param {any} a
 * @param {any} b
 * @param {boolean} equal
 * @returns {boolean}
 */
export function equals(a, b, equal = true) {
	if ((a == b) !== (get_proxied_value(a) == get_proxied_value(b))) {
		w.state_proxy_equality_mismatch(equal ? '==' : '!=');
	}

	return (a == b) === equal;
}


Review comment (suggestion):
```suggestion
	// try-catch needed because this tries to read properties of `a` and `b`,
	// which could be disallowed for example in a secure context
	try {
```

Code after:
			if (test) {
				w.state_proxy_equality_mismatch('array.includes(...)');
			}
		}

		return has;
	};

	// @ts-expect-error
	Array.__svelte_cleanup = () => {
		array_prototype.indexOf = indexOf;
		array_prototype.lastIndexOf = lastIndexOf;
		array_prototype.includes = includes;
	};
}

/**
 * @param {any} a
 * @param {any} b
 * @param {boolean} equal
 * @returns {boolean}
 */
export function strict_equals(a, b, equal = true) {
	// try-catch needed because this tries to read properties of `a` and `b`,
	// which could be disallowed for example in a secure context
	try {
		if ((a === b) !== (get_proxied_value(a) === get_proxied_value(b))) {
			w.state_proxy_equality_mismatch(equal ? '===' : '!==');
		}
	} catch {}

	return (a === b) === equal;
}

/**
 * @param {any} a
 * @param {any} b
 * @param {boolean} equal
 * @returns {boolean}
 */
export function equals(a, b, equal = true) {
	if ((a == b) !== (get_proxied_value(a) == get_proxied_value(b))) {
		w.state_proxy_equality_mismatch(equal ? '==' : '!=');
	}

	return (a == b) === equal;
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
	} finally {
		current_effect = previous_effect;
		current_component_context = previous_component_context;

		if (DEV) {
			dev_current_component_function = previous_component_fn;
		}
	}
}

function infinite_loop_guard() {
	if (flush_count > 1000) {
		flush_count = 0;
		if (DEV) {
			try {
				e.effect_update_depth_exceeded();
			} catch (e) {
				// stack is garbage, ignore. Instead add a console.error message.
				// eslint-disable-next-line no-console
				console.error(
					'Last ten effects were: ',
					dev_effect_stack.slice(-10).map((d) => d.fn)
				);
				dev_effect_stack = [];
				// @ts-expect-error
				e.stack = '';
				throw e;
			}
		} else {
			e.effect_update_depth_exceeded();
		}
	}
	flush_count++;
}

/**
 * @param {Array<Effect>} root_effects
 * @returns {void}
 */
function flush_queued_root_effects(root_effects) {
	var length = root_effects.length;
	if (length === 0) {
		return;
	}
	infinite_loop_guard();

	var previously_flushing_effect = is_flushing_effect;
	is_flushing_effect = true;

	try {
		for (var i = 0; i < length; i++) {

Review comment (suggestion):
I believe this needs to be `defineProperty` see `handle_error` in `runtime.js`

Code after:
	} finally {
		current_effect = previous_effect;
		current_component_context = previous_component_context;

		if (DEV) {
			dev_current_component_function = previous_component_fn;
		}
	}
}

function infinite_loop_guard() {
	if (flush_count > 1000) {
		flush_count = 0;
		if (DEV) {
			try {
				e.effect_update_depth_exceeded();
			} catch (error) {
				// stack is garbage, ignore. Instead add a console.error message.
				define_property(error, 'stack', {
					value: ''
				});
				// eslint-disable-next-line no-console
				console.error(
					'Last ten effects were: ',
					dev_effect_stack.slice(-10).map((d) => d.fn)
				);
				dev_effect_stack = [];
				throw error;
			}
		} else {
			e.effect_update_depth_exceeded();
		}
	}
	flush_count++;
}

/**
 * @param {Array<Effect>} root_effects
 * @returns {void}
 */
function flush_queued_root_effects(root_effects) {
	var length = root_effects.length;
	if (length === 0) {
		return;
	}
	infinite_loop_guard();

	var previously_flushing_effect = is_flushing_effect;
	is_flushing_effect = true;

	try {
Repository: sveltejs/svelte
File: packages/svelte/tests/migrate/samples/state-and-derivations-sequence/output.svelte (Svelte)

Code before:
<script>
	let count = ($state((void 0, 0)));
	// semicolon at the end
	let doubled = ($derived((void 0, count * 2)));
	let { quadrupled } = ($derived((void 0, { quadrupled: count * 4 })));
	// no semicolon at the end
	let time_8 = ($derived((void 0, count * 8)))
	let { time_16 } = ($derived((void 0, { time_16: count * 16 })))
</script>

<!-- reassign to migrate to state -->
<button onclick={()=> count++}></button>

{count} / {doubled} / {quadrupled} / {time_8} / {time_16}


Review comment (suggestion):
this output doesn't seem right

```suggestion
	let count = $state((void 0, 0));
```

Code after:
<script>
	let count = $state((void 0, 0));
	// semicolon at the end
	let doubled = $derived((void 0, count * 2));
	let { quadrupled } = $derived((void 0, { quadrupled: count * 4 }));
	// no semicolon at the end
	let time_8 = $derived((void 0, count * 8))
	let { time_16 } = $derived((void 0, { time_16: count * 16 }))
</script>

<!-- reassign to migrate to state -->
<button onclick={()=> count++}></button>

{count} / {doubled} / {quadrupled} / {time_8} / {time_16}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
			effect.f ^= CLEAN;
		}
	}

	queued_root_effects.push(effect);
}

/**
 *
 * This function both runs render effects and collects user effects in topological order
 * from the starting effect passed in. Effects will be collected when they match the filtered
 * bitwise flag passed in only. The collected effects array will be populated with all the user
 * effects to be flushed.
 *
 * @param {Effect} effect
 * @param {Effect[]} collected_effects
 * @returns {void}
 */
function process_effects(effect, collected_effects) {
	var current_effect = effect.first;
	var effects = [];

	main_loop: while (current_effect !== null) {
		var flags = current_effect.f;
		var is_branch = (flags & BRANCH_EFFECT) !== 0;
		var is_skippable_branch = is_branch && (flags & CLEAN) !== 0;

		if (!is_skippable_branch && (flags & INERT) === 0) {
			if (is_branch) {
				current_effect.f ^= CLEAN;
			}

			if ((flags & RENDER_EFFECT) !== 0) {
				if (!is_branch && check_dirtiness(current_effect)) {
					update_effect(current_effect);
				}

				// Child might have been mutated since running the effect or checking dirtiness
				var child = current_effect.first;

				if (child !== null) {
					current_effect = child;
					continue;
				}
			} else if ((flags & EFFECT) !== 0) {
				effects.push(current_effect);
			}
		}

		var sibling = current_effect.next;


Review comment (suggestion):
we could do this instead which would presumably be a tiny bit faster, but less readable

```suggestion
		var is_skippable_branch = (flags & (CLEAN | BRANCH_EFFECT)) === (CLEAN | BRANCH_EFFECT);
```

Code after:
			effect.f ^= CLEAN;
		}
	}

	queued_root_effects.push(effect);
}

/**
 *
 * This function both runs render effects and collects user effects in topological order
 * from the starting effect passed in. Effects will be collected when they match the filtered
 * bitwise flag passed in only. The collected effects array will be populated with all the user
 * effects to be flushed.
 *
 * @param {Effect} effect
 * @param {Effect[]} collected_effects
 * @returns {void}
 */
function process_effects(effect, collected_effects) {
	var current_effect = effect.first;
	var effects = [];

	main_loop: while (current_effect !== null) {
		var flags = current_effect.f;
		var is_branch = (flags & BRANCH_EFFECT) !== 0;
		var is_skippable_branch = is_branch && (flags & CLEAN) !== 0;

		if (!is_skippable_branch && (flags & INERT) === 0) {
			if ((flags & RENDER_EFFECT) !== 0) {
				if (is_branch) {
					current_effect.f ^= CLEAN;
				} else if (check_dirtiness(current_effect)) {
					update_effect(current_effect);
				}

				var child = current_effect.first;

				if (child !== null) {
					current_effect = child;
					continue;
				}
			} else if ((flags & EFFECT) !== 0) {
				effects.push(current_effect);
			}
		}

		var sibling = current_effect.next;

		if (sibling === null) {
			let parent = current_effect.parent;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/migrate/index.js (JavaScript)

Code before:
			if (context) {
				state.str.update(context.start, context.end, 'module');
			}
		}

		if (parsed.instance) {
			walk(parsed.instance.content, state, instance_script);
		}

		state = { ...state, scope: analysis.template.scope };
		walk(parsed.fragment, state, template);

		let insertion_point = parsed.instance
			? /** @type {number} */ (parsed.instance.content.start)
			: 0;

		const need_script =
			state.legacy_imports.size > 0 ||
			state.script_insertions.size > 0 ||
			state.props.length > 0 ||
			analysis.uses_rest_props ||
			analysis.uses_props;

		if (!parsed.instance && need_script) {
			const to_prepend = '<script>';
			str.appendRight(0, to_prepend);
		}

		const specifiers = [...state.legacy_imports].map((imported) => {
			const local = state.names[imported];
			return imported === local ? imported : `${imported} as ${local}`;
		});

		const legacy_import = `import { ${specifiers.join(', ')} } from 'svelte/legacy';\n`;

		if (state.legacy_imports.size > 0) {
			str.appendRight(insertion_point, `\n${indent}${legacy_import}`);
		}

		if (state.script_insertions.size > 0) {
			str.appendRight(
				insertion_point,
				`\n${indent}${[...state.script_insertions].join(`\n${indent}`)}`
			);
		}

		insertion_point = state.props_insertion_point;

		if (state.props.length > 0 || analysis.uses_rest_props || analysis.uses_props) {
			const has_many_props = state.props.length > 3;
			const newline_separator = `\n${indent}${indent}`;

Review comment (suggestion):

```suggestion
			str.appendRight(0, '<script>');
```

Code after:
			if (context) {
				state.str.update(context.start, context.end, 'module');
			}
		}

		if (parsed.instance) {
			walk(parsed.instance.content, state, instance_script);
		}

		state = { ...state, scope: analysis.template.scope };
		walk(parsed.fragment, state, template);

		let insertion_point = parsed.instance
			? /** @type {number} */ (parsed.instance.content.start)
			: 0;

		const need_script =
			state.legacy_imports.size > 0 ||
			state.script_insertions.size > 0 ||
			state.props.length > 0 ||
			analysis.uses_rest_props ||
			analysis.uses_props;

		if (!parsed.instance && need_script) {
			str.appendRight(0, '<script>');
		}

		const specifiers = [...state.legacy_imports].map((imported) => {
			const local = state.names[imported];
			return imported === local ? imported : `${imported} as ${local}`;
		});

		const legacy_import = `import { ${specifiers.join(', ')} } from 'svelte/legacy';\n`;

		if (state.legacy_imports.size > 0) {
			str.appendRight(insertion_point, `\n${indent}${legacy_import}`);
		}

		if (state.script_insertions.size > 0) {
			str.appendRight(
				insertion_point,
				`\n${indent}${[...state.script_insertions].join(`\n${indent}`)}`
			);
		}

		insertion_point = state.props_insertion_point;

		if (state.props.length > 0 || analysis.uses_rest_props || analysis.uses_props) {
			const has_many_props = state.props.length > 3;
			const newline_separator = `\n${indent}${indent}`;
			const props_separator = has_many_props ? newline_separator : ' ';
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/index.js (JavaScript)

Code before:
							end: -1,
							parent: null
						};

						if (Array.isArray(class_attribute.value)) {
							class_attribute.value.push(css_text);
						} else {
							class_attribute.value = [class_attribute.value, css_text];
						}
					}
				} else {
					element.attributes.push(
						create_attribute('class', -1, -1, [
							{
								type: 'Text',
								data: analysis.css.hash,
								raw: analysis.css.hash,
								parent: null,
								start: -1,
								end: -1
							}
						])
					);
					if (
						element.type === 'RegularElement' &&
						element.name.includes('-') &&
						element.attributes.length === 1
					) {
						mark_subtree_dynamic(element.metadata.path);
					}
				}
			}
		}
	}

	// TODO
	// analysis.stylesheet.warn_on_unused_selectors(analysis);

	return analysis;
}

/**
 * @param {Map<import('estree').LabeledStatement, ReactiveStatement>} unsorted_reactive_declarations
 */
function order_reactive_statements(unsorted_reactive_declarations) {
	/** @typedef {[import('estree').LabeledStatement, ReactiveStatement]} Tuple */

	/** @type {Map<string, Array<Tuple>>} */
	const lookup = new Map();

	for (const [node, declaration] of unsorted_reactive_declarations) {

Review comment (suggestion):
We have a "is_custom_element" (or sth like that) fn somewhere I believe.

Also wondering whether there's a better way, it feels a bit unfortunate to do that, but I also see no other good way if this only happens after traversal... mhm

Code after:
							end: -1,
							parent: null
						};

						if (Array.isArray(class_attribute.value)) {
							class_attribute.value.push(css_text);
						} else {
							class_attribute.value = [class_attribute.value, css_text];
						}
					}
				} else {
					element.attributes.push(
						create_attribute('class', -1, -1, [
							{
								type: 'Text',
								data: analysis.css.hash,
								raw: analysis.css.hash,
								parent: null,
								start: -1,
								end: -1
							}
						])
					);
					if (is_custom_element_node(element) && element.attributes.length === 1) {
						mark_subtree_dynamic(element.metadata.path);
					}
				}
			}
		}
	}

	// TODO
	// analysis.stylesheet.warn_on_unused_selectors(analysis);

	return analysis;
}

/**
 * @param {Map<import('estree').LabeledStatement, ReactiveStatement>} unsorted_reactive_declarations
 */
function order_reactive_statements(unsorted_reactive_declarations) {
	/** @typedef {[import('estree').LabeledStatement, ReactiveStatement]} Tuple */

	/** @type {Map<string, Array<Tuple>>} */
	const lookup = new Map();

	for (const [node, declaration] of unsorted_reactive_declarations) {
		for (const binding of declaration.assignments) {
			const statements = lookup.get(binding.node.name) ?? [];
			statements.push([node, declaration]);
			lookup.set(binding.node.name, statements);