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

Code before:
				name: consumedFile.name,
				needsCodeReference,
				source,
				type: 'asset'
			};
		}
	}

	private finalizeAssetsWithSameSource(
		consumedFiles: ReadonlyArray<ConsumedAsset>,
		sourceHash: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = '';
		let usedConsumedFile: ConsumedAsset;
		let needsCodeReference = true;
		for (const consumedFile of consumedFiles) {
			needsCodeReference &&= consumedFile.needsCodeReference;
			const assetFileName = generateAssetFileName(
				consumedFile.name,
				consumedFile.source!,
				sourceHash,
				outputOptions,
				bundle
			);
			if (!fileName || assetFileName < fileName) {
				fileName = assetFileName;
				usedConsumedFile = consumedFile;
			}
		}
		fileNamesBySource.set(sourceHash, fileName);

		for (const consumedFile of consumedFiles) {
			// We must not modify the original assets to avoid interaction between outputs
			const assetWithFileName = { ...consumedFile, fileName };
			this.filesByReferenceId.set(consumedFile.referenceId, assetWithFileName);
		}

		bundle[fileName] = {
			fileName,
			name: usedConsumedFile!.name,
			needsCodeReference,
			source: usedConsumedFile!.source!,
			type: 'asset'
		};
	}
}


Review comment (suggestion):
The `fileName` sort strategy is a bit different from #4912. The previous code always uses the shortest `fileName` while this one doesn't; I think it's better to keep the same strategy, otherwise this would be a breaking change.

Code after:
				name: consumedFile.name,
				needsCodeReference,
				source,
				type: 'asset'
			};
		}
	}

	private finalizeAssetsWithSameSource(
		consumedFiles: ReadonlyArray<ConsumedAsset>,
		sourceHash: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = '';
		let usedConsumedFile: ConsumedAsset;
		let needsCodeReference = true;
		for (const consumedFile of consumedFiles) {
			needsCodeReference &&= consumedFile.needsCodeReference;
			const assetFileName = generateAssetFileName(
				consumedFile.name,
				consumedFile.source!,
				sourceHash,
				outputOptions,
				bundle
			);
			if (
				!fileName ||
				assetFileName.length < fileName.length ||
				(assetFileName.length === fileName.length && assetFileName < fileName)
			) {
				fileName = assetFileName;
				usedConsumedFile = consumedFile;
			}
		}
		fileNamesBySource.set(sourceHash, fileName);

		for (const consumedFile of consumedFiles) {
			// We must not modify the original assets to avoid interaction between outputs
			const assetWithFileName = { ...consumedFile, fileName };
			this.filesByReferenceId.set(consumedFile.referenceId, assetWithFileName);
		}

		bundle[fileName] = {
			fileName,
			name: usedConsumedFile!.name,
			needsCodeReference,
			source: usedConsumedFile!.source!,
			type: 'asset'
		};
	}
}
Repository: rollup/rollup
File: src/finalisers/es.ts (TypeScript)

Code before:
								? specifier.imported
								: `${specifier.imported} as ${specifier.reexported}`
						)
						.join(`,${_}`)}${_}}${_}from${_}${pathWithAssertion}`
				);
			}
		}
	}
	return importBlock;
}

function isLegalJsIdentifier(name: string): boolean {
	return /^[$A-Z_a-z][\w$]*$/.test(name);
}

function getExportBlock(exports: ChunkExports, { _, cnst }: GenerateCodeSnippets): string[] {
	const exportBlock: string[] = [];
	const exportDeclaration: string[] = [];
	for (const specifier of exports) {
		if (specifier.expression) {
			exportBlock.push(`${cnst} ${specifier.local}${_}=${_}${specifier.expression};`);
		}
		exportDeclaration.push(
			specifier.exported === specifier.local
				? specifier.local
				: `${specifier.local} as ${
						isLegalJsIdentifier(specifier.exported)
							? specifier.exported
							: JSON.stringify(specifier.exported)
				  }`
		);
	}
	if (exportDeclaration.length > 0) {
		exportBlock.push(`export${_}{${_}${exportDeclaration.join(`,${_}`)}${_}};`);
	}
	return exportBlock;
}


Review comment (suggestion):
I **think** that this is the only place that has to be fixed here as no other code refers to `specifier.exported` and local specifiers (and reexporting ones) have to be legal already. It would be worth rechecking if I'm right though 

Code after:
			}
			if (namedReexports.length > 0) {
				importBlock.push(
					`export${_}{${_}${namedReexports
						.map(specifier =>
							specifier.imported === specifier.reexported
								? safeExportName(specifier.imported)
								: `${safeExportName(specifier.imported)} as ${safeExportName(specifier.reexported)}`
						)
						.join(`,${_}`)}${_}}${_}from${_}${pathWithAssertion}`
				);
			}
		}
	}
	return importBlock;
}

function getExportBlock(exports: ChunkExports, { _, cnst }: GenerateCodeSnippets): string[] {
	const exportBlock: string[] = [];
	const exportDeclaration: string[] = [];
	for (const specifier of exports) {
		if (specifier.expression) {
			exportBlock.push(`${cnst} ${specifier.local}${_}=${_}${specifier.expression};`);
		}
		exportDeclaration.push(
			specifier.exported === specifier.local
				? specifier.local
				: `${specifier.local} as ${safeExportName(specifier.exported)}`
		);
	}
	if (exportDeclaration.length > 0) {
		exportBlock.push(`export${_}{${_}${exportDeclaration.join(`,${_}`)}${_}};`);
	}
	return exportBlock;
}
Repository: rollup/rollup
File: test/function/samples/tree-shake-iterable/main.js (JavaScript)

Code before:
	[Symbol.iterator]() {
		return {
			next() {
				effects++;
				return { done: true };
			}
		};
	}
};

new Map(iterable);
new Set(iterable);
new WeakMap(iterable);
new WeakSet(iterable);
Float32Array.from(iterable);
Float64Array.from(iterable);
Int16Array.from(iterable);
Int32Array.from(iterable);
Int8Array.from(iterable);
Uint16Array.from(iterable);
Uint32Array.from(iterable);
Uint8Array.from(iterable);
Uint8ClampedArray.from(iterable);
Object.fromEntries(iterable);

assert.equal(effects, 14);


Review comment (suggestion):
we could add `Array.from`, `BigInt64Array.from`, `BigUint64Array.from` to the tests as well.

Code after:
	[Symbol.iterator]() {
		return {
			next() {
				effects++;
				return { done: true };
			}
		};
	}
};

new Map(iterable);
new Set(iterable);
new WeakMap(iterable);
new WeakSet(iterable);
Array.from(iterable);
BigInt64Array.from(iterable);
BigUint64Array.from(iterable);
Float32Array.from(iterable);
Float64Array.from(iterable);
Int16Array.from(iterable);
Int32Array.from(iterable);
Int8Array.from(iterable);
Uint16Array.from(iterable);
Uint32Array.from(iterable);
Uint8Array.from(iterable);
Uint8ClampedArray.from(iterable);
Object.fromEntries(iterable);

assert.equal(effects, 17);
Repository: rollup/rollup
File: test/function/samples/plugin-error-transform-pos/_config.js (JavaScript)

Code before:
module.exports = defineTest({
	description: '`this.error(...)` accepts number as second parameter (#5044)',
	options: {
		plugins: [
			{
				name: 'plugin1',
				transform(code) {
					return code + 'bar';
				}
			},
			{
				name: 'plugin2',
				transform() {
					this.error(new Error('error'), 26);
				}
			}
		]
	},
	error: {
		code: 'PLUGIN_ERROR',
		message: 'error',
		plugin: 'plugin2',
		hook: 'transform',
		id: path.join(__dirname, 'main.js'),
		watchFiles: [path.join(__dirname, 'main.js')],
		frame: "11: export const foo = 'foo'bar\n                             ^",
		loc: {
			column: 26,
			file: path.join(__dirname, 'main.js'),
			line: 1
		},
		pos: 26
	}
});


Review comment (bug):
I'm not sure why the test fails when `frame` is `1: export const foo = 'foo'bar\n                             ^`.

Code after:

module.exports = defineTest({
	description: '`this.error(...)` accepts number as second parameter (#5044)',
	options: {
		plugins: [
			{
				name: 'plugin1',
				transform(code) {
					return code + 'bar';
				}
			},
			{
				name: 'plugin2',
				transform() {
					this.error(new Error('error'), 26);
				}
			}
		]
	},
	error: {
		code: 'PLUGIN_ERROR',
		message: 'error',
		plugin: 'plugin2',
		hook: 'transform',
		id: ID_MAIN,
		watchFiles: [ID_MAIN],
		frame: `
			1: export const foo = 'foo'bar
			                             ^`,
		loc: {
			column: 26,
			file: ID_MAIN,
			line: 1
		},
		pos: 26
	}
});
Repository: rollup/rollup
File: src/utils/decodedSourcemap.ts (TypeScript)

Code before:
	SourceMapInput
} from '../rollup/types';

type Input = SourceMapInput | ExistingDecodedSourceMap | undefined;

export function decodedSourcemap(map: Input): ExistingDecodedSourceMap | null {
	if (!map) return null;

	if (typeof map === 'string') {
		map = JSON.parse(map) as ExistingRawSourceMap;
	}
	if (map.mappings === '') {
		return {
			mappings: [],
			names: [],
			sources: [],
			version: 3
		};
	}

	const decodedMap = {
		...(map as ExistingRawSourceMap | ExistingDecodedSourceMap),
		mappings: undefined as unknown as SourceMapMappings
	};

	const origMappings = map.mappings;
	let memoizedMappings: SourceMapMappings | undefined;

	Object.defineProperty(decodedMap, 'mappings', {
		enumerable: true,
		get() {
			if (memoizedMappings) {
				return memoizedMappings;
			}
			memoizedMappings = typeof origMappings === 'string' ? decode(origMappings) : origMappings;
			return memoizedMappings;
		},
		set(value) {
			memoizedMappings = value;
		}
	});

	return decodedMap;
}


Review comment (suggestion):
Minor style point for consistency: We try to avoid abbreviations, so `originalMappins` would be a preferred.

Code after:
	ExistingRawSourceMap,
	SourceMapInput
} from '../rollup/types';

type Input = SourceMapInput | ExistingDecodedSourceMap | undefined;

interface CachedSourcemapData {
	encodedMappings: string | undefined;
	decodedMappings: ExistingDecodedSourceMap['mappings'] | undefined;
}

const sourceMapCache = new WeakMap<ExistingDecodedSourceMap, CachedSourcemapData>();

/**
 * This clears the decoded array and falls back to the encoded string form.
 * Sourcemap mappings arrays can be very large and holding on to them for longer
 * than is necessary leads to poor heap utilization.
 */
function resetCacheToEncoded(cache: CachedSourcemapData) {
	if (cache.encodedMappings === undefined && cache.decodedMappings) {
		cache.encodedMappings = encode(cache.decodedMappings);
	}
	cache.decodedMappings = undefined;
}

export function resetSourcemapCache(
	map: ExistingDecodedSourceMap | null,
	sourcemapChain?: DecodedSourceMapOrMissing[]
) {
	if (map) {
		const cache = sourceMapCache.get(map);
		if (cache) {
			resetCacheToEncoded(cache);
		}
	}

	if (!sourcemapChain) {
		return;
	}

	for (const map of sourcemapChain) {
		if (map.missing) continue;

		resetSourcemapCache(map);
	}
}

export function decodedSourcemap(map: null | undefined): null;
export function decodedSourcemap(map: Exclude<Input, null | undefined>): ExistingDecodedSourceMap;
export function decodedSourcemap(map: Input): ExistingDecodedSourceMap | null;
export function decodedSourcemap(map: Input): ExistingDecodedSourceMap | null {
Repository: rollup/rollup
File: src/rollup/types.d.ts (TypeScript)

Code before:
	mappings: SourceMapSegment[][];
	names: string[];
	sourceRoot?: string;
	sources: string[];
	sourcesContent?: (string | null)[];
	version: number;
	x_google_ignoreList?: number[];
}

export interface ExistingRawSourceMap {
	file?: string;
	mappings: string;
	names: string[];
	sourceRoot?: string;
	sources: string[];
	sourcesContent?: (string | null)[];
	version: number;
	x_google_ignoreList?: number[];
}

export type DecodedSourceMapOrMissing =
	| {
			missing: true;
			plugin: string;
	  }
	| (ExistingDecodedSourceMap & { missing?: false });

export interface SourceMap {
	file: string;
	mappings: string;
	names: string[];
	sources: string[];
	sourcesContent: (string | null)[];
	version: number;
	toString(): string;
	toUrl(): string;
}

export type SourceMapInput = ExistingRawSourceMap | string | null | { mappings: '' };

interface ModuleOptions {
	assertions: Record<string, string>;
	meta: CustomPluginOptions;
	moduleSideEffects: boolean | 'no-treeshake';
	syntheticNamedExports: boolean | string;
}

export interface SourceDescription extends Partial<PartialNull<ModuleOptions>> {
	ast?: AcornNode;
	code: string;
	map?: SourceMapInput;

Review comment (suggestion):
This change was to avoid using `!map.mappings` to determine if the map was missing as accessing this property would trigger the decode.

With this change, the only option provided by intellisense is `.missing` and switching on that value correctly narrows the type using control flow analysis.

Code after:
	readonly mappings: SourceMapSegment[][];
	names: string[];
	sourceRoot?: string;
	sources: string[];
	sourcesContent?: (string | null)[];
	version: number;
	x_google_ignoreList?: number[];
}

export interface ExistingRawSourceMap {
	file?: string;
	mappings: string;
	names: string[];
	sourceRoot?: string;
	sources: string[];
	sourcesContent?: (string | null)[];
	version: number;
	x_google_ignoreList?: number[];
}

export type DecodedSourceMapOrMissing =
	| {
			missing: true;
			plugin: string;
	  }
	| (ExistingDecodedSourceMap & { missing?: false });

export interface SourceMap {
	file: string;
	mappings: string;
	names: string[];
	sources: string[];
	sourcesContent: (string | null)[];
	version: number;
	toString(): string;
	toUrl(): string;
}

export type SourceMapInput = ExistingRawSourceMap | string | null | { mappings: '' };

interface ModuleOptions {
	assertions: Record<string, string>;
	meta: CustomPluginOptions;
	moduleSideEffects: boolean | 'no-treeshake';
	syntheticNamedExports: boolean | string;
}

export interface SourceDescription extends Partial<PartialNull<ModuleOptions>> {
	ast?: AcornNode;
	code: string;
	map?: SourceMapInput;
Repository: rollup/rollup
File: src/finalisers/system.ts (TypeScript)

Code before:
	snippets: GenerateCodeSnippets
): string =>
	getExportsBlock(
		exports.filter(expt => expt.hoisted).map(expt => ({ name: expt.exported, value: expt.local })),
		t,
		snippets
	);

function getExportsBlock(
	exports: readonly { name: string; value: string }[],
	t: string,
	{ _, n }: GenerateCodeSnippets
): string {
	if (exports.length === 0) {
		return '';
	}
	if (exports.length === 1) {
		return `exports(${JSON.stringify(exports[0].name)},${_}${exports[0].value});${n}${n}`;
	}
	return (
		`exports({${n}${t}__proto__:${_}null` +
		exports
			.map(
				({ name, value }) =>
					`,${n}${t}${
						name === '__proto__' ? '["__proto__"]' : stringifyObjectKeyIfNeeded(name)
					}:${_}${value}`
			)
			.join('') +
		`${n}});${n}${n}`
	);
}

const getSyntheticExportsBlock = (
	exports: ChunkExports,
	t: string,
	snippets: GenerateCodeSnippets
): string =>
	getExportsBlock(
		exports
			.filter(expt => expt.expression)
			.map(expt => ({ name: expt.exported, value: expt.local })),
		t,
		snippets
	);

const getMissingExportsBlock = (
	exports: ChunkExports,
	t: string,
	snippets: GenerateCodeSnippets
): string =>

Review comment (refactor):
I wonder if it would not make sense to move this logic into `stirngifyObjectKeyIfNeeded`. Then you could avoid the extra logic in `generateCodeSnippets`.

Code after:
	snippets: GenerateCodeSnippets
): string =>
	getExportsBlock(
		exports.filter(expt => expt.hoisted).map(expt => ({ name: expt.exported, value: expt.local })),
		t,
		snippets
	);

function getExportsBlock(
	exports: readonly { name: string; value: string }[],
	t: string,
	{ _, n }: GenerateCodeSnippets
): string {
	if (exports.length === 0) {
		return '';
	}
	if (exports.length === 1) {
		return `exports(${JSON.stringify(exports[0].name)},${_}${exports[0].value});${n}${n}`;
	}
	return (
		`exports({${n}` +
		exports
			.map(({ name, value }) => `${t}${stringifyObjectKeyIfNeeded(name)}:${_}${value}`)
			.join(`,${n}`) +
		`${n}});${n}${n}`
	);
}

const getSyntheticExportsBlock = (
	exports: ChunkExports,
	t: string,
	snippets: GenerateCodeSnippets
): string =>
	getExportsBlock(
		exports
			.filter(expt => expt.expression)
			.map(expt => ({ name: expt.exported, value: expt.local })),
		t,
		snippets
	);

const getMissingExportsBlock = (
	exports: ChunkExports,
	t: string,
	snippets: GenerateCodeSnippets
): string =>
	getExportsBlock(
		exports
			.filter(expt => expt.local === MISSING_EXPORT_SHIM_VARIABLE)
			.map(expt => ({ name: expt.exported, value: MISSING_EXPORT_SHIM_VARIABLE })),
		t,
Repository: rollup/rollup
File: native.js (JavaScript)

Code before:
const { existsSync } = require('node:fs');
const { join } = require('node:path');
const { platform, arch, report } = require('node:process');

const isMusl = () => !report.getReport().header.glibcVersionRuntime;

const bindingsByPlatformAndArch = {
	android: {
		arm: { base: 'android-arm-eabi' },
		arm64: { base: 'android-arm64' }
	},
	darwin: {
		arm64: { base: 'darwin-arm64' },
		x64: { base: 'darwin-x64' }
	},
	linux: {
		arm: { base: 'linux-arm-gnueabihf', musl: null },
		arm64: { base: 'linux-arm64-gnu', musl: 'linux-arm64-musl' },
		riscv64: { base: 'linux-riscv64-gnu', musl: null },
		s390x: { base: 'linux-s390x-gnu', musl: null },
		x64: { base: 'linux-x64-gnu', musl: 'linux-x64-musl' }
	},
	win32: {
		arm64: { base: 'win32-arm64-msvc' },
		ia32: { base: 'win32-ia32-msvc' },
		x64: { base: 'win32-x64-msvc' }
	}
};

const msvcLinkFilenameByArch = {
	arm64: 'vc_redist.arm64.exe',
	ia32: 'vc_redist.x86.exe',
	x64: 'vc_redist.x64.exe'
};

const packageBase = getPackageBase();

if (!packageBase) {
	throw new Error(
		`Your current platform "${platform}" and architecture "${arch}" combination is not yet supported by the native Rollup build. Please use the WASM build "@rollup/wasm-node" instead.

