github / github/copilot-sdk

[Java] @CopilotTool ergonomics: Validate @Param defaultValue parseability at compile time

オープン
#1,789 コメント 0 件 リアクション 0 件 担当者 1 名 @edburns が担当を希望しています GitHub で見る
enhancement
主要言語
Java
スター
10.5k
フォーク
1.5k
平均マージ
1日 11時間
マージ済み PR(30日)
127

説明

## Overview

Add compile-time validation in `CopilotToolProcessor` that checks whether a `@Param(defaultValue="...")` string is parseable for the annotated parameter's type. Currently, an invalid default (e.g., `@Param(defaultValue="abc")` on an `int` parameter) will surface as either a compiler error in the generated `$$CopilotToolMeta` code or a runtime coercion error — and the error message will point at the generated class instead of the original annotation site.

This enhancement reports the error directly on the `@Param` annotation via `Messager.printMessage(ERROR, ...)`, giving developers a clear message at the source of the problem.

Branch: `edburns/1682-java-tool-ergonomics` on `upstream` (⚠️ NOT `main` — PRs must target this branch)

Origin: PR #1787 review comment [r3469523818](https://github.com/github/copilot-sdk/pull/1787#discussion_r3469523818)

## Prerequisites

• Tasks 4.1–4.4 must be complete and merged to the branch (they are).
• Before writing any code, read the existing validation logic in `CopilotToolProcessor.java` (lines 65–73 for the `required=true` + `defaultValue` conflict check).

## Deliverables

### File to modify

1. `java/src/main/java/com/github/copilot/tool/CopilotToolProcessor.java` — add a `validateDefaultValue(VariableElement param, TypeMirror type, String defaultValue)` method and call it from the existing `@Param` validation loop.

### Implementation specification

Add validation in the existing `@Param` validation loop (currently at lines 65–73) that checks parseability when `defaultValue` is non-empty. The validation must cover:

| Parameter type | Validation |
|---|---|
| `int` / `java.lang.Integer` | `Integer.parseInt(defaultValue)` succeeds |
| `long` / `java.lang.Long` | `Long.parseLong(defaultValue)` succeeds |
| `short` / `java.lang.Short` | `Short.parseShort(defaultValue)` succeeds |
| `byte` / `java.lang.Byte` | `Byte.parseByte(defaultValue)` succeeds |
| `float` / `java.lang.Float` | `Float.parseFloat(defaultValue)` succeeds |
| `double` / `java.lang.Double` | `Double.parseDouble(defaultValue)` succeeds |
| `boolean` / `java.lang.Boolean` | Value is exactly `"true"` or `"false"` (case-insensitive) |
| `char` / `java.lang.Character` | `defaultValue.length() == 1` |
| `java.lang.String` | Always valid — skip validation |
| Enum types | Value matches one of the enum constants (use `TypeElement.getEnclosedElements()` filtered by `ElementKind.ENUM_CONSTANT`) |

For types not in this table (records, POJOs, `java.time.*`, collections, etc.), **do NOT validate** — emit no error. These types use Jackson `convertValue()` at runtime and their string representation is too complex for compile-time validation.

### Error message format

```
@Param defaultValue "" is not a valid . Expected: .
```

Examples:
- `@Param defaultValue "abc" is not a valid int. Expected: an integer literal (e.g., "42").`
- `@Param defaultValue "maybe" is not a valid boolean. Expected: "true" or "false".`
- `@Param defaultValue "UNKNOWN" is not a valid com.example.Color. Expected one of: RED, GREEN, BLUE.`

### Where to integrate

In the existing validation loop in `process()`:

```java
// Validate @Param conflicts
for (VariableElement param : method.getParameters()) {
Param paramAnnotation = param.getAnnotation(Param.class);
if (paramAnnotation != null && paramAnnotation.required()
&& !paramAnnotation.defaultValue().isEmpty()) {
processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR,
"@Param cannot have both required=true and a non-empty defaultValue", param);
}
// ADD HERE: validate defaultValue parseability
if (paramAnnotation != null && !paramAnnotation.defaultValue().isEmpty()) {
validateDefaultValue(param, param.asType(), paramAnnotation.defaultValue());
}
}
```

### Helper method skeleton

