ronantakizawa/codereview-bench huggingface dataset:
Repository: apple/pkl
File: pkl-codegen-java/src/main/kotlin/org/pkl/codegen/java/CliJavaCodeGeneratorOptions.kt (Kotlin)

Code before:
  val paramsAnnotation: String? = null,

  /**
   * Fully qualified name of the annotation to use on non-null properties. If this option is not
   * set, [org.pkl.config.java.mapper.NonNull] will be used.
   */
  val nonNullAnnotation: String? = null,

  /** Whether to make generated classes implement [java.io.Serializable] */
  val implementSerializable: Boolean = false,

  /**
   * A rename mapping for class names.
   *
   * When you need to have Java class or package names different from the default names derived from
   * Pkl module names, you can define a rename mapping, where the key is a prefix of the original
   * Pkl module name, and the value is the desired replacement.
   */
  val renames: Map<String, String> = emptyMap()
) {
  fun toJavaCodegenOptions() =
    JavaCodegenOptions(
      indent,
      generateGetters,
      generateJavadoc,
      generateSpringBootConfig,
      paramsAnnotation,
      nonNullAnnotation,
      implementSerializable,
      renames
    )
}


Review comment (suggestion):
Note: we should apply the same deprecation to `org.pkl.codegen.kotlin.CliKotlinCodeGeneratorOptions#toKotlinCodegenOptions`

Code after:
  val paramsAnnotation: String? = null,

  /**
   * Fully qualified name of the annotation to use on non-null properties. If this option is not
   * set, [org.pkl.config.java.mapper.NonNull] will be used.
   */
  val nonNullAnnotation: String? = null,

  /** Whether to make generated classes implement [java.io.Serializable] */
  val implementSerializable: Boolean = false,

  /**
   * A rename mapping for class names.
   *
   * When you need to have Java class or package names different from the default names derived from
   * Pkl module names, you can define a rename mapping, where the key is a prefix of the original
   * Pkl module name, and the value is the desired replacement.
   */
  val renames: Map<String, String> = emptyMap()
) {
  @Suppress("DeprecatedCallableAddReplaceWith")
  @Deprecated("deprecated without replacement")
  fun toJavaCodegenOptions() = toJavaCodeGeneratorOptions()

  internal fun toJavaCodeGeneratorOptions() =
    JavaCodeGeneratorOptions(
      indent,
      generateGetters,
      generateJavadoc,
      generateSpringBootConfig,
      paramsAnnotation,
      nonNullAnnotation,
      implementSerializable,
      renames
    )
}
Repository: apple/pkl
File: pkl-core/src/main/java/org/pkl/core/stdlib/math/MathNodes.java (Java)

Code before:
      return StrictMath.acos(x);
    }
  }

  public abstract static class atan extends ExternalMethod1Node {
    @Specialization
    protected double eval(VmTyped self, long x) {
      return StrictMath.atan(x);
    }

    @Specialization
    protected double eval(VmTyped self, double x) {
      return StrictMath.atan(x);
    }
  }

  public abstract static class atan2 extends ExternalMethod2Node {
    @Specialization
    protected double eval(VmTyped self, long x, long y) {
      return StrictMath.atan2(x, y);
    }

    @Specialization
    protected double eval(VmTyped self, long x, double y) {
      return StrictMath.atan2(x, y);
    }
  }

  public abstract static class gcd extends ExternalMethod2Node {
    @TruffleBoundary
    @Specialization
    protected long eval(VmTyped self, long x, long y) {
      VmUtils.checkPositive(x);
      VmUtils.checkPositive(y);

      return MathUtils.gcd(x, y);
    }
  }

  public abstract static class lcm extends ExternalMethod2Node {
    @TruffleBoundary
    @Specialization
    protected long eval(VmTyped self, long x, long y) {
      VmUtils.checkPositive(x);
      VmUtils.checkPositive(y);

      if (x == 0 || y == 0) return 0;

      return VmSafeMath.abs(VmSafeMath.multiply(x / MathUtils.gcd(x, y), y));
    }
  }

Review comment (suggestion):
Need to handle all permutations of `long` and `double` here:

```suggestion
    }

    @Specialization
    protected double eval(VmTyped self, double x, double y) {
      return StrictMath.atan2(x, y);
    }

    @Specialization
    protected double eval(VmTyped self, double x, long y) {
      return StrictMath.atan2(x, y);
    }
```

