X Tutup
Skip to content

Add JSpecify annotations to language package nodes - redo (12 more) #4257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dondonz wants to merge 10 commits into
base: master
Choose a base branch
from
+108 −66
Open

Add JSpecify annotations to language package nodes - redo (12 more) #4257

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b261a43
Add corrections to prompt
dondonz Feb 21, 2026
4df4205
Add first lot of changes
dondonz Feb 22, 2026
8fb836e
Add operation definition
dondonz Feb 22, 2026
298fea2
Add pretty ast printer
dondonz Feb 22, 2026
82712c7
Override name as non null
dondonz Feb 22, 2026
7a9e810
Add operation validator
dondonz Feb 22, 2026
dd49296
Adjust AST Printer
dondonz Feb 22, 2026
9e31132
Account for a nullable op name
dondonz Feb 22, 2026
1063251
Fix test for non-null selection set
dondonz Feb 22, 2026
f4e0f1b
Make OperationDefinition.operation non-null, default to QUERY
dondonz Mar 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion .claude/commands/jspecify-annotate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ Analyze this Java class and add JSpecify annotations based on:
5. Method implementations that return null or check for null
6. GraphQL specification details (see details below)

## API Compatibility and Breaking Changes

When adding JSpecify annotations, **DO NOT break existing public APIs**.
- **Do not remove interfaces** from public classes (e.g., if a class implements `NamedNode`, it must continue to do so).
- **Be extremely careful when changing methods to return `@Nullable`**. If an interface contract (or widespread ecosystem usage) expects a non-null return value, changing it to `@Nullable` is a breaking change that will cause compilation errors or `NullPointerException`s for callers. For example, if a method returned `null` implicitly but its interface requires non-null, you must honor the non-null contract (e.g., returning an empty string or default value instead of `null`).
- **Do not change the binary signature** of methods or constructors in a way that breaks backwards compatibility.

Copy link
Member Author

@dondonz dondonz Feb 22, 2026

Choose a reason for hiding this comment

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

Previous attempt randomly deleted a constructor parameter which caused that PR to go off the rails. Let's fix that here

