ronantakizawa/codereview-bench huggingface dataset:
Repository: roboflow/supervision
File: supervision/detection/utils.py (Python)

Code before:
        return xyxy

    xyxy_padded = pad_boxes(xyxy, px=step)
    for _ in range(max_iterations):
        iou = box_iou_batch(xyxy_padded, xyxy_padded)
        np.fill_diagonal(iou, 0)

        if np.all(iou == 0):
            break

        i, j = np.unravel_index(np.argmax(iou), iou.shape)

        xyxy_i, xyxy_j = xyxy_padded[i], xyxy_padded[j]
        box_intersection = get_box_intersection(xyxy_i, xyxy_j)
        assert (
            box_intersection is not None
        ), "Since we checked IoU already, boxes should always intersect"

        intersection_center = (box_intersection[:2] + box_intersection[2:]) / 2
        xyxy_i_center = (xyxy_i[:2] + xyxy_i[2:]) / 2
        xyxy_j_center = (xyxy_j[:2] + xyxy_j[2:]) / 2

        unit_vector_i = get_unit_vector(intersection_center, xyxy_i_center)
        unit_vector_j = get_unit_vector(intersection_center, xyxy_j_center)

        xyxy_padded[i, [0, 2]] += int(unit_vector_i[0] * step)
        xyxy_padded[i, [1, 3]] += int(unit_vector_i[1] * step)
        xyxy_padded[j, [0, 2]] += int(unit_vector_j[0] * step)
        xyxy_padded[j, [1, 3]] += int(unit_vector_j[1] * step)

    return pad_boxes(xyxy_padded, px=-step)


Review comment (suggestion):
We have `move_boxes` util. Looks like we could replace:

```python
xyxy_padded[i, [0, 2]] += int(unit_vector_i[0] * step)
xyxy_padded[i, [1, 3]] += int(unit_vector_i[1] * step)
```

With:

```python
offset=(unit_vector_i * step).astype(int)
xyxy_padded = move_boxes(xyxy=xyxy_padded, offset=offset)
```

I find it a lot more readable.

Code after:
        # Nx2
        centers = (xyxy_padded[:, :2] + xyxy_padded[:, 2:]) / 2

        # NxNx2
        delta_centers = centers[:, np.newaxis, :] - centers[np.newaxis, :, :]
        delta_centers *= overlap_mask[:, :, np.newaxis]

        # Nx2
        delta_sum = np.sum(delta_centers, axis=1)
        delta_magnitude = np.linalg.norm(delta_sum, axis=1, keepdims=True)
        direction_vectors = np.divide(
            delta_sum,
            delta_magnitude,
            out=np.zeros_like(delta_sum),
            where=delta_magnitude != 0,
        )

        force_vectors = np.sum(iou, axis=1)
        force_vectors = force_vectors[:, np.newaxis] * direction_vectors

        force_vectors *= 10
        force_vectors[(force_vectors > 0) & (force_vectors < 2)] = 2
        force_vectors[(force_vectors < 0) & (force_vectors > -2)] = -2

        force_vectors = force_vectors.astype(int)

        xyxy_padded[:, [0, 1]] += force_vectors
        xyxy_padded[:, [2, 3]] += force_vectors

    return pad_boxes(xyxy_padded, px=-1)
Repository: rollup/rollup
File: test/function/samples/nested-plugin/_config.js (JavaScript)

Code before:
const plugin = [
	{
		name: 'nested-plugin-1',
		transform(code) {
			return code.replace('foo = 1', 'foo = 2');
		}
	},
	{
		name: 'nested-plugin-2',
		transform(code) {
			return code.replace('answer = 41', 'answer = 42');
		}
	}
];

module.exports = {
	description: 'works when nested plugin',
	options: {
		plugins: [plugin]
	}
};


Review comment (suggestion):
Maybe add a `false` or `undefined` to the array to test the nested filtering. The idea being that plugins can use simple switches to deactivate nested plugins. Rollup should just ignore this.

Code after:
const plugin = [
	{
		name: 'nested-plugin-1',
		transform(code) {
			return code.replace('foo = 1', 'foo = 2');
		}
	},
	{
		name: 'nested-plugin-2',
		transform(code) {
			return code.replace('answer = 41', 'answer = 42');
		}
	}
];

module.exports = {
	description: 'works when nested plugin',
	options: {
		// eslint-disable-next-line no-sparse-arrays
		plugins: [plugin, [undefined, [null]], ,]
	}
};
Repository: rollup/rollup
File: src/utils/FileEmitter.ts (TypeScript)

