ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/html.js (JavaScript)

Code before:

/**
 * @param {Element | Text | Comment} node
 * @param {() => string} get_value
 * @param {boolean} svg
 * @param {boolean} mathml
 * @param {boolean} [skip_warning]
 * @returns {void}
 */
export function html(node, get_value, svg, mathml, skip_warning) {
	var anchor = node;

	var value = '';

	/** @type {Effect | undefined} */
	var effect;

	block(() => {
		if (value === (value = get_value())) {
			if (hydrating) {
				hydrate_next();
			}
			return;
		}

		if (effect) {
			destroy_effect(effect);
			effect = undefined;
		}

		if (value === '') return;

		effect = branch(() => {
			if (hydrating) {
				// We're deliberately not trying to repair mismatches between server and client,
				// as it's costly and error-prone (and it's an edge case to have a mismatch anyway)
				var hash = /** @type {Comment} */ (hydrate_node).data;
				var next = hydrate_next();
				var last = next;

				while (
					next !== null &&
					(next.nodeType !== 8 || /** @type {Comment} */ (next).data !== '')
				) {
					last = next;
					next = /** @type {TemplateNode} */ (get_next_sibling(next));
				}

				if (next === null) {
					w.hydration_mismatch();
					throw HYDRATION_ERROR;

Review comment (suggestion):
since we do this in other places...

```suggestion
		if (effect !== undefined) {
```

Code after:

/**
 * @param {Element | Text | Comment} node
 * @param {() => string} get_value
 * @param {boolean} svg
 * @param {boolean} mathml
 * @param {boolean} [skip_warning]
 * @returns {void}
 */
export function html(node, get_value, svg, mathml, skip_warning) {
	var anchor = node;

	var value = '';

	/** @type {Effect | undefined} */
	var effect;

	block(() => {
		if (value === (value = get_value())) {
			if (hydrating) {
				hydrate_next();
			}
			return;
		}

		if (effect !== undefined) {
			destroy_effect(effect);
			effect = undefined;
		}

		if (value === '') return;

		effect = branch(() => {
			if (hydrating) {
				// We're deliberately not trying to repair mismatches between server and client,
				// as it's costly and error-prone (and it's an edge case to have a mismatch anyway)
				var hash = /** @type {Comment} */ (hydrate_node).data;
				var next = hydrate_next();
				var last = next;

				while (
					next !== null &&
					(next.nodeType !== 8 || /** @type {Comment} */ (next).data !== '')
				) {
					last = next;
					next = /** @type {TemplateNode} */ (get_next_sibling(next));
				}

				if (next === null) {
					w.hydration_mismatch();
					throw HYDRATION_ERROR;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/function.js (JavaScript)

Code before:
	let state = { ...context.state, in_constructor: false };
	let in_constructor = false;

	if (node.type === 'FunctionExpression') {
		const parent = /** @type {Node} */ (context.path.at(-1));
		in_constructor = state.in_constructor =
			parent.type === 'MethodDefinition' && parent.kind === 'constructor';
	}

	if (metadata?.hoisted === true) {
		const params = build_hoisted_params(node, context);

		return /** @type {FunctionExpression} */ ({
			...node,
			params,
			body: context.visit(node.body, state)
		});
	}

	if (in_constructor && (state.private_state.size > 0 || state.public_state.size > 0)) {
		const params = /** @type {Pattern[]} */ (node.params.map((p) => context.visit(p, state)));
		const body = /** @type {BlockStatement} */ (context.visit(node.body, state));
		const has_super_call =
			body.body[0]?.type === 'ExpressionStatement' &&
			body.body[0].expression.type === 'CallExpression' &&
			body.body[0].expression.callee.type === 'Super';
		const enter = b.var('$$', b.call('$.enter_constructor'));
		const leave = b.stmt(b.call('$.leave_constructor', b.id('$$')));

		body.body.splice(has_super_call ? 1 : 0, 0, enter);
		body.body.push(leave);

		return {
			...node,
			params,
			body
		};
	} else {
		context.next(state);
	}
};


Review comment (suggestion):
the `super()` call doesn't need to be the first thing in the constructor. it doesn't even need to be at the top level

Code after:
/** @import { ArrowFunctionExpression, FunctionExpression, Node } from 'estree' */
/** @import { ComponentContext } from '../../types' */
import { build_hoisted_params } from '../../utils.js';

/**
 * @param {ArrowFunctionExpression | FunctionExpression} node
 * @param {ComponentContext} context
 */
export const visit_function = (node, context) => {
	const metadata = node.metadata;

	let state = { ...context.state, in_constructor: false };

	if (node.type === 'FunctionExpression') {
		const parent = /** @type {Node} */ (context.path.at(-1));
		state.in_constructor = parent.type === 'MethodDefinition' && parent.kind === 'constructor';
	}

	if (metadata?.hoisted === true) {
		const params = build_hoisted_params(node, context);

		return /** @type {FunctionExpression} */ ({
			...node,
			params,
			body: context.visit(node.body, state)
		});
	}

	context.next(state);
};
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/types/template.d.ts (TypeScript)

Code before:
	then: Fragment | null;
	catch: Fragment | null;
}

export interface KeyBlock extends BaseNode {
	type: 'KeyBlock';
	expression: Expression;
	fragment: Fragment;
}

export interface SnippetBlock extends BaseNode {
	type: 'SnippetBlock';
	expression: Identifier;
	parameters: Pattern[];
	body: Fragment;
}

export type Block = EachBlock | IfBlock | AwaitBlock | KeyBlock | SnippetBlock;

export interface Attribute extends BaseNode {
	type: 'Attribute';
	name: string;
	/**
	 * The attribute may be omitted when false.
	 * String values are represented by an array since it may be a combination of text and expression
	 * values such as `style="color: {color} !import"`.
	 */
	value: true | ExpressionTag | Array<Text | ExpressionTag>;
	metadata: {
		expression: ExpressionMetadata;
		/** May be set if this is an event attribute */
		delegated: null | DelegatedEvent;
	};
}

export interface SpreadAttribute extends BaseNode {
	type: 'SpreadAttribute';
	expression: Expression;
	metadata: {
		expression: ExpressionMetadata;
	};
}

export type TemplateNode =
	| Root
	| Text
	| Tag
	| ElementLike
	| Attribute
	| SpreadAttribute
	| Directive

Review comment (suggestion):
not sure why we would need to specify 'attribute may be omitted when false' — for one thing non-boolean attributes are never `false`, they either exist or they don't

```suggestion
	 * Quoted/string values are represented by an array, even if they contain a single expression like `"{x}"`
```

Code after:
	then: Fragment | null;
	catch: Fragment | null;
}

export interface KeyBlock extends BaseNode {
	type: 'KeyBlock';
	expression: Expression;
	fragment: Fragment;
}

export interface SnippetBlock extends BaseNode {
	type: 'SnippetBlock';
	expression: Identifier;
	parameters: Pattern[];
	body: Fragment;
}

export type Block = EachBlock | IfBlock | AwaitBlock | KeyBlock | SnippetBlock;

export interface Attribute extends BaseNode {
	type: 'Attribute';
	name: string;
	/**
	 * Quoted/string values are represented by an array, even if they contain a single expression like `"{x}"`
	 */
	value: true | ExpressionTag | Array<Text | ExpressionTag>;
	metadata: {
		expression: ExpressionMetadata;
		/** May be set if this is an event attribute */
		delegated: null | DelegatedEvent;
	};
}

export interface SpreadAttribute extends BaseNode {
	type: 'SpreadAttribute';
	expression: Expression;
	metadata: {
		expression: ExpressionMetadata;
	};
}

export type TemplateNode =
	| Root
	| Text
	| Tag
	| ElementLike
	| Attribute
	| SpreadAttribute
	| Directive
	| Comment
	| Block;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/types/template.d.ts (TypeScript)

Code before:
	error: Pattern | null;
	pending: Fragment | null;
	then: Fragment | null;
	catch: Fragment | null;
}

export interface KeyBlock extends BaseNode {
	type: 'KeyBlock';
	expression: Expression;
	fragment: Fragment;
}

export interface SnippetBlock extends BaseNode {
	type: 'SnippetBlock';
	expression: Identifier;
	parameters: Pattern[];
	body: Fragment;
}

export type Block = EachBlock | IfBlock | AwaitBlock | KeyBlock | SnippetBlock;

export interface Attribute extends BaseNode {
	type: 'Attribute';
	name: string;
	/**
	 * A boolean attribute is either present (true) or not.
	 * Quoted/string values are represented as an array of alternating text and expressions whether
	 * complex (`"foo{x}bar"`) or holding just a single expression like `"{x}"`.
	 */
	value: true | ExpressionTag | Array<Text | ExpressionTag>;
	metadata: {
		expression: ExpressionMetadata;
		/** May be set if this is an event attribute */
		delegated: null | DelegatedEvent;
	};
}

export interface SpreadAttribute extends BaseNode {
	type: 'SpreadAttribute';
	expression: Expression;
	metadata: {
		expression: ExpressionMetadata;
	};
}

export type TemplateNode =
	| Root
	| Text
	| Tag
	| ElementLike
	| Attribute

Review comment (suggestion):
I mean this still isn't accurate. You can have `<button disabled={condition} />` where `condition` can be truthy or falsy, and `value` would be an `ExpressionTag`. Can we just get rid of it? It's superfluous and confusing

```suggestion
```

Code after:
	error: Pattern | null;
	pending: Fragment | null;
	then: Fragment | null;
	catch: Fragment | null;
}

export interface KeyBlock extends BaseNode {
	type: 'KeyBlock';
	expression: Expression;
	fragment: Fragment;
}

export interface SnippetBlock extends BaseNode {
	type: 'SnippetBlock';
	expression: Identifier;
	parameters: Pattern[];
	body: Fragment;
}

export type Block = EachBlock | IfBlock | AwaitBlock | KeyBlock | SnippetBlock;

export interface Attribute extends BaseNode {
	type: 'Attribute';
	name: string;
	/**
	 * Quoted/string values are represented by an array, even if they contain a single expression like `"{x}"`
	 */
	value: true | ExpressionTag | Array<Text | ExpressionTag>;
	metadata: {
		expression: ExpressionMetadata;
		/** May be set if this is an event attribute */
		delegated: null | DelegatedEvent;
	};
}

export interface SpreadAttribute extends BaseNode {
	type: 'SpreadAttribute';
	expression: Expression;
	metadata: {
		expression: ExpressionMetadata;
	};
}

export type TemplateNode =
	| Root
	| Text
	| Tag
	| ElementLike
	| Attribute
	| SpreadAttribute
	| Directive
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/types/template.d.ts (TypeScript)

Code before:
	then: Fragment | null;
	catch: Fragment | null;
}

export interface KeyBlock extends BaseNode {
	type: 'KeyBlock';
	expression: Expression;
	fragment: Fragment;
}

export interface SnippetBlock extends BaseNode {
	type: 'SnippetBlock';
	expression: Identifier;
	parameters: Pattern[];
	body: Fragment;
}

export type Block = EachBlock | IfBlock | AwaitBlock | KeyBlock | SnippetBlock;

export interface Attribute extends BaseNode {
	type: 'Attribute';
	name: string;
	/**
	 * A boolean attribute is either present (true) or not.
	 * Quoted/string values are represented as an array of alternating text and expressions whether
	 * complex (`"foo{x}bar"`) or holding just a single expression like `"{x}"`.
	 */
	value: true | ExpressionTag | Array<Text | ExpressionTag>;
	metadata: {
		expression: ExpressionMetadata;
		/** May be set if this is an event attribute */
		delegated: null | DelegatedEvent;
	};
}

export interface SpreadAttribute extends BaseNode {
	type: 'SpreadAttribute';
	expression: Expression;
	metadata: {
		expression: ExpressionMetadata;
	};
}

export type TemplateNode =
	| Root
	| Text
	| Tag
	| ElementLike
	| Attribute
	| SpreadAttribute
	| Directive

Review comment (suggestion):
This isn't accurate — they don't need to be alternating (`"{a}{b}"` is just two consecutive `ExpressionTag` nodes). We already know what they're an array _of_, it's right there in the type definition

```suggestion
	 * Quoted/string values are represented by an array, even if they contain a single expression like `"{x}"`
```

Code after:
	then: Fragment | null;
	catch: Fragment | null;
}

export interface KeyBlock extends BaseNode {
	type: 'KeyBlock';
	expression: Expression;
	fragment: Fragment;
}

export interface SnippetBlock extends BaseNode {
	type: 'SnippetBlock';
	expression: Identifier;
	parameters: Pattern[];
	body: Fragment;
}

export type Block = EachBlock | IfBlock | AwaitBlock | KeyBlock | SnippetBlock;

export interface Attribute extends BaseNode {
	type: 'Attribute';
	name: string;
	/**
	 * Quoted/string values are represented by an array, even if they contain a single expression like `"{x}"`
	 */
	value: true | ExpressionTag | Array<Text | ExpressionTag>;
	metadata: {
		expression: ExpressionMetadata;
		/** May be set if this is an event attribute */
		delegated: null | DelegatedEvent;
	};
}

export interface SpreadAttribute extends BaseNode {
	type: 'SpreadAttribute';
	expression: Expression;
	metadata: {
		expression: ExpressionMetadata;
	};
}

export type TemplateNode =
	| Root
	| Text
	| Tag
	| ElementLike
	| Attribute
	| SpreadAttribute
	| Directive
	| Comment
	| Block;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js (JavaScript)

Code before:
	/** @type {Statement} */
	let update;

	if (name === 'class') {
		update = b.stmt(
			b.call(
				is_svg ? '$.set_svg_class' : is_mathml ? '$.set_mathml_class' : '$.set_class',
				node_id,
				value
			)
		);
	} else if (name === 'value') {
		update = b.stmt(b.call('$.set_value', node_id, value));
	} else if (name === 'checked') {
		update = b.stmt(b.call('$.set_checked', node_id, value));
	} else if (is_dom_property(name)) {
		update = b.stmt(b.assignment('=', b.member(node_id, name), value));
	} else {
		const callee = name.startsWith('xlink') ? '$.set_xlink_attribute' : '$.set_attribute';
		update = b.stmt(
			b.call(
				callee,
				node_id,
				b.literal(name),
				value,
				is_ignored(element, 'hydration_attribute_changed') && b.true
			)
		);
	}

	const inlinable_expression =
		attribute.value === true
			? false // not an expression
			: is_inlinable_expression(
					Array.isArray(attribute.value) ? attribute.value : [attribute.value],
					context.state
				);
	if (attribute.metadata.expression.has_state) {
		if (has_call) {
			state.init.push(build_update(update));
		} else {
			state.update.push(update);
		}
		return true;
	} else {
		if (inlinable_expression) {
			push_template_quasi(context.state, ` ${name}="`);
			push_template_expression(context.state, value);
			push_template_quasi(context.state, '"');
		} else {
			state.init.push(update);

Review comment (suggestion):
shouldn't the comparison with true be part of the is_inlinable_expression function? also confused how it can be both a boolean `true` or an AST.ExpressionTag 

Code after:
				is_svg ? '$.set_svg_class' : is_mathml ? '$.set_mathml_class' : '$.set_class',
				node_id,
				value
			)
		);
	} else if (name === 'value') {
		update = b.stmt(b.call('$.set_value', node_id, value));
	} else if (name === 'checked') {
		update = b.stmt(b.call('$.set_checked', node_id, value));
	} else if (is_dom_property(name)) {
		update = b.stmt(b.assignment('=', b.member(node_id, name), value));
	} else {
		const callee = name.startsWith('xlink') ? '$.set_xlink_attribute' : '$.set_attribute';
		update = b.stmt(
			b.call(
				callee,
				node_id,
				b.literal(name),
				value,
				is_ignored(element, 'hydration_attribute_changed') && b.true
			)
		);
	}

	const inlinable_expression =
		attribute.value === true
			? false // not an expression
			: is_inlinable_expression(
					Array.isArray(attribute.value) ? attribute.value : [attribute.value],
					context.state
				);
	if (attribute.metadata.expression.has_state) {
		if (has_call) {
			state.init.push(build_update(update));
		} else {
			state.update.push(update);
		}
		return true;
	} else {
		if (inlinable_expression) {
			context.state.template.push_quasi(` ${name}="`);
			context.state.template.push_expression(value);
			context.state.template.push_quasi('"');
		} else {
			state.init.push(update);
		}
		return false;
	}
}

/**
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/snippet.js (JavaScript)

Code before:

	/** @type {SnippetFn | null | undefined} */
	var snippet = /** @type {SnippetFn} */ (empty_snippet);

	/** @type {Effect | null} */
	var snippet_effect;

	block(() => {
		if (snippet === (snippet = get_snippet())) return;

		if (snippet_effect) {
			destroy_effect(snippet_effect);
			snippet_effect = null;
		}

		snippet_effect = branch(() => /** @type {SnippetFn} */ (snippet)(anchor, ...args));
	}, EFFECT_TRANSPARENT);

	if (hydrating) {
		anchor = hydrate_node;
	}
}

// Just an empty function
/** @type {(...args: any[]) => void} */
export const empty_snippet = () => {};

/**
 * In development, wrap the snippet function so that it passes validation, and so that the
 * correct component context is set for ownership checks
 * @param {any} component
 * @param {(node: TemplateNode, ...args: any[]) => void} fn
 */
export function wrap_snippet(component, fn) {
	return (/** @type {TemplateNode} */ node, /** @type {any[]} */ ...args) => {
		var previous_component_function = dev_current_component_function;
		set_dev_current_component_function(component);

		try {
			return fn(node, ...args);
		} finally {
			set_dev_current_component_function(previous_component_function);
		}
	};
}

/**
 * Create a snippet programmatically
 * @template {unknown[]} Params
 * @param {(...params: Getters<Params>) => {
 *   render: () => string

Review comment (suggestion):
You can use `noop` from `internal/shared/utils.js` instead

Code after:
export function snippet(node, get_snippet, ...args) {
	var anchor = node;

	/** @type {SnippetFn | null | undefined} */
	// @ts-ignore
	var snippet = noop;

	/** @type {Effect | null} */
	var snippet_effect;

	block(() => {
		if (snippet === (snippet = get_snippet())) return;

		if (snippet_effect) {
			destroy_effect(snippet_effect);
			snippet_effect = null;
		}

		if (DEV && snippet == null) {
			e.invalid_snippet();
		}

		snippet_effect = branch(() => /** @type {SnippetFn} */ (snippet)(anchor, ...args));
	}, EFFECT_TRANSPARENT);

	if (hydrating) {
		anchor = hydrate_node;
	}
}

/**
 * In development, wrap the snippet function so that it passes validation, and so that the
 * correct component context is set for ownership checks
 * @param {any} component
 * @param {(node: TemplateNode, ...args: any[]) => void} fn
 */
export function wrap_snippet(component, fn) {
	return (/** @type {TemplateNode} */ node, /** @type {any[]} */ ...args) => {
		var previous_component_function = dev_current_component_function;
		set_dev_current_component_function(component);

		try {
			return fn(node, ...args);
		} finally {
			set_dev_current_component_function(previous_component_function);
		}
	};
}

/**
 * Create a snippet programmatically
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/snippet.js (JavaScript)

Code before:
 */
export function snippet(node, get_snippet, ...args) {
	var anchor = node;

	/** @type {SnippetFn | null | undefined} */
	var snippet = /** @type {SnippetFn} */ (empty_snippet);

	/** @type {Effect | null} */
	var snippet_effect;

	block(() => {
		if (snippet === (snippet = get_snippet())) return;

		if (snippet_effect) {
			destroy_effect(snippet_effect);
			snippet_effect = null;
		}

		snippet_effect = branch(() => /** @type {SnippetFn} */ (snippet)(anchor, ...args));
	}, EFFECT_TRANSPARENT);

	if (hydrating) {
		anchor = hydrate_node;
	}
}

// Just an empty function
/** @type {(...args: any[]) => void} */
export const empty_snippet = () => {};

/**
 * In development, wrap the snippet function so that it passes validation, and so that the
 * correct component context is set for ownership checks
 * @param {any} component
 * @param {(node: TemplateNode, ...args: any[]) => void} fn
 */
export function wrap_snippet(component, fn) {
	return (/** @type {TemplateNode} */ node, /** @type {any[]} */ ...args) => {
		var previous_component_function = dev_current_component_function;
		set_dev_current_component_function(component);

		try {
			return fn(node, ...args);
		} finally {
			set_dev_current_component_function(previous_component_function);
		}
	};
}

/**
 * Create a snippet programmatically

Review comment (suggestion):
Will we throw a more descriptive error at dev time here? Like, before the `snippet_effect = ...` line a `if (DEV && !snippet) e.blabla()`

Code after:
 * @param {(() => any)[]} args
 * @returns {void}
 */
export function snippet(node, get_snippet, ...args) {
	var anchor = node;

	/** @type {SnippetFn | null | undefined} */
	// @ts-ignore
	var snippet = noop;

	/** @type {Effect | null} */
	var snippet_effect;

	block(() => {
		if (snippet === (snippet = get_snippet())) return;

		if (snippet_effect) {
			destroy_effect(snippet_effect);
			snippet_effect = null;
		}

		if (DEV && snippet == null) {
			e.invalid_snippet();
		}

		snippet_effect = branch(() => /** @type {SnippetFn} */ (snippet)(anchor, ...args));
	}, EFFECT_TRANSPARENT);

	if (hydrating) {
		anchor = hydrate_node;
	}
}

/**
 * In development, wrap the snippet function so that it passes validation, and so that the
 * correct component context is set for ownership checks
 * @param {any} component
 * @param {(node: TemplateNode, ...args: any[]) => void} fn
 */
export function wrap_snippet(component, fn) {
	return (/** @type {TemplateNode} */ node, /** @type {any[]} */ ...args) => {
		var previous_component_function = dev_current_component_function;
		set_dev_current_component_function(component);

		try {
			return fn(node, ...args);
		} finally {
			set_dev_current_component_function(previous_component_function);
		}
	};
}