## GraphQL Specification Compliance
This is a GraphQL implementation. When determining nullability, consult the GraphQL specification (https://spec.graphql.org/draft/) for the relevant concept. Key principles:

Expand All @@ -26,7 +33,9 @@ If you find NullAway errors, try and make the smallest possible change to fix th

## Formatting Guidelines

Do not make spacing or formatting changes. Avoid adjusting whitespace, line breaks, or other formatting when editing code. These changes make diffs messy and harder to review. Only make the minimal changes necessary to accomplish the task.
- **Zero Formatting Changes**: Do NOT reformat the code.
- **Minimise Whitespace/Newline Changes**: Do not add or remove blank lines unless absolutely necessary. Keep the diff as clean as possible to ease the review process.
- Avoid adjusting whitespace, line breaks, or other formatting when editing code. These changes make diffs messy and harder to review. Only make the minimal changes necessary to accomplish the task.

## Cleaning up
Finally, can you remove this class from the JSpecifyAnnotationsCheck as an exemption
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/graphql/language/AstPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,8 @@ private NodePrinter<OperationDefinition> operationDefinition() {
&& node.getOperation() == OperationDefinition.Operation.QUERY) {
node(out, node.getSelectionSet());
} else {
out.append(node.getOperation().toString().toLowerCase());
OperationDefinition.Operation op = node.getOperation();
out.append(op.toString().toLowerCase());
if (!isEmpty(name)) {
out.append(' ');
out.append(name);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/language/AstSorter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ private Comparator<Definition> comparingDefinitions() {
Function<Definition, Integer> byType = d -> {
if (d instanceof OperationDefinition) {
OperationDefinition.Operation operation = ((OperationDefinition) d).getOperation();
if (OperationDefinition.Operation.QUERY == operation || operation == null) {
if (OperationDefinition.Operation.QUERY == operation) {
return 101;
}
if (OperationDefinition.Operation.MUTATION == operation) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/graphql/language/NamedNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

import graphql.PublicApi;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.Nullable;

/**
* Represents a language node that has a name
Expand All @@ -12,7 +13,7 @@
public interface NamedNode<T extends NamedNode> extends Node<T> {

/**
* @return the name of this node
* @return the name of this node, or null if this node is anonymous (e.g. an anonymous operation definition)
*/
String getName();
@Nullable String getName();
Copy link
Member Author

@dondonz dondonz Feb 22, 2026

Choose a reason for hiding this comment

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

This has to be nullable because an operation definition might not have a name

}
2 changes: 2 additions & 0 deletions src/main/java/graphql/language/NodeTraverser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import graphql.util.Traverser;
import graphql.util.TraverserContext;
import graphql.util.TraverserVisitor;
import org.jspecify.annotations.NullMarked;

import java.util.Collection;
import java.util.Collections;
Expand All @@ -18,6 +19,7 @@
* Lets you traverse a {@link Node} tree.
*/
@PublicApi
@NullMarked
public class NodeTraverser {


Expand Down
11 changes: 8 additions & 3 deletions src/main/java/graphql/language/NonNullType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
import graphql.PublicApi;
import graphql.util.TraversalControl;
import graphql.util.TraverserContext;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;

import java.util.LinkedHashMap;
import java.util.List;
Expand All @@ -18,14 +21,15 @@
import static graphql.language.NodeChildrenContainer.newNodeChildrenContainer;

@PublicApi
@NullMarked
public class NonNullType extends AbstractNode<NonNullType> implements Type<NonNullType> {

private final Type type;

public static final String CHILD_TYPE = "type";

@Internal
protected NonNullType(Type type, SourceLocation sourceLocation, List<Comment> comments, IgnoredChars ignoredChars, Map<String, String> additionalData) {
protected NonNullType(Type type, @Nullable SourceLocation sourceLocation, List<Comment> comments, IgnoredChars ignoredChars, Map<String, String> additionalData) {
super(sourceLocation, comments, ignoredChars, additionalData);
this.type = type;
}
Expand Down Expand Up @@ -63,7 +67,7 @@ public NonNullType withNewChildren(NodeChildrenContainer newChildren) {
}

@Override
public boolean isEqualTo(Node o) {
public boolean isEqualTo(@Nullable Node o) {
if (this == o) {
return true;
}
Expand All @@ -77,7 +81,7 @@ public boolean isEqualTo(Node o) {

@Override
public NonNullType deepCopy() {
return new NonNullType(deepCopy(type), getSourceLocation(), getComments(), getIgnoredChars(), getAdditionalData());
return new NonNullType(assertNotNull(deepCopy(type), "type deepCopy should not return null"), getSourceLocation(), getComments(), getIgnoredChars(), getAdditionalData());
}

@Override
Expand Down Expand Up @@ -106,6 +110,7 @@ public NonNullType transform(Consumer<Builder> builderConsumer) {
return builder.build();
}

@NullUnmarked
public static final class Builder implements NodeBuilder {
private SourceLocation sourceLocation;
private Type type;
Expand Down
11 changes: 8 additions & 3 deletions src/main/java/graphql/language/ObjectField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
import graphql.collect.ImmutableKit;
import graphql.util.TraversalControl;
import graphql.util.TraverserContext;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;

import java.util.LinkedHashMap;
import java.util.List;
Expand All @@ -19,6 +22,7 @@
import static graphql.language.NodeChildrenContainer.newNodeChildrenContainer;

@PublicApi
@NullMarked
public class ObjectField extends AbstractNode<ObjectField> implements NamedNode<ObjectField> {

private final String name;
Expand All @@ -27,7 +31,7 @@ public class ObjectField extends AbstractNode<ObjectField> implements NamedNode<
public static final String CHILD_VALUE = "value";

@Internal
protected ObjectField(String name, Value value, SourceLocation sourceLocation, List<Comment> comments, IgnoredChars ignoredChars, Map<String, String> additionalData) {
protected ObjectField(String name, Value value, @Nullable SourceLocation sourceLocation, List<Comment> comments, IgnoredChars ignoredChars, Map<String, String> additionalData) {
super(sourceLocation, comments, ignoredChars, additionalData);
this.name = assertNotNull(name);
this.value = assertNotNull(value);
Expand Down Expand Up @@ -72,7 +76,7 @@ public ObjectField withNewChildren(NodeChildrenContainer newChildren) {
}

@Override
public boolean isEqualTo(Node o) {
public boolean isEqualTo(@Nullable Node o) {
if (this == o) {
return true;
}
Expand All @@ -88,7 +92,7 @@ public boolean isEqualTo(Node o) {

@Override
public ObjectField deepCopy() {
return new ObjectField(name, deepCopy(this.value), getSourceLocation(), getComments(), getIgnoredChars(), getAdditionalData());
return new ObjectField(name, assertNotNull(deepCopy(this.value), "value deepCopy should not return null"), getSourceLocation(), getComments(), getIgnoredChars(), getAdditionalData());
Copy link
Member

@bbakerman bbakerman Mar 7, 2026

Choose a reason for hiding this comment

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

I used to think assertion messages matter but they dont. A stacktrace would lead to this code location - so less code can be better code

Copy link
Member Author

@dondonz dondonz Mar 7, 2026

Choose a reason for hiding this comment

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

This assert is a special one to satisfy the NullAway check

The problem is that deepCopy is a generic utility and I can't guarantee that always returns not-nullable result

}

@Override
Expand All @@ -114,6 +118,7 @@ public ObjectField transform(Consumer<Builder> builderConsumer) {
return builder.build();
}

@NullUnmarked
public static final class Builder implements NodeBuilder {
private SourceLocation sourceLocation;
private String name;
Expand Down
17 changes: 11 additions & 6 deletions src/main/java/graphql/language/ObjectTypeDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
import graphql.collect.ImmutableKit;
import graphql.util.TraversalControl;
import graphql.util.TraverserContext;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;

import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand All @@ -21,6 +24,7 @@
import static graphql.language.NodeChildrenContainer.newNodeChildrenContainer;

@PublicApi
@NullMarked
public class ObjectTypeDefinition extends AbstractDescribedNode<ObjectTypeDefinition> implements ImplementingTypeDefinition<ObjectTypeDefinition>, DirectivesContainer<ObjectTypeDefinition>, NamedNode<ObjectTypeDefinition> {
private final String name;
private final ImmutableList<Type> implementz;
Expand All @@ -36,8 +40,8 @@ protected ObjectTypeDefinition(String name,
List<Type> implementz,
List<Directive> directives,
List<FieldDefinition> fieldDefinitions,
Description description,
SourceLocation sourceLocation,
@Nullable Description description,
@Nullable SourceLocation sourceLocation,
List<Comment> comments,
IgnoredChars ignoredChars,
Map<String, String> additionalData) {
Expand Down Expand Up @@ -117,7 +121,7 @@ public ObjectTypeDefinition withNewChildren(NodeChildrenContainer newChildren) {
}

@Override
public boolean isEqualTo(Node o) {
public boolean isEqualTo(@Nullable Node o) {
if (this == o) {
return true;
}
Expand All @@ -133,9 +137,9 @@ public boolean isEqualTo(Node o) {
@Override
public ObjectTypeDefinition deepCopy() {
return new ObjectTypeDefinition(name,
deepCopy(implementz),
deepCopy(directives.getDirectives()),
deepCopy(fieldDefinitions),
assertNotNull(deepCopy(implementz), "implementz deepCopy should not return null"),
assertNotNull(deepCopy(directives.getDirectives()), "directives deepCopy should not return null"),
assertNotNull(deepCopy(fieldDefinitions), "fieldDefinitions deepCopy should not return null"),
description,
getSourceLocation(),
getComments(),
Expand Down Expand Up @@ -168,6 +172,7 @@ public ObjectTypeDefinition transform(Consumer<Builder> builderConsumer) {
return builder.build();
}

@NullUnmarked
public static final class Builder implements NodeDirectivesBuilder {
private SourceLocation sourceLocation;
private ImmutableList<Comment> comments = emptyList();
Expand Down
15 changes: 10 additions & 5 deletions src/main/java/graphql/language/ObjectTypeExtensionDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
import graphql.Internal;
import graphql.PublicApi;
import graphql.collect.ImmutableKit;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;

import java.util.LinkedHashMap;
import java.util.List;
Expand All @@ -16,15 +19,16 @@
import static graphql.collect.ImmutableKit.emptyMap;

@PublicApi
@NullMarked
public class ObjectTypeExtensionDefinition extends ObjectTypeDefinition implements SDLExtensionDefinition {

@Internal
protected ObjectTypeExtensionDefinition(String name,
List<Type> implementz,
List<Directive> directives,
List<FieldDefinition> fieldDefinitions,
Description description,
SourceLocation sourceLocation,
@Nullable Description description,
@Nullable SourceLocation sourceLocation,
List<Comment> comments,
IgnoredChars ignoredChars,
Map<String, String> additionalData) {
Expand All @@ -44,9 +48,9 @@ public ObjectTypeExtensionDefinition(String name) {
@Override
public ObjectTypeExtensionDefinition deepCopy() {
return new ObjectTypeExtensionDefinition(getName(),
deepCopy(getImplements()),
deepCopy(getDirectives()),
deepCopy(getFieldDefinitions()),
assertNotNull(deepCopy(getImplements()), "implementz deepCopy should not return null"),
assertNotNull(deepCopy(getDirectives()), "directives deepCopy should not return null"),
assertNotNull(deepCopy(getFieldDefinitions()), "fieldDefinitions deepCopy should not return null"),
getDescription(),
getSourceLocation(),
getComments(),
Expand Down Expand Up @@ -81,6 +85,7 @@ public ObjectTypeExtensionDefinition transformExtension(Consumer<Builder> builde
return builder.build();
}

@NullUnmarked
public static final class Builder implements NodeDirectivesBuilder {
private SourceLocation sourceLocation;
private ImmutableList<Comment> comments = emptyList();
Expand Down
33 changes: 15 additions & 18 deletions src/main/java/graphql/language/OperationDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
import graphql.language.NodeUtil.DirectivesHolder;
import graphql.util.TraversalControl;
import graphql.util.TraverserContext;
import org.jspecify.annotations.NullMarked;
import org.jspecify.annotations.NullUnmarked;
import org.jspecify.annotations.Nullable;

import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand All @@ -22,13 +25,14 @@
import static graphql.language.NodeChildrenContainer.newNodeChildrenContainer;

@PublicApi
@NullMarked
public class OperationDefinition extends AbstractNode<OperationDefinition> implements Definition<OperationDefinition>, SelectionSetContainer<OperationDefinition>, DirectivesContainer<OperationDefinition>, NamedNode<OperationDefinition> {

public enum Operation {
QUERY, MUTATION, SUBSCRIPTION
}

private final String name;
private final @Nullable String name;

private final Operation operation;
private final ImmutableList<VariableDefinition> variableDefinitions;
Expand All @@ -40,12 +44,12 @@ public enum Operation {
public static final String CHILD_SELECTION_SET = "selectionSet";

@Internal
protected OperationDefinition(String name,
protected OperationDefinition(@Nullable String name,
Operation operation,
List<VariableDefinition> variableDefinitions,
List<Directive> directives,
SelectionSet selectionSet,
SourceLocation sourceLocation,
@Nullable SourceLocation sourceLocation,
List<Comment> comments,
IgnoredChars ignoredChars,
Map<String, String> additionalData) {
Expand All @@ -57,15 +61,6 @@ protected OperationDefinition(String name,
this.selectionSet = selectionSet;
}

public OperationDefinition(String name,
Copy link
Member Author

@dondonz dondonz Feb 22, 2026

Choose a reason for hiding this comment

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

These constructors date back to 2018, no longer needed with the builders we have today.

I am removing these lines because these parameters should be not-nullable.

I would usually first deprecate the methods, then remove, but if I keep them here with a deprecated annotation, the nullability annotations will not be correct.

Copy link
Member Author

@dondonz dondonz Feb 22, 2026

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

Operation operation) {
this(name, operation, emptyList(), emptyList(), null, null, emptyList(), IgnoredChars.EMPTY, emptyMap());
}

public OperationDefinition(String name) {
this(name, null, emptyList(), emptyList(), null, null, emptyList(), IgnoredChars.EMPTY, emptyMap());
}

Copy link
Member

@bbakerman bbakerman Mar 7, 2026

Choose a reason for hiding this comment

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

Fair enough - cruft must be removed at some point

@Override
public List<Node> getChildren() {
List<Node> result = new ArrayList<>();
Expand Down Expand Up @@ -93,7 +88,8 @@ public OperationDefinition withNewChildren(NodeChildrenContainer newChildren) {
);
}

public String getName() {
@Override
public @Nullable String getName() {
return name;
}

Expand Down Expand Up @@ -130,7 +126,7 @@ public SelectionSet getSelectionSet() {
}

@Override
public boolean isEqualTo(Node o) {
public boolean isEqualTo(@Nullable Node o) {
if (this == o) {
return true;
}
Expand All @@ -148,9 +144,9 @@ public boolean isEqualTo(Node o) {
public OperationDefinition deepCopy() {
return new OperationDefinition(name,
operation,
deepCopy(variableDefinitions),
deepCopy(directives.getDirectives()),
deepCopy(selectionSet),
assertNotNull(deepCopy(variableDefinitions), "variableDefinitions deepCopy should not return null"),
assertNotNull(deepCopy(directives.getDirectives()), "directives deepCopy should not return null"),
assertNotNull(deepCopy(selectionSet), "selectionSet deepCopy should not return null"),
getSourceLocation(),
getComments(),
getIgnoredChars(),
Expand Down Expand Up @@ -183,11 +179,12 @@ public OperationDefinition transform(Consumer<Builder> builderConsumer) {
return builder.build();
}

@NullUnmarked
public static final class Builder implements NodeDirectivesBuilder {
private SourceLocation sourceLocation;
private ImmutableList<Comment> comments = emptyList();
private String name;
private Operation operation;
private Operation operation = Operation.QUERY;
private ImmutableList<VariableDefinition> variableDefinitions = emptyList();
private ImmutableList<Directive> directives = emptyList();
private SelectionSet selectionSet;
Expand Down
Loading
X Tutup