Code after:
      return StrictMath.acos(x);
    }
  }

  public abstract static class atan extends ExternalMethod1Node {
    @Specialization
    protected double eval(VmTyped self, long x) {
      return StrictMath.atan(x);
    }

    @Specialization
    protected double eval(VmTyped self, double x) {
      return StrictMath.atan(x);
    }
  }

  public abstract static class atan2 extends ExternalMethod2Node {
    @Specialization
    protected double eval(VmTyped self, long x, long y) {
      return StrictMath.atan2(x, y);
    }

    @Specialization
    protected double eval(VmTyped self, long x, double y) {
      return StrictMath.atan2(x, y);
    }

    @Specialization
    protected double eval(VmTyped self, double x, double y) {
      return StrictMath.atan2(x, y);
    }

    @Specialization
    protected double eval(VmTyped self, double x, long y) {
      return StrictMath.atan2(x, y);
    }
  }

  public abstract static class gcd extends ExternalMethod2Node {
    @TruffleBoundary
    @Specialization
    protected long eval(VmTyped self, long x, long y) {
      VmUtils.checkPositive(x);
      VmUtils.checkPositive(y);

      return MathUtils.gcd(x, y);
    }
  }

  public abstract static class lcm extends ExternalMethod2Node {
    @TruffleBoundary
Repository: apple/pkl
File: pkl-cli/src/test/kotlin/org/pkl/cli/CliTestRunnerTest.kt (Kotlin)

Code before:
        }
        """
            .trimIndent()
    val input = tempDir.resolve("test.pkl").writeString(code).toString()
    val out = StringWriter()
    val err = StringWriter()
    val opts = CliBaseOptions(sourceModules = listOf(input.toUri()), settings = URI("pkl:settings"))
    val testOpts = CliTestOptions()
    val runner = CliTestRunner(opts, testOpts, consoleWriter = out, errWriter = err)
    runner.run()

    assertThat(out.toString().stripFileAndLines(tempDir))
      .isEqualTo(
        """
            module test
              facts
                ✔ localeTest

            100.0% tests pass [1 passed], 100.0% asserts pass [1 passed]

            """
          .trimIndent()
      )
    assertThat(err.toString()).isEqualTo("")

    Locale.setDefault(originalLocale)
  }

  private fun String.stripFileAndLines(tmpDir: Path): String {
    // handle platform differences in handling of file URIs
    // (file:/// on *nix vs. file:/ on Windows)
    return replace(tmpDir.toFile().toURI().toString(), "/tempDir/")
      .replace(tmpDir.toUri().toString(), "/tempDir/")
      .replace(Regex("line \\d+"), "line xx")
  }

  private fun noopWriter(): Writer =
    object : Writer() {
      override fun close() {}

      override fun flush() {}

      override fun write(cbuf: CharArray, off: Int, len: Int) {}
    }
}


Review comment (bug):
These assertions will throw errors, which means that `Locale.setDefault(...)` won't get called if any of these tests fail.

Let's put lines 496 through 527 in a `try/finally`, where `Locale.setDefault` is in the finally so we can ensure that this test cleans up the state that it's mucking with

Code after:
            }
        }
        """
          .trimIndent()
      val input = tempDir.resolve("test.pkl").writeString(code).toString()
      val out = StringWriter()
      val err = StringWriter()
      val opts =
        CliBaseOptions(sourceModules = listOf(input.toUri()), settings = URI("pkl:settings"))
      val testOpts = CliTestOptions()
      val runner = CliTestRunner(opts, testOpts, consoleWriter = out, errWriter = err)
      runner.run()

      assertThat(out.toString().stripFileAndLines(tempDir))
        .isEqualTo(
          """
            module test
              facts
                ✔ localeTest

            100.0% tests pass [1 passed], 100.0% asserts pass [1 passed]

            """
            .trimIndent()
        )
      assertThat(err.toString()).isEqualTo("")
    } finally {
      Locale.setDefault(originalLocale)
    }
  }

  private fun String.stripFileAndLines(tmpDir: Path): String {
    // handle platform differences in handling of file URIs
    // (file:/// on *nix vs. file:/ on Windows)
    return replace(tmpDir.toFile().toURI().toString(), "/tempDir/")
      .replace(tmpDir.toUri().toString(), "/tempDir/")
      .replace(Regex("line \\d+"), "line xx")
  }

  private fun noopWriter(): Writer =
    object : Writer() {
      override fun close() {}

      override fun flush() {}

      override fun write(cbuf: CharArray, off: Int, len: Int) {}
    }
}
Repository: apple/pkl
File: pkl-cli/src/test/kotlin/org/pkl/cli/CliMainTest.kt (Kotlin)