Code before:
				// Avoid unhandled Promise rejection as the error will be thrown later
				// once module loading has finished
			});

		return this.assignReferenceId(consumedChunk, emittedChunk.id);
	}

	private finalizeAsset(
		consumedFile: ConsumedFile,
		source: string | Uint8Array,
		referenceId: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = consumedFile.fileName;

		// Deduplicate assets if an explicit fileName is not provided
		if (!fileName) {
			const pregenerated = pregenerateAssetFileName(consumedFile.name, source, outputOptions);
			// Reuse source hash if it was computed as part of the asset file name
			const sourceHash = pregenerated.sourceHash ?? getSourceHash(source);
			fileName = fileNamesBySource.get(sourceHash);
			if (!fileName) {
				fileName = makeUnique(pregenerated.fileName, bundle);
				fileNamesBySource.set(sourceHash, fileName);
			}
		}

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

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


Review comment (suggestion):
Removed these lines because `fileNamesBySource` is only being used in `finalizeAsset`, and the deduplication can be contained inside it.

Code after:
				// Avoid unhandled Promise rejection as the error will be thrown later
				// once module loading has finished
			});

		return this.assignReferenceId(consumedChunk, emittedChunk.id);
	}

	private finalizeAsset(
		consumedFile: ConsumedFile,
		source: string | Uint8Array,
		referenceId: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = consumedFile.fileName;

		// Deduplicate assets if an explicit fileName is not provided
		if (!fileName) {
			const sourceHash = getSourceHash(source);
			fileName = fileNamesBySource.get(sourceHash);
			if (!fileName) {
				fileName = generateAssetFileName(
					consumedFile.name,
					source,
					sourceHash,
					outputOptions,
					bundle
				);
				fileNamesBySource.set(sourceHash, fileName);
			}
		}

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

		bundle[fileName] = {
			fileName,
			name: consumedFile.name,
			source,
			type: 'asset'
		};
	}
}
Repository: rollup/rollup
File: src/utils/FileEmitter.ts (TypeScript)

Code before:
			type: 'chunk'
		};
		this.graph.moduleLoader
			.emitChunk(emittedChunk as unknown as EmittedChunk)
			.then(module => (consumedChunk.module = module))
			.catch(() => {
				// Avoid unhandled Promise rejection as the error will be thrown later
				// once module loading has finished
			});

		return this.assignReferenceId(consumedChunk, emittedChunk.id);
	}

	private finalizeAsset(
		consumedFile: ConsumedFile,
		source: string | Uint8Array,
		referenceId: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = consumedFile.fileName;

		// Deduplicate assets if an explicit fileName is not provided
		if (!fileName) {
			const pregenerated = pregenerateAssetFileName(consumedFile.name, source, outputOptions);
			// Reuse source hash if it was computed as part of the asset file name
			const sourceHash = pregenerated.sourceHash ?? getSourceHash(source);
			fileName = fileNamesBySource.get(sourceHash);
			if (!fileName) {
				fileName = makeUnique(pregenerated.fileName, bundle);
				fileNamesBySource.set(sourceHash, fileName);
			}
		}

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

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


Review comment (refactor):
If I see this correctly, this is the only usage of `pregenerateAssetFileName`. In that case, you could just always directly generate `sourceHash` in  `pregenerateAssetFileName` and simplify the code a bit.

Code after:
			type: 'chunk'
		};
		this.graph.moduleLoader
			.emitChunk(emittedChunk as unknown as EmittedChunk)
			.then(module => (consumedChunk.module = module))
			.catch(() => {
				// Avoid unhandled Promise rejection as the error will be thrown later
				// once module loading has finished
			});

		return this.assignReferenceId(consumedChunk, emittedChunk.id);
	}

	private finalizeAsset(
		consumedFile: ConsumedFile,
		source: string | Uint8Array,
		referenceId: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = consumedFile.fileName;

		// Deduplicate assets if an explicit fileName is not provided
		if (!fileName) {
			const sourceHash = getSourceHash(source);
			fileName = fileNamesBySource.get(sourceHash);
			if (!fileName) {
				fileName = generateAssetFileName(
					consumedFile.name,
					source,
					sourceHash,
					outputOptions,
					bundle
				);
				fileNamesBySource.set(sourceHash, fileName);
			}
		}

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

		bundle[fileName] = {
			fileName,
			name: consumedFile.name,
			source,
			type: 'asset'
		};
	}
}
Repository: rollup/rollup
File: src/ast/nodes/ChainExpression.ts (TypeScript)

