Skip to content

Add JSpecify annotations to 10 language package classes#4219

Open
Copilot wants to merge 6 commits intomasterfrom
copilot/add-jspecify-annotations-another-one
Open

Add JSpecify annotations to 10 language package classes#4219
Copilot wants to merge 6 commits intomasterfrom
copilot/add-jspecify-annotations-another-one

Conversation

Copy link
Contributor

Copilot AI commented Jan 25, 2026

Annotates 10 classes in graphql.language with JSpecify nullability markers per the established pattern.

Classes Annotated

Interfaces:

  • ImplementingTypeDefinition - @NullMarked
  • NodeDirectivesBuilder - @NullMarked

Classes with Builders:

  • InlineFragment - @NullMarked class, @NullUnmarked Builder
  • InputObjectTypeDefinition + Extension - @NullMarked classes, @NullUnmarked Builders
  • InputValueDefinition - @NullMarked class, @NullUnmarked Builder
  • InterfaceTypeDefinition + Extension - @NullMarked classes, @NullUnmarked Builders
  • ListType - @NullMarked class, @NullUnmarked Builder

Other:

  • NodeParentTree - @NullMarked

Nullability Decisions

Fields marked @Nullable based on GraphQL spec and usage analysis:

  • InlineFragment.typeCondition - Inline fragments can omit type conditions (spec allows bare ... { field })
  • InputValueDefinition.defaultValue - Default values are optional
  • NodeParentTree.parent - Root nodes have no parent
  • All Description parameters - Descriptions are optional in GraphQL SDL
  • All SourceLocation parameters - Inherited from AbstractNode pattern

Technical Fixes

deepCopy methods: Added assertNotNull calls where AbstractNode.deepCopy() returns nullable types but target constructors expect non-null:

public InlineFragment deepCopy() {
    return new InlineFragment(
            deepCopy(typeCondition),  // nullable is fine
            assertNotNull(deepCopy(directives.getDirectives()), "directives cannot be null"),
            assertNotNull(deepCopy(selectionSet), "selectionSet cannot be null"),
            // ...
    );
}

InlineFragment constructor: Provides default SelectionSet instead of null since selection sets are required.

Anonymizer: Added null check for typeCondition before dereferencing in visitInlineFragment.

Removes all 10 classes from JSpecify exemption 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.ImplementingTypeDefinition
  2. graphql.language.InlineFragment
  3. graphql.language.InputObjectTypeDefinition
  4. graphql.language.InputObjectTypeExtensionDefinition
  5. graphql.language.InputValueDefinition
  6. graphql.language.InterfaceTypeDefinition
  7. graphql.language.InterfaceTypeExtensionDefinition
  8. graphql.language.ListType
  9. graphql.language.NodeDirectivesBuilder
  10. graphql.language.NodeParentTree

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 and others added 4 commits January 24, 2026 20:32
Co-authored-by: dondonz <13839920+dondonz@users.noreply.github.com>
Co-authored-by: dondonz <13839920+dondonz@users.noreply.github.com>
- Added @NullMarked to ImplementingTypeDefinition interface
- Added @NullMarked to InlineFragment with @nullable typeCondition
- Added @NullMarked to InputObjectTypeDefinition with @NullUnmarked Builder
- Added @NullMarked to InputObjectTypeExtensionDefinition with @NullUnmarked Builder
- Added @NullMarked to InputValueDefinition with @nullable defaultValue
- Added @NullMarked to InterfaceTypeDefinition with @NullUnmarked Builder
- Added @NullMarked to InterfaceTypeExtensionDefinition with @NullUnmarked Builder
- Added @NullMarked to ListType with @NullUnmarked Builder
- Added @NullMarked to NodeDirectivesBuilder interface
- Added @NullMarked to NodeParentTree with @nullable parent
- Fixed deepCopy methods with assertNotNull for non-null fields
- Fixed InlineFragment constructor to provide default SelectionSet
- Fixed Anonymizer to handle nullable typeCondition
- Removed completed classes from JSpecify exemption list

Co-authored-by: dondonz <13839920+dondonz@users.noreply.github.com>
Copilot AI changed the title [WIP] Add JSpecify annotations to GraphQL language classes Add JSpecify annotations to 10 language package classes Jan 25, 2026
Copilot AI requested a review from dondonz January 25, 2026 06:49
@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

github-actions bot commented Feb 20, 2026

Test Results

  335 files  ±0    335 suites  ±0   5m 4s ⏱️ -1s
5 376 tests +5  5 367 ✅ +4  9 💤 +1  0 ❌ ±0 
5 465 runs  +5  5 456 ✅ +4  9 💤 +1  0 ❌ ±0 

Results for commit 0fb0195. ± Comparison against base commit 89b4446.

This pull request removes 449 and adds 430 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@75de29c0 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@fc807c1 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@296e281a 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@6dcc40f5 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@2b680207 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@70887727 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@730f9695 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@146dcfe6 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@1b1f5012 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@51c959a4 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.

public InlineFragment(TypeName typeCondition) {
this(typeCondition, emptyList(), null, null, emptyList(), IgnoredChars.EMPTY, emptyMap());
public InlineFragment(@Nullable TypeName typeCondition) {
this(typeCondition, emptyList(), SelectionSet.newSelectionSet().build(), null, emptyList(), IgnoredChars.EMPTY, emptyMap());
Copy link
Member

Choose a reason for hiding this comment

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

This is technically a breaking change, although it's to make this more correct

An inline fragment should not have an empty selection set, as per the specification.

@dondonz
Copy link
Member

dondonz commented Feb 21, 2026

This PR will start enforcing selection sets in inline fragments as not nullable, so this is technically a breaking change, but one that's also a bugfix

@dondonz dondonz added breaking change requires a new major version to be relased labels Feb 21, 2026
@dondonz dondonz marked this pull request as ready for review February 21, 2026 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change requires a new major version to be relased

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants