Skip to content

Add JSpecify annotations to 10 instrumentation classes#4226

Draft
Copilot wants to merge 7 commits intomasterfrom
copilot/add-jspecify-annotations-ce85e181-c15c-45b6-ace7-d0ef898a87a8
Draft

Add JSpecify annotations to 10 instrumentation classes#4226
Copilot wants to merge 7 commits intomasterfrom
copilot/add-jspecify-annotations-ce85e181-c15c-45b6-ace7-d0ef898a87a8

Conversation

Copy link
Contributor

Copilot AI commented Jan 25, 2026

Annotates 10 classes in graphql.execution.instrumentation package with JSpecify nullability markers, reducing the exemption list and improving null-safety guarantees.

Changes

  • Class-level annotations: Added @NullMarked to all 10 target classes (DeferredExecution, ChainedInstrumentation, DocumentAndVariables, NoContextChainedInstrumentation, ResponseMapFactory, SimpleInstrumentation, SimpleInstrumentationContext, SimplePerformantInstrumentation, FieldAndArguments, FieldValidationEnvironment)

  • Inner class handling: Marked Builder and private static classes as @NullUnmarked per established pattern

  • Nullable annotations: Added @Nullable to:

    • NoContextChainedInstrumentation override methods that intentionally return null
    • ChainedInstrumentation methods matching interface contracts
    • SimpleInstrumentationContext nullable fields and parameters
    • DeferredExecution.getLabel() return type (label can be null per GraphQL defer spec)
  • Exemption list: Removed all 10 classes from JSpecifyAnnotationsCheck.groovy

Example

@PublicApi
@NullMarked
public class DocumentAndVariables {
    // Fields are non-null by default under @NullMarked
    private final Document document;
    
    @NullUnmarked  // Builder doesn't need fine-grained annotations
    public static class Builder {
        // ...
    }
}
Original prompt

Task

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

Classes to annotate

  1. graphql.execution.incremental.DeferredExecution
  2. graphql.execution.instrumentation.ChainedInstrumentation
  3. graphql.execution.instrumentation.DocumentAndVariables
  4. graphql.execution.instrumentation.NoContextChainedInstrumentation
  5. graphql.execution.ResponseMapFactory
  6. graphql.execution.instrumentation.SimpleInstrumentation
  7. graphql.execution.instrumentation.SimpleInstrumentationContext
  8. graphql.execution.instrumentation.SimplePerformantInstrumentation
  9. graphql.execution.instrumentation.fieldvalidation.FieldAndArguments
  10. graphql.execution.instrumentation.fieldvalidation.FieldValidationEnvironment

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 in the repository for reference patterns.

This pull request was created from Copilot chat.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Add JSpecify annotations to 10 classes in instrumentation package Add JSpecify annotations to 10 instrumentation classes Jan 25, 2026
Copilot AI requested a review from dondonz January 25, 2026 07:17
}

@NullUnmarked
static class ChainedInstrumentationState implements InstrumentationState {
Copy link
Member

Choose a reason for hiding this comment

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

TODO: I think the prompt has confused builders and static classes

@dondonz dondonz changed the base branch from copilot/add-jspecify-annotations-to-classes to master February 8, 2026 07:15
@dondonz dondonz marked this pull request as ready for review February 14, 2026 22:04
…c15c-45b6-ace7-d0ef898a87a8

# Conflicts:
#	.claude/commands/jspecify-annotate.md
#	src/main/java/graphql/schema/GraphQLSchema.java
@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2026

Test Results

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

Results for commit 87ebe35. ± Comparison against base commit 89b4446.

This pull request removes 196 and adds 172 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@5f5effb0 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@3468ee6e 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@751ae8a4 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@b0a1231 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@3b1dc579 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@3b7eac14 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@5115f590 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@4483d35 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@3a8d467e 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@332bcab0 delegate=inaccessible owner=inaccessible thisObject=inaccessible resolveStrategy=inaccessible directive=inaccessible parameterTypes=inaccessible maximumNumberOfParameters=inaccessible bcw=inaccessible thisType=inaccessible>, expectedMessage: error arg1, #0]
…

♻️ This comment has been updated with latest results.

@dondonz dondonz marked this pull request as draft February 14, 2026 22:23
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