Code before:
import type { LiteralValueOrUnknown } from './shared/Expression';
import { UnknownValue } from './shared/Expression';
import { NodeBase } from './shared/Node';

const unset = Symbol('unset');

export default class ChainExpression extends NodeBase implements DeoptimizableEntity {
	declare expression: CallExpression | MemberExpression;
	declare type: NodeType.tChainExpression;
	private objectValue: LiteralValueOrUnknown | typeof unset = unset;

	deoptimizeCache(): void {
		this.objectValue = UnknownValue;
	}

	getLiteralValueAtPath(): LiteralValueOrUnknown {
		if (this.getObjectValue() == null) return undefined;
		return UnknownValue;
	}

	hasEffects(context: HasEffectsContext): boolean {
		if (this.getObjectValue() == null) return false;
		return this.expression.hasEffects(context);
	}

	render(code: MagicString, options: RenderOptions): void {
		if (this.getObjectValue() == null) {
			code.remove(this.start, this.end);
		} else super.render(code, options);
	}

	private getObjectValue() {
		if (this.objectValue === unset) {
			let object =
				this.expression.type === 'CallExpression' ? this.expression.callee : this.expression.object;
			if (object.type === 'MemberExpression') object = (object as MemberExpression).object;
			this.objectValue = object.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this);
		}
		return this.objectValue;
	}
}


Review comment (bug):
You do not need the special rendering logic because the actual removal of code happens in the Nodes that can actually have children removed, i.e. `Program`, `BlockStatement` and some edge cases like `IfStatement`. The important part is the `hasEffects`, which can mark this expression as free of side effects.

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

const unset = Symbol('unset');

export default class ChainExpression extends NodeBase implements DeoptimizableEntity {
	declare expression: CallExpression | MemberExpression;
	declare type: NodeType.tChainExpression;
	private objectValue: LiteralValueOrUnknown | typeof unset = unset;

	deoptimizeCache(): void {
		this.objectValue = UnknownValue;
	}

	getLiteralValueAtPath(): LiteralValueOrUnknown {
		if (this.getObjectValue() == null) return undefined;
		return UnknownValue;
	}

	hasEffects(context: HasEffectsContext): boolean {
		if (this.getObjectValue() == null) return false;
		return this.expression.hasEffects(context);
	}

	private getObjectValue() {
		if (this.objectValue === unset) {
			let object =
				this.expression.type === 'CallExpression' ? this.expression.callee : this.expression.object;
			if (object.type === 'MemberExpression') object = (object as MemberExpression).object;
			this.objectValue = object.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this);
		}
		return this.objectValue;
	}
}
Repository: rollup/rollup
File: src/ast/variables/LocalVariable.ts (TypeScript)

