ronantakizawa/codereview-bench huggingface dataset:
Repository: parcel-bundler/parcel
File: packages/transformers/js/core/src/collect.rs (Rust)

Code before:
      is_esm: collect.is_esm,
    }
  }
}

impl Visit for Collect {
  fn visit_module(&mut self, node: &Module) {
    self.in_module_this = true;
    self.in_top_level = true;
    self.in_function = false;
    // Visit all imports first so that all imports are known when collecting used_imports
    for n in &node.body {
      if n.is_module_decl() {
        n.visit_with(self);
      }
    }
    for n in &node.body {
      if !n.is_module_decl() {
        n.visit_with(self);
      }
    }
    self.in_module_this = false;

    let this_exprs = std::mem::take(&mut self.this_exprs);
    for (_key, (ident, span)) in this_exprs {
      // for (_key, (ident, span)) in self.this_exprs.clone() {
      if self.exports.contains_key(&ident.sym) {
        self.should_wrap = true;
        self.add_bailout(span, BailoutReason::ThisInExport);
      }
    }

    if let Some(bailouts) = &mut self.bailouts {
      for (key, Import { specifier, .. }) in &self.imports {
        if specifier == "*" {
          if let Some(spans) = self.non_static_access.get(key) {
            for span in spans {
              bailouts.push(Bailout {
                loc: SourceLocation::from(&self.source_map, *span),
                reason: BailoutReason::NonStaticAccess,
              })
            }
          }
        }
      }

      bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap());
    }
  }

  collect_visit_fn!(visit_function, Function);

Review comment (suggestion):
```suggestion
    for (_key, (ident, span)) in std::mem::take(&mut self.this_exprs) {
```

Code after:
      is_esm: collect.is_esm,
    }
  }
}

impl Visit for Collect {
  fn visit_module(&mut self, node: &Module) {
    self.in_module_this = true;
    self.in_top_level = true;
    self.in_function = false;
    // Visit all imports first so that all imports are known when collecting used_imports
    for n in &node.body {
      if n.is_module_decl() {
        n.visit_with(self);
      }
    }
    for n in &node.body {
      if !n.is_module_decl() {
        n.visit_with(self);
      }
    }
    self.in_module_this = false;

    for (_key, (ident, span)) in std::mem::take(&mut self.this_exprs) {
      if self.exports.contains_key(&ident.sym) {
        self.should_wrap = true;
        self.add_bailout(span, BailoutReason::ThisInExport);
      }
    }

    if let Some(bailouts) = &mut self.bailouts {
      for (key, Import { specifier, .. }) in &self.imports {
        if specifier == "*" {
          if let Some(spans) = self.non_static_access.get(key) {
            for span in spans {
              bailouts.push(Bailout {
                loc: SourceLocation::from(&self.source_map, *span),
                reason: BailoutReason::NonStaticAccess,
              })
            }
          }
        }
      }

      bailouts.sort_by(|a, b| a.loc.partial_cmp(&b.loc).unwrap());
    }
  }

