ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/server/transform-server.js (JavaScript)

Code before:
				node.prefix ? '$.update_store_pre' : '$.update_store',
				b.assignment('??=', b.id('$$store_subs'), b.object([])),
				b.literal(argument.name),
				b.id(argument.name.slice(1)),
				node.operator === '--' && b.literal(-1)
			);
		}

		return next();
	},
	CallExpression(node, context) {
		const rune = get_rune(node, context.state.scope);

		if (rune === '$host') {
			return b.id('undefined');
		}

		if (rune === '$effect.tracking') {
			return b.literal(false);
		}

		if (rune === '$effect.root') {
			const args = /** @type {import('estree').Expression[]} */ (
				node.arguments.map((arg) => context.visit(arg))
			);
			return b.call('$.effect_root', ...args);
		}

		if (rune === '$state.snapshot') {
			return /** @type {import('estree').Expression} */ (context.visit(node.arguments[0]));
		}

		if (rune === '$state.is') {
			return b.call(
				'Object.is',
				/** @type {import('estree').Expression} */ (context.visit(node.arguments[0])),
				/** @type {import('estree').Expression} */ (context.visit(node.arguments[1]))
			);
		}

		if (rune === '$inspect' || rune === '$inspect().with') {
			return transform_inspect_rune(node, context);
		}

		context.next();
	}
};

/** @type {import('./types').Visitors} */
const javascript_visitors_runes = {
	ClassBody(node, { state, visit }) {

Review comment (suggestion):
AFAIK the `effect_root` function isn't exposed from the server entry point. Which makes me wonder why the test still passes.

Code after:
				node.prefix ? '$.update_store_pre' : '$.update_store',
				b.assignment('??=', b.id('$$store_subs'), b.object([])),
				b.literal(argument.name),
				b.id(argument.name.slice(1)),
				node.operator === '--' && b.literal(-1)
			);
		}

		return next();
	},
	CallExpression(node, context) {
		const rune = get_rune(node, context.state.scope);

		if (rune === '$host') {
			return b.id('undefined');
		}

		if (rune === '$effect.tracking') {
			return b.literal(false);
		}

		if (rune === '$effect.root') {
			const args = /** @type {import('estree').Expression[]} */ (
				node.arguments.map((arg) => context.visit(arg))
			);
			// Just call the function directly
			return b.call(args[0]);
		}

		if (rune === '$state.snapshot') {
			return /** @type {import('estree').Expression} */ (context.visit(node.arguments[0]));
		}

		if (rune === '$state.is') {
			return b.call(
				'Object.is',
				/** @type {import('estree').Expression} */ (context.visit(node.arguments[0])),
				/** @type {import('estree').Expression} */ (context.visit(node.arguments[1]))
			);
		}

		if (rune === '$inspect' || rune === '$inspect().with') {
			return transform_inspect_rune(node, context);
		}

		context.next();
	}
};

/** @type {import('./types').Visitors} */
const javascript_visitors_runes = {
Repository: sveltejs/svelte
File: packages/svelte/scripts/process-messages/templates/compile-errors.js (JavaScript)

Code before:
import { CompileDiagnostic } from './utils/compile_diagnostic.js';

/** @typedef {{ start?: number, end?: number }} NodeLike */

export class InternalCompileError extends CompileDiagnostic {
	name = 'CompileError';

	/**
	 * @param {string} code
	 * @param {string} message
	 * @param {[number, number] | undefined} position
	 */
	constructor(code, message, position) {
		super(code, message, position);
	}
}

/**
 * @param {null | number | NodeLike} node
 * @param {string} code
 * @param {string} message
 * @returns {never}
 */
function e(node, code, message) {
	const start = typeof node === 'number' ? node : node?.start;
	const end = typeof node === 'number' ? node : node?.end;

	throw new InternalCompileError(
		code,
		message,
		start !== undefined && end !== undefined ? [start, end] : undefined

Review comment (suggestion):
side note: I would've called this "CompilerError" (note the additional r). Is "compile error" more correct? I stumble over that mentally each time.

Code after:
import { CompileDiagnostic } from './utils/compile_diagnostic.js';

/** @typedef {{ start?: number, end?: number }} NodeLike */

export class InternalCompileError extends CompileDiagnostic {
	name = 'CompileError';

	/**
	 * @param {string} code
	 * @param {string} message
	 * @param {[number, number] | undefined} position
	 */
	constructor(code, message, position) {
		super(code, message, position);
	}
}

/**
 * @param {null | number | NodeLike} node
 * @param {string} code
 * @param {string} message
 * @returns {never}
 */
function e(node, code, message) {
	const start = typeof node === 'number' ? node : node?.start;
	const end = typeof node === 'number' ? node : node?.end;

	throw new InternalCompileError(
		code,
		message,
		start !== undefined ? [start, end ?? start] : undefined
Repository: sveltejs/svelte
File: packages/svelte/src/legacy/legacy-client.js (JavaScript)

Code before:

class Svelte4Component {
	/** @type {any} */
	#events;

	/** @type {Record<string, any>} */
	#instance;

	/**
	 * @param {ComponentConstructorOptions & {
	 *  component: any;
	 * 	immutable?: boolean;
	 * 	hydrate?: boolean;
	 * 	recover?: false;
	 * }} options
	 */
	constructor(options) {
		var sources = new Map();
		var add_source = (/** @type {string | symbol} */ key) => {
			var s = mutable_source(0);
			sources.set(key, s);
			return s;
		};
		// Using proxy state here isn't completely mirroring the Svelte 4 behavior, because mutations to a property
		// cause fine-grained updates to only the places where that property is used, and not the entire property.
		// Reactive statements and actions (the things where this matters) are handling this properly regardless, so it should be fine in practise.
		const props = new Proxy(
			{ ...(options.props || {}), $$events: {} },
			{
				get(target, prop, receiver) {
					var value = Reflect.get(target, prop, receiver);
					var s = sources.get(prop);
					if (s === undefined) {
						s = add_source(prop);
					}
					get(s);
					return value;
				},
				has(target, prop) {
					var value = Reflect.has(target, prop);
					var s = sources.get(prop);
					if (s !== undefined) {
						get(s);
					}
					return value;
				},
				set(target, prop, value) {
					var s = sources.get(prop);
					// @ts-ignore
					var prev_value = target[prop];
					if (s === undefined) {

Review comment (suggestion):
```suggestion
		// Replicate coarse-grained props through a proxy that has a version source for
		// each property, which is increment on updates to the property itself. Do not
		// use our $state proxy because that one has fine-grained reactivity.
```

Code after:

class Svelte4Component {
	/** @type {any} */
	#events;

	/** @type {Record<string, any>} */
	#instance;

	/**
	 * @param {ComponentConstructorOptions & {
	 *  component: any;
	 * 	immutable?: boolean;
	 * 	hydrate?: boolean;
	 * 	recover?: false;
	 * }} options
	 */
	constructor(options) {
		var sources = new Map();
		var add_source = (/** @type {string | symbol} */ key) => {
			var s = mutable_source(0);
			sources.set(key, s);
			return s;
		};
		// Replicate coarse-grained props through a proxy that has a version source for
		// each property, which is increment on updates to the property itself. Do not
		// use our $state proxy because that one has fine-grained reactivity.
		const props = new Proxy(
			{ ...(options.props || {}), $$events: {} },
			{
				get(target, prop, receiver) {
					var value = Reflect.get(target, prop, receiver);
					var s = sources.get(prop);
					if (s === undefined) {
						s = add_source(prop);
					}
					get(s);
					return value;
				},
				has(target, prop) {
					var value = Reflect.has(target, prop);
					var s = sources.get(prop);
					if (s !== undefined) {
						get(s);
					}
					return value;
				},
				set(target, prop, value) {
					var s = sources.get(prop);
					// @ts-ignore
					var prev_value = target[prop];
					if (s === undefined) {
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
				if (check_dirtiness(/** @type {import('#client').Derived} */ (dependency))) {
					update_derived(/** @type {import('#client').Derived} */ (dependency));
				}

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

				if (is_unowned) {
					// TODO is there a more logical place to do this work?
					if (!current_skip_reaction && !dependency?.reactions?.includes(reaction)) {
						// 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.
						(dependency.reactions ??= []).push(reaction);
					}
				} else if (is_disconnected) {
					reconnect_derived(reaction);
				}
			}
		}

		if (is_disconnected) {
			reaction.f ^= DISCONNECTED;
		}

		set_signal_status(reaction, CLEAN);
	}

	return false;
}

/**
 * @param {Error} error
 * @param {import("#client").Effect} effect
 * @param {import("#client").ComponentContext | null} component_context
 */
function handle_error(error, effect, component_context) {
	// Given we don't yet have error boundaries, we will just always throw.
	if (!DEV || handled_errors.has(error) || component_context === null) {
		throw error;
	}

	const component_stack = [];

	const effect_name = effect.fn?.name;

	if (effect_name) {
		component_stack.push(effect_name);
	}


Review comment (suggestion):
we can do this inside `reconnect_derived`

```suggestion
```

Code after:
				}

				reaction.f ^= DISCONNECTED;
			}

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

				if (check_dirtiness(/** @type {import('#client').Derived} */ (dependency))) {
					update_derived(/** @type {import('#client').Derived} */ (dependency));
				}

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

				if (is_unowned) {
					// TODO is there a more logical place to do this work?
					if (!current_skip_reaction && !dependency?.reactions?.includes(reaction)) {
						// 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.
						(dependency.reactions ??= []).push(reaction);
					}
				}
			}
		}

		set_signal_status(reaction, CLEAN);
	}

	return false;
}

/**
 * @param {Error} error
 * @param {import("#client").Effect} effect
 * @param {import("#client").ComponentContext | null} component_context
 */
function handle_error(error, effect, component_context) {
	// Given we don't yet have error boundaries, we will just always throw.
	if (!DEV || handled_errors.has(error) || component_context === null) {
		throw error;
	}

	const component_stack = [];

	const effect_name = effect.fn?.name;

	if (effect_name) {
		component_stack.push(effect_name);
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
				new_deps.push(signal);
			}
		}

		if (
			current_untracked_writes !== null &&
			current_effect !== null &&
			(current_effect.f & CLEAN) !== 0 &&
			(current_effect.f & BRANCH_EFFECT) === 0 &&
			current_untracked_writes.includes(signal)
		) {
			set_signal_status(current_effect, DIRTY);
			schedule_effect(current_effect);
		}
	}

	if ((flags & DERIVED) !== 0) {
		var derived = /** @type {import('#client').Derived} */ (signal);

		if (check_dirtiness(derived)) {
			update_derived(derived);
		}

		if ((flags & DISCONNECTED) !== 0) {
			reconnect_derived(derived);
			derived.f ^= DISCONNECTED;
		}
	}

	return signal.v;
}

/**
 * Invokes a function and captures all signals that are read during the invocation,
 * then invalidates them.
 * @param {() => any} fn
 */
export function invalidate_inner_signals(fn) {
	var previous_is_signals_recorded = is_signals_recorded;
	var previous_captured_signals = captured_signals;
	is_signals_recorded = true;
	captured_signals = new Set();
	var captured = captured_signals;
	var signal;
	try {
		untrack(fn);
	} finally {
		is_signals_recorded = previous_is_signals_recorded;
		if (is_signals_recorded) {
			for (signal of captured_signals) {
				previous_captured_signals.add(signal);

Review comment (suggestion):
we can do this inside `reconnect_derived`

```suggestion
```

Code after:
				new_deps = [signal];
			} else if (new_deps[new_deps.length - 1] !== signal) {
				new_deps.push(signal);
			}
		}

		if (
			current_untracked_writes !== null &&
			current_effect !== null &&
			(current_effect.f & CLEAN) !== 0 &&
			(current_effect.f & BRANCH_EFFECT) === 0 &&
			current_untracked_writes.includes(signal)
		) {
			set_signal_status(current_effect, DIRTY);
			schedule_effect(current_effect);
		}
	}

	if ((flags & DERIVED) !== 0) {
		var derived = /** @type {import('#client').Derived} */ (signal);

		if (check_dirtiness(derived)) {
			update_derived(derived);
		}
	}

	return signal.v;
}

/**
 * Invokes a function and captures all signals that are read during the invocation,
 * then invalidates them.
 * @param {() => any} fn
 */
export function invalidate_inner_signals(fn) {
	var previous_is_signals_recorded = is_signals_recorded;
	var previous_captured_signals = captured_signals;
	is_signals_recorded = true;
	captured_signals = new Set();
	var captured = captured_signals;
	var signal;
	try {
		untrack(fn);
	} finally {
		is_signals_recorded = previous_is_signals_recorded;
		if (is_signals_recorded) {
			for (signal of captured_signals) {
				previous_captured_signals.add(signal);
			}
		}
		captured_signals = previous_captured_signals;
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-runes/samples/spread-props/_config.js (JavaScript)

Code before:
import { test } from '../../test';

export default test({
	compileOptions: {
		dev: false
	},

	async test({ assert, target }) {
		assert.htmlEqual(
			target.innerHTML,
			`<button data-attr="">Hello world</button><button data-attr="">Hello world</button>`
		);
	}
});


Review comment (bug):
I added comments to the other two but this seems like a legit bug?

```suggestion
```

Code after:
import { test } from '../../test';

export default test({
	async test({ assert, target }) {
		assert.htmlEqual(
			target.innerHTML,
			`<button data-attr="">Hello world</button><button data-attr="">Hello world</button>`
		);
	}
});
Repository: sveltejs/svelte
File: packages/svelte/tests/snapshot/samples/hmr/_expected/client/index.svelte.js (JavaScript)

Code before:
import "svelte/internal/disclose-version";
import * as $ from "svelte/internal/client";

var root = $.template(`<h1>hello world</h1>`);

function Hmr($$anchor) {
	var h1 = root();

	$.append($$anchor, h1);
}

if (import.meta.hot) {
	const s = $.source(Hmr);
	const filename = Hmr.filename;

	Hmr = $.hmr(s);
	Hmr.filename = filename;

	import.meta.hot.accept((module) => {
		$.set(s, module.default);
	});
}

export default Hmr;

Review comment (suggestion):
should `filename` and `original` be behind a symbol, or at least non-enumerable? Didn't occur to me until now but it's probably a bit weird that we're adding these properties for anyone to look at

Code after:
import "svelte/internal/disclose-version";
import * as $ from "svelte/internal/client";

var root = $.template(`<h1>hello world</h1>`);

function Hmr($$anchor) {
	var h1 = root();

	$.append($$anchor, h1);
}

if (import.meta.hot) {
	const s = $.source(Hmr);
	const filename = Hmr.filename;
	const $$original = Hmr;

	Hmr = $.hmr(s);
	Hmr.filename = filename;
	Hmr.original = $$original;

	import.meta.hot.accept((module) => {
		$.set(s, module.default.original);
	});
}

export default Hmr;
Repository: sveltejs/svelte
File: packages/svelte/src/ambient.d.ts (TypeScript)

Code before:
	 * Declares reactive read-only state that is shallowly immutable.
	 *
	 * Example:
	 * ```ts
	 * <script>
	 *   let items = $state.frozen([0]);
	 *
	 *   const addItem = () => {
	 *     items = [...items, items.length];
	 *   };
	 * </script>
	 *
	 * <button on:click={addItem}>
	 *   {items.join(', ')}
	 * </button>
	 * ```
	 *
	 * https://svelte-5-preview.vercel.app/docs/runes#$state-raw
	 *
	 * @param initial The initial value
	 */
	export function frozen<T>(initial: T): Readonly<T>;
	export function frozen<T>(): Readonly<T> | undefined;

	/**
	 * Declares reactive state that is linked to another value. Local mutations will override the linked value
	 * until the linked value changes.
	 *
	 * Example:
	 * ```ts
	 * <script>
	 *   let { text } = $props();
	 *   let local_text = $state.link(text);
	 * </script>
	 *
	 * <input type="text" bind:value={local_text} />
	 * ```
	 *
	 * https://svelte-5-preview.vercel.app/docs/runes#$state-link
	 *
	 * @param value The linked value
	 */
	export function link<T>(value: T): T;

	/**
	 * To take a static snapshot of a deeply reactive `$state` proxy, use `$state.snapshot`:
	 *
	 * Example:
	 * ```ts
	 * <script>
	 *   let counter = $state({ count: 0 });

Review comment (suggestion):
The word "mutations" sounds like `local_text.foo = bar` will not propagate back up to the original value. I think "reassignments" is a better word (and we likely need to explain this nuance in the `.md` docs)

Code after:
	 * Declares state that is _not_ made deeply reactive — instead of mutating it,
	 * you must reassign it.
	 *
	 * Example:
	 * ```ts
	 * <script>
	 *   let items = $state.raw([0]);
	 *
	 *   const addItem = () => {
	 *     items = [...items, items.length];
	 *   };
	 * </script>
	 *
	 * <button on:click={addItem}>
	 *   {items.join(', ')}
	 * </button>
	 * ```
	 *
	 * https://svelte-5-preview.vercel.app/docs/runes#$state-raw
	 *
	 * @param initial The initial value
	 */

	/**
	 * Declares reactive state that is linked to another value. Local reassignments
	 * will override the linked value until the linked value changes.
	 *
	 * Example:
	 * ```ts
	 * let a = $state(0);
	 * let b = $state.link(a);

	 * a = 1;
	 * console.log(a, b); // 1, 1

	 * b = 2; // unlink
	 * console.log(a, b); // 1, 2
	 *
	 * a = 3; // relink
	 * console.log(a, b); // 3, 3
	 * ```
	 *
	 * https://svelte-5-preview.vercel.app/docs/runes#$state-link
	 *
	 * @param value The linked value
	 */
	export function link<T>(value: T, callback?: (value: T) => void): T;

	export function raw<T>(initial: T): T;
	export function raw<T>(): T | undefined;
	/**
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/render.js (JavaScript)

Code before:
			anchor &&
			(anchor.nodeType !== 8 || /** @type {Comment} */ (anchor).data !== HYDRATION_START)
		) {
			anchor = /** @type {import('#client').TemplateNode} */ (anchor.nextSibling);
		}

		if (!anchor) {
			throw HYDRATION_ERROR;
		}

		set_hydrating(true);
		set_hydrate_node(/** @type {Comment} */ (anchor));
		hydrate_next();

		const instance = _mount(component, { ...options, anchor });

		if (
			hydrate_node.nodeType !== 8 ||
			/** @type {Comment} */ (hydrate_node).data !== HYDRATION_END
		) {
			w.hydration_mismatch();
			throw HYDRATION_ERROR;
		}

		// flush_sync will run this callback and then synchronously run any pending effects,
		// which don't belong to the hydration phase anymore - therefore reset it here
		set_hydrating(false);

		return /**  @type {Exports} */ (instance);
	} catch (error) {
		if (error === HYDRATION_ERROR) {
			// TODO it's possible for event listeners to have been added and
			// not removed, e.g. with `<svelte:window>` or `<svelte:document>`

			if (options.recover === false) {
				e.hydration_failed();
			}

			// If an error occured above, the operations might not yet have been initialised.
			init_operations();
			clear_text_content(target);

			set_hydrating(false);
			return mount(component, options);
		}

		throw error;
	} finally {
		set_hydrating(was_hydrating);
		set_hydrate_node(previous_hydrate_node);
		reset_head_anchor();

Review comment (suggestion):
this comment seems out of date, and also a little superfluous anyway — shall we just get rid of it?

```suggestion
```

Code after:
			anchor &&
			(anchor.nodeType !== 8 || /** @type {Comment} */ (anchor).data !== HYDRATION_START)
		) {
			anchor = /** @type {import('#client').TemplateNode} */ (anchor.nextSibling);
		}

		if (!anchor) {
			throw HYDRATION_ERROR;
		}

		set_hydrating(true);
		set_hydrate_node(/** @type {Comment} */ (anchor));
		hydrate_next();

		const instance = _mount(component, { ...options, anchor });

		if (
			hydrate_node.nodeType !== 8 ||
			/** @type {Comment} */ (hydrate_node).data !== HYDRATION_END
		) {
			w.hydration_mismatch();
			throw HYDRATION_ERROR;
		}

		set_hydrating(false);

		return /**  @type {Exports} */ (instance);
	} catch (error) {
		if (error === HYDRATION_ERROR) {
			// TODO it's possible for event listeners to have been added and
			// not removed, e.g. with `<svelte:window>` or `<svelte:document>`

			if (options.recover === false) {
				e.hydration_failed();
			}

			// If an error occured above, the operations might not yet have been initialised.
			init_operations();
			clear_text_content(target);

			set_hydrating(false);
			return mount(component, options);
		}

		throw error;
	} finally {
		set_hydrating(was_hydrating);
		set_hydrate_node(previous_hydrate_node);
		reset_head_anchor();
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/server/visitors/AssignmentExpression.js (JavaScript)

Code before:
		if (!changed) {
			// No change to output -> nothing to transform -> we can keep the original assignment
			return context.next();
		}

		const sequence = b.sequence(assignments);

		if (!is_standalone) {
			// this is part of an expression, we need the sequence to end with the value
			sequence.expressions.push(rhs);
		}

		if (should_cache) {
			// the right hand side is a complex expression, wrap in an IIFE to cache it
			return b.call(b.arrow([rhs], sequence), value);
		}

		return sequence;
	}

	return serialize_assignment(node.operator, node.left, node.right, context) || context.next();
}

/**
 * @param {AssignmentOperator} operator
 * @param {Pattern} left
 * @param {Expression} right
 * @param {import('zimmerframe').Context<SvelteNode, ServerTransformState>} context
 * @returns {Expression | null}
 */
function serialize_assignment(operator, left, right, context) {
	let object = left;

	while (object.type === 'MemberExpression') {
		// @ts-expect-error
		object = object.object;
	}

	if (object.type !== 'Identifier' || !is_store_name(object.name)) {
		return null;
	}

	const name = object.name.slice(1);

	if (!context.state.scope.get(name)) {
		return null;
	}

	if (object === left) {
		let value = /** @type {Expression} */ (context.visit(right));


Review comment (suggestion):
```suggestion
 * Only returns an expression if this is not a `$store` assignment, as others can be kept as-is
 * @param {AssignmentOperator} operator
```

Code after:
		if (!changed) {
			// No change to output -> nothing to transform -> we can keep the original assignment
			return context.next();
		}

		const sequence = b.sequence(assignments);

		if (!is_standalone) {
			// this is part of an expression, we need the sequence to end with the value
			sequence.expressions.push(rhs);
		}

		if (should_cache) {
			// the right hand side is a complex expression, wrap in an IIFE to cache it
			return b.call(b.arrow([rhs], sequence), value);
		}

		return sequence;
	}

	return serialize_assignment(node.operator, node.left, node.right, context) || context.next();
}

/**
 * Only returns an expression if this is not a `$store` assignment, as others can be kept as-is
 * @param {AssignmentOperator} operator
 * @param {Pattern} left
 * @param {Expression} right
 * @param {import('zimmerframe').Context<SvelteNode, ServerTransformState>} context
 * @returns {Expression | null}
 */
function serialize_assignment(operator, left, right, context) {
	let object = left;

	while (object.type === 'MemberExpression') {
		// @ts-expect-error
		object = object.object;
	}

	if (object.type !== 'Identifier' || !is_store_name(object.name)) {
		return null;
	}

	const name = object.name.slice(1);

	if (!context.state.scope.get(name)) {
		return null;
	}

	if (object === left) {
		let value = /** @type {Expression} */ (context.visit(right));