```java
/**
* Validates that a @Param defaultValue string can be parsed as the given parameter type.
* Emits a compile error on the parameter if validation fails.
* Types that cannot be validated at compile time (records, POJOs, java.time.*, etc.)
* are silently skipped.
*/
private void validateDefaultValue(VariableElement param, TypeMirror type, String defaultValue) {
// Handle primitives
if (type.getKind().isPrimitive()) {
switch (type.getKind()) {
case INT:
tryParse(() -> Integer.parseInt(defaultValue), param, "int", "an integer literal (e.g., \"42\")");
break;
case LONG:
tryParse(() -> Long.parseLong(defaultValue), param, "long", "a long literal (e.g., \"100\")");
break;
// ... short, byte, float, double, boolean, char
}
return;
}
// Handle declared types (boxed, String, enums)
if (type.getKind() == TypeKind.DECLARED) {
TypeElement typeElement = (TypeElement) ((DeclaredType) type).asElement();
String qualifiedName = typeElement.getQualifiedName().toString();
if ("java.lang.String".equals(qualifiedName)) {
return; // always valid
}
if ("java.lang.Integer".equals(qualifiedName)) {
tryParse(() -> Integer.parseInt(defaultValue), param, "Integer", "an integer literal");
return;
}
// ... other boxed types, Boolean, Character
if (typeElement.getKind() == ElementKind.ENUM) {
validateEnumDefault(param, typeElement, defaultValue);
return;
}
}
// Other types: skip validation (too complex for compile time)
}

private void tryParse(Runnable parser, VariableElement param, String typeName, String hint) {
try {
parser.run();
} catch (NumberFormatException e) {
processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR,
"@Param defaultValue \"" + /* defaultValue */ + "\" is not a valid " + typeName
+ ". Expected: " + hint + ".", param);
}
}

private void validateEnumDefault(VariableElement param, TypeElement enumType, String defaultValue) {
List constants = enumType.getEnclosedElements().stream()
.filter(e -> e.getKind() == ElementKind.ENUM_CONSTANT)
.map(e -> e.getSimpleName().toString())
.collect(Collectors.toList());
if (!constants.contains(defaultValue)) {
processingEnv.getMessager().printMessage(Diagnostic.Kind.ERROR,
"@Param defaultValue \"" + defaultValue + "\" is not a valid "
+ enumType.getQualifiedName() + ". Expected one of: "
+ String.join(", ", constants) + ".", param);
}
}
```

## Gating tests and criteria

All of the following must pass before this task is considered complete:

1. **Invalid int default test**: Compile a class with `@Param(defaultValue="abc") int count` — verify compile ERROR with message containing `"not a valid int"`.
2. **Invalid long default test**: `@Param(defaultValue="not_a_long") long id` — verify compile ERROR.
3. **Invalid boolean default test**: `@Param(defaultValue="maybe") boolean flag` — verify compile ERROR with message mentioning `"true" or "false"`.
4. **Invalid char default test**: `@Param(defaultValue="ab") char c` — verify compile ERROR (length != 1).
5. **Invalid enum default test**: `@Param(defaultValue="UNKNOWN") MyEnum e` where `MyEnum` has `{RED, GREEN, BLUE}` — verify compile ERROR listing the valid constants.
6. **Valid int default test**: `@Param(defaultValue="42") int count` — verify NO compile error (compilation succeeds).
7. **Valid boolean default test**: `@Param(defaultValue="true") boolean flag` — verify NO compile error.
8. **Valid enum default test**: `@Param(defaultValue="RED") MyEnum e` — verify NO compile error.
9. **Skipped complex type test**: `@Param(defaultValue="2024-01-01T00:00:00Z") java.time.OffsetDateTime dt` — verify NO compile error (validation is skipped for non-primitive, non-boxed, non-enum types).
10. **Valid String default test**: `@Param(defaultValue="anything") String s` — verify NO compile error.
11. **Boxed types test**: `@Param(defaultValue="xyz") Integer n` — verify compile ERROR.
12. **Existing required+defaultValue conflict still works**: Ensure the existing `required=true` + `defaultValue` validation is unchanged and still reports its own error.
13. **Spotless format check**: `mvn spotless:check` passes.
14. **Full test suite**: `mvn clean verify` passes (existing tests not broken).

## Constraints

• ✅✅ YOU MUST run `mvn spotless:apply` before every commit.
• Do NOT modify any files outside the `java/` directory.
• Follow existing code style (4-space indent, Javadoc on public APIs).
• Add tests to the existing `CopilotToolProcessorTest.java` test class.
• The `isBoxedNumeric()` helper already exists in `CopilotToolProcessor` — reuse it for the boxed numeric type checks.
• The validation must NOT reject complex types it cannot validate — silently skip them.
• Boolean validation must be case-insensitive (`"True"`, `"FALSE"`, `"true"` are all valid).

コントリビューションガイド

コントリビューションガイドを開く

評価

この issue はまだ評価されていません。

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。