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

Code before:

					const is_prop =
						binding.kind === 'prop' ||
						binding.kind === 'rest_prop' ||
						binding.kind === 'bindable_prop';

					if (!binding.updated && binding.initial !== null && !is_prop) {
						const evaluation = binding.scope.evaluate(/** @type {Expression} */ (binding.initial));
						for (const value of evaluation.values) {
							this.values.add(value);
						}
						break;
					}

					// TODO each index is always defined
				}

				// TODO glean what we can from reassignments
				// TODO one day, expose props and imports somehow

				this.values.add(UNKNOWN);
				break;

			case 'BinaryExpression':
				var a = scope.evaluate(/** @type {Expression} */ (expression.left)); // `left` cannot be `PrivateIdentifier` unless operator is `in`
				var b = scope.evaluate(expression.right);

				if (a.is_known && b.is_known) {
					this.values.add(binary[expression.operator](a.value, b.value));
					break;
				}

				switch (expression.operator) {
					case '!=':
					case '!==':
					case '<':
					case '<=':
					case '>':
					case '>=':
					case '==':
					case '===':
					case 'in':
					case 'instanceof':
						this.values.add(true);
						this.values.add(false);
						break;

					case '%':
					case '&':
					case '*':
					case '**':

Review comment (suggestion):
I see this switch statement is using `var` in a couple places - on purpose or muscle memory from the runtime? I'd rather use `let` etc here, also to make the code below (which just does `a = ...` in some places, which looks like reassignment but actually isn't) easier to reason about.

Code after:
						break;
					}

					const is_prop =
						binding.kind === 'prop' ||
						binding.kind === 'rest_prop' ||
						binding.kind === 'bindable_prop';

					if (!binding.updated && binding.initial !== null && !is_prop) {
						const evaluation = binding.scope.evaluate(/** @type {Expression} */ (binding.initial));
						for (const value of evaluation.values) {
							this.values.add(value);
						}
						break;
					}

					// TODO each index is always defined
				}

				// TODO glean what we can from reassignments
				// TODO one day, expose props and imports somehow

				this.values.add(UNKNOWN);
				break;
			}

			case 'BinaryExpression': {
				const a = scope.evaluate(/** @type {Expression} */ (expression.left)); // `left` cannot be `PrivateIdentifier` unless operator is `in`
				const b = scope.evaluate(expression.right);

				if (a.is_known && b.is_known) {
					this.values.add(binary[expression.operator](a.value, b.value));
					break;
				}

				switch (expression.operator) {
					case '!=':
					case '!==':
					case '<':
					case '<=':
					case '>':
					case '>=':
					case '==':
					case '===':
					case 'in':
					case 'instanceof':
						this.values.add(true);
						this.values.add(false);
						break;

					case '%':
Repository: sveltejs/svelte
File: packages/svelte/src/internal/server/dev.js (JavaScript)

Code before:

		while (ancestor != null) {
			ancestors.push(ancestor.tag);
			const ancestor_loc = ancestor.filename
				? `${ancestor.filename}:${ancestor.line}:${ancestor.column}`
				: undefined;

			const message = is_tag_valid_with_ancestor(tag, ancestors, child_loc, ancestor_loc);
			if (message) print_error(payload, message);

			ancestor = ancestor.parent;
		}
	}

	parent = child;
}

export function pop_element() {
	parent = /** @type {Element} */ (parent).parent;
}

/**
 * @param {Payload} payload
 */
export function validate_snippet_args(payload) {
	if (typeof payload !== 'object' || !('out' in payload)) {
		invalid_snippet_arguments();
	}
}


Review comment (suggestion):
I know very little about Svelte's internals, but could a symbol be a better way to validate this? If someone happens to pass in an object with an `out` property it'd pass this check.

Code after:
		if (message) print_error(payload, message);

		while (ancestor != null) {
			ancestors.push(ancestor.tag);
			const ancestor_loc = ancestor.filename
				? `${ancestor.filename}:${ancestor.line}:${ancestor.column}`
				: undefined;

			const message = is_tag_valid_with_ancestor(tag, ancestors, child_loc, ancestor_loc);
			if (message) print_error(payload, message);

			ancestor = ancestor.parent;
		}
	}

	parent = child;
}

export function pop_element() {
	parent = /** @type {Element} */ (parent).parent;
}

/**
 * @param {Payload} payload
 */
export function validate_snippet_args(payload) {
	if (typeof payload !== 'object' || !(payload instanceof Payload)) {
		invalid_snippet_arguments();
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/proxy.js (JavaScript)

Code before:
			return own_keys;
		},

		setPrototypeOf() {
			e.state_prototype_fixed();
		}
	});
}

/**
 * @param {Source<number>} signal
 * @param {1 | -1} [d]
 */
function update_version(signal, d = 1) {
	set(signal, signal.v + d);
}

/**
 * @param {any} value
 */
export function get_proxied_value(value) {
	try {
		if (value !== null && typeof value === 'object' && STATE_SYMBOL in value) {
			return value[STATE_SYMBOL];
		}
	} catch (err) {
		// the above if check can throw an error if the value in question
		// is the contentWindow of an iframe on another domain, in which
		// case we want to just return the value (because it's definitely
		// not a proxied value) so we don't break any JavaScript interacting
		// with that iframe (such as various payment companies client side
		// JavaScript libraries interacting with their iframes on the same
		// domain)
	}

	return value;
}

/**
 * @param {any} a
 * @param {any} b
 */
export function is(a, b) {
	return Object.is(get_proxied_value(a), get_proxied_value(b));
}


Review comment (suggestion):
since we're not using `err` we can remove it

```suggestion
	} catch {
```

Code after:
			return own_keys;
		},

		setPrototypeOf() {
			e.state_prototype_fixed();
		}
	});
}

/**
 * @param {Source<number>} signal
 * @param {1 | -1} [d]
 */
function update_version(signal, d = 1) {
	set(signal, signal.v + d);
}

/**
 * @param {any} value
 */
export function get_proxied_value(value) {
	try {
		if (value !== null && typeof value === 'object' && STATE_SYMBOL in value) {
			return value[STATE_SYMBOL];
		}
	} catch {
		// the above if check can throw an error if the value in question
		// is the contentWindow of an iframe on another domain, in which
		// case we want to just return the value (because it's definitely
		// not a proxied value) so we don't break any JavaScript interacting
		// with that iframe (such as various payment companies client side
		// JavaScript libraries interacting with their iframes on the same
		// domain)
	}

	return value;
}

/**
 * @param {any} a
 * @param {any} b
 */
export function is(a, b) {
	return Object.is(get_proxied_value(a), get_proxied_value(b));
}
Repository: sveltejs/svelte
File: packages/svelte/tests/signals/test.ts (TypeScript)

