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

Code before:
		expression: Expression;
		fragment: Fragment;
	}

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

	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>;
		/** @internal */
		metadata: {
			expression: ExpressionMetadata;
			/** May be set if this is an event attribute */
			delegated: null | DelegatedEvent;
		};
	}

	export interface SpreadAttribute extends BaseNode {
		type: 'SpreadAttribute';
		expression: Expression;
		/** @internal */
		metadata: {
			expression: ExpressionMetadata;
		};
	}

	export interface Script extends BaseNode {
		type: 'Script';
		context: 'default' | 'module';
		content: Program;
		attributes: Attribute[];
	}
}

type Tag = AST.ExpressionTag | AST.HtmlTag | AST.ConstTag | AST.DebugTag | AST.RenderTag;

export type Directive =
	| AST.AnimateDirective
	| AST.BindDirective

Review comment (suggestion):
So that it doesn't show up in the generated public types

```suggestion
		/** @internal */
		metadata: {
```

Code after:
	export interface AwaitBlock extends BaseNode {
		type: 'AwaitBlock';
		expression: Expression;
		// TODO can/should we move these inside the ThenBlock and CatchBlock?
		/** The resolved value inside the `then` block */
		value: Pattern | null;
		/** The rejection reason inside the `catch` block */
		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;
		/** @internal */
		metadata: {
			can_hoist: boolean;
			/** The set of components/render tags that could render this snippet,
			 * used for CSS pruning */
			sites: Set<Component | SvelteComponent | SvelteSelf | RenderTag>;
		};
	}

	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>;
		/** @internal */
		metadata: {
			expression: ExpressionMetadata;
			/** May be set if this is an event attribute */
			delegated: null | DelegatedEvent;
		};
	}

	export interface SpreadAttribute extends BaseNode {
		type: 'SpreadAttribute';
		expression: Expression;
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/2-analyze/visitors/shared/utils.js (JavaScript)

Code before:

	const binding = scope.get(node.name);
	if (!binding) return true;

	if (binding.kind === 'store_sub') {
		return is_safe_identifier({ name: node.name.slice(1), type: 'Identifier' }, scope);
	}

	return (
		binding.declaration_kind !== 'import' &&
		binding.kind !== 'prop' &&
		binding.kind !== 'bindable_prop' &&
		binding.kind !== 'rest_prop'
	);
}

/**
 * @param {Expression | Super} node
 * @param {Context} context
 * @returns {boolean}
 */
export function is_pure(node, context) {
	if (node.type !== 'Identifier' && node.type !== 'MemberExpression') {
		return false;
	}
	if (node.type === 'MemberExpression' && node.object.type === 'Literal') {
		return true;
	}

	const left = object(node);
	if (!left) return false;

	if (left.type === 'Identifier') {
		const binding = context.state.scope.get(left.name);
		if (binding === null) return true; // globals are assumed to be safe
	}

	// TODO add more cases (safe Svelte imports, etc)
	return false;
}

/**
 * Checks if the name is valid, which it is when it's not starting with (or is) a dollar sign or if it's a function parameter.
 * The second argument is the depth of the scope, which is there for backwards compatibility reasons: In Svelte 4, you
 * were allowed to define `$`-prefixed variables anywhere below the top level of components. Once legacy mode is gone, this
 * argument can be removed / the call sites adjusted accordingly.
 * @param {Binding | null} binding
 * @param {number | undefined} [function_depth]
 */
export function validate_identifier_name(binding, function_depth) {
	if (!binding) return;

Review comment (suggestion):
```suggestion

	if (node.type === 'MemberExpression' && node.object.type === 'Literal') {
```

Code after:

	const binding = scope.get(node.name);
	if (!binding) return true;

	if (binding.kind === 'store_sub') {
		return is_safe_identifier({ name: node.name.slice(1), type: 'Identifier' }, scope);
	}

	return (
		binding.declaration_kind !== 'import' &&
		binding.kind !== 'prop' &&
		binding.kind !== 'bindable_prop' &&
		binding.kind !== 'rest_prop'
	);
}

/**
 * @param {Expression | Literal | Super} node
 * @param {Context} context
 * @returns {boolean}
 */
