immutable: Allow optional fields as null

[jdmwood]

One comment from my colleagues when trying to persuade them to use Immutables to replace existing code is they don't like the use of Optional for optional fields.

Personally, I'm still undecided about this one, I can see arguments both for and against. If we were starting from a green field then I'd think I'd be happy using Optional.

The one tricky thing was that switching to Optional for legacy beans can cause really subtle bugs if you're not careful. E.g. imagine this legacy code:

class Foo {
    private String foo;
    String getFoo() {
     return foo;
    }
}


...
   if (value.getFoo() == null) {
       ... do something
   }

Now if I change to an Immutables like this...

abstract class Foo {
    abstract Optional<String> getFoo()
}

... I won't receive a compiler error, but my logic in the "if" is broken.

Could we just have, say, an @Value.Optional annotation (Or even use the @javax.annotation.Nullable one), to mark a field optional?

That way, code which uses the old nullable style can work without any changes.

Yes, I understand you're trading off one problem for another (ambiguity about when getFoo() will return null), but this might suit some people better.

Thanks!

[elucash]

@Nullable is a way to go with this. I would argue about such usage, but you already understand all implications ), and this should be supported.

BTW @Value.Getters does unpacking of optionals to nullable, but that, definitely, is not a solution for this issue.

[jdmwood]

Thanks for the info - I will investigate the Getters alternative.

[elucash]

Support for @Nullable annotation is added to the master with 081cebb. While release is not yet ready, but you can evaluate.

[jdmwood]

Awesome! Thanks! I will take a look.

[jdmwood]

I've checked out this commit and it looks good!

My only comment is that it doesn't work for primitive return types.

I guess this makes sense because having @Nullable on a primitive return type is non sensical anyway.

I got around this by just using @Value.Default for primitives. This is fine but I just wondered if it might have been slightly cleaner to have something like:

@Value.Immutable
public abstract class Foo {

    @Nullable
    public abstract String name();

    @OptionalPrimitive
    public boolean defaultReport();
}

where OptionalPrimitive is a new annotation.

Maybe, maybe not...

Anyway, this is awesome as it is - thanks again!

[elucash]

We did thought-work on this with other contributors some time ago. The conclusion was that the best way for this is to use @Value.Default as you mentioned. This is most expressive and has least potential to introduce sneaky errors with non-initialized values.

@Value.Default
public boolean defaultReport() {
   return false;
}

Also, you can use @Nullable on boxed primitives like Integer, Boolean

[jdmwood]

Yep - makes total sense. I guess it's making it clear what you want the default to be, and not just use the JVM default. Thanks!