Null safe getter generation for lists

[Mohit Mayank]

I was thinking it would be good to have Lombok return an empty list when the field is null.

Example:

With lombok

@Getter
private List<String> names;

Without lombok (generated code):

private List<String> names;

public List<String> getNames() {
  if (names == null) {
    names = new ArrayList<>();
  }
  return names;
}

I'd like to work on this if this looks like something worth doing.

[Marco Servetto]

This seams to be a common request....
But I can not understand why... Why do not you all just put
Collections.empyList() or List.of()
inside of your fields?
Those calls will not create a new object but simply return a constant,
so no extra memory/cpu consumption eather... they are really just
"better nulls" for lists.
Note how no one seams to ask for empty strings for null string fields,
... that is basically the same concept.

> --
> You received this message because you are subscribed to the Google Groups "Project Lombok" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/b704049c-02cf-45a3-a6a2-5673c5e9cda4%40googlegroups.com.

[Mohit Mayank]

Could you please provide an example / snippet of what you mean?

> To unsubscribe from this group and stop receiving emails from it, send an email to project...@googlegroups.com.

[Daniel López]

Probably...

@Getter

@NonNull

private List<String> names =  Collections.empyList(); 

Wouldn't that work?

D.

To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/68432585-34c6-4537-89e9-6f5b670162d4%40googlegroups.com.

[Mohit Mayank]

Okay, that makes sense. Thanks a lot!

Just a little aside, it would be better to use new ArrayList<>() instead of Collections.emptyList() as the latter won't allow adding any element to the existing list.

To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/68432585-34c6-4537-89e9-6f5b670162d4%40googlegroups.com.

[Marco Servetto]

> Okay, that makes sense. Thanks a lot!
> Just a little aside, it would be better to use new ArrayList<>() instead of Collections.emptyList() as the latter won't allow adding any element to the existing list.

??
I do not understand.
Your original request was to be able to get an empty list while reading a null.
You was planning to add elements on this "null-like" list?
Was you expecting a second call to the getter to return a new mutable
list or a refence to the old list?

> To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/c5d287d2-9a97-44d5-ac68-4fa45c947ed6%40googlegroups.com.

[Mohit Mayank]

My intention is to avoid NPEs.

Examples:

names.size()
names.add("Jon");

These should not throw NPE but work as if there is always a list (maybe empty, maybe not).

> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/c5d287d2-9a97-44d5-ac68-4fa45c947ed6%40googlegroups.com.

[Marco Servetto]

I understood you wanted to avoid NPE... but what is the desired semantic?

assert names.size()==0;
names.add("Jon");
assert names.size()==1;//or zero? are you ok to get a zero here?

> To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/7fca7169-082b-419d-a924-fb4403ae4c73%40googlegroups.com.

[Mohit Mayank]

No, certainly not.

It should return 1 at the third step.

> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/7fca7169-082b-419d-a924-fb4403ae4c73%40googlegroups.com.

[Martin Grajcar]

Actually, the request does make some sense. Imagine, you need a mutable list and memory is at premium. You can use

private List<String> names =  new ArrayList<>();

and waste a few bytes for the ArrayList object. It's rather cheap as the ArrayList gets initialized with an empty singleton array https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/util/ArrayList.java#L158. Lombok could make it completely free by initializing the field with null and providing a "smart" getter. Note than @Getter(lazy=true) has itself some memory overhead, so it's probably unusable here.

However, there are some downsides:

- the access would probably be a bit slower

- the field can't be final which may a problem

- some framework may see the null field and throw (improbable with deserialization)

- it's not thread-safe (but neither is ArrayList, so it doesn't matter)

