ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/types/template.d.ts (TypeScript)

Code before:
			dynamic: boolean;
		};
	}

	export interface TitleElement extends BaseElement {
		type: 'TitleElement';
		name: 'title';
	}

	export interface SlotElement extends BaseElement {
		type: 'SlotElement';
		name: 'slot';
	}

	export interface RegularElement extends BaseElement {
		type: 'RegularElement';
		/** @internal */
		metadata: {
			/** `true` if this is an svg element */
			svg: boolean;
			/** `true` if this is a mathml element */
			mathml: boolean;
			/** `true` if contains a SpreadAttribute */
			has_spread: boolean;
			scoped: boolean;
			path?: SvelteNode[];
		};
	}

	export interface SvelteBody extends BaseElement {
		type: 'SvelteBody';
		name: 'svelte:body';
	}

	export interface SvelteComponent extends BaseElement {
		type: 'SvelteComponent';
		name: 'svelte:component';
		expression: Expression;
		/** @internal */
		metadata: {
			scopes: Record<string, Scope>;
		};
	}

	export interface SvelteDocument extends BaseElement {
		type: 'SvelteDocument';
		name: 'svelte:document';
	}

	export interface SvelteElement extends BaseElement {
		type: 'SvelteElement';

Review comment (suggestion):
It's not really optional, isn't it? It's always set. This could be a bit confusing on the consumer side (in fact the if condition is unnecessariy checking for path existing). The one drawback is that you'd have to initialize it during AST creation, but I think that's fine.

Code after:
			dynamic: boolean;
		};
	}

	export interface TitleElement extends BaseElement {
		type: 'TitleElement';
		name: 'title';
	}

	export interface SlotElement extends BaseElement {
		type: 'SlotElement';
		name: 'slot';
	}

	export interface RegularElement extends BaseElement {
		type: 'RegularElement';
		/** @internal */
		metadata: {
			/** `true` if this is an svg element */
			svg: boolean;
			/** `true` if this is a mathml element */
			mathml: boolean;
			/** `true` if contains a SpreadAttribute */
			has_spread: boolean;
			scoped: boolean;
			path: SvelteNode[];
		};
	}

	export interface SvelteBody extends BaseElement {
		type: 'SvelteBody';
		name: 'svelte:body';
	}

	export interface SvelteComponent extends BaseElement {
		type: 'SvelteComponent';
		name: 'svelte:component';
		expression: Expression;
		/** @internal */
		metadata: {
			scopes: Record<string, Scope>;
		};
	}

	export interface SvelteDocument extends BaseElement {
		type: 'SvelteDocument';
		name: 'svelte:document';
	}

	export interface SvelteElement extends BaseElement {
		type: 'SvelteElement';
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/index.js (JavaScript)

Code before:
							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 &&
						element.metadata.path
					) {
						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) {

Review comment (suggestion):
```suggestion
						element.attributes.length === 1
```

Code after:
							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);
		}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/index.js (JavaScript)

Code before:
							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) ?? [];

Review comment (suggestion):
```suggestion
					if (is_custom_element_node(element) && element.attributes.length === 1) {
```

Code after:
							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);
		}
Repository: sveltejs/svelte
File: packages/svelte/tests/migrate/samples/single-assignment-labeled/input.svelte (Svelte)

Code before:
		sixteen_times = count * 16;
	}

	let alot_times;
	$:{
		// reassigned in multiple labeled
		alot_times = count * 32;
	}
	$:{
		// reassigned in multiple labeled
		alot_times = count * 32;
	}

	let evenmore;
	let evenmore_doubled;
	$:{
		// multiple stuff in label
		evenmore = count * 64;
		evenmore_doubled = evenmore * 2;
	}

	let almost_infinity;
	$: almost_infinity = count * 128;

	let should_be_state;
	$: should_be_state = 42;
</script>

<button on:click={()=>{
	count++;
	eight_times++;
	sixteen_times += 1;
}}>click</button>

Review comment (suggestion):
I also saw things like
```js
	$: should_also_be_state = 42;
```
without a preceeding `let` with the same name in the wild. Would be good to handle that aswell. and transform that into `let should_also_be_state = $state(42)`

