Skip to content

Add JSpecify annotations to 10 language package classes#4218

Open
Copilot wants to merge 19 commits intomasterfrom
copilot/add-jspecify-annotations-again
Open

Add JSpecify annotations to 10 language package classes#4218
Copilot wants to merge 19 commits intomasterfrom
copilot/add-jspecify-annotations-again

Conversation

Copy link
Contributor

Copilot AI commented Jan 25, 2026

Annotates 10 classes in graphql.language package with JSpecify nullability annotations and removes them from the exemption list.

Annotated Classes

  • DirectiveLocation, DirectivesContainer (interface), Document
  • EnumTypeDefinition, EnumTypeExtensionDefinition, EnumValueDefinition
  • Field, FieldDefinition, FragmentDefinition, FragmentSpread

Approach

Field nullability: Core fields (name, type, typeCondition) are marked @Nullable to allow test code to create incomplete instances for validation testing.

Getter enforcement: Getters use assertNotNull() to satisfy NamedNode contract and fail fast:

private final @Nullable String name;

@Override
public String getName() {
    return assertNotNull(name, () -> "name cannot be null");
}

Builder leniency: Builders accept nullable values without validation, supporting existing test patterns that create invalid AST nodes.

List copying: deepCopy() methods use assertNotNull() on list results since AbstractNode's deepCopy(List) returns @Nullable List.

Original prompt

Task

Add JSpecify annotations to the following 10 classes in the graphql.language package, following the established JSpecify annotation pattern documented in .claude/commands/jspecify-annotate.md.

Classes to annotate

  1. graphql.language.DirectiveLocation
  2. graphql.language.DirectivesContainer
  3. graphql.language.Document
  4. graphql.language.EnumTypeDefinition
  5. graphql.language.EnumTypeExtensionDefinition
  6. graphql.language.EnumValueDefinition
  7. graphql.language.Field
  8. graphql.language.FieldDefinition
  9. graphql.language.FragmentDefinition
  10. graphql.language.FragmentSpread

Instructions

Follow the JSpecify annotation process documented in .claude/commands/jspecify-annotate.md:

  1. Set each class to be @NullMarked at the class level
  2. Remove all redundant @NonNull annotations that IntelliJ may have added
  3. Check Javadoc for @param tags mentioning "null", "nullable", "may be null"
  4. Check Javadoc @return tags mentioning "null", "optional", "if available"
  5. Inspect method implementations that return null or check for null
  6. Consider GraphQL specification details when determining nullability
  7. For Builder static classes, label them @NullUnmarked and no further annotations needed

Validation

After making changes, run the NullAway compile check:

./gradlew compileJava

If you find NullAway errors, make the smallest possible change to fix them. You can use assertNotNull with a message if needed.

Cleanup

  1. Remove each completed class from the exemption list in src/test/groovy/graphql/archunit/JSpecifyAnnotationsCheck.groovy
  2. Delete all unused imports from the classes you've annotated
  3. Do not make spacing or formatting changes - only make minimal changes necessary for the annotations

Reference

Look at existing annotated classes like IntValue.java, StringValue.java, AbstractNode.java in the same package for reference patterns.

This pull request was created from Copilot chat.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Add JSpecify annotations to 10 classes in graphql.language package Add JSpecify annotations to 10 language package classes Jan 25, 2026
Copilot AI requested a review from dondonz January 25, 2026 07:00
@dondonz dondonz changed the base branch from copilot/add-jspecify-annotations-to-classes to master February 8, 2026 07:16
@github-actions
Copy link
Contributor

Test Results

0 files   -   335  0 suites   - 335   0s ⏱️ - 5m 5s
0 tests  - 5 371  0 ✅  - 5 363  0 💤  - 8  0 ❌ ±0 
0 runs   - 5 460  0 ✅  - 5 452  0 💤  - 8  0 ❌ ±0 

Results for commit baa9a0e. ± Comparison against base commit 89b4446.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 15, 2026

Test Results

  335 files  ±0    335 suites  ±0   5m 5s ⏱️ ±0s
5 371 tests ±0  5 362 ✅  - 1  9 💤 +1  0 ❌ ±0 
5 460 runs  ±0  5 451 ✅  - 1  9 💤 +1  0 ❌ ±0 

Results for commit 72ea92a. ± Comparison against base commit 89b4446.

This pull request removes 204 and adds 180 tests. Note that renamed tests count towards both.
	?

	, expected: combo-\"\\\b\f\n\r\t, #4]
                __schema { types { fields { args { type { name fields { name }}}}}}
                __schema { types { fields { type { name fields { name }}}}}
                __schema { types { inputFields { type { inputFields { name }}}}}
                __schema { types { interfaces { fields { type { interfaces { name } } } } } }
                __schema { types { name} }
                __type(name : "t") { name }
                a1: __schema { types { name} }
                a1: __type(name : "t") { name }
…
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure23@2cc75074 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure24@445bb139 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args but false does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_21prov0_closure25@b9a77c8 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure20@489091bd delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure21@512d6e60 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertFalse with different number of error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_20prov0_closure22@1de9b505 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure3@73844119 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure4@2f4c2cd4 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2, #1]
graphql.AssertTest ‑ assertNotNull with different number of  error args throws assertions [toRun: <graphql.AssertTest$__spock_feature_0_5prov0_closure5@561b7d53 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1 arg2 arg3, #2]
graphql.AssertTest ‑ assertNotNull with different number of error args with non null does not throw assertions [toRun: <graphql.AssertTest$__spock_feature_0_6prov0_closure6@4f89331f delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
…
This pull request skips 1 test.
graphql.schema.fetching.LambdaFetchingSupportTest ‑ different class loaders induce certain behaviours

♻️ This comment has been updated with latest results.

Note that JSpecify is already used in this repository so it's already imported.

If you see a builder static class, you can label it `@NullUnmarked` and not need to do anymore for this static class in terms of annotations.
**IMPORTANT: Builder classes MUST be annotated with `@NullUnmarked`.** When you encounter a `public static final class Builder` or `public static class Builder` inside a `@NullMarked` class, you MUST:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A number of builder annotations were missed, so I've updated the prompt

Interesting there is a difference between "you can" and "you must" for LLMs

@dondonz
Copy link
Member

dondonz commented Feb 17, 2026

Item for discussion - this now enforces a selection set on fragment definition (as per spec), and now requires fields to have a name

@dondonz dondonz marked this pull request as ready for review February 18, 2026 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments