ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/tests/signals/test.ts (TypeScript)

Code before:
	test('set version state does not trigger self-dependency guard', () => {
		return () => {
			const set = new SvelteSet();
			const d = derived(() => set.has('test'));

			set.add('test');
			assert.equal($.get(d), true);
		};
	});

	test('deriveds read inside the root/branches are cleaned up', () => {
		return () => {
			const a = state(0);

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

			destroy();

			assert.deepEqual(a.reactions, null);
		};
	});

	test('nested deriveds clean up the releationships when used with untrack', () => {
		return () => {
			let a = render_effect(() => {});

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

			assert.deepEqual(a.deriveds?.length, 1);

			destroy();

			assert.deepEqual(a.deriveds, null);
		};
	});
});

Review comment (refactor):
```suggestion
	test('nested deriveds clean up the relationships when used with untrack', () => {
```

Code after:
	test('set version state does not trigger self-dependency guard', () => {
		return () => {
			const set = new SvelteSet();
			const d = derived(() => set.has('test'));

			set.add('test');
			assert.equal($.get(d), true);
		};
	});

	test('deriveds read inside the root/branches are cleaned up', () => {
		return () => {
			const a = state(0);

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

			destroy();

			assert.deepEqual(a.reactions, null);
		};
	});

	test('nested deriveds clean up the relationships when used with untrack', () => {
		return () => {
			let a = render_effect(() => {});

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

			assert.deepEqual(a.deriveds?.length, 1);

			destroy();

			assert.deepEqual(a.deriveds, null);
		};
	});
});
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/deriveds.js (JavaScript)

Code before:

	if (active_effect === null) {
		flags |= UNOWNED;
	} else {
		// Since deriveds are evaluated lazily, any effects created inside them are
		// created too late to ensure that the parent effect is added to the tree
		active_effect.f |= EFFECT_HAS_DERIVED;
	}

	var parent_derived =
		active_reaction !== null && (active_reaction.f & DERIVED) !== 0
			? /** @type {Derived} */ (active_reaction)
			: null;

	/** @type {Derived<V>} */
	const signal = {
		children: null,
		ctx: component_context,
		deps: null,
		equals,
		f: flags,
		fn,
		reactions: null,
		v: /** @type {V} */ (null),
		version: 0,
		parent: parent_derived || active_effect
	};

	if (parent_derived !== null) {
		(parent_derived.children ??= []).push(signal);
	}

	return signal;
}

/**
 * @template V
 * @param {() => V} fn
 * @returns {Derived<V>}
 */
/*#__NO_SIDE_EFFECTS__*/
export function derived_safe_equal(fn) {
	const signal = derived(fn);
	signal.equals = safe_equals;
	return signal;
}

/**
 * @param {Derived} derived
 * @returns {void}
 */

Review comment (suggestion):
```suggestion
		parent: parent_derived ?? active_effect
```

Code after:

	if (active_effect === null) {
		flags |= UNOWNED;
	} else {
		// Since deriveds are evaluated lazily, any effects created inside them are
		// created too late to ensure that the parent effect is added to the tree
		active_effect.f |= EFFECT_HAS_DERIVED;
	}

	var parent_derived =
		active_reaction !== null && (active_reaction.f & DERIVED) !== 0
			? /** @type {Derived} */ (active_reaction)
			: null;

	/** @type {Derived<V>} */
	const signal = {
		children: null,
		ctx: component_context,
		deps: null,
		equals,
		f: flags,
		fn,
		reactions: null,
		v: /** @type {V} */ (null),
		version: 0,
		parent: parent_derived ?? active_effect
	};

	if (parent_derived !== null) {
		(parent_derived.children ??= []).push(signal);
	}

	return signal;
}

/**
 * @template V
 * @param {() => V} fn
 * @returns {Derived<V>}
 */
/*#__NO_SIDE_EFFECTS__*/
export function derived_safe_equal(fn) {
	const signal = derived(fn);
	signal.equals = safe_equals;
	return signal;
}