The following platform-architecture combinations are supported:
${Object.entries(bindingsByPlatformAndArch)
	.flatMap(([platformName, architectures]) =>

Review comment (suggestion):
@pavolloffay @lukastaegert 
I guess this line and `cpu` field in package.json need to be `ppc64` instead of `ppc64le`. `process.arch` seems to return `ppc64`.
https://github.com/konveyor/tackle2-ui/actions/runs/8150208242/job/22276088934#step:7:89



Code after:
const { existsSync } = require('node:fs');
const { join } = require('node:path');
const { platform, arch, report } = require('node:process');

const isMusl = () => !report.getReport().header.glibcVersionRuntime;

const bindingsByPlatformAndArch = {
	android: {
		arm: { base: 'android-arm-eabi' },
		arm64: { base: 'android-arm64' }
	},
	darwin: {
		arm64: { base: 'darwin-arm64' },
		x64: { base: 'darwin-x64' }
	},
	linux: {
		arm: { base: 'linux-arm-gnueabihf', musl: null },
		arm64: { base: 'linux-arm64-gnu', musl: 'linux-arm64-musl' },
		ppc64le: { base: 'linux-ppc64le-gnu', musl: null },
		riscv64: { base: 'linux-riscv64-gnu', musl: null },
		s390x: { base: 'linux-s390x-gnu', musl: null },
		x64: { base: 'linux-x64-gnu', musl: 'linux-x64-musl' }
	},
	win32: {
		arm64: { base: 'win32-arm64-msvc' },
		ia32: { base: 'win32-ia32-msvc' },
		x64: { base: 'win32-x64-msvc' }
	}
};

const msvcLinkFilenameByArch = {
	arm64: 'vc_redist.arm64.exe',
	ia32: 'vc_redist.x86.exe',
	x64: 'vc_redist.x64.exe'
};

const packageBase = getPackageBase();

if (!packageBase) {
	throw new Error(
		`Your current platform "${platform}" and architecture "${arch}" combination is not yet supported by the native Rollup build. Please use the WASM build "@rollup/wasm-node" instead.

The following platform-architecture combinations are supported:
${Object.entries(bindingsByPlatformAndArch)
Repository: rollup/rollup
File: src/utils/getter.ts (TypeScript)

Code before:
export function cacheObjectGetters<T, K extends PropertyKey = keyof T>(
	object: T,
	getterProperties: K[]
) {
	for (const property of getterProperties) {
		const propertyGetter = Object.getOwnPropertyDescriptor(object, property)?.get;
		if (propertyGetter) {
			let cached: unknown;
			Object.defineProperty(object, property, {
				get: () => {
					if (cached === undefined) {
						cached = propertyGetter.call(object);
					}
					return cached;
				}
			});
		}
	}
}


Review comment (performance):
It seems that the second parameter is unnecessary, we can get all `get` properties by iterating the `this.info`. That way, we can make the calling of `cacheObjectGetters` more concise, and the above comments also no longer needed. Although this incur a slight performance overhead, I feel it's acceptable, considering `this.info` doesn't have many properties.

Code after:
export function cacheObjectGetters<T, K extends PropertyKey = keyof T>(
	object: T,
	getterProperties: K[]
) {
	for (const property of getterProperties) {
		const propertyGetter = Object.getOwnPropertyDescriptor(object, property)!.get!;
		Object.defineProperty(object, property, {
			get() {
				const value = propertyGetter.call(object);
				// This replaces the getter with a fixed value for subsequent calls
				Object.defineProperty(object, property, { value });
				return value;
			}
		});
	}
}
Repository: rollup/rollup
File: src/utils/getter.ts (TypeScript)

Code before:
export function cacheObjectGetters<T, K extends PropertyKey = keyof T>(
	object: T,
	getterProperties: K[]
) {
	for (const property of getterProperties) {
		const propertyGetter = Object.getOwnPropertyDescriptor(object, property)?.get;
		if (propertyGetter) {
			let cached: unknown;
			Object.defineProperty(object, property, {
				get: () => {
					if (cached === undefined) {
						cached = propertyGetter.call(object);
					}
					return cached;
				}
			});
		}
	}
}


Review comment (suggestion):
A stupid question: Wouldn't it make more sense to do the caching by replacing the
property with a non-getter fixed value on first call? That would also remove the overhead of the function call for subsequent calls:

```suggestion
			Object.defineProperty(object, property, {
				get: () => {
					const value = propertyGetter.call(object);
					// This replaces the getter with a fixed value for subsequent calls
					Object.defineProperty(object, property, { value });
					return value;
				}
			});
```

Code after:
export function cacheObjectGetters<T, K extends PropertyKey = keyof T>(
	object: T,
	getterProperties: K[]
) {
	for (const property of getterProperties) {
		const propertyGetter = Object.getOwnPropertyDescriptor(object, property)!.get!;
		Object.defineProperty(object, property, {
			get() {
				const value = propertyGetter.call(object);
				// This replaces the getter with a fixed value for subsequent calls
				Object.defineProperty(object, property, { value });
				return value;
			}
		});
	}
}