  collect_visit_fn!(visit_function, Function);
  collect_visit_fn!(visit_getter_prop, GetterProp);
  collect_visit_fn!(visit_setter_prop, SetterProp);
Repository: parcel-bundler/parcel
File: packages/core/graph/src/Graph.js (JavaScript)

Code before:
    startNodeId: ?NodeId,
  ): ?TContext {
    let traversalStartNode = nullthrows(
      startNodeId ?? this.rootNodeId,
      'A start node is required to traverse',
    );
    this._assertHasNodeId(traversalStartNode);

    let visited;
    if (!this._visited || this._visited.capacity < this.nodes.length) {
      this._visited = new BitSet(this.nodes.length);
      visited = this._visited;
    } else {
      visited = this._visited;
      visited.clear();
    }
    this._visited = null;

    let stopped = false;
    let skipped = false;
    // $FlowFixMe[prop-missing]: skipChildren() doesn't apply here since this is a post-order traversal
    let actions: TraversalActions = {
      stop() {
        stopped = true;
      },
    };

    let queue = [{nodeId: traversalStartNode, context: null}];
    while (queue.length !== 0) {
      let {nodeId, context} = queue[queue.length - 1];

      if (!visited.has(nodeId)) {
        visited.add(nodeId);

        if (!skipped) {
          this.adjacencyList.forEachNodeIdConnectedFromReverse(
            nodeId,
            child => {
              if (!visited.has(child)) {
                queue.push({
                  nodeId: child,
                  context,
                });
              }
              return false;
            },
          );
        }
      } else {
        queue.pop();
        let newContext = visit(nodeId, context, actions);

Review comment (suggestion):
```suggestion
    let actions: TraversalActions = {
      stop() {
        stopped = true;
      },
      skipChildren() {
      	throw new Error("Calling skipChildren inside a post-order traversal is not allowed");
      }
    };
```

Code after:
    startNodeId: ?NodeId,
  ): void {
    let traversalStartNode = nullthrows(
      startNodeId ?? this.rootNodeId,
      'A start node is required to traverse',
    );
    this._assertHasNodeId(traversalStartNode);

    let visited;
    if (!this._visited || this._visited.capacity < this.nodes.length) {
      this._visited = new BitSet(this.nodes.length);
      visited = this._visited;
    } else {
      visited = this._visited;
      visited.clear();
    }
    this._visited = null;

    let stopped = false;
    let actions: TraversalActions = {
      stop() {
        stopped = true;
      },
      skipChildren() {
        throw new Error(
          'Calling skipChildren inside a post-order traversal is not allowed',
        );
      },
    };

    let queue = [traversalStartNode];
    while (queue.length !== 0) {
      let nodeId = queue[queue.length - 1];

      if (!visited.has(nodeId)) {
        visited.add(nodeId);

        this.adjacencyList.forEachNodeIdConnectedFromReverse(nodeId, child => {
          if (!visited.has(child)) {
            queue.push(child);
          }
          return false;
        });
      } else {
        queue.pop();
        visit(nodeId, null, actions);

        if (stopped) {
          this._visited = visited;
          return;
        }
Repository: parcel-bundler/parcel
File: packages/core/core/src/Parcel.js (JavaScript)

Code before:

    this.#initialized = true;
  }

  async run(): Promise<BuildSuccessEvent> {
    let startTime = Date.now();
    if (!this.#initialized) {
      await this._init();
    }

    let result = await this._build({startTime});
    await this._end();

    if (result.type === 'buildFailure') {
      throw new BuildError(result.diagnostics);
    }

    return result;
  }

  async _end(): Promise<void> {
    this.#initialized = false;

    await Promise.all([
      await this.#requestTracker.writeToCache(),
      this.#disposable.dispose(),
    ]);
  }

  async _startNextBuild(): Promise<?BuildEvent> {
    this.#watchAbortController = new AbortController();
    await this.#farm.callAllWorkers('clearConfigCache', []);

    try {
      let buildEvent = await this._build({
        signal: this.#watchAbortController.signal,
      });

      this.#watchEvents.emit({
        buildEvent,
      });

      return buildEvent;
    } catch (err) {
      // Ignore BuildAbortErrors and only emit critical errors.
      if (!(err instanceof BuildAbortError)) {
        throw err;
      }
    }
  }


Review comment (performance):
We can't dispose the reporter yet because we're sending events in the `writeToCache` function now so I had to change the order here

Code after:

    this.#initialized = true;
  }

  async run(): Promise<BuildSuccessEvent> {
    let startTime = Date.now();
    if (!this.#initialized) {
      await this._init();
    }

    let result = await this._build({startTime});
    await this._end();

    if (result.type === 'buildFailure') {
      throw new BuildError(result.diagnostics);
    }

    return result;
  }

  async _end(): Promise<void> {
    this.#initialized = false;

    await this.#requestTracker.writeToCache();
    await this.#disposable.dispose();
  }

  async _startNextBuild(): Promise<?BuildEvent> {
    this.#watchAbortController = new AbortController();
    await this.#farm.callAllWorkers('clearConfigCache', []);

    try {
      let buildEvent = await this._build({
        signal: this.#watchAbortController.signal,
      });

      this.#watchEvents.emit({
        buildEvent,
      });

      return buildEvent;
    } catch (err) {
      // Ignore BuildAbortErrors and only emit critical errors.
      if (!(err instanceof BuildAbortError)) {
        throw err;
      }
    }
  }

  async watch(
    cb?: (err: ?Error, buildEvent?: BuildEvent) => mixed,
Repository: parcel-bundler/parcel
File: packages/transformers/posthtml/src/PostHTMLTransformer.js (JavaScript)

Code before:
      type: 'posthtml',
      version: '0.4.1',
      program: parse(await asset.getCode(), {
        lowerCaseAttributeNames: true,
        sourceLocations: true,
        xmlMode: asset.type === 'xhtml',
      }),
    };
  },

  async transform({asset, config}) {
    if (!config) {
      return [asset];
    }

    let ast = nullthrows(await asset.getAST());
    let res = await posthtml(config.plugins).process(ast.program, {
      ...config,
      plugins: config.plugins,
    });

    for (let {type, file: filePath} of res.messages) {
      if (type === 'dependency') {
        asset.invalidateOnFileChange(filePath);
      }
    }

    asset.setAST({
      type: 'posthtml',
      version: '0.4.1',
      program: JSON.parse(JSON.stringify(res.tree)), // posthtml adds functions to the AST that are not serializable
    });

    return [asset];
  },

  generate({ast, asset}) {
    return {
      content: render(ast.program, {
        closingSingleTag: asset.type === 'xhtml' ? 'slash' : undefined,
      }),
    };
  },
}): Transformer);


