Optional vs Nullable

[Piotr Nowojski <pi...@data-artisans.com>]

Hi,

Since we switched to Java 8, I would like to start a discussion about a deprecating usage of @Nullable parameters and return values in Flink and enforcing strict ban on nulls in those places. I just think that having a very explicit way, enforced by a static type checking mechanism of optional values is just so much better and safer compared to constantly thinking if the value that you have can be a null or it can not. 

Personally I would go even one step further and I would ban nulls as fields/local variables and always use in those cases Optional, with a very rare exceptions for performance reasons. This however has two drawbacks:

1. Optional was not intended by the Java committee to be used on fields. I don’t why, because for me it is just better compared to handling nulls.
2. Optional is not serializable (that’s the collateral damage caused by 1.). Thus in many places for fields we would have to use our own custom OptionalSerializable.

Regardless of using Optional for a fields as well, transition from Nullable to Optional should be fairly simple. We could go module by module deprecating old methods taking @Nullable and replacing them with overloaded Optional versions. The only issue is that in the short term it could confuse a little bit users, if we present them two versions of methods and with inconsistency between modules. However I think it’s worth it in the long term.

As a side note it is also easier to maintain the ban on nulls, compared to putting @Nullable annotation in appropriate places. With Optional, reviewer could always immediately spot each usage of “null". With @Nullable, if new pull request adds some invocation of a method:

foo.bar(arg1, null, arg3);

It is impossible to know whether it is allowed, without going to the implementation of the method and checking for presence or lack of the annotation. Thus we have some places in the code where there are undocumented/not annotated @Nullable fields/parameters (even in the public api).

What do you think?

Piotrek

Re: Optional vs Nullable

[Piotr Nowojski <pi...@data-artisans.com>]

Chesney you are looking at this in wrong way. With Optionals, your proposed myMethod wouldn’t be allowed by our code style/convention. It should look like this:

  public void myMethod(Optional<String> parameter) {
  	checkNotNull(parameter, “parameter is null”);
	if (parameter.isPresent()) {
		<do something with it>
  	}
  }

Just as we treat non @Nullable fields in Flink right now - we do not if check every field for being null.

> As for your example as to how it is "/easier to maintain the ban on null",/ since passing null is just as valid if the parameter was an optional wouldn't I still have to go to the implementation to check it?

No, with Optional convention you don’t have to check anything. Just never pass nulls, never expect nulls as a return values and (optionally) never expect a field to be null. Simple as that.

Dawid:

Of course if it is feasible to create overloaded methods (that do not share or duplicate code), it is a better solution to both Optional and @Nullable. The question here is not whether we want this:

public void myMethod(String arg1, Optonal<String> arg2);

Or this:

public void myMethod(String arg1);
public void myMethod(String arg1, String arg2);

But whether we want this:

public void myMethod(String arg1, Optonal<String> arg2);

Or this:

public void myMethod(String arg1, @Nullable String arg2);

The issue with overloading is that more often then not it just brings down that the simpler method calls the more advanced with default values. If those default values are nulls, that’s the case that I do not like.

PS, the schema that I have seen sometimes is like this:

public void myMethod(String arg1) {
    myMethod(arg1, Optional.empty())
}
public void myMethod(String arg1, String arg2) {
    myMethod(arg1, Optional.of(arg2));
}
private/public void myMethod(String arg1, Optional<String> arg2);

Again, the benefit of this approach is that it’s impossible to miss “Optional” both by method caller and while implementing myMethod body - the compiler will just not allow for that. While when using @Nullable it is more problematic… Especially in long and complicated methods. Same logic applies to @Nullable fields.

Piotrek