Code before:
      .hasMessage(error)

    assertThatCode { rootCmd.parse(arrayOf("eval", "-m", testOut, "-o", "/tmp/test", testIn)) }
      .hasMessage(error)
  }

  @Test
  fun `showing version works`() {
    assertThatCode { rootCmd.parse(arrayOf("--version")) }.hasMessage(Release.current().versionInfo)
  }

  @Test
  fun `file paths get parsed into URIs`(@TempDir tempDir: Path) {
    val cmd = RootCommand()
    cmd.parse(arrayOf("eval", makeInput(tempDir, "my file.txt")))

    val evalCmd = cmd.registeredSubcommands().filterIsInstance<EvalCommand>().first()
    val modules = evalCmd.baseOptions.baseOptions(evalCmd.modules).normalizedSourceModules
    assertThat(modules).hasSize(1)
    assertThat(modules[0].path).endsWith("my file.txt")
  }

  @Test
  fun `invalid URIs are not accepted`() {
    val ex =
      assertThrows<BadParameterValue> { RootCommand().parse(arrayOf("eval", "file:my file.txt")) }

    assertThat(ex.message).contains("URI `file:my file.txt` has invalid syntax")
  }

  private fun makeInput(tempDir: Path, fileName: String = "test.pkl"): String {
    val code = "x = 1"
    return tempDir.resolve(fileName).writeString(code).toString()
  }

  private fun makeSymdir(baseDir: Path, name: String, linkName: String): Path {
    val dir = baseDir.resolve(name)
    val link = baseDir.resolve(linkName)
    dir.createDirectory()
    link.createSymbolicLinkPointingTo(dir)
    return link
  }
}


Review comment (suggestion):
```suggestion
      assertThrows<BadParameterValue> { rootCmd.parse(arrayOf("eval", "file:my file.txt")) }
```
nit

Code after:
      .hasMessage(error)

    assertThatCode { rootCmd.parse(arrayOf("eval", "-m", testOut, "-o", "/tmp/test", testIn)) }
      .hasMessage(error)
  }

  @Test
  fun `showing version works`() {
    assertThatCode { rootCmd.parse(arrayOf("--version")) }.hasMessage(Release.current().versionInfo)
  }

  @Test
  fun `file paths get parsed into URIs`(@TempDir tempDir: Path) {
    val cmd = RootCommand()
    cmd.parse(arrayOf("eval", makeInput(tempDir, "my file.txt")))

    val evalCmd = cmd.registeredSubcommands().filterIsInstance<EvalCommand>().first()
    val modules = evalCmd.baseOptions.baseOptions(evalCmd.modules).normalizedSourceModules
    assertThat(modules).hasSize(1)
    assertThat(modules[0].path).endsWith("my file.txt")
  }

  @Test
  fun `invalid URIs are not accepted`() {
    val ex = assertThrows<BadParameterValue> { rootCmd.parse(arrayOf("eval", "file:my file.txt")) }

    assertThat(ex.message).contains("URI `file:my file.txt` has invalid syntax")
  }

  private fun makeInput(tempDir: Path, fileName: String = "test.pkl"): String {
    val code = "x = 1"
    return tempDir.resolve(fileName).writeString(code).toString()
  }

  private fun makeSymdir(baseDir: Path, name: String, linkName: String): Path {
    val dir = baseDir.resolve(name)
    val link = baseDir.resolve(linkName)
    dir.createDirectory()
    link.createSymbolicLinkPointingTo(dir)
    return link
  }
}
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/commands/RootCommand.kt (Kotlin)