Review comment (suggestion):
Let's keep the `if`, not sure if `messages` might be unset sometimes
```suggestion
    if (res.messages) {
      for (let {type, file: filePath} of res.messages) {
        if (type === 'dependency') {
          asset.invalidateOnFileChange(filePath);
        }
      }
    }
```

Code after:
      type: 'posthtml',
      version: '0.4.1',
      program: parse(await asset.getCode(), {
        lowerCaseAttributeNames: true,
        sourceLocations: true,
        xmlMode: asset.type === 'xhtml',
      }),
    };
  },

  async transform({asset, config}) {
    if (!config) {
      return [asset];
    }

    let ast = nullthrows(await asset.getAST());
    let res = await posthtml(config.plugins).process(ast.program, {
      ...config,
      plugins: config.plugins,
    });

    if (res.messages) {
      for (let {type, file: filePath} of res.messages) {
        if (type === 'dependency') {
          asset.invalidateOnFileChange(filePath);
        }
      }
    }

    asset.setAST({
      type: 'posthtml',
      version: '0.4.1',
      program: JSON.parse(JSON.stringify(res.tree)), // posthtml adds functions to the AST that are not serializable
    });

    return [asset];
  },

  generate({ast, asset}) {
    return {
      content: render(ast.program, {
        closingSingleTag: asset.type === 'xhtml' ? 'slash' : undefined,
      }),
    };
  },
}): Transformer);
Repository: parcel-bundler/parcel
File: packages/core/core/src/RequestTracker.js (JavaScript)

Code before:
type RequestNode = {|
  id: ContentKey,
  +type: 'request',
  +requestType?: RequestType,
  invalidateReason: InvalidateReason,
  result?: mixed,
  resultCacheKey?: ?string,
  hash?: string,
|};

export type RequestType =
  | 'parcel_build_request'
  | 'bundle_graph_request'
  | 'asset_graph_request'
  | 'entry_request'
  | 'target_request'
  | 'parcel_config_request'
  | 'path_request'
  | 'dev_dep_request'
  | 'asset_request'
  | 'config_request'
  | 'write_bundles_request'
  | 'package_request'
  | 'write_bundle_request'
  | 'validation_request'
  | string;

