Support for lazy evaluation of arguments in Preconditions

[aioobe]

A seemingly innocent Precondition that looks like this...

// Make sure 'user' is null
Preconditions.checkState(user == null, "User is not null. User id: %s", user.getId());

...actually throws an NPE when the precondition is met (user == null) since user.getId() is eagerly evaluated. The obvious workaround is to provide an argument like...

Preconditions.checkState(user == null, "User is not null. User id: %s", new Object() {
    @Override
    public String toString() {
        return user.getId();
    }
});

...but it feels clunky at best.

It would be very slick if Preconditions would support lazy evaluation of parameters, for example by special treatment of Supplier<?> arguments, so one could write something like

Preconditions.checkState(user == null, "User is not null. User id: %s", (Supplier<?>) () -> user.getId());

The toString implementation of Supplier is pretty meaningless so I don't this would infringe on other use-cases or cause any back compat issues.

Another alternative could be to use a dedicated interface, such as LazyArgument and have a factory method in Preconditions to allow for something like

Preconditions.checkState(user == null, "User is not null. User id: %s", Preconditions.lazyArg(() -> user.getId()));

[kevinb9n]

For context, Flogger (another project of ours) has this. But it has to, because logging can be conditional on factors the user can't even see.

But here, it's just not enough better than regular Java:

if (user != null) {
  throw new IllegalStateException("User is not null. User id: " + user.getId());
}

We once considered exposing fail methods, which cuts it down a bit

if (user != null) {
  failState("User is not null. User id: " + user.getId());
}

I don't remember what happened; but it's likely we might not have been convinced this use case comes up frequently.

[aioobe]

Nice. I didn't know about Flogger. Personally I usually put something like...

public static Object lazyArg(Supplier<?> arg) {
    return new Object() {
        @Override
        public String toString() {
            return Objects.toString(arg.get());
        }
    };
}

...in a utility class somewhere to keep the call sites clean. I guess that is similar to what LazyArg does.

I don't want to drag out on something like this. When you say "it's just not enough better than regular Java", should that be interpreted as, "does not carry its weight, the team has decided to not move forward"? If yes, feel free to close as Won't fix, if no, what would the next step be? Should I file a PR?

[kevinb9n]

(We should promote flogger more.)

I try to never say never, but the lambda approach is probably unlikely to fly.

They might not be what you want, but I'll leave this open while we think about failState & failArgument a sec.

[cpovirk]

I recall discussion of methods to be used like throw illegalState("foo %s bar", baz); -- or inside lambdas for Optional.orElseThrow -- in an ancient internal doc named "Optional.get() with error messages." The advantage of that over the fail* methods is that the compiler can tell that an exception is being thrown. I'd suggest that we close this current issue in favor of #1382 (which I have no memory of touching less than a year ago?).

[kevinb9n]

You're right, defeating the compiler's knowledge of abrupt termination is no bueno.

But throw illegalState doesn't save much vs. throw new IllegalStateException, and the "%s" support isn't important when it's not a conditional throw anymore.

So I think this approach probably won't fly either.

[oliverhenlich]

Hi @kevinb9n,

Is it possible to elaborate on this a little.

I try to never say never, but the lambda approach is probably unlikely to fly.

We have just hit the same issue too while trying to convert all our assert (keyword) usages to use Preconditions. But we have many asserts that have elaborate error messages (which are currently lazily evaluated). This is problematic because:

• Loads of our tests now fail because they don't have the right stuff mocked (for the error messages)
• We will incur the runtime overhead of all those error messages being computed.

The workaround with the Object parameter and something like lazyArg() isn't ideal either and I feel like it'll be too easy for other team members to do it the "wrong" way.

So I'm considering writing our own Preconditions class that requires you to pass in a Supplier<String> but would really like to hear your reasoning for this not existing in the guava version before going down this path.

[kevinb9n]

Sorry I missed that.

We think that

checkState(user == null, "User is not null. User id: %s", lazy(() -> user.getId()));

is not sufficiently better code than

if (user != null) {
  throw new IllegalStateException("User is not null. User id: " + user.getId());
}

And in fact, anyone can read the latter code and know exactly what it does, which isn't the case at top.

Guava has never wanted to be a different way to do things. We have always strived to add only features that provide better ways than the universal idioms. Preconditions provides value just by being usable most of the time; it doesn't need to be "all of the time".