/**
 * @param {Derived} derived
 * @returns {void}
 */
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/elements/attributes.js (JavaScript)

Code before:
			} else if (setters.includes(name) && (is_custom_element || typeof value !== 'string')) {
				// @ts-ignore
				element[name] = value;
			} else if (typeof value !== 'function') {
				if (hydrating && (name === 'src' || name === 'href' || name === 'srcset')) {
					if (!skip_warning) check_src_in_dev_hydration(element, name, value ?? '');
				} else {
					set_attribute(element, name, value);
				}
			}
		}
		if (key === 'style' && '__styles' in element) {
			// reset styles to force style: directive to update
			element.__styles = {};
		}
	}

	return current;
}

/** @type {Map<string, string[]>} */
var setters_cache = new Map();

/** @param {Element} element */
function get_setters(element) {
	var setters = setters_cache.get(element.nodeName);
	setters_cache.set(element.nodeName, (setters = []));

	var descriptors;
	var proto = element; // In the case of custom elements there might be setters on the instance
	var element_proto = Element.prototype;

	// Stop at Element, from there on there's only unnecessary setters we're not interested in
	// Do not use contructor.name here as that's unreliable in some browser environments
	while (element_proto !== proto) {
		descriptors = get_descriptors(proto);

		for (var key in descriptors) {
			if (descriptors[key].set) {
				setters.push(key);
			}
		}

		proto = get_prototype_of(proto);
	}

	return setters;
}

/**
 * @param {any} element

Review comment (performance):
```suggestion
	var setters = setters_cache.get(element.nodeName);
	if (setters) return setters;
```

Code after:
			} else if (setters.includes(name) && (is_custom_element || typeof value !== 'string')) {
				// @ts-ignore
				element[name] = value;
			} else if (typeof value !== 'function') {
				if (hydrating && (name === 'src' || name === 'href' || name === 'srcset')) {
					if (!skip_warning) check_src_in_dev_hydration(element, name, value ?? '');
				} else {
					set_attribute(element, name, value);
				}
			}
		}
		if (key === 'style' && '__styles' in element) {
			// reset styles to force style: directive to update
			element.__styles = {};
		}
	}

	return current;
}

/** @type {Map<string, string[]>} */
var setters_cache = new Map();

/** @param {Element} element */
function get_setters(element) {
	var setters = setters_cache.get(element.nodeName);
	if (setters) return setters;
	setters_cache.set(element.nodeName, (setters = []));

	var descriptors;
	var proto = element; // In the case of custom elements there might be setters on the instance
	var element_proto = Element.prototype;

	// Stop at Element, from there on there's only unnecessary setters we're not interested in
	// Do not use contructor.name here as that's unreliable in some browser environments
	while (element_proto !== proto) {
		descriptors = get_descriptors(proto);

		for (var key in descriptors) {
			if (descriptors[key].set) {
				setters.push(key);
			}
		}

		proto = get_prototype_of(proto);
	}

	return setters;
}

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

Code before:

/**
 * @param {AST.IfBlock} node
 * @param {ComponentContext} context
 */
export function IfBlock(node, context) {
	context.state.template.push('<!>');

	const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));
	const consequent_id = context.state.scope.generate('consequent');

	context.state.init.push(b.var(b.id(consequent_id), b.arrow([b.id('$$anchor')], consequent)));

	let alternate_id;

	if (node.alternate) {
		const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
		alternate_id = context.state.scope.generate('alternate');
		context.state.init.push(b.var(b.id(alternate_id), b.arrow([b.id('$$anchor')], alternate)));
	}

	/** @type {Expression[]} */
	const args = [
		context.state.node,
		b.arrow(
			[b.id('$$branch')],
			b.block([
				b.if(
					/** @type {Expression} */ (context.visit(node.test)),
					b.stmt(b.call(b.id('$$branch'), b.literal(0), b.id(consequent_id))),
					alternate_id
						? b.stmt(b.call(b.id('$$branch'), b.literal(1), b.id(alternate_id)))
						: undefined
				)
			])
		)
	];

	if (node.elseif) {
		// We treat this...
		//
		//   {#if x}
		//     ...
		//   {:else}
		//     {#if y}
		//       <div transition:foo>...</div>
		//     {/if}
		//   {/if}
		//
		// ...slightly differently to this...
		//

Review comment (suggestion):
maybe `$$render` instead of `$$branch`? that way it reads like an instruction — `$$render(consequent)`

```suggestion
			[b.id('$$render')],
```

Code after:

/**
 * @param {AST.IfBlock} node
 * @param {ComponentContext} context
 */
export function IfBlock(node, context) {
	context.state.template.push('<!>');
	const statements = [];

	const consequent = /** @type {BlockStatement} */ (context.visit(node.consequent));
	const consequent_id = context.state.scope.generate('consequent');

	statements.push(b.var(b.id(consequent_id), b.arrow([b.id('$$anchor')], consequent)));

	let alternate_id;

	if (node.alternate) {
		const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
		alternate_id = context.state.scope.generate('alternate');
		statements.push(b.var(b.id(alternate_id), b.arrow([b.id('$$anchor')], alternate)));
	}

	/** @type {Expression[]} */
	const args = [
		context.state.node,
		b.arrow(
			[b.id('$$render')],
			b.block([
				b.if(
					/** @type {Expression} */ (context.visit(node.test)),
					b.stmt(b.call(b.id('$$render'), b.id(consequent_id))),
					alternate_id
						? b.stmt(
								b.call(
									b.id('$$render'),
									b.id(alternate_id),
									node.alternate ? b.literal(false) : undefined
								)
							)
						: undefined
				)
			])
		)
	];

	if (node.elseif) {
		// We treat this...
		//
		//   {#if x}
		//     ...
		//   {:else}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
			for (signal of captured_signals) {
				previous_captured_signals.add(signal);
			}
		}
	} finally {
		captured_signals = previous_captured_signals;
	}

	return captured;
}

/**
 * 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 captured = capture_signals(() => untrack(fn));

	for (var signal of captured) {
		// Go one level up because derived signals created as part of props in legacy mode
		if ((signal.f & LEGACY_DERIVED_PROP) !== 0) {
			for (const dep of /** @type {Derived} */ (signal).deps || []) {
				if ((dep.f & DERIVED) === 0) {
					internal_set(dep, dep.v);
				}
			}
		} else {
			internal_set(signal, signal.v);
		}
	}
}

/**
 * When used inside a [`$derived`](https://svelte.dev/docs/svelte/$derived) or [`$effect`](https://svelte.dev/docs/svelte/$effect),
 * any state read inside `fn` will not be treated as a dependency.
 *
 * ```ts
 * $effect(() => {
 *   // this will run when `data` changes, but not when `time` changes
 *   save(data, {
 *     timestamp: untrack(() => time)
 *   });
 * });
 * ```
 * @template T
 * @param {() => T} fn
 * @returns {T}
 */
export function untrack(fn) {
	const previous_reaction = active_reaction;

Review comment (suggestion):
```suggestion
					// Use internal_set instead of set here and below to avoid mutation validation
					internal_set(dep, dep.v);
```

Code after:
			for (signal of captured_signals) {
				previous_captured_signals.add(signal);
			}
		}
	} finally {
		captured_signals = previous_captured_signals;
	}

	return captured;
}

/**
 * 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 captured = capture_signals(() => untrack(fn));

	for (var signal of captured) {
		// Go one level up because derived signals created as part of props in legacy mode
		if ((signal.f & LEGACY_DERIVED_PROP) !== 0) {
			for (const dep of /** @type {Derived} */ (signal).deps || []) {
				if ((dep.f & DERIVED) === 0) {
					// Use internal_set instead of set here and below to avoid mutation validation
					internal_set(dep, dep.v);
				}
			}
		} else {
			internal_set(signal, signal.v);
		}
	}
}