type RequestGraphNode =
  | RequestNode
  | FileNode
  | GlobNode
  | FileNameNode
  | EnvNode
  | OptionNode;

export type RunAPI<TResult> = {|
  invalidateOnFileCreate: InternalFileCreateInvalidation => void,
  invalidateOnFileDelete: ProjectPath => void,
  invalidateOnFileUpdate: ProjectPath => void,
  invalidateOnStartup: () => void,
  invalidateOnBuild: () => void,
  invalidateOnEnvChange: string => void,
  invalidateOnOptionChange: string => void,
  getInvalidations(): Array<RequestInvalidation>,
  storeResult(result: TResult, cacheKey?: string): void,
  getRequestResult<T>(contentKey: ContentKey): Async<?T>,
  getPreviousResult<T>(ifMatch?: string): Async<?T>,
  getSubRequests(): Array<RequestNode>,
  getInvalidSubRequests(): Array<RequestNode>,
  canSkipSubrequest(ContentKey): boolean,
  runRequest: <TInput, TResult>(

Review comment (suggestion):
I'm iffy about this `string` here. I added it to appease Flow because we use a `mock_request` type in the RequestTracker's unit tests, but I wonder if I should just flow-ignore those instead.

Code after:
  invalidateReason: InvalidateReason,
  result?: mixed,
  resultCacheKey?: ?string,
  hash?: string,
|};

type RequestType =
  | 'parcel_build_request'
  | 'bundle_graph_request'
  | 'asset_graph_request'
  | 'entry_request'
  | 'target_request'
  | 'parcel_config_request'
  | 'path_request'
  | 'dev_dep_request'
  | 'asset_request'
  | 'config_request'
  | 'write_bundles_request'
  | 'package_request'
  | 'write_bundle_request'
  | 'validation_request';

type RequestGraphNode =
  | RequestNode
  | FileNode
  | GlobNode
  | FileNameNode
  | EnvNode
  | OptionNode;

export type RunAPI<TResult> = {|
  invalidateOnFileCreate: InternalFileCreateInvalidation => void,
  invalidateOnFileDelete: ProjectPath => void,
  invalidateOnFileUpdate: ProjectPath => void,
  invalidateOnStartup: () => void,
  invalidateOnBuild: () => void,
  invalidateOnEnvChange: string => void,
  invalidateOnOptionChange: string => void,
  getInvalidations(): Array<RequestInvalidation>,
  storeResult(result: TResult, cacheKey?: string): void,
  getRequestResult<T>(contentKey: ContentKey): Async<?T>,
  getPreviousResult<T>(ifMatch?: string): Async<?T>,
  getSubRequests(): Array<RequestNode>,
  getInvalidSubRequests(): Array<RequestNode>,
  canSkipSubrequest(ContentKey): boolean,
  runRequest: <TInput, TResult>(
    subRequest: Request<TInput, TResult>,
    opts?: RunRequestOpts,
  ) => Promise<TResult>,
|};
Repository: parcel-bundler/parcel
File: packages/core/core/src/serializer.js (JavaScript)

Code before:
// @flow
import {createBuildCache} from './buildCache';
import {serializeRaw, deserializeRaw} from './serializerCore';

export {serializeRaw, deserializeRaw} from './serializerCore';

const nameToCtor: Map<string, Class<any>> = new Map();
export const ctorToName: Map<Class<any>, string> = new Map();

export function registerSerializableClass(name: string, ctor: Class<any>) {
  if (ctorToName.has(ctor)) {
    throw new Error('Class already registered with serializer');
  }

  nameToCtor.set(name, ctor);
  ctorToName.set(ctor, name);
}

export function unregisterSerializableClass(name: string, ctor: Class<any>) {
  if (nameToCtor.get(name) === ctor) {
    nameToCtor.delete(name);
  }

  if (ctorToName.get(ctor) === name) {
    ctorToName.delete(ctor);
  }
}

function shallowCopy(object: any) {
  if (object && typeof object === 'object') {
    if (Array.isArray(object)) {
      return [...object];
    }

Review comment (suggestion):
```suggestion
const ctorToName: Map<Class<any>, string> = new Map();
```

don't need to export that anymore

Code after:
// @flow
import {createBuildCache} from './buildCache';
import {serializeRaw, deserializeRaw} from './serializerCore';

export {serializeRaw, deserializeRaw} from './serializerCore';

const nameToCtor: Map<string, Class<any>> = new Map();
const ctorToName: Map<Class<any>, string> = new Map();

export function registerSerializableClass(name: string, ctor: Class<any>) {
  if (ctorToName.has(ctor)) {
    throw new Error('Class already registered with serializer');
  }

  nameToCtor.set(name, ctor);
  ctorToName.set(ctor, name);
}

export function unregisterSerializableClass(name: string, ctor: Class<any>) {
  if (nameToCtor.get(name) === ctor) {
    nameToCtor.delete(name);
  }

  if (ctorToName.get(ctor) === name) {
    ctorToName.delete(ctor);
  }
}

function shallowCopy(object: any) {
  if (object && typeof object === 'object') {
    if (Array.isArray(object)) {
      return [...object];
    }
Repository: parcel-bundler/parcel
File: packages/core/core/src/PackagerRunner.js (JavaScript)

Code before:
          pluginName,
          this.options,
        );
      }
    }

    let devDepHashes = await this.getDevDepHashes(bundle);
    for (let inlineBundle of bundleGraph.getInlineBundles(bundle)) {
      devDepHashes += await this.getDevDepHashes(inlineBundle);
    }

    let invalidationHash = await getInvalidationHash(
      invalidations,
      this.options,
    );

    return hashString(
      PARCEL_VERSION +
        devDepHashes +
        invalidationHash +
        bundle.target.publicUrl +
        bundleGraph.getHash(bundle) +
        JSON.stringify(configResults) +
        JSON.stringify(globalInfoResults) +
        this.options.mode +
        `${this.options.shouldBuildLazily ? 'lazy' : 'eager'}`,
    );
  }

  async getDevDepHashes(bundle: InternalBundle): Promise<string> {
    let name = nullthrows(bundle.name);
    let packager = await this.config.getPackager(name);
    let optimizers = await this.config.getOptimizers(name);

    let key = `${packager.name}:${fromProjectPathRelative(
      packager.resolveFrom,
    )}`;
    let devDepHashes =
      this.devDepRequests.get(key)?.hash ?? this.previousDevDeps.get(key) ?? '';
    for (let {name, resolveFrom} of optimizers) {
      let key = `${name}:${fromProjectPathRelative(resolveFrom)}`;
      devDepHashes +=
        this.devDepRequests.get(key)?.hash ??
        this.previousDevDeps.get(key) ??
        '';
    }

    return devDepHashes;
  }

  async readFromCache(cacheKey: string): Promise<?{|

Review comment (suggestion):
```suggestion
        this.options.shouldBuildLazily ? 'lazy' : 'eager',
```

Code after:
          pluginName,
          this.options,
        );
      }
    }

    let devDepHashes = await this.getDevDepHashes(bundle);
    for (let inlineBundle of bundleGraph.getInlineBundles(bundle)) {
      devDepHashes += await this.getDevDepHashes(inlineBundle);
    }

    let invalidationHash = await getInvalidationHash(
      invalidations,
      this.options,
    );

    return hashString(
      PARCEL_VERSION +
        devDepHashes +
        invalidationHash +
        bundle.target.publicUrl +
        bundleGraph.getHash(bundle) +
        JSON.stringify(configResults) +
        JSON.stringify(globalInfoResults) +
        this.options.mode +
        (this.options.shouldBuildLazily ? 'lazy' : 'eager'),
    );
  }

  async getDevDepHashes(bundle: InternalBundle): Promise<string> {
    let name = nullthrows(bundle.name);
    let packager = await this.config.getPackager(name);
    let optimizers = await this.config.getOptimizers(name);

    let key = `${packager.name}:${fromProjectPathRelative(
      packager.resolveFrom,
    )}`;
    let devDepHashes =
      this.devDepRequests.get(key)?.hash ?? this.previousDevDeps.get(key) ?? '';
    for (let {name, resolveFrom} of optimizers) {
      let key = `${name}:${fromProjectPathRelative(resolveFrom)}`;
      devDepHashes +=
        this.devDepRequests.get(key)?.hash ??
        this.previousDevDeps.get(key) ??
        '';
    }

    return devDepHashes;
  }

  async readFromCache(cacheKey: string): Promise<?{|
Repository: parcel-bundler/parcel
File: packages/core/core/src/InternalConfig.js (JavaScript)

Code before:
// @flow strict-local

import type {PackageName, ConfigResult} from '@parcel/types';
import type {
  Config,
  Environment,
  InternalFileCreateInvalidation,
  InternalDevDepOptions,
} from './types';
import type {ProjectPath} from './projectPath';

import {fromProjectPathRelative} from './projectPath';
import {createEnvironment} from './Environment';
import {hashString} from '@parcel/rust';

type ConfigOpts = {|
  plugin: PackageName,
  searchPath: ProjectPath,
  isSource?: boolean,
  env?: Environment,
  result?: ConfigResult,
  invalidateOnFileChange?: Set<ProjectPath>,
  invalidateOnPackageKeyChange?: Array<{|
    filePath: ProjectPath,
    packageKey: string,
  |}>,
  invalidateOnFileCreate?: Array<InternalFileCreateInvalidation>,
  invalidateOnEnvChange?: Set<string>,
  invalidateOnOptionChange?: Set<string>,
  devDeps?: Array<InternalDevDepOptions>,
  invalidateOnStartup?: boolean,
  invalidateOnBuild?: boolean,
|};

export function createConfig({
  plugin,
  isSource,
  searchPath,
  env,
  result,
  invalidateOnFileChange,
  invalidateOnPackageKeyChange,
  invalidateOnFileCreate,
  invalidateOnEnvChange,
  invalidateOnOptionChange,
  devDeps,
  invalidateOnStartup,
  invalidateOnBuild,

Review comment (security):
Would it make sense to make the invalidation part less “package key” specific and more generic “config key”? I’m thinking for plugin authors it could apply to other config files as well? The current implementation of `pacakgeKey` in getConfigFrom assumes `pacakge.json`, but the invalidation at least at the moment supports generic JSON, and could support other formats.

Code after:
// @flow strict-local

import type {PackageName, ConfigResult} from '@parcel/types';
import type {
  Config,
  Environment,
  InternalFileCreateInvalidation,
  InternalDevDepOptions,
} from './types';
import type {ProjectPath} from './projectPath';

import {fromProjectPathRelative} from './projectPath';
import {createEnvironment} from './Environment';
import {hashString} from '@parcel/rust';

type ConfigOpts = {|
  plugin: PackageName,
  searchPath: ProjectPath,
  isSource?: boolean,
  env?: Environment,
  result?: ConfigResult,
  invalidateOnFileChange?: Set<ProjectPath>,
  invalidateOnConfigKeyChange?: Array<{|
    filePath: ProjectPath,
    configKey: string,
  |}>,
  invalidateOnFileCreate?: Array<InternalFileCreateInvalidation>,
  invalidateOnEnvChange?: Set<string>,
  invalidateOnOptionChange?: Set<string>,
  devDeps?: Array<InternalDevDepOptions>,
  invalidateOnStartup?: boolean,
  invalidateOnBuild?: boolean,
|};

export function createConfig({
  plugin,
  isSource,
  searchPath,
  env,
  result,
  invalidateOnFileChange,
  invalidateOnConfigKeyChange,
  invalidateOnFileCreate,
  invalidateOnEnvChange,
  invalidateOnOptionChange,
  devDeps,
  invalidateOnStartup,
  invalidateOnBuild,