export function is_pure(node, context) {
	if (node.type === 'Literal') {
		return true;
	}
	if (node.type === 'CallExpression') {
		if (!is_pure(node.callee, context)) {
			return false;
		}
		for (let arg of node.arguments) {
			if (!is_pure(arg.type === 'SpreadElement' ? arg.argument : arg, context)) {
				return false;
			}
		}
		return true;
	}
	if (node.type !== 'Identifier' && node.type !== 'MemberExpression') {
		return false;
	}

	/** @type {Expression | Super | null} */
	let left = node;
	while (left.type === 'MemberExpression') {
		left = left.object;
	}

	if (!left) return false;

	if (left.type === 'Identifier') {
		const binding = context.state.scope.get(left.name);
		if (binding === null) return true; // globals are assumed to be safe
Repository: sveltejs/svelte
File: benchmarking/run.js (JavaScript)

Code before:
import * as $ from '../packages/svelte/src/internal/client/index.js';
import { reactivity_benchmarks } from './benchmarks/reactivity/index.js';
import { ssr_benchmarks } from './benchmarks/ssr/index.js';

let total_time = 0;
let total_gc_time = 0;

const suites = [
	// { benchmarks: reactivity_benchmarks, name: 'reactivity benchmarks' },
	{ benchmarks: ssr_benchmarks, name: 'server-side rendering benchmarks' }
];

// eslint-disable-next-line no-console
console.log('\x1b[1m', '-- Benchmarking Started --', '\x1b[0m');
$.push({}, true);
try {
	for (const { benchmarks, name } of suites) {
		let suite_time = 0;
		let suite_gc_time = 0;
		// eslint-disable-next-line no-console
		console.log(`\nRunning ${name}...\n`);

		for (const benchmark of benchmarks) {
			const results = await benchmark();
			// eslint-disable-next-line no-console
			console.log(results);
			total_time += Number(results.time);
			total_gc_time += Number(results.gc_time);
			suite_time += Number(results.time);
			suite_gc_time += Number(results.gc_time);
		}

		console.log(`\nFinished ${name}.\n`);


Review comment (suggestion):
```suggestion
	{ benchmarks: reactivity_benchmarks, name: 'reactivity benchmarks' },
```

Code after:
import * as $ from '../packages/svelte/src/internal/client/index.js';
import { reactivity_benchmarks } from './benchmarks/reactivity/index.js';
import { ssr_benchmarks } from './benchmarks/ssr/index.js';

let total_time = 0;
let total_gc_time = 0;

const suites = [
	{ benchmarks: reactivity_benchmarks, name: 'reactivity benchmarks' },
	{ benchmarks: ssr_benchmarks, name: 'server-side rendering benchmarks' }
];

// eslint-disable-next-line no-console
console.log('\x1b[1m', '-- Benchmarking Started --', '\x1b[0m');
$.push({}, true);
try {
	for (const { benchmarks, name } of suites) {
		let suite_time = 0;
		let suite_gc_time = 0;
		// eslint-disable-next-line no-console
		console.log(`\nRunning ${name}...\n`);

		for (const benchmark of benchmarks) {
			const results = await benchmark();
			// eslint-disable-next-line no-console
			console.log(results);
			total_time += Number(results.time);
			total_gc_time += Number(results.gc_time);
			suite_time += Number(results.time);
			suite_gc_time += Number(results.gc_time);
		}

		console.log(`\nFinished ${name}.\n`);
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js (JavaScript)

Code before:
export function get_states_and_calls(values) {
	let states = 0;
	let calls = 0;
	for (let i = 0; i < values.length; i++) {
		const node = values[i];

		if (node.type === 'ExpressionTag') {
			if (node.metadata.expression.has_call) {
				calls++;
			}
			if (node.metadata.expression.has_state) {
				states++;
			}
		}
	}

	return { states, calls };
}

/**
 * @param {Array<AST.Text | AST.ExpressionTag>} values
 * @param {(node: SvelteNode, state: any) => any} visit
 * @param {ComponentClientTransformState} state
 * @returns {{ value: Expression, has_state: boolean, has_call: boolean }}
 */
export function build_template_literal(values, visit, state) {
	/** @type {Expression[]} */
	const expressions = [];

	let quasi = b.quasi('');
	const quasis = [quasi];

	const { states, calls } = get_states_and_calls(values);

	let has_call = calls > 0;
	let has_state = states > 0;
	let contains_multiple_call_expression = calls > 1;

	for (let i = 0; i < values.length; i++) {
		const node = values[i];

		if (node.type === 'Text') {
			quasi.value.cooked += node.data;
		} else if (node.type === 'ExpressionTag' && node.expression.type === 'Literal') {
			if (node.expression.value != null) {
				quasi.value.cooked += node.expression.value + '';
			}
		} else {
			if (contains_multiple_call_expression) {
				const id = b.id(state.scope.generate('stringified_text'));
				state.init.push(

Review comment (suggestion):
let's keep this rename, `build_template_literal` is a misleading name since it doesn't always produce a template literal

```suggestion
export function build_template_chunk(values, visit, state) {
```

Code after:
 * @param {ComponentClientTransformState} state
 * @returns {{ value: Expression, has_state: boolean, has_call: boolean }}
 */
export function build_template_chunk(values, visit, state) {
	/** @type {Expression[]} */
	const expressions = [];

	let quasi = b.quasi('');
	const quasis = [quasi];

	let has_call = false;
	let has_state = false;
	let contains_multiple_call_expression = false;

	for (const node of values) {
		if (node.type === 'ExpressionTag') {
			const metadata = node.metadata.expression;

			contains_multiple_call_expression ||= has_call && metadata.has_call;
			has_call ||= metadata.has_call;
			has_state ||= metadata.has_state;
		}
	}

	for (let i = 0; i < values.length; i++) {
		const node = values[i];

		if (node.type === 'Text') {
			quasi.value.cooked += node.data;
		} else if (node.type === 'ExpressionTag' && node.expression.type === 'Literal') {
			if (node.expression.value != null) {
				quasi.value.cooked += node.expression.value + '';
			}
		} else {
			if (contains_multiple_call_expression) {
				const id = b.id(state.scope.generate('stringified_text'));
				state.init.push(
					b.const(
						id,
						create_derived(
							state,
							b.thunk(
								b.logical(
									'??',
									/** @type {Expression} */ (visit(node.expression, state)),
									b.literal('')
								)
							)
						)
					)
				);
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/utils.js (JavaScript)

Code before:
/** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Pattern, PrivateIdentifier, Statement } from 'estree' */
/** @import { AST, Binding, SvelteNode } from '#compiler' */
/** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */
/** @import { Analysis } from '../../types.js' */
/** @import { Scope } from '../../scope.js' */
import * as b from '../../../utils/builders.js';
import { extract_identifiers, is_simple_expression } from '../../../utils/ast.js';
import {
	PROPS_IS_LAZY_INITIAL,
	PROPS_IS_IMMUTABLE,
	PROPS_IS_RUNES,
	PROPS_IS_UPDATED,
	PROPS_IS_BINDABLE
} from '../../../../constants.js';
import { dev } from '../../../state.js';
import { get_value } from './visitors/shared/declarations.js';

/**
 * @param {Binding} binding
 * @param {Analysis} analysis
 * @returns {boolean}
 */
export function is_state_source(binding, analysis) {
	return (
		(binding.kind === 'state' || binding.kind === 'raw_state') &&
		(!analysis.immutable || binding.reassigned || analysis.accessors)
	);

Review comment (suggestion):
```suggestion
/** @import { Binding, SvelteNode } from '#compiler' */
```

Code after:
/** @import { ArrowFunctionExpression, Expression, FunctionDeclaration, FunctionExpression, Identifier, Pattern, PrivateIdentifier, Statement } from 'estree' */
/** @import { Binding, SvelteNode } from '#compiler' */
/** @import { ClientTransformState, ComponentClientTransformState, ComponentContext } from './types.js' */
/** @import { Analysis } from '../../types.js' */
/** @import { Scope } from '../../scope.js' */
import * as b from '../../../utils/builders.js';
import { extract_identifiers, is_simple_expression } from '../../../utils/ast.js';
import {
	PROPS_IS_LAZY_INITIAL,
	PROPS_IS_IMMUTABLE,
	PROPS_IS_RUNES,
	PROPS_IS_UPDATED,
	PROPS_IS_BINDABLE
} from '../../../../constants.js';
import { dev } from '../../../state.js';
import { get_value } from './visitors/shared/declarations.js';

/**
 * @param {Binding} binding
 * @param {Analysis} analysis
 * @returns {boolean}
 */
export function is_state_source(binding, analysis) {
	return (
		(binding.kind === 'state' || binding.kind === 'raw_state') &&
		(!analysis.immutable || binding.reassigned || analysis.accessors)
	);
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js (JavaScript)

Code before:
	is_boolean_attribute,
	is_dom_property,
	is_load_error_element,
	is_void
} from '../../../../../utils.js';
import { escape_html } from '../../../../../escaping.js';
import { dev, is_ignored, locator } from '../../../../state.js';
import { is_event_attribute, is_text_attribute } from '../../../../utils/ast.js';
import * as b from '../../../../utils/builders.js';
import { is_custom_element_node } from '../../../nodes.js';
import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
import { build_getter, create_derived } from '../utils.js';
import {
	get_attribute_name,
	build_attribute_value,
	build_class_directives,
	build_style_directives,
	build_set_attributes
} from './shared/element.js';
import { process_children } from './shared/fragment.js';
import {
	build_render_statement,
	build_template_chunk,
	build_update,
	build_update_assignment,
	get_states_and_calls
} from './shared/utils.js';
import { visit_event_attribute } from './shared/events.js';

/**
 * @param {AST.RegularElement} node
 * @param {ComponentContext} context
 */
export function RegularElement(node, context) {
	/** @type {SourceLocation} */
	let location = [-1, -1];

	if (dev) {
		const loc = locator(node.start);
		if (loc) {
			location[0] = loc.line;
			location[1] = loc.column;
			context.state.locations.push(location);
		}
	}

	if (node.name === 'noscript') {
		context.state.template.push('<noscript></noscript>');
		return;
	}


Review comment (suggestion):
goes with the other suggestion

```suggestion
	build_update_assignment
```

Code after:
	is_boolean_attribute,
	is_dom_property,
	is_load_error_element,
	is_void
} from '../../../../../utils.js';
import { escape_html } from '../../../../../escaping.js';
import { dev, is_ignored, locator } from '../../../../state.js';
import { is_event_attribute, is_text_attribute } from '../../../../utils/ast.js';
import * as b from '../../../../utils/builders.js';
import { is_custom_element_node } from '../../../nodes.js';
import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
import { build_getter, create_derived } from '../utils.js';
import {
	get_attribute_name,
	build_attribute_value,
	build_class_directives,
	build_style_directives,
	build_set_attributes
} from './shared/element.js';
import { process_children } from './shared/fragment.js';
import {
	build_render_statement,
	build_template_chunk,
	build_update,
	build_update_assignment
} from './shared/utils.js';
import { visit_event_attribute } from './shared/events.js';

/**
 * @param {AST.RegularElement} node
 * @param {ComponentContext} context
 */
export function RegularElement(node, context) {
	/** @type {SourceLocation} */
	let location = [-1, -1];

	if (dev) {
		const loc = locator(node.start);
		if (loc) {
			location[0] = loc.line;
			location[1] = loc.column;
			context.state.locations.push(location);
		}
	}

	if (node.name === 'noscript') {
		context.state.template.push('<noscript></noscript>');
		return;
	}

	const is_custom_element = is_custom_element_node(node);
Repository: sveltejs/svelte
File: packages/svelte/src/reactivity/media-query.js (JavaScript)

Code before:
import { get } from '../internal/client/runtime.js';
import { set, source } from '../internal/client/reactivity/sources.js';
import { effect_tracking } from '../internal/client/reactivity/effects.js';
import { createStartStopNotifier } from './start-stop-notifier.js';

/**
 * Creates a media query and provides a `current` property that reflects whether or not it matches.
 */
export class MediaQuery {
	#version = source(0);
	#query;
	#notify;

	get current() {
		if (effect_tracking()) {
			get(this.#version);
			this.#notify();
		}

		return this.#query.matches;
	}

	/**
	 * @param {string} query A media query string (don't forget the braces)
	 * @param {boolean} [matches] Fallback value for the server
	 */
	constructor(query, matches) {
		this.#query = window.matchMedia(query);
		this.#notify = createStartStopNotifier(() => {
			const listener = () => set(this.#version, this.#version.v + 1);
			this.#query.addEventListener('change', listener);
			return () => this.#query.removeEventListener('change', listener);
		});
	}
}


Review comment (suggestion):
we could do the setup here instead of in the constructor, feels slightly neater. we can also use `on` (from `../events/index.js`) and `increment` (from `./utils.js`)

```suggestion
	#notify = createStartStopNotifier(() => {
		return on(this.#query, 'change', () => increment(this.#version));
	});
```

Code after:
import { createSubscriber } from './create-subscriber.js';
import { on } from '../events/index.js';

/**
 * Creates a media query and provides a `current` property that reflects whether or not it matches.
 *
 * Use it carefully — during server-side rendering, there is no way to know what the correct value should be, potentially causing content to change upon hydration.
 * If you can use the media query in CSS to achieve the same effect, do that.
 *
 * ```svelte
 * <script>
 * 	import { MediaQuery } from 'svelte/reactivity';
 *
 * 	const large = new MediaQuery('min-width: 800px');
 * </script>
 *
 * <h1>{large.current ? 'large screen' : 'small screen'}</h1>
 * ```
 * @since 5.7.0
 */
export class MediaQuery {
	#query;
	#subscribe = createSubscriber((update) => {
		return on(this.#query, 'change', update);
	});

	get current() {
		this.#subscribe();

		return this.#query.matches;
	}

	/**
	 * @param {string} query A media query string
	 * @param {boolean} [matches] Fallback value for the server
	 */
	constructor(query, matches) {
Repository: sveltejs/svelte
File: packages/svelte/src/reactivity/media-query.js (JavaScript)

Code before:
import { get } from '../internal/client/runtime.js';
import { set, source } from '../internal/client/reactivity/sources.js';
import { effect_tracking } from '../internal/client/reactivity/effects.js';
import { createStartStopNotifier } from './start-stop-notifier.js';

/**
 * Creates a media query and provides a `current` property that reflects whether or not it matches.
 */
export class MediaQuery {
	#version = source(0);
	#query;
	#notify;

	get current() {
		if (effect_tracking()) {
			get(this.#version);
			this.#notify();
		}

		return this.#query.matches;
	}

	/**
	 * @param {string} query A media query string (don't forget the braces)
	 * @param {boolean} [matches] Fallback value for the server
	 */
	constructor(query, matches) {
		this.#query = window.matchMedia(query);
		this.#notify = createStartStopNotifier(() => {
			const listener = () => set(this.#version, this.#version.v + 1);
			this.#query.addEventListener('change', listener);
			return () => this.#query.removeEventListener('change', listener);
		});
	}
}


Review comment (suggestion):
Additional parens (as in `((min-width: 800px))`) are ignored, so we can add surrounding parens ourselves and make this unnecessary

```suggestion
	 * @param {string} query A media query string
```

Code after:
import { createSubscriber } from './create-subscriber.js';
import { on } from '../events/index.js';

/**
 * Creates a media query and provides a `current` property that reflects whether or not it matches.
 *
 * Use it carefully — during server-side rendering, there is no way to know what the correct value should be, potentially causing content to change upon hydration.
 * If you can use the media query in CSS to achieve the same effect, do that.
 *
 * ```svelte
 * <script>
 * 	import { MediaQuery } from 'svelte/reactivity';
 *
 * 	const large = new MediaQuery('min-width: 800px');
 * </script>
 *
 * <h1>{large.current ? 'large screen' : 'small screen'}</h1>
 * ```
 * @since 5.7.0
 */
export class MediaQuery {
	#query;
	#subscribe = createSubscriber((update) => {
		return on(this.#query, 'change', update);
	});

	get current() {
		this.#subscribe();

		return this.#query.matches;
	}

	/**
	 * @param {string} query A media query string
	 * @param {boolean} [matches] Fallback value for the server
	 */
	constructor(query, matches) {
		// For convenience (and because people likely forget them) we add the parentheses; double parentheses are not a problem
		this.#query = window.matchMedia(`(${query})`);
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/reactivity/media-query.js (JavaScript)

Code before:
import { effect_tracking } from '../internal/client/reactivity/effects.js';
import { createStartStopNotifier } from './start-stop-notifier.js';

/**
 * Creates a media query and provides a `current` property that reflects whether or not it matches.
 */
export class MediaQuery {
	#version = source(0);
	#query;
	#notify;

	get current() {
		if (effect_tracking()) {
			get(this.#version);
			this.#notify();
		}

		return this.#query.matches;
	}

	/**
	 * @param {string} query A media query string (don't forget the braces)
	 * @param {boolean} [matches] Fallback value for the server
	 */
	constructor(query, matches) {
		this.#query = window.matchMedia(query);
		this.#notify = createStartStopNotifier(() => {
			const listener = () => set(this.#version, this.#version.v + 1);
			this.#query.addEventListener('change', listener);
			return () => this.#query.removeEventListener('change', listener);
		});
	}
}


Review comment (suggestion):
```suggestion
		this.#query = window.matchMedia(`(${query})`);
```

Code after:

/**
 * Creates a media query and provides a `current` property that reflects whether or not it matches.
 *
 * Use it carefully — during server-side rendering, there is no way to know what the correct value should be, potentially causing content to change upon hydration.
 * If you can use the media query in CSS to achieve the same effect, do that.
 *
 * ```svelte
 * <script>
 * 	import { MediaQuery } from 'svelte/reactivity';
 *
 * 	const large = new MediaQuery('min-width: 800px');
 * </script>
 *
 * <h1>{large.current ? 'large screen' : 'small screen'}</h1>
 * ```
 * @since 5.7.0
 */
export class MediaQuery {
	#query;
	#subscribe = createSubscriber((update) => {
		return on(this.#query, 'change', update);
	});

	get current() {
		this.#subscribe();

		return this.#query.matches;
	}

	/**
	 * @param {string} query A media query string
	 * @param {boolean} [matches] Fallback value for the server
	 */
	constructor(query, matches) {
		// For convenience (and because people likely forget them) we add the parentheses; double parentheses are not a problem
		this.#query = window.matchMedia(`(${query})`);
	}
}