Code before:
					path,
					interaction,
					recursionTracker,
					origin
				);
			},
			UNKNOWN_RETURN_EXPRESSION
		);
	}

	hasEffectsOnInteractionAtPath(
		path: ObjectPath,
		interaction: NodeInteraction,
		context: HasEffectsContext
	): boolean {
		switch (interaction.type) {
			case INTERACTION_ACCESSED: {
				if (this.isReassigned) return true;
				return !!(
					this.init &&
					!context.accessed.trackEntityAtPathAndGetIfTracked(path, this) &&
					this.init.hasEffectsOnInteractionAtPath(path, interaction, context)
				);
			}
			case INTERACTION_ASSIGNED: {
				if (this.included && !context.ignore.this) return true;
				if (path.length === 0) return false;
				if (this.isReassigned) return true;
				return !!(
					this.init &&
					!context.assigned.trackEntityAtPathAndGetIfTracked(path, this) &&
					this.init.hasEffectsOnInteractionAtPath(path, interaction, context)
				);
			}
			case INTERACTION_CALLED: {
				if (this.isReassigned) return true;
				return !!(
					this.init &&
					!(
						interaction.withNew ? context.instantiated : context.called
					).trackEntityAtPathAndGetIfTracked(path, interaction.args, this) &&
					this.init.hasEffectsOnInteractionAtPath(path, interaction, context)
				);
			}
		}
	}

	include(): void {
		if (!this.included) {
			this.included = true;
			for (const declaration of this.declarations) {

Review comment (bug):
@lukastaegert I'm not sure why these tests are failing on CI but succeeding on my local machine, and It seems that all failing tests are related to `sourcemap`, Do you have any idea? By the way, If I remove this change, the
previous tests succeed.

Code after:
					path,
					interaction,
					recursionTracker,
					origin
				);
			},
			UNKNOWN_RETURN_EXPRESSION
		);
	}

	hasEffectsOnInteractionAtPath(
		path: ObjectPath,
		interaction: NodeInteraction,
		context: HasEffectsContext
	): boolean {
		switch (interaction.type) {
			case INTERACTION_ACCESSED: {
				if (this.isReassigned) return true;
				return !!(
					this.init &&
					!context.accessed.trackEntityAtPathAndGetIfTracked(path, this) &&
					this.init.hasEffectsOnInteractionAtPath(path, interaction, context)
				);
			}
			case INTERACTION_ASSIGNED: {
				if (this.included) return true;
				if (path.length === 0) return false;
				if (this.isReassigned) return true;
				return !!(
					this.init &&
					!context.assigned.trackEntityAtPathAndGetIfTracked(path, this) &&
					this.init.hasEffectsOnInteractionAtPath(path, interaction, context)
				);
			}
			case INTERACTION_CALLED: {
				if (this.isReassigned) return true;
				return !!(
					this.init &&
					!(
						interaction.withNew ? context.instantiated : context.called
					).trackEntityAtPathAndGetIfTracked(path, interaction.args, this) &&
					this.init.hasEffectsOnInteractionAtPath(path, interaction, context)
				);
			}
		}
	}

	include(): void {
		if (!this.included) {
			this.included = true;
			for (const declaration of this.declarations) {
Repository: rollup/rollup
File: build-plugins/copy-types.ts (TypeScript)

Code before:
import { readFile } from 'node:fs/promises';
import { resolve } from 'node:path';
import type { Plugin } from 'rollup';

export default function copyTypes(fileName: string): Plugin {
	return {
		async generateBundle(_options, _bundle, isWrite) {
			if (isWrite) {
				this.emitFile({
					fileName,
					source: await readFile(resolve('src/rollup/types.d.ts'), 'utf8'),
					type: 'asset'
				});
			}
		},
		name: 'copy-types'
	};
}

export function copyAllTypes(): Plugin[] {
	return [
		copyTypes('rollup.d.ts'),
		{
			async generateBundle(_options, _bundle, isWrite) {
				if (isWrite) {
					this.emitFile({
						fileName: 'loadConfigFile.d.ts',
						source: (await readFile(resolve('cli/run/loadConfigFileType.d.ts'), 'utf8')).replace(
							'../../src/rollup/types',
							'./rollup'
						),
						type: 'asset'
					});
				}
			},
			name: 'copy-loadConfigFile-type'
		}
	];
}


Review comment (suggestion):
As `copyTypes` is now only used for the browser types, maybe rename it to `copyBrowserTypes` and remove/hard code the `fileName` parameter to `rollup.browser.d.ts`.

Then `copyAllTypes` could be renamed `copyNodeTypes`

Code after:
import { readFile } from 'node:fs/promises';
import { resolve } from 'node:path';
import type { Plugin } from 'rollup';

function copyRollupType(fileName: string): Plugin {
	return {
		async generateBundle(_options, _bundle, isWrite) {
			if (isWrite) {
				this.emitFile({
					fileName,
					source: await readFile(resolve('src/rollup/types.d.ts'), 'utf8'),
					type: 'asset'
				});
			}
		},
		name: 'copy-rollup-type'
	};
}

export function copyBrowserTypes(): Plugin {
	return copyRollupType('rollup.browser.d.ts');
}

export function copyNodeTypes(): Plugin[] {
	return [
		copyRollupType('rollup.d.ts'),
		{
			async generateBundle(_options, _bundle, isWrite) {
				if (isWrite) {
					this.emitFile({
						fileName: 'loadConfigFile.d.ts',
						source: (await readFile(resolve('cli/run/loadConfigFileType.d.ts'), 'utf8')).replace(
							'../../src/rollup/types',
							'./rollup'
						),
						type: 'asset'
					});
				}
			},
			name: 'copy-loadConfigFile-type'
		}
	];
}
Repository: rollup/rollup
File: src/utils/FileEmitter.ts (TypeScript)

Code before:
	}

	private finalizeAsset(
		consumedFile: Readonly<ConsumedAsset>,
		source: string | Uint8Array,
		referenceId: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = consumedFile.fileName;

		// Deduplicate assets if an explicit fileName is not provided
		if (!fileName) {
			const sourceHash = getSourceHash(source);
			fileName = fileNamesBySource.get(sourceHash);
			const newFileName = generateAssetFileName(
				consumedFile.name,
				source,
				sourceHash,
				outputOptions,
				bundle
			);
			// make sure file name deterministic in parallel emits, always use the shorter and smaller file name
			if (
				!fileName ||
				fileName.length > newFileName.length ||
				(fileName.length === newFileName.length && fileName.localeCompare(newFileName) > 0)
			) {
				if (fileName) {
					delete bundle[fileName];
				}
				fileName = newFileName;
				fileNamesBySource.set(sourceHash, fileName);
			}
		}

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

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


Review comment (bug):
Are you sure you want to use `localeCompare` here over just using `>`? My worry is that `localCompare` may
- make the output depend on the system language (in edge cases)
- can potentially determine two "different" strings to be the same, in which case again the output would not be deterministic (I think, though not 100% sure)

Code after:
	}

	private finalizeAsset(
		consumedFile: Readonly<ConsumedAsset>,
		source: string | Uint8Array,
		referenceId: string,
		{ bundle, fileNamesBySource, outputOptions }: FileEmitterOutput
	): void {
		let fileName = consumedFile.fileName;

		// Deduplicate assets if an explicit fileName is not provided
		if (!fileName) {
			const sourceHash = getSourceHash(source);
			fileName = fileNamesBySource.get(sourceHash);
			const newFileName = generateAssetFileName(
				consumedFile.name,
				source,
				sourceHash,
				outputOptions,
				bundle
			);
			// make sure file name deterministic in parallel emits, always use the shorter and smaller file name
			if (
				!fileName ||
				fileName.length > newFileName.length ||
				(fileName.length === newFileName.length && fileName > newFileName)
			) {
				if (fileName) {
					delete bundle[fileName];
				}
				fileName = newFileName;
				fileNamesBySource.set(sourceHash, fileName);
			}
		}

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

		bundle[fileName] = {
			fileName,
			name: consumedFile.name,
			needsCodeReference: consumedFile.needsCodeReference,
			source,
			type: 'asset'
		};
	}
}
Repository: rollup/rollup
File: test/chunking-form/samples/emit-file/deduplicate-assets/_config.js (JavaScript)

Code before:
function asyncify(callback) {
	const time = Math.round(Math.random() * 100);

	return new Promise(resolve => {
		setTimeout(() => {
			callback();
			resolve();
		}, time);
	});
}
module.exports = {
	description: 'deduplicates asset that have the same source',
	solo: true,
	options: {
		input: ['main.js'],
		plugins: {
			async buildStart() {
				await Promise.all([
					asyncify(() => this.emitFile({ type: 'asset', name: 'string1.txt', source: 'string' })),
					asyncify(() => this.emitFile({ type: 'asset', name: 'string2.txt', source: 'string' })),
					asyncify(() =>
						this.emitFile({ type: 'asset', name: 'stringSameSource.txt', source: 'string' })
					),
					asyncify(() =>
						this.emitFile({
							type: 'asset',

Review comment (suggestion):
Instead of making this test random, which could potentially mean that the test is sometimes red and sometimes green but in an indeterministic way, why not work with e.g. fixed numbers as names but in a random order?
e.g. emit 1.txt, 2.txt (same source as 1.txt), 4.txt, 3.txt (same source as 4.txt) and assert that you get 1.txt and 3.txt as output (or 2 and 4, but I hope you see what I mean). Would also make the test nearly 100ms faster 😉

Code after:
module.exports = {
	description: 'deduplicates asset that have the same source',
	options: {
		input: ['main.js'],
		plugins: {
			buildStart() {
				// emit 'string' source in a random order
				this.emitFile({ type: 'asset', name: 'stringSameSource.txt', source: 'string' });
				this.emitFile({ type: 'asset', name: 'string2.txt', source: 'string' });
				this.emitFile({ type: 'asset', name: 'string1.txt', source: 'string' });
				this.emitFile({
					type: 'asset',
					name: 'sameStringAsBuffer.txt',
					source: Buffer.from('string') // Test cross Buffer/string deduplication
				});

				// Different string source
				this.emitFile({ type: 'asset', name: 'otherString.txt', source: 'otherString' });

				const bufferSource = () => Buffer.from([0x62, 0x75, 0x66, 0x66, 0x65, 0x72]);
				this.emitFile({
					type: 'asset',
					name: 'buffer.txt',
					source: bufferSource()
				});
				this.emitFile({