Code after:
		sixteen_times = count * 16;
	}

	let alot_times;
	$:{
		// reassigned in multiple labeled
		alot_times = count * 32;
	}
	$:{
		// reassigned in multiple labeled
		alot_times = count * 32;
	}

	let evenmore;
	let evenmore_doubled;
	$:{
		// multiple stuff in label
		evenmore = count * 64;
		evenmore_doubled = evenmore * 2;
	}

	let almost_infinity;
	$: almost_infinity = count * 128;

	let should_be_state;
	$: should_be_state = 42;
	$: should_be_state_too = 42;
</script>

<button on:click={()=>{
	count++;
	eight_times++;
	sixteen_times += 1;
	should_be_state_too++;
}}>click</button>
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/each.js (JavaScript)

Code before:
				// to our seen Set as the item is already being handled
				if ((current.e.f & INERT) === 0) {
					(seen ??= new Set()).add(current);
				}
				stashed.push(current);
				current = current.next;
			}

			if (current === null) {
				continue;
			}

			item = current;
		}

		matched.push(item);
		prev = item;
		current = item.next;
	}

	if (current !== null || seen !== undefined) {
		var to_destroy = seen === undefined ? [] : array_from(seen);

		while (current !== null) {
			if ((current.e.f & INERT) === 0) {
				to_destroy.push(current);
			}
			current = current.next;
		}

		var destroy_length = to_destroy.length;

		if (destroy_length > 0) {
			var controlled_anchor = (flags & EACH_IS_CONTROLLED) !== 0 && length === 0 ? anchor : null;

			if (is_animated) {
				for (i = 0; i < destroy_length; i += 1) {
					to_destroy[i].a?.measure();
				}

				for (i = 0; i < destroy_length; i += 1) {
					to_destroy[i].a?.fix();
				}
			}

			pause_effects(state, to_destroy, controlled_anchor, items);
		}
	}

	if (is_animated) {
		queue_micro_task(() => {

Review comment (suggestion):
```suggestion
			// Inert effects are currently outroing and will be removed once the transition is finished
			if ((current.e.f & INERT) === 0) {
```

Code after:
				// to our seen Set as the item is already being handled
				if ((current.e.f & INERT) === 0) {
					(seen ??= new Set()).add(current);
				}
				stashed.push(current);
				current = current.next;
			}

			if (current === null) {
				continue;
			}

			item = current;
		}

		matched.push(item);
		prev = item;
		current = item.next;
	}

	if (current !== null || seen !== undefined) {
		var to_destroy = seen === undefined ? [] : array_from(seen);

		while (current !== null) {
			// Inert effects are currently outroing and will be removed once the transition is finished
			if ((current.e.f & INERT) === 0) {
				to_destroy.push(current);
			}
			current = current.next;
		}

		var destroy_length = to_destroy.length;

		if (destroy_length > 0) {
			var controlled_anchor = (flags & EACH_IS_CONTROLLED) !== 0 && length === 0 ? anchor : null;

			if (is_animated) {
				for (i = 0; i < destroy_length; i += 1) {
					to_destroy[i].a?.measure();
				}

				for (i = 0; i < destroy_length; i += 1) {
					to_destroy[i].a?.fix();
				}
			}

			pause_effects(state, to_destroy, controlled_anchor, items);
		}
	}

	if (is_animated) {
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-runes/samples/each-was-empty/_config.js (JavaScript)

Code before:
			target.querySelectorAll('button');

		const [span] = target.querySelectorAll('span');

		let count = 0;
		assert.equal(span.innerHTML, `${count}`);

		for (const button of [set_undefined, set_null, set_empty, set_undefined, set_null, set_empty]) {
			flushSync(() => {
				button.click();
			});
			flushSync(() => {
				increment.click();
				count++;
			});
			assert.equal(span.innerHTML, `${count}`);
		}

		flushSync(() => {
			set_list.click();
		});

		assert.htmlEqual(
			target.innerHTML,
			`<button>clicks: ${count}</button><button>undefined</button><button>null</button><button>empty</button><button>[1,2,3]</button><ul><li>item : 1</li><li>item : 2</li><li>item : 3</li></ul>`
		);
	}
});


Review comment (bug):
I know it's a bit more of manual work but i think tests should be as manual as possible...if for some reason in the future we break the logic of the test it could fail even if we don't have a problem in the library. Can you please rewrite this test with all manual actions?