> On Aug 25, 2017, at 10:29 AM, Dawid Wysakowicz <wy...@gmail.com> wrote:
> 
> I agree with Chesnay, that Optional is not so perfect especially in places other than return value.
> 
> Some other problems I could see:
> 
> 1. Optional is a generic type, and generic types not always behave well with the TypeInformation due to type erasure.
> 2. Optionals due not work well with Reflection
> 3. Using Optional as an argument creates lots of boilerplate code:
> 
> If we have a method like:
> 
> 	public void myMethod(String arg1, Optonal<String> arg2);
> 
> We would need to call it:
> 
> 	myMethod(arg1, Optional.ofNullable(arg2));
> 	myMethod(arg1, Optional.empty());
> 
> Instead I think a better solution is just overloading:
> 
> 	public void myMethod(String arg1);
> 	public void myMethod(String arg1, String arg2);
> 
> Therefore I think Optional fits only in cases where we want explicitly tell that a method may not return any value.
> 
> 
> 
>> On 25 Aug 2017, at 10:10, Chesnay Schepler <ch...@apache.org> wrote:
>> 
>> I think you're overselling optional a little bit.
>> 
>> The unfortunate fact is that any Optional /can still be null itself/.
>> 
>> As such, if there a method accepts an Optional it still has to check whether it is null, /and in addition whether it contains a null.
>> /i.e.
>> 
>>  public void myMethod(Optional<String> parameter) {
>>  	if (parameter != null) {
>>  		if (parameter.isPresent()) {
>>  			<do something with it>
>>  		}
>>  	}
>>  }
>> 
>> Effectively this just introduces redundancy, albeit with some syntactic sugar on top.
>> The same applies for Optional fields.
>> 
>> As for your example as to how it is "/easier to maintain the ban on null",/ since passing null is just as valid if
>> the parameter was an optional wouldn't I still have to go to the implementation to check it?
>> 
>> On 24.08.2017 16:58, Piotr Nowojski wrote:
>>> Hi,
>>> 
>>> Since we switched to Java 8, I would like to start a discussion about a deprecating usage of @Nullable parameters and return values in Flink and enforcing strict ban on nulls in those places. I just think that having a very explicit way, enforced by a static type checking mechanism of optional values is just so much better and safer compared to constantly thinking if the value that you have can be a null or it can not.
>>> 
>>> Personally I would go even one step further and I would ban nulls as fields/local variables and always use in those cases Optional, with a very rare exceptions for performance reasons. This however has two drawbacks:
>>> 
>>> 1. Optional was not intended by the Java committee to be used on fields. I don’t why, because for me it is just better compared to handling nulls.
>>> 2. Optional is not serializable (that’s the collateral damage caused by 1.). Thus in many places for fields we would have to use our own custom OptionalSerializable.
>>> 
>>> Regardless of using Optional for a fields as well, transition from Nullable to Optional should be fairly simple. We could go module by module deprecating old methods taking @Nullable and replacing them with overloaded Optional versions. The only issue is that in the short term it could confuse a little bit users, if we present them two versions of methods and with inconsistency between modules. However I think it’s worth it in the long term.
>>> 
>>> As a side note it is also easier to maintain the ban on nulls, compared to putting @Nullable annotation in appropriate places. With Optional, reviewer could always immediately spot each usage of “null". With @Nullable, if new pull request adds some invocation of a method:
>>> 
>>> foo.bar(arg1, null, arg3);
>>> 
>>> It is impossible to know whether it is allowed, without going to the implementation of the method and checking for presence or lack of the annotation. Thus we have some places in the code where there are undocumented/not annotated @Nullable fields/parameters (even in the public api).
>>> 
>>> What do you think?
>>> 
>>> Piotrek
>> 
>> 
> 

Re: Optional vs Nullable

[Dawid Wysakowicz <wy...@gmail.com>]

I agree with Chesnay, that Optional is not so perfect especially in places other than return value.

Some other problems I could see:

1. Optional is a generic type, and generic types not always behave well with the TypeInformation due to type erasure.
2. Optionals due not work well with Reflection
3. Using Optional as an argument creates lots of boilerplate code:

If we have a method like:

	public void myMethod(String arg1, Optonal<String> arg2);

We would need to call it:

	myMethod(arg1, Optional.ofNullable(arg2));
	myMethod(arg1, Optional.empty());

Instead I think a better solution is just overloading:

	public void myMethod(String arg1);
	public void myMethod(String arg1, String arg2);

Therefore I think Optional fits only in cases where we want explicitly tell that a method may not return any value.



