ronantakizawa/codereview-bench huggingface dataset:
Repository: rollup/rollup
File: src/Module.ts (TypeScript)

Code before:

	basename(): string {
		const base = basename(this.id);
		const extension = extname(this.id);

		return makeLegal(extension ? base.slice(0, -extension.length) : base);
	}

	bindReferences(): void {
		this.ast!.bind();
	}

	cacheInfoGetters(): void {
		cacheObjectGetters(this.info, [
			'dynamicallyImportedIdResolutions',
			'dynamicallyImportedIds',
			'dynamicImporters',
			'exportedBindings',
			'exports',
			'hasDefaultExport',
			'implicitlyLoadedAfterOneOf',
			'implicitlyLoadedBefore',
			'importedIdResolutions',
			'importedIds',
			'importers',
			'isIncluded'
		]);
	}

	error(properties: RollupError, pos: number | undefined): never {
		pos !== undefined && this.addLocationToLogProps(properties, pos);
		return error(properties);
	}

	// sum up the length of all ast nodes that are included
	estimateSize(): number {
		let size = 0;
		for (const node of this.ast!.body) {
			if (node.included) {
				size += node.end - node.start;
			}
		}
		return size;
	}

	getAllExportNames(): Set<string> {
		if (this.allExportNames) {
			return this.allExportNames;
		}
		this.allExportNames = new Set([...this.exports.keys(), ...this.reexportDescriptions.keys()]);
		for (const module of this.exportAllModules) {

Review comment (performance):
While it is fine to cache most of these properties after `generateModuleGraph`, `isIncluded` must not be cached at this point. The value is only known after the end of the build phase when tree-shaking is done. A simple fix could be to just exclude it from this list, though, as the getter is not very expensive.

Code after:
	basename(): string {
		const base = basename(this.id);
		const extension = extname(this.id);

		return makeLegal(extension ? base.slice(0, -extension.length) : base);
	}

	bindReferences(): void {
		this.ast!.bind();
	}

	cacheInfoGetters(): void {
		cacheObjectGetters(this.info, [
			'dynamicallyImportedIdResolutions',
			'dynamicallyImportedIds',
			'dynamicImporters',
			'exportedBindings',
			'exports',
			'hasDefaultExport',
			'implicitlyLoadedAfterOneOf',
			'implicitlyLoadedBefore',
			'importedIdResolutions',
			'importedIds',
			'importers'
		]);
	}

	error(properties: RollupError, pos: number | undefined): never {
		pos !== undefined && this.addLocationToLogProps(properties, pos);
		return error(properties);
	}

	// sum up the length of all ast nodes that are included
	estimateSize(): number {
		let size = 0;
		for (const node of this.ast!.body) {
			if (node.included) {
				size += node.end - node.start;
			}
		}
		return size;
	}

	getAllExportNames(): Set<string> {
		if (this.allExportNames) {
			return this.allExportNames;
		}
		this.allExportNames = new Set([...this.exports.keys(), ...this.reexportDescriptions.keys()]);
		for (const module of this.exportAllModules) {
			if (module instanceof ExternalModule) {
				this.allExportNames.add(`*${module.id}`);
Repository: rollup/rollup
File: src/ast/nodes/FunctionExpression.ts (TypeScript)

Code before:
import type { GenericEsTreeNode } from './shared/Node';

export default class FunctionExpression extends FunctionNode {
	declare type: NodeType.tFunctionExpression;
	declare idScope: ChildScope;

	createScope(parentScope: ChildScope) {
		super.createScope((this.idScope = new ChildScope(parentScope, parentScope.context)));
	}

	parseNode(esTreeNode: GenericEsTreeNode): this {
		if (esTreeNode.id !== null) {
			this.id = new Identifier(this, this.idScope).parseNode(
				esTreeNode.id
			) as IdentifierWithVariable;
		}
		return super.parseNode(esTreeNode);
	}

	protected onlyFunctionCallUsed(): boolean {
		const isIIFE =
			this.parent.type === NodeType.CallExpression &&
			(this.parent as CallExpression).callee === this &&
			!this.id;
		return isIIFE || super.onlyFunctionCallUsed();
	}

	render(
		code: MagicString,
		options: RenderOptions,
		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
	): void {
		super.render(code, options);
		if (renderedSurroundingElement === NodeType.ExpressionStatement) {
			code.appendRight(this.start, '(');
			code.prependLeft(this.end, ')');
		}
	}
}


Review comment (suggestion):
so it looks like the functional change here is making `isIIFE` compute to false for named functions?

(slightly better would be to check that the function id is referenced anywhere? I don't know how easy/quick that is.)

EDIT: I see the PR description basically covered what I said in this comment. Nice.

Code after:
import type { GenericEsTreeNode } from './shared/Node';

export default class FunctionExpression extends FunctionNode {
	declare type: NodeType.tFunctionExpression;
	declare idScope: ChildScope;

	createScope(parentScope: ChildScope) {
		super.createScope((this.idScope = new ChildScope(parentScope, parentScope.context)));
	}

	parseNode(esTreeNode: GenericEsTreeNode): this {
		if (esTreeNode.id !== null) {
			this.id = new Identifier(this, this.idScope).parseNode(
				esTreeNode.id
			) as IdentifierWithVariable;
		}
		return super.parseNode(esTreeNode);
	}

	protected onlyFunctionCallUsed(): boolean {
		const isIIFE =
			this.parent.type === NodeType.CallExpression &&
			(this.parent as CallExpression).callee === this &&
			(this.id === null || this.id.variable.getOnlyFunctionCallUsed());
		return isIIFE || super.onlyFunctionCallUsed();
	}

	render(
		code: MagicString,
		options: RenderOptions,
		{ renderedSurroundingElement }: NodeRenderOptions = BLANK
	): void {
		super.render(code, options);
		if (renderedSurroundingElement === NodeType.ExpressionStatement) {
			code.appendRight(this.start, '(');
			code.prependLeft(this.end, ')');
		}
	}
}
Repository: rollup/rollup
File: src/utils/sanitizeFileName.ts (TypeScript)

Code before:
// https://datatracker.ietf.org/doc/html/rfc2396
// eslint-disable-next-line no-control-regex
const INVALID_CHAR_REGEX = /[\u0000-\u001F"#$&*+,:;<=>?[\]^`{|}\u007F]/g;
const DRIVE_LETTER_REGEX = /^[a-z]:/i;
const PERCENT_REGEX = /%/g;

const decodeURIComponentSafe =
	typeof decodeURIComponent === 'undefined' ? undefined : decodeURIComponent;

export function sanitizeFileName(name: string): string {
	// A `:` is only allowed as part of a windows drive letter (ex: C:\foo)
	// Otherwise, avoid them because they can refer to NTFS alternate data streams.
	const match = DRIVE_LETTER_REGEX.exec(name);
	const driveLetter = match ? match[0] : '';
	let path = name.slice(driveLetter.length).replace(INVALID_CHAR_REGEX, '_');

	// Replace % with _ if there is invalid escape sequence
	if (path.includes('%')) {
		if (decodeURIComponentSafe) {
			try {
				decodeURIComponentSafe(path);
			} catch {
				path = path.replace(PERCENT_REGEX, '_');
			}
		} else {
			path = path.replace(PERCENT_REGEX, '_');
		}
	}

	return driveLetter + path;
}


Review comment (suggestion):
Slight thought, `decodeURIComponent` has been widely supported. I think we can always assume it exists. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/decodeURIComponent#browser_compatibility

Code after:
// https://datatracker.ietf.org/doc/html/rfc2396
// eslint-disable-next-line no-control-regex
const INVALID_CHAR_REGEX = /[\u0000-\u001F"#$%&*+,:;<=>?[\]^`{|}\u007F]/g;
const DRIVE_LETTER_REGEX = /^[a-z]:/i;

export function sanitizeFileName(name: string): string {
	const match = DRIVE_LETTER_REGEX.exec(name);
	const driveLetter = match ? match[0] : '';

	// A `:` is only allowed as part of a windows drive letter (ex: C:\foo)
	// Otherwise, avoid them because they can refer to NTFS alternate data streams.
	return driveLetter + name.slice(driveLetter.length).replace(INVALID_CHAR_REGEX, '_');
}
Repository: rollup/rollup
File: src/ast/nodes/UnaryExpression.ts (TypeScript)

Code before:

	include(
		context: InclusionContext,
		includeChildrenRecursively: IncludeChildren,
		_options?: InclusionOptions
	): void {
		if (!this.deoptimized) this.applyDeoptimizations();
		this.included = true;
		const literalValue = includeChildrenRecursively
			? UnknownValue
			: this.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this);
		if (typeof literalValue === 'symbol' || this.argument.shouldBeIncluded(context)) {
			this.argument.include(context, includeChildrenRecursively);
			this.literalValue = UnknownValue;
		} else {
			this.literalValue = literalValue;
		}
	}

	render(code: MagicString, options: RenderOptions) {
		if (typeof this.literalValue === 'symbol') {
			super.render(code, options);
		} else {
			code.overwrite(this.start, this.end, getRenderString(this.literalValue));
		}
	}
}

function getRenderString(value: unknown) {
	if (value === undefined) {
		return 'void 0';
	}
	if (typeof value === 'string') {
		return JSON.stringify(value);
	}
	if (typeof value === 'number') {
		return simplifyNumber(value);
	}
	return String(value);
}

function simplifyNumber(value: number) {
	if (Object.is(-0, value)) {
		return '-0';
	}
	if ((Math.abs(value) > 1000 && value % 1000 === 0) || (value !== 0 && Math.abs(value) < 0.01)) {
		const exp = value.toExponential();
		const [base, exponent] = exp.split('e');
		const floatLength = base.split('.')[1]?.length || 0;
		return `${base.replace('.', '')}e${parseInt(exponent) - floatLength}`;
	}

Review comment (performance):
To slightly improve performance, we could run this logic only if `this.literalValue` does not have a value yet. Maybe extract it to a helper method, e.g. `this.getLiteralValue()`?

Code after:
		}
	}

	getRenderedLiteralValue(includeChildrenRecursively: IncludeChildren) {
		if (this.renderedLiteralValue !== UNASSIGNED) return this.renderedLiteralValue;
		return (this.renderedLiteralValue = includeChildrenRecursively
			? UnknownValue
			: getSimplifiedLiterals(
					this.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this)
				));
	}

	include(
		context: InclusionContext,
		includeChildrenRecursively: IncludeChildren,
		_options?: InclusionOptions
	): void {
		if (!this.deoptimized) this.applyDeoptimizations();
		this.included = true;
		if (
			typeof this.getRenderedLiteralValue(includeChildrenRecursively) === 'symbol' ||
			this.argument.shouldBeIncluded(context)
		) {
			this.argument.include(context, includeChildrenRecursively);
			this.renderedLiteralValue = UnknownValue;
		}
	}

	render(code: MagicString, options: RenderOptions) {
		if (typeof this.renderedLiteralValue === 'symbol') {
			super.render(code, options);
		} else {
			code.overwrite(this.start, this.end, this.renderedLiteralValue);
		}
	}
}

function getSimplifiedLiterals(value: unknown) {
	if (value === undefined || typeof value === 'boolean') {
		return String(value);
	}
	if (typeof value === 'string') {
		return JSON.stringify(value);
	}
	if (typeof value === 'number') {
		return getSimplifiedNumber(value);
	}
	return UnknownValue;
}

function getSimplifiedNumber(value: number) {
Repository: rollup/rollup
File: test/form/samples/treeshake-unary/_expected.js (JavaScript)

Code before:
console.log(true);
console.log(false);
console.log(true);

function foo2() {
	console.log('has side effect');
	return false;
}
console.log(!foo2());

console.log(void 0);
console.log(typeof foo2);
console.log(-Infinity);
console.log(-180n);
console.log(-1000000100);
console.log(-1000001e3);
console.log(-0.011222);
console.log(-1222e-6);
console.log(-1e28);
console.log(-0);
console.log(Infinity);
console.log(-2);
console.log(false);

function foo3() {
	return false;
}


Review comment (suggestion):
If we go for the smallest possible representation, we could do what minifiers do and write `true` as `!0` and `false` as `!1`. Considering some of the other transformations we do, I think this should not be an issue.

Code after:
console.log(true);
console.log(false);
console.log(true);

function foo2() {
	console.log('has side effect');
	return false;
}
console.log(!foo2());

console.log(undefined);
console.log(typeof foo2);
console.log(-Infinity);
console.log(-180n);
console.log(-1000000100);
console.log(-1000001e3);
console.log(-0.011222);
console.log(-1222e-6);
console.log(-1e28);
console.log(-0);
console.log(Infinity);
console.log(-2);
console.log(false);

function foo3() {
	return false;
}
Repository: rollup/rollup
File: src/utils/pluginFilter.ts (TypeScript)

Code before:
const FALLBACK_FALSE = 0;
type FallbackValues = typeof FALLBACK_TRUE | typeof FALLBACK_FALSE;

type PluginFilterWithFallback = (input: string) => boolean | FallbackValues;

export type PluginFilter = (input: string) => boolean;
export type TransformHookFilter = (id: string, code: string) => boolean;

interface NormalizedStringFilter {
	include?: StringOrRegExp[];
	exclude?: StringOrRegExp[];
}

function patternToIdFilter(pattern: StringOrRegExp): PluginFilter {
	if (pattern instanceof RegExp) {
		return (id: string) => {
			const normalizedId = normalize(id);
			const result = pattern.test(normalizedId);
			pattern.lastIndex = 0;
			return result;
		};
	}
	const cwd = process.cwd();
	const matcher = picomatch(pattern, { dot: true });
	return (id: string) => {
		const normalizedId = normalize(relative(cwd, id));
		return matcher(normalizedId);
	};
}

function patternToCodeFilter(pattern: StringOrRegExp): PluginFilter {
	if (pattern instanceof RegExp) {
		return (code: string) => {
			const result = pattern.test(code);
			pattern.lastIndex = 0;
			return result;
		};
	}
	return (code: string) => code.includes(pattern);
}

function createFilter(
	exclude: PluginFilter[] | undefined,
	include: PluginFilter[] | undefined
): PluginFilterWithFallback | undefined {
	if (!exclude && !include) {
		return;
	}

	return input => {
		if (exclude?.some(filter => filter(input))) {

Review comment (suggestion):
This `relative(cwd, id)` normalisation was found to be problematic in rolldown (https://github.com/rolldown/rolldown/pull/3972), so I'm going to add the fix made in rolldown.

Code after:
const FALLBACK_FALSE = 0;
type FallbackValues = typeof FALLBACK_TRUE | typeof FALLBACK_FALSE;

type PluginFilterWithFallback = (input: string) => boolean | FallbackValues;

export type PluginFilter = (input: string) => boolean;
export type TransformHookFilter = (id: string, code: string) => boolean;

interface NormalizedStringFilter {
	include?: StringOrRegExp[];
	exclude?: StringOrRegExp[];
}

function getMatcherString(glob: string, cwd: string) {
	if (glob.startsWith('**') || isAbsolute(glob)) {
		return normalize(glob);
	}

	const resolved = resolve(cwd, glob);
	return normalize(resolved);
}

function patternToIdFilter(pattern: StringOrRegExp): PluginFilter {
	if (pattern instanceof RegExp) {
		return (id: string) => {
			const normalizedId = normalize(id);
			const result = pattern.test(normalizedId);
			pattern.lastIndex = 0;
			return result;
		};
	}
	const cwd = process.cwd();
	const glob = getMatcherString(pattern, cwd);
	const matcher = picomatch(glob, { dot: true });
	return (id: string) => {
		const normalizedId = normalize(id);
		return matcher(normalizedId);
	};
}

function patternToCodeFilter(pattern: StringOrRegExp): PluginFilter {
	if (pattern instanceof RegExp) {
		return (code: string) => {
			const result = pattern.test(code);
			pattern.lastIndex = 0;
			return result;
		};
	}
	return (code: string) => code.includes(pattern);
}
Repository: rollup/rollup
File: src/utils/pluginFilter.ts (TypeScript)

Code before:
	if (pattern instanceof RegExp) {
		return (code: string) => {
			const result = pattern.test(code);
			pattern.lastIndex = 0;
			return result;
		};
	}
	return (code: string) => code.includes(pattern);
}

function createFilter(
	exclude: PluginFilter[] | undefined,
	include: PluginFilter[] | undefined
): PluginFilter | undefined {
	if (!exclude && !include) {
		return;
	}

	return input => {
		if (exclude?.some(filter => filter(input))) {
			return false;
		}
		if (include?.some(filter => filter(input))) {
			return true;
		}
		return !!include && include.length > 0 ? false : true;
	};
}

function normalizeFilter(filter: StringFilter): NormalizedStringFilter {
	if (typeof filter === 'string' || filter instanceof RegExp) {
		return {
			include: [filter]
		};
	}
	if (Array.isArray(filter)) {
		return {
			include: ensureArray(filter)
		};
	}
	return {
		exclude: filter.exclude ? ensureArray(filter.exclude) : undefined,
		include: filter.include ? ensureArray(filter.include) : undefined
	};
}

function createIdFilter(filter: StringFilter | undefined): PluginFilter | undefined {
	if (!filter) return;
	const { exclude, include } = normalizeFilter(filter);
	const excludeFilter = exclude?.map(patternToIdFilter);
	const includeFilter = include?.map(patternToIdFilter);

Review comment (suggestion):
```suggestion
		return !(include && include.length > 0)
```

Code after:
	if (pattern instanceof RegExp) {
		return (code: string) => {
			const result = pattern.test(code);
			pattern.lastIndex = 0;
			return result;
		};
	}
	return (code: string) => code.includes(pattern);
}

function createFilter(
	exclude: PluginFilter[] | undefined,
	include: PluginFilter[] | undefined
): PluginFilter | undefined {
	if (!exclude && !include) {
		return;
	}

	return input => {
		if (exclude?.some(filter => filter(input))) {
			return false;
		}
		if (include?.some(filter => filter(input))) {
			return true;
		}
		return !(include && include.length > 0);
	};
}

function normalizeFilter(filter: StringFilter): NormalizedStringFilter {
	if (typeof filter === 'string' || filter instanceof RegExp) {
		return {
			include: [filter]
		};
	}
	if (Array.isArray(filter)) {
		return {
			include: filter
		};
	}
	return {
		exclude: filter.exclude ? ensureArray(filter.exclude) : undefined,
		include: filter.include ? ensureArray(filter.include) : undefined
	};
}

function createIdFilter(filter: StringFilter | undefined): PluginFilter | undefined {
	if (!filter) return;
	const { exclude, include } = normalizeFilter(filter);
	const excludeFilter = exclude?.map(patternToIdFilter);
	const includeFilter = include?.map(patternToIdFilter);
Repository: rollup/rollup
File: src/watch/watch-proxy.ts (TypeScript)

Code before:
				if (subPath) {
					return false;
				}
			}
		}
	}
	return true;
}

async function watchInternal(configs: MaybeArray<RollupOptions>, emitter: RollupWatcher) {
	const optionsList = await Promise.all(
		ensureArray(configs).map(config => mergeOptions(config, true))
	);
	const watchOptionsList = optionsList.filter(config => config.watch !== false);
	if (watchOptionsList.length === 0) {
		return error(
			logInvalidOption(
				'watch',
				URL_WATCH,
				'there must be at least one config where "watch" is not set to "false"'
			)
		);
	}
	if (!checkWatchConfig(watchOptionsList)) {
		return error(
			logInvalidOption('watch', URL_WATCH, 'the input should not be inside the output dir')
		);
	}
	await loadFsEvents();
	const { Watcher } = await import('./watch');
	new Watcher(watchOptionsList, emitter);
}


Review comment (suggestion):
If we throw the error within checkWatchConfig instead, then we can actually print the offending paths, which users might like if they have multiple configs.

Code after:
	for (const item of config) {
		if (item.input && item.output) {
			const input = typeof item.input === 'string' ? ensureArray(item.input) : item.input;
			const output = ensureArray(item.output);
			for (const index in input) {
				const inputPath = input[index as keyof typeof input] as string;
				const subPath = output.find(o => {
					if (!o.dir || typeof inputPath !== 'string') {
						return false;
					}
					const _outPath = withTrailingSlash(o.dir);
					const _inputPath = withTrailingSlash(inputPath);
					return _inputPath.startsWith(_outPath);
				});
				if (subPath) {
					error(
						logInvalidOption(
							'watch',
							URL_WATCH,
							`the input "${inputPath}" is a subpath of the output "${subPath.dir}"`
						)
					);
				}
			}
		}
	}
}

async function watchInternal(configs: MaybeArray<RollupOptions>, emitter: RollupWatcher) {
	const optionsList = await Promise.all(
		ensureArray(configs).map(config => mergeOptions(config, true))
	);
	const watchOptionsList = optionsList.filter(config => config.watch !== false);
	if (watchOptionsList.length === 0) {
		return error(
			logInvalidOption(
				'watch',
				URL_WATCH,
				'there must be at least one config where "watch" is not set to "false"'
			)
		);
	}
	checkWatchConfig(watchOptionsList);
	await loadFsEvents();
	const { Watcher } = await import('./watch');
	new Watcher(watchOptionsList, emitter);
}
Repository: rollup/rollup
File: test/function/samples/resolveid-recursive-call/_config.js (JavaScript)

Code before:
module.exports = defineTest({
	description:
		'skipSelf: true option in resolveId hook option should skip the plugin if it has been called before with the same id and importer, see #5768 for more details',
	options: {
		plugins: [
			{
				name: 'r1',
				resolveId(id) {
					console.log('r1', id);
					const importer = 'foo';
					return this.resolve(id, importer) ?? 'success';
				},
				load(id) {
					if (id === 'success') {
						return { code: 'export default 1' };
					}
				}
			},
			{
				name: 'r2',
				resolveId(id) {
					console.log('r2', id);
					const importer = 'bar';
					return this.resolve(id, importer);
				}
			}
		]
	}
});


Review comment (bug):
Please do not `console.log` in tests as this spams the test output. While this is nice for debugging, if you want to check which hooks are called, why not instead push values into an array and at the end of the test, assert which calls happened in which order? This would immediately show what the intended outcome is and possibly even improve the test.

Code after:
module.exports = defineTest({
	description:
		'skipSelf: true option in resolveId hook option should skip the plugin if it has been called before with the same id and importer, see #5768 for more details',
	options: {
		plugins: [
			{
				name: 'r1',
				async resolveId(id) {
					const importer = 'foo';
					return (await this.resolve(id, importer)) ?? 'success';
				},
				load(id) {
					if (id === 'success') {
						return { code: 'export default 1' };
					}
				}
			},
			{
				name: 'r2',
				resolveId(id) {
					const importer = 'bar';
					return this.resolve(id, importer);
				}
			}
		]
	}
});