Code before:
  override val printHelpOnEmptyArgs = true

  override fun helpEpilog(context: Context) = "For more information, visit $helpLink"

  init {
    context {
      suggestTypoCorrection = { given, possible ->
        if (!given.startsWith("-")) {
          registeredSubcommands().map { it.commandName }
        } else possible
      }
    }

    installCommonOptions()

    subcommands(
      EvalCommand(),
      ReplCommand(),
      ServerCommand(),
      TestCommand(),
      ProjectCommand(),
      DownloadPackageCommand(),
      AnalyzeCommand(),
      CompletionCommand(
        name = "completion",
        help = "Generate shell completion scripts",
        epilog = "For more information, visit $helpLink",
      ),
    )
  }
}


Review comment (bug):
This will be a pretty rarely used command, so, I'd prefer if this had a more specific name (in case we have something else called complete in the future).

I think `shell-completion` or `generate-completion` (the default name from Clikt) work.

Curious what @stackoverflow and @holzensp think

Code after:
  override val printHelpOnEmptyArgs = true

  override fun helpEpilog(context: Context) = "For more information, visit $helpLink"

  init {
    context {
      suggestTypoCorrection = { given, possible ->
        if (!given.startsWith("-")) {
          registeredSubcommands().map { it.commandName }
        } else possible
      }
    }

    installCommonOptions()

    subcommands(
      EvalCommand(),
      ReplCommand(),
      ServerCommand(),
      TestCommand(),
      ProjectCommand(),
      DownloadPackageCommand(),
      AnalyzeCommand(),
      CompletionCommand(
        name = "shell-completion",
        help = "Generate a completion script for the given shell",
        epilog = "For more information, visit $helpLink",
      ),
    )
  }
}
Repository: apple/pkl
File: pkl-cli/src/main/kotlin/org/pkl/cli/commands/RootCommand.kt (Kotlin)

Code before:
  override val printHelpOnEmptyArgs = true

  override fun helpEpilog(context: Context) = "For more information, visit $helpLink"

  init {
    context {
      suggestTypoCorrection = { given, possible ->
        if (!given.startsWith("-")) {
          registeredSubcommands().map { it.commandName }
        } else possible
      }
    }

    installCommonOptions()

    subcommands(
      EvalCommand(),
      ReplCommand(),
      ServerCommand(),
      TestCommand(),
      ProjectCommand(),
      DownloadPackageCommand(),
      AnalyzeCommand(),
      CompletionCommand(
        name = "shell-completion",
        help = "Generate shell completion scripts",
        epilog = "For more information, visit $helpLink",
      ),
    )
  }
}


Review comment (suggestion):
```suggestion
        help = "Generate a completion script for the given shell",
```

Code after:
  override val printHelpOnEmptyArgs = true

  override fun helpEpilog(context: Context) = "For more information, visit $helpLink"

  init {
    context {
      suggestTypoCorrection = { given, possible ->
        if (!given.startsWith("-")) {
          registeredSubcommands().map { it.commandName }
        } else possible
      }
    }

    installCommonOptions()

    subcommands(
      EvalCommand(),
      ReplCommand(),
      ServerCommand(),
      TestCommand(),
      ProjectCommand(),
      DownloadPackageCommand(),
      AnalyzeCommand(),
      CompletionCommand(
        name = "shell-completion",
        help = "Generate a completion script for the given shell",
        epilog = "For more information, visit $helpLink",
      ),
    )
  }
}
Repository: apple/pkl
File: pkl-cli/src/test/kotlin/org/pkl/cli/CliShellCompletionTest.kt (Kotlin)

Code before:
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package org.pkl.cli

import com.github.ajalt.clikt.testing.test
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import org.pkl.cli.commands.RootCommand

class CliShellCompletionTest {
  @Test
  fun `shell completion command supports required shells`() {
    val shellList = listOf("bash", "zsh", "fish")

    for (shell in shellList) {
      val command = RootCommand()
      val result = command.test("shell-completion $shell")

      assertThat(result.stdout).contains("Command completion for pkl")
    }
  }
}


Review comment (suggestion):
```suggestion
      val result = RootCommand().test("shell-completion $shell")
```
nit

Code after:
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package org.pkl.cli

import com.github.ajalt.clikt.testing.test
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import org.pkl.cli.commands.RootCommand

class CliShellCompletionTest {
  @Test
  fun `shell completion command supports required shells`() {
    val shellList = listOf("bash", "zsh", "fish")

    for (shell in shellList) {
      val result = RootCommand().test("shell-completion $shell")

      assertThat(result.stdout).contains("Command completion for pkl")
    }
  }
}
Repository: apple/pkl
File: pkl-gradle/src/main/java/org/pkl/gradle/spec/JavaCodeGenSpec.java (Java)