Code after:
			target.querySelectorAll('button');

		let [span] = target.querySelectorAll('span');

		// initial value
		assert.exists(span);
		assert.equal(span.innerHTML, '0');

		// increment value
		flushSync(() => increment.click());
		assert.equal(span.innerHTML, '1');

		// change collection to undefined
		flushSync(() => set_undefined.click());
		// increment value
		flushSync(() => increment.click());
		assert.equal(span.innerHTML, '2');

		// change collection to null
		flushSync(() => set_null.click());
		// increment value
		flushSync(() => increment.click());
		assert.equal(span.innerHTML, '3');

		// change collection to empty
		flushSync(() => set_empty.click());
		// increment value
		flushSync(() => increment.click());
		assert.equal(span.innerHTML, '4');

		// change collection to undefined
		flushSync(() => set_undefined.click());
		// increment value
		flushSync(() => increment.click());
		assert.equal(span.innerHTML, '5');

		// change collection to [1,2,3]
		flushSync(() => set_list.click());
		[span] = target.querySelectorAll('span');
		assert.notExists(span);
		assert.equal(target.querySelectorAll('li').length, 3);

		// change collection to undefined
		flushSync(() => set_undefined.click());
		[span] = target.querySelectorAll('span');
		assert.exists(span);
		assert.equal(span.innerHTML, '5');

		// increment value
		flushSync(() => increment.click());
		assert.equal(span.innerHTML, '6');
Repository: sveltejs/svelte
File: packages/svelte/tests/signals/test.ts (TypeScript)

Code before:
				$.get(a);
			});
			assert.deepEqual(log, ['inner', 2]);
			destroy();
		};
	});

	test('owned deriveds correctly cleanup when no longer connected to graph', () => {
		let a: Derived<unknown>;
		let s = state(0);

		const destroy = effect_root(() => {
			render_effect(() => {
				a = derived(() => {
					$.get(s);
				});
				$.get(a);
			});
		});

		return () => {
			flushSync();
			assert.equal(a?.deps?.length, 1);
			assert.equal(s?.reactions?.length, 1);
			destroy();
			assert.equal(a?.deps?.length, 1);
			assert.equal(s?.reactions, null);
		};
	});

	test('deriveds update upon reconnection #1', () => {
		let a = state(false);
		let b = state(false);

		let c = derived(() => $.get(a));
		let d = derived(() => $.get(c));

		let last: Record<string, boolean | null> = {};

		render_effect(() => {
			last = {
				a: $.get(a),
				b: $.get(b),
				c: $.get(c),
				d: $.get(a) || $.get(b) ? $.get(d) : null
			};
		});

		return () => {
			assert.deepEqual(last, { a: false, b: false, c: false, d: null });


Review comment (refactor):
The derived still gets GC'd so this check isn't all that important. I think it's best to remove this line.

Code after:
				$.get(a);
			});
			assert.deepEqual(log, ['inner', 2]);
			destroy();
		};
	});

	test('owned deriveds correctly cleanup when no longer connected to graph', () => {
		let a: Derived<unknown>;
		let s = state(0);

		const destroy = effect_root(() => {
			render_effect(() => {
				a = derived(() => {
					$.get(s);
				});
				$.get(a);
			});
		});

		return () => {
			flushSync();
			assert.equal(a?.deps?.length, 1);
			assert.equal(s?.reactions?.length, 1);
			destroy();
			assert.equal(s?.reactions, null);
		};
	});

	test('deriveds update upon reconnection #1', () => {
		let a = state(false);
		let b = state(false);

		let c = derived(() => $.get(a));
		let d = derived(() => $.get(c));

		let last: Record<string, boolean | null> = {};

		render_effect(() => {
			last = {
				a: $.get(a),
				b: $.get(b),
				c: $.get(c),
				d: $.get(a) || $.get(b) ? $.get(d) : null
			};
		});

		return () => {
			assert.deepEqual(last, { a: false, b: false, c: false, d: null });

			flushSync(() => set(a, true));
Repository: sveltejs/svelte
File: packages/svelte/scripts/process-messages/index.js (JavaScript)

Code before:
		});

		// @ts-expect-error
		ast.body.push(clone);
	}

	// @ts-expect-error
	const module = esrap.print(ast);

	fs.writeFileSync(
		dest,
		`/* This file is generated by scripts/process-messages/index.js. Do not edit! */\n\n` +
			module.code,
		'utf-8'
	);
}

transform('compile-errors', 'src/compiler/errors.js');
transform('compile-warnings', 'src/compiler/warnings.js');

