makes the consuming code easier to read

Did you mean to remove the name of the type in this error message?

Yes I did - why?

because a static value that takes in no "enclosed" variables allocates no memory - the Supplier becomes a global singleton compiled into place.

but if we "enclose" a variable then it allocates the Supplier on every call.

So in the 0.0001% case where we have an error, yes the error message is better BUT we pay a memory price on the 99.9999% of the time where there is no error.

So yes its a less informative error message, but its also better at memory usage!

what if the input type is non-null list of non-null type? it should be handled here? IIRC I'm using this method somewhere in one project

GraphqlNonNull has code to prevent it being double wrapped.

So

field( arg : String!!)

aka

nonNull(nonNull(GraphqlString)) is actually illegal and wont ever be allowed

Ok that is good but what about a type like

ids: [ID!]!?

I suspect that the while loop was there to handle that

In the case of [ID!]! it's not non-null wrapping a non-null, it would be non-null wrapping a list, wrapping a non-null

Ok but I think this could be a breaking change for some projects (and this class is not marked as @internal IIRC)

Wrapping a non-null with another non-null is not allowed as per the specification: in the grammar it can only wrap a named or list type, not another non-null type. https://spec.graphql.org/draft/#sec-Type-References

Is there a situation where you would have had a non-null wrapping another non-null in your experience?

I thought about the breaking change aspect and its not there. We already had assertion code to prevent ids : ID!! as a type so the code was overly defensive and I suspect written without the understanding that double non nulls is actually illegal.

So there is no breaking change here. Just a more efficient unwrapping of a single non null layer.

the problem I see is that the previous code was unwrapping all the wrapper types (because unwrapOne/getWrappedType work for List too):
so, previously, a client of this method could call it with [ID!]! to get ID. Is there a unit test for this behavior?

Of course, I could be wrong ... I'm pretty sure my code could be broken by this change but I don't have my laptop with me now.

The problem I see is that the previous code was unwrapping all the wrapper types (because unwrapOne/getWrappedType work for List too):

Its ok - itt didnt work like that. It only every unwrapped a non null . The code was

    public static GraphQLType unwrapNonNull(GraphQLType type) {
       while (isNonNull(type)) {
               type = unwrapOne(type);
    }


    public static GraphQLType unwrapOne(GraphQLType type) {
        if (isNonNull(type)) {
            return ((GraphQLNonNull) type).getWrappedType();
        } else if (isList(type)) {
            return ((GraphQLList) type).getWrappedType();
        }
        return type;
    }

So it would check if its non null and stop once it reached a type that is not non null. The reason its faster is that unwrapOne does another isNonNull instanceof check inside itself. So this avoids that.

Its not a major saving but its some saving. Thats why I said its a smidge faster

smidgen
/ˈsmɪdʒɪn/
nouninformal
noun: smidgen; plural noun: smidgens; noun: smidgin; plural noun: smidgins; noun: smidgeon; plural noun: smidgeons
a small amount of something.
"add a smidgen of cayenne"

looks good then, thanks for the explanation!