github / github/copilot-sdk

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

Abierto
#1,789 0 comentarios 0 reacciones 1 asignado Reclamado por @edburns Ver en GitHub
enhancement
Lenguaje dominante
Java
Estrellas
10.5k
Forks
1.5k
Merge medio
1 d 11 h
PR fusionados (30 d)
127

Descripción

## 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).

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.