Lombok could go even further and make every access to the field null-safe, independently of whether a getter gets generated (assuming the field is private as it can't do any magic across source files).

So this could be a separate feature like

@SaveMemory
private List<String> names = new ArrayList<>():

That all said, I don't claim this is useful enough.

To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/CAOu%2BafnEv_o1uqKuUA-8rnHqHvpGHtY7%3D%2BAJpMcQD38fCs7HLw%40mail.gmail.com.

[Reinier Zwitserloot]

The overhead getterlazy introduces is required for the operation to be threadsafe.

Making a 'thread unsafe lazy getter', especially one where null has special meaning (with the current impl of lazygetter, an expensive expression that produces null is still 'cached', if you want maximum cheapness, we can say that null is the same as 'to be calculated', effectively) – would indeed be faster.

But here's the unfortunate truth: That is either sucky code, or more likely a misjudgement of priorities, where programmers are wasting developer time thinking of irrelevant things like 'maybe this is more efficient if I lazily init this list'.

Lombok is not in the business of enabling bad behaviour. No matter how widespread this bad behaviour is.

This feature is not happening.

> To unsubscribe from this group and stop receiving emails from it, send an email to project-lombok+unsubscribe@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/c5d287d2-9a97-44d5-ac68-4fa45c947ed6%40googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Project Lombok" group.

To unsubscribe from this group and stop receiving emails from it, send an email to project-lombok+unsubscribe@googlegroups.com.

[Martin Grajcar]

On Mon, Feb 17, 2020 at 7:05 PM Reinier Zwitserloot <rein...@gmail.com> wrote:

Just to make sure, we understand each other:

- I was just trying to interpret the original request so it makes an useful feature

- I'm rather sure, the feature I proposed (which may not fit the original purpose) is useful (not just a bad style)

- I'm in no way claiming, it's useful enough

The overhead getterlazy introduces is required for the operation to be threadsafe.

Making a 'thread unsafe lazy getter', especially one where null has special meaning (with the current impl of lazygetter, an expensive expression that produces null is still 'cached', if you want maximum cheapness, we can say that null is the same as 'to be calculated', effectively) – would indeed be faster.

What I was after is rather unrelated to lazygetter:

- It saves memory instead of time

- It's not thread-safe and it's to be used mainly with collections like ArrayList or HashMap

 

But here's the unfortunate truth: That is either sucky code, or more likely a misjudgement of priorities, where programmers are wasting developer time thinking of irrelevant things like 'maybe this is more efficient if I lazily init this list'.

Thinking about saving time by not initializing empty collections is definitely a waste of time. But there's a sure memory saving possible. Doing this manually makes hardly ever sense, but getting this in a safe way and basically for free makes sense, when you have many thousands of such objects where most of the collections stay empty.

It's like what we got when ArrayList was changed to start with an empty singleton array in order to save about 32 bytes per field. This feature would save additional 32 bytes.

That all said, I'm not claiming that it's important enough not I'm claiming it's what the OP wanted. I'm only claiming that it's not a bad behavior support.

 

> To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/c5d287d2-9a97-44d5-ac68-4fa45c947ed6%40googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "Project Lombok" group.

To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/CAOu%2BafnEv_o1uqKuUA-8rnHqHvpGHtY7%3D%2BAJpMcQD38fCs7HLw%40mail.gmail.com.

--

You received this message because you are subscribed to the Google Groups "Project Lombok" group.

To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/3e00a662-922d-4080-828b-81e1ce25157a%40googlegroups.com.

[Reinier Zwitserloot]

On Tuesday, February 18, 2020 at 1:54:42 PM UTC+1, Maaartin G wrote:

On Mon, Feb 17, 2020 at 7:05 PM Reinier Zwitserloot <rein...@gmail.com> wrote:

Just to make sure, we understand each other:

- I was just trying to interpret the original request so it makes an useful feature

I do apologize, rereading what I said, that came across as needlessly and unintentionally harsh.

What I was trying to communicate was: Thanks for brainstorming on the topic, figuring out the use case and what catering to it would look like. Taking into account this use case and all the options we've come up with, as well as the feedback provided so far, we don't think it is a good idea to add this feature in the foreseeable future, and we're pretty sure that the brainstorming has gone on long enough that it'd take some remarkable new evidence to 'reopen the case', as it were. We really appreciate this feedback and the brainstorming, but to ensure nobody gets too tired out, once it is clear that a feature is not happening, we should say so, so everybody can move on.

But what I said sounded a lot meaner than that, so, that's my bad!

[Logan Russell]

I just want to add from my point of view on this. 

Whenever I'm using lists, I want to make sure that the list is never null so I can avoid null checks anytime the list is used.
To achieve this, I use a special "list getter/setter" or "lister" because I use the same for both getter & setter. 

Here's how it usually looks:

public class ClassWithLists{
   private List<String> strings;
   private List<Integer> numbers;

   private <T> List<T> lister(List<T> list){
       return list == null ? new ArrayList<>() : list;
   }

    public List<String> getStrings(){
        return lister(this.strings);
    }

    public void setStrings(List<String> list){
        this.strings = lister(list);
    }

    ... (other stuff)
}

I think an annotation would be great for eliminating this "boilerplate" code in this scenario. I envision something like this

@Data
public class ClassWithLists{
   @NeverNull(ArrayList.class)
   private List<String> strings;
   @NeverNull(LinkedList.class)
   private List<Integer> numbers;
    ... (other stuff)
}

This will prevent the lists from ever being null, and greatly cleans up the rest of the code from null checks. 
This also would allow you to set the default implementation for the List in case it is null, and an empty, mutable list needs to be allocated.

The above enables code like the following to be safe:

public static void main(String[] args){
    ClassWithLists cwl = new ClassWithLists();
    System.out.println(cwl.getStrings().size());     // "0"
    cwl.setStrings(null);                            // void
    System.out.println(cwl.getStrings().add("Test"));// "True" (successfully added to whatever list was returned by cwl.getStrings
    System.out.println(cwl.getStrings().size());     // "1" (shows that the list returned was actually
    List<Strings> ls = cwl.getStrings();             //
    ls = null;                                       // This is just to show that in java, this doesn't behave like a pointer and change the list inside cwl object
    System.out.println(cwl.getStrings().size());     // This is the verification the the previous line didn't actually change the object, and that it still has 1 String in it
}

With a standard getter & setter, you never know if the list will be null or not, and so you have to put null checks all over to avoid Exceptions. 

The @Data, @Getter, and @Setter annotations are already super useful, but having to manually override them by typing these out for each class that uses Lists is an annoyance since I'm used to not having to do that for everything else.

Lastly, the only other configurable option I could see with this is the behavior when null. As I've shown above, null acts as a "reset" or "clear" by discarding whatever list was previously set, and using a completely new List object, potentially a different implementation as well. A reset-on-null option could be provided in the annotation, where the setter will do nothing when passed a null value if the option is set to false. Additionally, I could foresee an option being added to the @Data(and any other annotation that includes both @Getter & @Setter) annotation so that the @NeverNull doesn't have to be listed on each List field. 


That's just my take on the matter. I hope you can see this from a new point a view. If anything I'm doing is fundamentally wrong, please let me know. Thanks.

[Marco Servetto]

> I just want to add from my point of view on this.
> Whenever I'm using lists, I want to make sure that the list is never null so I can avoid null checks anytime the list is used.

I understand what you are doing, I do not understand why you are
coding in this way.
-Do you have anything against the general idea that the constructor
should "initialise" the object? so that after calling the constructor
if a field is null, then it is "null on purpose".
Otherwise, the constructor would initialise such field; in your case
with an empty list.

-On your RESET on null notion:
I assume you are aware that collection have a .clear() method.
Are you instead worrying about losing control of the aliasing
relationship of your list objects?
In this case, you may want to use Collections.unmodifiableXXX(..) wrappers.

[Reinier Zwitserloot]

@Logan

your code makes no sense.

1. Why don't you just write `private List<String> strings = new ArrayList<>()` instead?

2. What possibly point is there in returning a brand new arraylist if the field is null? Why not return `List.of()` instead?

To highlight how weird your code is, let me javadoc your setter for you:

/**

 * Sets the strings list.

 * I don't care if the provided list is mutable or not; your choice.

 * If you pass null, then the setter will be blanked out. However, any calls to the get method will get a brand new empty mutable array list; each caller gets their own list.

 * If you pass non-null, even if you pass an empty list or an immutable empty list, all calls to the get method will get that exact list.

 */

Note that your test code is broken; the second invoke to cwl.getStrings.size() would be returning 0. Even if we fix it, I'm not sure you'd want your API to work this way. But if you did, why not [A] init the field with an empty arraylist, and [B] update the setter to: this.strings = strings == null ? new ArrayList<>() : strings; – and now you get this API without this strange nullguarding. Lombok cannot remove the initialization for you (We have no idea what kind of list you'd want), so then all lombok is doing is removing that ternary op? Lombok wouldn't do that - that is still a code smell! null is not the same as empty, and if you treat it like that, you're messing up. An empty list is easy to make and represents an empty list. There's no need to bring null into this. The proper solution would be for the setter to throw if you pass null to it, and lombok already supports that.

Can we close this chapter?

[Logan Russell]

1. I have never said that you can't initialize the List however you want. Writing `private List<String> strings = new ArrayList<>()` would be perfectly fine. You could also initialize it in a constructor. 
2. The purpose of not using `List.of()` is that returns an unmodifiable list. Returning a new ArrayList means the returned list is modifiable. 

I'm not sure why you're saying that the code is broken. It returns all the values that I've put in the comments after each line. Paste it into a class, initialize the list however you please, and then run the main. You'll see the list getting modified. 

The second invocation of the cwl.getStrings().size() does not return 0. I'm running the code myself. It really does print "1" as the list has been successfully modified. 

The point I'm trying to make is this:

As programmers, we get to decide which values are valid arguments. For example, the Java Collections API says this when talking about concurrent collection implementations and why null values aren't allowed: "null arguments and return values cannot be reliably distinguished from the absence of elements." https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentSkipListSet.html -> from this same logic, when someone passes a value of null to the setter, the API is making a clearly documented assumption, that null can be used to signal something else. The reason it can be used to signal something else is because Java allows methods to be called with null as an argument. Even though we don't want to accept it, our options are either, throw an exception, or handle it some way. The API as I've outlined, says that null will be used to scrap the current list and start a new one. Other valid options include a no-op, so that nothing will happen, or a typical setter, in which the list would be set to null (this is the specific scenario I'm trying to avoid), or clearing the existing list, but using the same reference.

I know in this example, I was using an ArrayList which is not a concurrent collection implementation, but I'd still, just the same as most concurrent collection implementations, disallow null arguments and return values. Rather than throwing an IllegalArgumentException, I'd like the null value to be handled in some way. It could either do nothing, clear the list, or something else entirely. It's up to the API creator to decide what that null value actually does, and the API users to read the documentation for the API that they're calling. 

An API that will never return null is very beneficial. Since you didn't like the previous example I put out, maybe you'd be more inclined to ponder this example:

public class OtherClassWithLists {

    private List<String> strings;
    private List<Integer> numbers;

    public OtherClassWithLists() {
        strings = new ArrayList<>();
    }

    public List<String> getStrings(){
        return this.strings;

    }

    public void setStrings(List<String> list){

        if(list == null)
            strings.clear();
        else
            strings = list;

    }

    public static void main(String[] args) {

        OtherClassWithLists cwl = new OtherClassWithLists();

        System.out.println(cwl.getStrings().size());     // "0"
        cwl.setStrings(null);                            // void

        System.out.println(cwl.getStrings().add("Test"));// "true" (successfully added to whatever list was returned by cwl.getStrings

        System.out.println(cwl.getStrings().size());     // "1" (shows that the list returned was actually

        List<String> ls = cwl.getStrings();              //

        ls = null;                                       // This is just to show that in java, this doesn't behave like a pointer and change the list inside cwl object
        System.out.println(cwl.getStrings().size());     // This is the verification the the previous line didn't actually change the object, and that it still has 1 String in it

        List<String> list1 = cwl.getStrings();
        cwl.setStrings(null);
        System.out.println(cwl.getStrings().size());     // "0" null = clear list
        System.out.println(list1==cwl.getStrings());     // "true" - they are the same reference
    }
}

If this kind of "never null" behavior could be added with an annotation for Lists, it would save a lot of time is all that I'm saying. It's not unprecedented to disallow null arguments and/or return values. When this type of setup is desired, the typical @Setter annotation isn't helpful, because you'd still have to go through and add the if-else in each setter method. 

@NeverNull
private List<String> strings;
where a null value clears the list, as shown above, would be a super helpful annotation. In this case, it's just a specialization for the @Setter annotation.

Using this annotation would bring all the benefits that I've showed above, that the list will never be null and cleans up the code significantly. For me, I use this mostly for Lists, but it could be extended to Collections in general or a variety of different Objects

[Marco Servetto]

After reading this code, I think you are thinking about collections in
an objects in the UML/relational sense instead of the "lists are just
objects" sense.
If that is the case, then your code should look like the following:
class Person{
private List<Foo> foos=new ArrayList<Foo>();//created internally, so
we control the aliasing of this list
public List<Foo> getFoos(){return new Collections.unmodifiableList(foos);}
public void setFoos(List<Foo> foos){
this.foos.clear();
if(foos!=null){this.foos.addAll(foos);}//still, I think throwing
error on null would be cleaner!
}
public Person(List<Foo> foos){setFoos(foos);}
}

Basically, I think the point we are making on this mailing list, is
that there are many ways to make sure that objects keep their state
well encapsulate, and doing it in a fully general way is impossible
with the weak java type system, so any solution need to be customised
to your specific case, so it can not be just generated.

> --
> You received this message because you are subscribed to the Google Groups "Project Lombok" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to project-lombo...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/project-lombok/0753db09-efe1-40f9-a4ce-016c61b1b662%40googlegroups.com.

[Martin Grajcar]

On Thu, Mar 19, 2020 at 9:08 PM Logan Russell <logan.w...@gmail.com> wrote:

As programmers, we get to decide which values are valid arguments. For example, the Java Collections API says this when talking about concurrent collection implementations and why null values aren't allowed: "null arguments and return values cannot be reliably distinguished from the absence of elements." https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentSkipListSet.html

This is a valid argument, but it's not the only one. Another reason is that avoiding null everywhere makes you pretty safe from NPE. This is the approach of Guava (one of the most used libraries) and I can confirm it works well.

 

-> from this same logic, when someone passes a value of null to the setter, the API is making a clearly documented assumption, that null can be used to signal something else. The reason it can be used to signal something else is because Java allows methods to be called with null as an argument. Even though we don't want to accept it, our options are either, throw an exception, or handle it some way. The API as I've outlined, says that null will be used to scrap the current list and start a new one. Other valid options include a no-op, so that nothing will happen, or a typical setter, in which the list would be set to null (this is the specific scenario I'm trying to avoid), or clearing the existing list, but using the same reference.

You're right, there are many options. The API designer can do anything they want and the poor API consumer will suffer. I'm using a simple convention with a minor exception: Just always throw, except when accepting input from the outside (JSON), where null can be converted to empty or ignored. This should happen ASAP, so maybe 99% of the code is null-hostile.

This works fine and makes the API easy to use. When I want to clear the list, I call clear. When I want something else, I call something else. No surprise, no problem.

[Reinier Zwitserloot]

On Thursday, March 19, 2020 at 9:08:11 PM UTC+1, Logan Russell wrote:

As programmers, we get to decide which values are valid arguments.

Certainly; and lombok does not get in the way if you write your own getter and setter.

But if you want lombok to generate it, the name of the annotation, such as '@NeverNull' must both strongly suggest what lombok would generate, and the feature as is should be useful to a large-ish group of people.

Let's put this to rest; the case has not been made for either.