transform('client-warnings', 'src/internal/client/warnings.js');
transform('client-errors', 'src/internal/client/errors.js');
transform('server-errors', 'src/internal/server/errors.js');
transform('shared-errors', 'src/internal/shared/errors.js');
transform('shared-warnings', 'src/internal/shared/warnings.js');
transform('legacy-warnings', 'src/legacy/warnings.js');


Review comment (suggestion):
we don't need a new file for this, it should go in `compile-errors/script.md`

Code after:
		});

		// @ts-expect-error
		ast.body.push(clone);
	}

	// @ts-expect-error
	const module = esrap.print(ast);

	fs.writeFileSync(
		dest,
		`/* This file is generated by scripts/process-messages/index.js. Do not edit! */\n\n` +
			module.code,
		'utf-8'
	);
}

transform('compile-errors', 'src/compiler/errors.js');
transform('compile-warnings', 'src/compiler/warnings.js');

transform('client-warnings', 'src/internal/client/warnings.js');
transform('client-errors', 'src/internal/client/errors.js');
transform('server-errors', 'src/internal/server/errors.js');
transform('shared-errors', 'src/internal/shared/errors.js');
transform('shared-warnings', 'src/internal/shared/warnings.js');
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/effects.js (JavaScript)

Code before:
		}

		context.l.r2.v = false; // set directly to avoid rerunning this effect
	});
}

/**
 * @param {() => void | (() => void)} fn
 * @returns {Effect}
 */
export function render_effect(fn) {
	return create_effect(RENDER_EFFECT, fn, true);
}

/**
 * @param {() => void | (() => void)} fn
 * @returns {Effect}
 */
export function template_effect(fn) {
	if (DEV) {
		define_property(fn, 'name', {
			value: '{expression}'
		});
	}
	return block(fn);
}

/**
 * @param {(() => void)} fn
 * @param {number} flags
 */
export function block(fn, flags = 0) {
	return create_effect(RENDER_EFFECT | BLOCK_EFFECT | flags, fn, true);
}

/**
 * @param {(() => void)} fn
 * @param {boolean} [push]
 */
export function branch(fn, push = true) {
	return create_effect(RENDER_EFFECT | BRANCH_EFFECT, fn, true, push);
}

/**
 * @param {Effect} effect
 */
export function execute_effect_teardown(effect) {
	var teardown = effect.teardown;
	if (teardown !== null) {
		const previously_destroying_effect = is_destroying_effect;
		const previous_reaction = active_reaction;

Review comment (suggestion):
I'm confused, can we just change the type of effect? I would've expected this has unintended side effects.

Code after:
			token.ran = false;
		}

		context.l.r2.v = false; // set directly to avoid rerunning this effect
	});
}

/**
 * @param {() => void | (() => void)} fn
 * @returns {Effect}
 */
export function render_effect(fn) {
	return create_effect(RENDER_EFFECT, fn, true);
}

/**
 * @param {() => void | (() => void)} fn
 * @returns {Effect}
 */
export function template_effect(fn) {
	if (DEV) {
		define_property(fn, 'name', {
			value: '{expression}'
		});
	}
	return block(fn);
}

/**
 * @param {(() => void)} fn
 * @param {number} flags
 */
export function block(fn, flags = 0) {
	return create_effect(RENDER_EFFECT | BLOCK_EFFECT | flags, fn, true);
}

/**
 * @param {(() => void)} fn
 * @param {boolean} [push]
 */
export function branch(fn, push = true) {
	return create_effect(RENDER_EFFECT | BRANCH_EFFECT, fn, true, push);
}

/**
 * @param {Effect} effect
 */
export function execute_effect_teardown(effect) {
	var teardown = effect.teardown;
	if (teardown !== null) {
		const previously_destroying_effect = is_destroying_effect;
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-runes/samples/await-no-catch-error/main.svelte (Svelte)

Code before:
<script>
	const promise = new Promise((res, rej) => {
		rej(new Error('Test'));
	})
	let toggle = $state(false);
</script>

<button onclick={() => toggle = !toggle}>toggle</button>

{#if toggle}
	{#await promise}{/await}
{/if}


Review comment (suggestion):
AFAIK this should also do it
```suggestion
	const promise = Promise.reject('Test');
```

Code after:
<script>
	const promise = Promise.reject('Test');
	let toggle = $state(false);
</script>

<button onclick={() => toggle = !toggle}>toggle</button>

{#if toggle}
	{#await promise}{/await}
{/if}