The VariableReference.Builder now allows a null name. Consider adding validation in the build() method to ensure that a non-null name is provided if that is an invariant in your domain.

name in the above constructor and builder method are not nullable, did you meant to make this field nullable?

I think in practice, a VariableReference should always have a non-nullable name

The EnumValue.Builder permits a null name. It may be beneficial to enforce a non-null name during object construction if that is required by the API contract.

Hello, thanks for your patience while away for Easter.

In this builder, name is set to nullable, but the constructor above has name as non-nullable, and the builder method below has it as non-nullable. Did you mean for the Builder's name to be instead non-nullable?

No, nullable fields in the builder are expected.
By default, when a builder instance is created, all values are set to their defaults (null for reference fields) - thus, it's necessary to mark these fields @Nullable.
This initial Builder state represents an incomplete configuration, and the EnumValue should not be constructible in this state - calling the build() method should result in an IllegalStateException.

I planned to add something like this later:

public EnumValue build() {
    if (name == null) {
        throw new IllegalStateException("name must be set");
    }
    return new EnumValue(name, sourceLocation, comments, ignoredChars, additionalData);
}

The IntValue.Builder now accepts a null value. Consider adding a check in the build() method to prevent creating an IntValue instance with a null value if that contradicts expected behavior.

Same comment as for FloatValue, I will double check if a null value is in practice possible or not

The FloatValue.Builder now permits a null value. If a non-null value is expected, it would be prudent to validate this in the build() method to prevent possible misuse, as noted in the PR description.

This builder should be consistent with the constructor for value

@andimarek @bbakerman: is it possible in practice for FloatValue to ever be constructed with a null? Or would such an object always be constructed as a NullValue?

Similar to above Value classes, I will double check if value can be nullable or if in practice it is represented by a NullValue

Want to call out this is a small change to not allow a null value. A list will be used instead. Without this change, there is a NullAway error

After giving a few classes the JSpecify treatment: we now generally annotate Builders with @NullUnmarked

Similar to ArrayValue, want to call out this is a small change to not allow a null value. A list will be used instead. Without this change, there is a NullAway error

I don't understand this change @dondonz ... why should we allow null values here?

The first line we check whether the input is nullable if (nullableObj == null) { so I have allowed the input to be nullable

I agree it's strange in the context of Value like FloatValue.

Maybe there was another reason to allow this to be nullable in the first place?

So we have two options here: either declare V as a type that also can be null or declare input and output as Nullable. We have done both here, which doesn't make sense I think.

Repeating conversation here: I will change the method signature to become

protected <V extends Node> @Nullable V deepCopy(@Nullable V nullableObj)

This in English is meant to express that V extending Node is not nullable, but because the method will handle null inputs, it may still return a null

So we have two options here: either declare V as a type that also can be null or declare input and output as Nullable. We have done both here, which doesn't make sense I think.

Add an assert here to keep NullAway happy

In practice it is not possible to have null values. Have to put this here otherwise you'll get a NullAway error

Add this assert to keep NullAway happy, even though objectFields is not null