/**
 * When used inside a [`$derived`](https://svelte.dev/docs/svelte/$derived) or [`$effect`](https://svelte.dev/docs/svelte/$effect),
 * any state read inside `fn` will not be treated as a dependency.
 *
 * ```ts
 * $effect(() => {
 *   // this will run when `data` changes, but not when `time` changes
 *   save(data, {
 *     timestamp: untrack(() => time)
 *   });
 * });
 * ```
 * @template T
 * @param {() => T} fn
 * @returns {T}
 */
export function untrack(fn) {
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/1-parse/read/expression.js (JavaScript)

Code before:
			index = node.trailingComments.at(-1).end;
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

			index += 1;
		}

		parser.index = index;

		return /** @type {Expression} */ (node);
	} catch (err) {
		/**
		 * If we are in an each loop we need the error to be thrown in cases like
		 *
		 * `as { y = z }`
		 *
		 * so we still throw and handle the error there
		 */
		if (parser.loose && !is_each) {
			// Find the next } and treat it as the end of the expression
			const expression = get_loose_identifier(parser, opening_token);
			if (expression) {
				return expression;
			}
		}

		parser.acorn_error(err);
	}
}


Review comment (suggestion):
```suggestion
		 // If we are in an each loop we need the error to be thrown in cases like
		 // `as { y = z }` so we still throw and handle the error there
```