Code before:
/*
 * Copyright © 2024-2025 Apple Inc. and the Pkl project authors. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package org.pkl.gradle.spec;

import org.gradle.api.provider.Property;

/** Configuration options for Java code generators. Documented in user manual. */
public interface JavaCodeGenSpec extends CodeGenSpec {
  Property<Boolean> getGeneratedAnnotation();

  Property<Boolean> getGenerateGetters();

  Property<Boolean> getGenerateJavadoc();

  Property<String> getParamsAnnotation();

  Property<String> getNonNullAnnotation();
}


Review comment (refactor):
Can you move this to `CodeGenSpec` instead? This way, we can add this same flag to the kotlin code generator. And let's rename this to `getAddGeneratedAnnotation`. Same for `JavaCodeGenTask`.

Code after:
/*
 * Copyright © 2024-2025 Apple Inc. and the Pkl project authors. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     https://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
package org.pkl.gradle.spec;

import org.gradle.api.provider.Property;

/** Configuration options for Java code generators. Documented in user manual. */
public interface JavaCodeGenSpec extends CodeGenSpec {
  Property<Boolean> getGenerateGetters();

  Property<Boolean> getGenerateJavadoc();

  Property<String> getParamsAnnotation();

  Property<String> getNonNullAnnotation();
}
Repository: apple/pkl
File: pkl-parser/src/main/java/org/pkl/parser/Parser.java (Java)

Code before:
    res.add(parser.get());
    while (lookahead == separator) {
      next();
      res.add(parser.get());
    }
    return res;
  }

  private List<Parameter> parseListOfParameter(Token separator) {
    var res = new ArrayList<Parameter>();
    res.add(parseParameter());
    while (lookahead == separator) {
      next();
      res.add(parseParameter());
    }
    return res;
  }

  private ParserError parserError(String messageKey, Object... args) {
    return new ParserError(ErrorMessages.create(messageKey, args), spanLookahead);
  }

  private record MemberHeader(
      @Nullable DocComment docComment, List<Annotation> annotations, List<Modifier> modifiers) {
    boolean isNotEmpty() {
      return !(docComment == null && annotations.isEmpty() && modifiers.isEmpty());
    }

    @SuppressWarnings("DataFlowIssue")
    @Nullable
    Span span() {
      return span(null);
    }

    Span span(Span or) {
      if (docComment != null) {
        return docComment.span();
      }
      if (!annotations().isEmpty()) {
        return annotations.get(0).span();
      }
      if (!modifiers().isEmpty()) {
        return modifiers.get(0).span();
      }
      return or;
    }

    Span modifierSpan(Span or) {
      if (!modifiers.isEmpty()) {
        return modifiers.get(0).span();
      }

Review comment (suggestion):
This extra check is needed because some callers parse the element of the list themselves (while determining what construct it is) so it's possible for this function to be reached while at `terminator`.

Code after:
    res.add(parser.get());
    while (lookahead == separator) {
      next();
      res.add(parser.get());
    }
    return res;
  }

  private <T> List<T> parseListOf(Token separator, Token terminator, Supplier<T> parser) {
    var res = new ArrayList<T>();
    res.add(parser.get());
    while (lookahead == separator) {
      next();
      if (lookahead == terminator) {
        break;
      }
      res.add(parser.get());
    }
    return res;
  }

  private List<Parameter> parseListOfParameter(Token separator, Token terminator) {
    var res = new ArrayList<Parameter>();
    if (lookahead == terminator) {
      return res;
    }

    res.add(parseParameter());
    while (lookahead == separator) {
      next();
      if (lookahead == terminator) {
        break;
      }
      res.add(parseParameter());
    }
    return res;
  }

  private ParserError parserError(String messageKey, Object... args) {
    return new ParserError(ErrorMessages.create(messageKey, args), spanLookahead);
  }

  private record MemberHeader(
      @Nullable DocComment docComment, List<Annotation> annotations, List<Modifier> modifiers) {
    boolean isNotEmpty() {
      return !(docComment == null && annotations.isEmpty() && modifiers.isEmpty());
    }

    @SuppressWarnings("DataFlowIssue")
    @Nullable
    Span span() {