Code before:
					});
					$.get(c);
				});
			});

			assert.equal(c!.effects?.length, 1);
			assert.equal(a.first, a.last);

			set(b, 1);

			flushSync();

			assert.equal(c!.effects?.length, 1);
			assert.equal(a.first, a.last);

			destroy();

			assert.equal(a.first, null);

			assert.equal(effects.length, 2);
			assert.equal((effects[0].f & DESTROYED) !== 0, true);
			assert.equal((effects[1].f & DESTROYED) !== 0, true);
		};
	});

	test.only("deriveds set after they are DIRTY doesn't get updated on get", () => {
		return () => {
			const a = state(0);
			const b = derived(() => $.get(a));

			set(b, 1);
			assert.equal($.get(b), 1);

			set(a, 2);
			set(b, 3);

			assert.equal($.get(b), 3);
		};
	});

	test('deriveds containing effects work correctly when used with untrack', () => {
		return () => {
			let a = render_effect(() => {});
			let b = state(0);
			let c;
			let effects: Effect[] = [];

			const destroy = effect_root(() => {
				a = render_effect(() => {
					c = derived(() => {
						$.untrack(() => {

Review comment (suggestion):
```suggestion
	test("deriveds set after they are DIRTY doesn't get updated on get", () => {
```

Code after:
					});
					$.get(c);
				});
			});

			assert.equal(c!.effects?.length, 1);
			assert.equal(a.first, a.last);

			set(b, 1);

			flushSync();

			assert.equal(c!.effects?.length, 1);
			assert.equal(a.first, a.last);

			destroy();

			assert.equal(a.first, null);

			assert.equal(effects.length, 2);
			assert.equal((effects[0].f & DESTROYED) !== 0, true);
			assert.equal((effects[1].f & DESTROYED) !== 0, true);
		};
	});

	test("deriveds set after they are DIRTY doesn't get updated on get", () => {
		return () => {
			const a = state(0);
			const b = derived(() => $.get(a));

			set(b, 1);
			assert.equal($.get(b), 1);

			set(a, 2);
			assert.equal($.get(b), 2);
			set(b, 3);

			assert.equal($.get(b), 3);
		};
	});

	test("unowned deriveds set after they are DIRTY doesn't get updated on get", () => {
		return () => {
			const a = state(0);
			const b = derived(() => $.get(a));
			const c = derived(() => $.get(b));

			set(b, 1);
			assert.equal($.get(c), 1);

			set(a, 2);
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/sources.js (JavaScript)

Code before:
 * @param {V} value
 * @returns {V}
 */
export function internal_set(source, value) {
	if (!source.equals(value)) {
		var old_value = source.v;

		if (is_destroying_effect) {
			old_values.set(source, value);
		} else {
			old_values.set(source, old_value);
		}

		source.v = value;
		source.wv = increment_write_version();

		if (DEV && tracing_mode_flag) {
			source.updated = get_stack('UpdatedAt');
			if (active_effect != null) {
				source.trace_need_increase = true;
				source.trace_v ??= old_value;
			}
		}

		if ((source.f & DERIVED) !== 0) {
			set_signal_status(/** @type {Derived} */ (source), CLEAN);
		}

		mark_reactions(source, DIRTY);

		// It's possible that the current reaction might not have up-to-date dependencies
		// whilst it's actively running. So in the case of ensuring it registers the reaction
		// properly for itself, we need to ensure the current effect actually gets
		// scheduled. i.e: `$effect(() => x++)`
		if (
			is_runes() &&
			active_effect !== null &&
			(active_effect.f & CLEAN) !== 0 &&
			(active_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0
		) {
			if (untracked_writes === null) {
				set_untracked_writes([source]);
			} else {
				untracked_writes.push(source);
			}
		}

		if (DEV && inspect_effects.size > 0) {
			const inspects = Array.from(inspect_effects);

			for (const effect of inspects) {

Review comment (suggestion):
```suggestion
			set_signal_status(source, CLEAN);
```

Code after:
 * @param {V} value
 * @returns {V}
 */
export function internal_set(source, value) {
	if (!source.equals(value)) {
		var old_value = source.v;

		if (is_destroying_effect) {
			old_values.set(source, value);
		} else {
			old_values.set(source, old_value);
		}

		source.v = value;
		source.wv = increment_write_version();

		if (DEV && tracing_mode_flag) {
			source.updated = get_stack('UpdatedAt');
			if (active_effect != null) {
				source.trace_need_increase = true;
				source.trace_v ??= old_value;
			}
		}

		if ((source.f & DERIVED) !== 0) {
			// if we are assigning to a dirty derived we set it to clean/maybe dirty but we also eagerly execute it to track the dependencies
			if ((source.f & DIRTY) !== 0) {
				execute_derived(/** @type {Derived} */ (source));
			}
			set_signal_status(source, (source.f & UNOWNED) === 0 ? CLEAN : MAYBE_DIRTY);
		}

		mark_reactions(source, DIRTY);

		// It's possible that the current reaction might not have up-to-date dependencies
		// whilst it's actively running. So in the case of ensuring it registers the reaction
		// properly for itself, we need to ensure the current effect actually gets
		// scheduled. i.e: `$effect(() => x++)`
		if (
			is_runes() &&
			active_effect !== null &&
			(active_effect.f & CLEAN) !== 0 &&
			(active_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0
		) {
			if (untracked_writes === null) {
				set_untracked_writes([source]);
			} else {
				untracked_writes.push(source);
			}
		}
Repository: sveltejs/svelte
File: packages/svelte/tests/html_equal.js (JavaScript)

Code before:
	}
}

/**
 * @param {Window} window
 * @param {string} html
 * @param {{ removeDataSvelte?: boolean, preserveComments?: boolean }} param2
 */
export function normalize_html(
	window,
	html,
	{ removeDataSvelte = false, preserveComments = false }
) {
	try {
		const node = window.document.createElement('div');
		node.innerHTML = html
			.replace(/(<!(--)?.*?\2>)/g, preserveComments ? '$1' : '')
			.replace(/(data-svelte-h="[^"]+")/g, removeDataSvelte ? '' : '$1')
			.replace(/>[ \t\n\r\f]+</g, '><')
			// Strip out the special onload/onerror hydration events from the test output
			.replace(/\s?onerror="this.__e=event"|\s?onload="this.__e=event"/g, '')
			.trim();
		clean_children(node);
		return node.innerHTML;
	} catch (err) {
		throw new Error(`Failed to normalize HTML:\n${html}\nCause: ${err.message}`);
	}
}

/**
 * @param {string} html
 * @returns {string}
 */
export function normalize_new_line(html) {
	return html.replace(/\r\n/g, '\n');
}

/**
 * @param {{ removeDataSvelte?: boolean }} options
 */
export function setup_html_equal(options = {}) {
	/**
	 * @param {string} actual
	 * @param {string} expected
	 * @param {string} [message]
	 */
	const assert_html_equal = (actual, expected, message) => {
		try {
			assert.deepEqual(
				normalize_html(window, actual, options),
				normalize_html(window, expected, options),

Review comment (bug):
```suggestion
		throw new Error(`Failed to normalize HTML:\n${html}\nCause: ${err}`);
```

Code after:
	}
}

/**
 * @param {Window} window
 * @param {string} html
 * @param {{ removeDataSvelte?: boolean, preserveComments?: boolean }} param2
 */
export function normalize_html(
	window,
	html,
	{ removeDataSvelte = false, preserveComments = false }
) {
	try {
		const node = window.document.createElement('div');
		node.innerHTML = html
			.replace(/(<!(--)?.*?\2>)/g, preserveComments ? '$1' : '')
			.replace(/(data-svelte-h="[^"]+")/g, removeDataSvelte ? '' : '$1')
			.replace(/>[ \t\n\r\f]+</g, '><')
			// Strip out the special onload/onerror hydration events from the test output
			.replace(/\s?onerror="this.__e=event"|\s?onload="this.__e=event"/g, '')
			.trim();
		clean_children(node);
		return node.innerHTML;
	} catch (err) {
		throw new Error(`Failed to normalize HTML:\n${html}\nCause: ${err}`);
	}
}

/**
 * @param {string} html
 * @returns {string}
 */
export function normalize_new_line(html) {
	return html.replace(/\r\n/g, '\n');
}

/**
 * @param {{ removeDataSvelte?: boolean }} options
 */
export function setup_html_equal(options = {}) {
	/**
	 * @param {string} actual
	 * @param {string} expected
	 * @param {string} [message]
	 */
	const assert_html_equal = (actual, expected, message) => {
		try {
			assert.deepEqual(
				normalize_html(window, actual, options),
				normalize_html(window, expected, options),
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/scope.js (JavaScript)

Code before:
						this.values.add(NUMBER);
						break;

					case 'typeof':
						this.values.add(STRING);
						break;

					case 'void':
						this.values.add(undefined);
						break;

					default:
						this.values.add(UNKNOWN);
				}
				break;
			}

			case 'CallExpression':
				{
					const rune = get_rune(expression, scope);
					if (
						rune &&
						scope.get(
							object(/** @type {Identifier | MemberExpression} */ (expression.callee))?.name ?? ''
						) === null
					) {
						switch (rune) {
							case '$state':
							case '$state.raw':
								if (expression.arguments.length) {
									const evaluated = scope.evaluate(
										/** @type {Expression} */ (expression.arguments[0])
									);
									for (let value of evaluated.values) {
										this.values.add(value);
									}
								} else {
									this.values.add(undefined);
								}
								break;
							case '$props.id':
								this.values.add(STRING);
								break;
							case '$effect.tracking':
								this.values.add(false);
								this.values.add(true);
								break;
							case '$derived': {
								const evaluated = scope.evaluate(
									/** @type {Expression} */ (expression.arguments[0])
								);

Review comment (suggestion):
no need for the check here, it's taken care of inside `get_rune`

```suggestion
					if (rune) {
```

Code after:
						this.values.add(value);
					}

					for (const value of alternate.values) {
						this.values.add(value);
					}
				}
				break;
			}

			case 'LogicalExpression': {
				const a = scope.evaluate(expression.left);
				const b = scope.evaluate(expression.right);

				if (a.is_known) {
					if (b.is_known) {
						this.values.add(logical[expression.operator](a.value, b.value));
						break;
					}

					if (
						(expression.operator === '&&' && !a.value) ||
						(expression.operator === '||' && a.value) ||
						(expression.operator === '??' && a.value != null)
					) {
						this.values.add(a.value);
					} else {
						for (const value of b.values) {
							this.values.add(value);
						}
					}

					break;
				}

				for (const value of a.values) {
					this.values.add(value);
				}

				for (const value of b.values) {
					this.values.add(value);
				}
				break;
			}

			case 'UnaryExpression': {
				const argument = scope.evaluate(expression.argument);

				if (argument.is_known) {
					this.values.add(unary[expression.operator](argument.value));
					break;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/scope.js (JavaScript)

Code before:
					}
				} else if (
					expression.callee.type === 'Identifier' &&
					scope.get(expression.callee.name) === null
				) {
					switch (expression.callee.name) {
						case 'Number': {
							if (expression.arguments.length) {
								const arg = scope.evaluate(/** @type {Expression} */ (expression.arguments[0]));
								if (arg.is_known) {
									this.values.add(Number(arg.value));
								} else if (arg.is_number) {
									this.values.add(NUMBER);
								} else {
									for (let value of arg.values) {
										switch (value) {
											case STRING:
											case NUMBER:
											case UNKNOWN:
												this.values.add(NUMBER);
												break;
											default:
												this.values.add(Number(value));
										}
									}
								}
							} else {
								this.values.add(0);
							}
							break;
						}
						case 'String':
							if (expression.arguments.length) {
								const arg = scope.evaluate(/** @type {Expression} */ (expression.arguments[0]));
								if (arg.is_known) {
									this.values.add(String(arg.value));
								} else if (arg.is_number || arg.is_string) {
									this.values.add(STRING);
								} else {
									for (let value of arg.values) {
										switch (value) {
											case STRING:
											case NUMBER:
											case UNKNOWN:
												this.values.add(STRING);
												break;
											default:
												this.values.add(String(value));
										}
									}
								}

Review comment (refactor):
I think in the name of simplicity it's fine to just treat everything as `NUMBER` — there's no real utility in making a note of the fact that `Number(str as '1' | '2' | '3')` will be either `1`, `2` or `3`. Maybe that'll change in future but in the meantime it's worth keeping this stuff pared down

```suggestion
								} else {
									this.values.add(NUMBER);
								}
```

Code after:
						this.values.add(undefined);
						break;

					default:
						this.values.add(UNKNOWN);
				}
				break;
			}

			case 'CallExpression': {
				const keypath = get_global_keypath(expression.callee, scope);

				if (keypath) {
					if (is_rune(keypath)) {
						const arg = /** @type {Expression | undefined} */ (expression.arguments[0]);

						switch (keypath) {
							case '$state':
							case '$state.raw':
							case '$derived':
								if (arg) {
									scope.evaluate(arg, this.values);
								} else {
									this.values.add(undefined);
								}
								break;

							case '$props.id':
								this.values.add(STRING);
								break;

							case '$effect.tracking':
								this.values.add(false);
								this.values.add(true);
								break;

							case '$derived.by':
								if (arg?.type === 'ArrowFunctionExpression' && arg.body.type !== 'BlockStatement') {
									scope.evaluate(arg.body, this.values);
									break;
								}

								this.values.add(UNKNOWN);
								break;

							default: {
								this.values.add(UNKNOWN);
							}
						}

						break;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/scope.js (JavaScript)

Code before:
								}
							} else {
								this.values.add(0);
							}
							break;
						}
						case 'String':
							if (expression.arguments.length) {
								const arg = scope.evaluate(/** @type {Expression} */ (expression.arguments[0]));
								if (arg.is_known) {
									this.values.add(String(arg.value));
								} else if (arg.is_number || arg.is_string) {
									this.values.add(STRING);
								} else {
									for (let value of arg.values) {
										switch (value) {
											case STRING:
											case NUMBER:
											case UNKNOWN:
												this.values.add(STRING);
												break;
											default:
												this.values.add(String(value));
										}
									}
								}
							} else {
								this.values.add('');
							}
							break;
						default:
							this.values.add(UNKNOWN);
					}
				} else {
					this.values.add(UNKNOWN);
				}

				break;
			}

			default: {
				this.values.add(UNKNOWN);
			}
		}

		for (const value of this.values) {
			this.value = value; // saves having special logic for `size === 1`

			if (value !== STRING && typeof value !== 'string') {
				this.is_string = false;
			}

Review comment (suggestion):
ditto

```suggestion
								} else {
									this.values.add(STRING);
								}
```

Code after:
								break;

							case '$props.id':
								this.values.add(STRING);
								break;

							case '$effect.tracking':
								this.values.add(false);
								this.values.add(true);
								break;

							case '$derived.by':
								if (arg?.type === 'ArrowFunctionExpression' && arg.body.type !== 'BlockStatement') {
									scope.evaluate(arg.body, this.values);
									break;
								}

								this.values.add(UNKNOWN);
								break;

							default: {
								this.values.add(UNKNOWN);
							}
						}

						break;
					}

					if (
						Object.hasOwn(globals, keypath) &&
						expression.arguments.every((arg) => arg.type !== 'SpreadElement')
					) {
						const [type, fn] = globals[keypath];
						const values = expression.arguments.map((arg) => scope.evaluate(arg));

						if (fn && values.every((e) => e.is_known)) {
							this.values.add(fn(...values.map((e) => e.value)));
						} else {
							this.values.add(type);
						}

						break;
					}
				}

				this.values.add(UNKNOWN);
				break;
			}

			case 'TemplateLiteral': {
				let result = expression.quasis[0].value.cooked;