Code after:
			index = node.trailingComments.at(-1).end;
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

			index += 1;
		}

		parser.index = index;

		return /** @type {Expression} */ (node);
	} catch (err) {
		// If we are in an each loop we need the error to be thrown in cases like
		// `as { y = z }` so we still throw and handle the error there
		if (parser.loose && !disallow_loose) {
			const expression = get_loose_identifier(parser, opening_token);
			if (expression) {
				return expression;
			}
		}

		parser.acorn_error(err);
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/1-parse/read/expression.js (JavaScript)

Code before:

/**
 * @param {Parser} parser
 * @param {string} [opening_token]
 * @returns {Expression | undefined}
 */
export function get_loose_identifier(parser, opening_token) {
	// Find the next } and treat it as the end of the expression
	const end = find_matching_bracket(parser.template, parser.index, opening_token ?? '{');
	if (end) {
		const start = parser.index;
		parser.index = end;
		// We don't know what the expression is and signal this by returning an empty identifier
		return {
			type: 'Identifier',
			start,
			end,
			name: ''
		};
	}
}

/**
 * @param {Parser} parser
 * @param {string} [opening_token]
 * @param {boolean} [is_each]
 * @returns {Expression}
 */
export default function read_expression(parser, opening_token, is_each) {
	try {
		const node = parse_expression_at(parser.template, parser.ts, parser.index);

		let num_parens = 0;

		for (let i = parser.index; i < /** @type {number} */ (node.start); i += 1) {
			if (parser.template[i] === '(') num_parens += 1;
		}

		let index = /** @type {number} */ (node.end);
		if (node.trailingComments !== undefined && node.trailingComments.length > 0) {
			index = node.trailingComments.at(-1).end;
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

Review comment (suggestion):
```suggestion
 * @param {boolean} [disallow_loose]
```

Code after:

/**
 * @param {Parser} parser
 * @param {string} [opening_token]
 * @returns {Expression | undefined}
 */
export function get_loose_identifier(parser, opening_token) {
	// Find the next } and treat it as the end of the expression
	const end = find_matching_bracket(parser.template, parser.index, opening_token ?? '{');
	if (end) {
		const start = parser.index;
		parser.index = end;
		// We don't know what the expression is and signal this by returning an empty identifier
		return {
			type: 'Identifier',
			start,
			end,
			name: ''
		};
	}
}

/**
 * @param {Parser} parser
 * @param {string} [opening_token]
 * @param {boolean} [disallow_loose]
 * @returns {Expression}
 */
export default function read_expression(parser, opening_token, disallow_loose) {
	try {
		const node = parse_expression_at(parser.template, parser.ts, parser.index);

		let num_parens = 0;

		for (let i = parser.index; i < /** @type {number} */ (node.start); i += 1) {
			if (parser.template[i] === '(') num_parens += 1;
		}

		let index = /** @type {number} */ (node.end);
		if (node.trailingComments !== undefined && node.trailingComments.length > 0) {
			index = node.trailingComments.at(-1).end;
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/1-parse/read/expression.js (JavaScript)

Code before:
 * @param {string} [opening_token]
 * @returns {Expression | undefined}
 */
export function get_loose_identifier(parser, opening_token) {
	// Find the next } and treat it as the end of the expression
	const end = find_matching_bracket(parser.template, parser.index, opening_token ?? '{');
	if (end) {
		const start = parser.index;
		parser.index = end;
		// We don't know what the expression is and signal this by returning an empty identifier
		return {
			type: 'Identifier',
			start,
			end,
			name: ''
		};
	}
}

/**
 * @param {Parser} parser
 * @param {string} [opening_token]
 * @param {boolean} [is_each]
 * @returns {Expression}
 */
export default function read_expression(parser, opening_token, is_each) {
	try {
		const node = parse_expression_at(parser.template, parser.ts, parser.index);

		let num_parens = 0;

		for (let i = parser.index; i < /** @type {number} */ (node.start); i += 1) {
			if (parser.template[i] === '(') num_parens += 1;
		}

		let index = /** @type {number} */ (node.end);
		if (node.trailingComments !== undefined && node.trailingComments.length > 0) {
			index = node.trailingComments.at(-1).end;
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

			index += 1;
		}

Review comment (suggestion):
```suggestion
export default function read_expression(parser, opening_token, disallow_loose) {
```

Code after:
 * @param {string} [opening_token]
 * @returns {Expression | undefined}
 */
export function get_loose_identifier(parser, opening_token) {
	// Find the next } and treat it as the end of the expression
	const end = find_matching_bracket(parser.template, parser.index, opening_token ?? '{');
	if (end) {
		const start = parser.index;
		parser.index = end;
		// We don't know what the expression is and signal this by returning an empty identifier
		return {
			type: 'Identifier',
			start,
			end,
			name: ''
		};
	}
}

/**
 * @param {Parser} parser
 * @param {string} [opening_token]
 * @param {boolean} [disallow_loose]
 * @returns {Expression}
 */
export default function read_expression(parser, opening_token, disallow_loose) {
	try {
		const node = parse_expression_at(parser.template, parser.ts, parser.index);

		let num_parens = 0;

		for (let i = parser.index; i < /** @type {number} */ (node.start); i += 1) {
			if (parser.template[i] === '(') num_parens += 1;
		}

		let index = /** @type {number} */ (node.end);
		if (node.trailingComments !== undefined && node.trailingComments.length > 0) {
			index = node.trailingComments.at(-1).end;
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

			index += 1;
		}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/1-parse/read/expression.js (JavaScript)

Code before:
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

			index += 1;
		}

		parser.index = index;

		return /** @type {Expression} */ (node);
	} catch (err) {
		/**
		 * If we are in an each loop we need the error to be thrown in cases like
		 *
		 * `as { y = z }`
		 *
		 * so we still throw and handle the error there
		 */
		if (parser.loose && !is_each) {
			// Find the next } and treat it as the end of the expression
			const expression = get_loose_identifier(parser, opening_token);
			if (expression) {
				return expression;
			}
		}

		parser.acorn_error(err);
	}
}


Review comment (suggestion):
```suggestion
		if (parser.loose && !disallow_loose) {
```

Code after:
		}

		while (num_parens > 0) {
			const char = parser.template[index];

			if (char === ')') {
				num_parens -= 1;
			} else if (!regex_whitespace.test(char)) {
				e.expected_token(index, ')');
			}

			index += 1;
		}

		parser.index = index;

		return /** @type {Expression} */ (node);
	} catch (err) {
		// If we are in an each loop we need the error to be thrown in cases like
		// `as { y = z }` so we still throw and handle the error there
		if (parser.loose && !disallow_loose) {
			const expression = get_loose_identifier(parser, opening_token);
			if (expression) {
				return expression;
			}
		}

		parser.acorn_error(err);
	}
}