> On 25 Aug 2017, at 10:10, Chesnay Schepler <ch...@apache.org> wrote:
> 
> I think you're overselling optional a little bit.
> 
> The unfortunate fact is that any Optional /can still be null itself/.
> 
> As such, if there a method accepts an Optional it still has to check whether it is null, /and in addition whether it contains a null.
> /i.e.
> 
>   public void myMethod(Optional<String> parameter) {
>   	if (parameter != null) {
>   		if (parameter.isPresent()) {
>   			<do something with it>
>   		}
>   	}
>   }
> 
> Effectively this just introduces redundancy, albeit with some syntactic sugar on top.
> The same applies for Optional fields.
> 
> As for your example as to how it is "/easier to maintain the ban on null",/ since passing null is just as valid if
> the parameter was an optional wouldn't I still have to go to the implementation to check it?
> 
> On 24.08.2017 16:58, Piotr Nowojski wrote:
>> Hi,
>> 
>> Since we switched to Java 8, I would like to start a discussion about a deprecating usage of @Nullable parameters and return values in Flink and enforcing strict ban on nulls in those places. I just think that having a very explicit way, enforced by a static type checking mechanism of optional values is just so much better and safer compared to constantly thinking if the value that you have can be a null or it can not.
>> 
>> Personally I would go even one step further and I would ban nulls as fields/local variables and always use in those cases Optional, with a very rare exceptions for performance reasons. This however has two drawbacks:
>> 
>> 1. Optional was not intended by the Java committee to be used on fields. I don’t why, because for me it is just better compared to handling nulls.
>> 2. Optional is not serializable (that’s the collateral damage caused by 1.). Thus in many places for fields we would have to use our own custom OptionalSerializable.
>> 
>> Regardless of using Optional for a fields as well, transition from Nullable to Optional should be fairly simple. We could go module by module deprecating old methods taking @Nullable and replacing them with overloaded Optional versions. The only issue is that in the short term it could confuse a little bit users, if we present them two versions of methods and with inconsistency between modules. However I think it’s worth it in the long term.
>> 
>> As a side note it is also easier to maintain the ban on nulls, compared to putting @Nullable annotation in appropriate places. With Optional, reviewer could always immediately spot each usage of “null". With @Nullable, if new pull request adds some invocation of a method:
>> 
>> foo.bar(arg1, null, arg3);
>> 
>> It is impossible to know whether it is allowed, without going to the implementation of the method and checking for presence or lack of the annotation. Thus we have some places in the code where there are undocumented/not annotated @Nullable fields/parameters (even in the public api).
>> 
>> What do you think?
>> 
>> Piotrek
> 
> 

Re: Optional vs Nullable

[Chesnay Schepler <ch...@apache.org>]

I think you're overselling optional a little bit.

The unfortunate fact is that any Optional /can still be null itself/.

As such, if there a method accepts an Optional it still has to check 
whether it is null, /and in addition whether it contains a null.
/i.e.

    public void myMethod(Optional<String> parameter) {
    	if (parameter != null) {
    		if (parameter.isPresent()) {
    			<do something with it>
    		}
    	}
    }

Effectively this just introduces redundancy, albeit with some syntactic 
sugar on top.
The same applies for Optional fields.

As for your example as to how it is "/easier to maintain the ban on 
null",/ since passing null is just as valid if
the parameter was an optional wouldn't I still have to go to the 
implementation to check it?

On 24.08.2017 16:58, Piotr Nowojski wrote:
> Hi,
>
> Since we switched to Java 8, I would like to start a discussion about a deprecating usage of @Nullable parameters and return values in Flink and enforcing strict ban on nulls in those places. I just think that having a very explicit way, enforced by a static type checking mechanism of optional values is just so much better and safer compared to constantly thinking if the value that you have can be a null or it can not.
>
> Personally I would go even one step further and I would ban nulls as fields/local variables and always use in those cases Optional, with a very rare exceptions for performance reasons. This however has two drawbacks:
>
> 1. Optional was not intended by the Java committee to be used on fields. I don’t why, because for me it is just better compared to handling nulls.
> 2. Optional is not serializable (that’s the collateral damage caused by 1.). Thus in many places for fields we would have to use our own custom OptionalSerializable.
>
> Regardless of using Optional for a fields as well, transition from Nullable to Optional should be fairly simple. We could go module by module deprecating old methods taking @Nullable and replacing them with overloaded Optional versions. The only issue is that in the short term it could confuse a little bit users, if we present them two versions of methods and with inconsistency between modules. However I think it’s worth it in the long term.
>
> As a side note it is also easier to maintain the ban on nulls, compared to putting @Nullable annotation in appropriate places. With Optional, reviewer could always immediately spot each usage of “null". With @Nullable, if new pull request adds some invocation of a method:
>
> foo.bar(arg1, null, arg3);
>
> It is impossible to know whether it is allowed, without going to the implementation of the method and checking for presence or lack of the annotation. Thus we have some places in the code where there are undocumented/not annotated @Nullable fields/parameters (even in the public api).
>
> What do you think?
>
> Piotrek