inferred / FreeBuilder

Automatic generation of the Builder pattern for Java
https://freebuilder.inferred.org
Apache License 2.0
831 stars 101 forks source link

Support @CheckForNull annotation #237

Open tkruse opened 7 years ago

tkruse commented 7 years ago

Hi, when annotating a method for freebuilder 1.12.3 like so:

package com.beantest.freebuilder;

import org.inferred.freebuilder.FreeBuilder;

import javax.annotation.CheckForNull;
import javax.annotation.Nullable;

/**
 */
@FreeBuilder
public abstract class Bus {

    @CheckForNull // freebuilder ignores CheckForNull
    @Nullable  // passed on by freebuilder, but ignored by Findbugs
    abstract String driverName();

    static class Builder extends Bus_Builder {
    }
}

Then the generated classes will not have the @javax.annotation.CheckForNull annotation (However, the @Nullable annotation is passed on). The @CheckForNull annotation however is the one that findbugs will use for class members (it's a long-standing confusion over findbugs and jsr305).

I.e. the following will not raise a warning using FIndbugs:

        Bus bus3 = new Bus.Builder().build();
        // Not OK: Findbug no warnings about NullPointerDereference
        System.out.println(bus3.driverName().length());

I'd recommend doing the same for @CheckForNull as for @Nullable.

tkruse commented 7 years ago

Note AutoValue does pass on @CheckForNull

alicederyn commented 7 years ago

Would you expect the @CheckForNull annotations to be placed on the private member fields, too? Or should those be marked @Nullable and only the getter methods marked @CheckForNull? I'm not familiar with how FindBugs treats the annotation.

tkruse commented 7 years ago

Tricky question. The problem with Findbugs is, that it's behavior often defies rational thinking. So I tried this out manually, and all I can say is that with the current version of findbugs 3.0.1, I only got consistently good results if @CheckForNull was only applied to the accessor method, and NOT to the private field.

In particular combining @CheckForNull with @Nullable, I got inconsistent behavior when both were being added to the private fields.

This might well a due to bugs in findbugs, so if you want to document the design you choose, I think you have no other option than to write down that the annotation is not applied to the field because this is known to have undesired effect in Findbugs 3.0.1

Nobody knows for sure how Findbugs should or will treat those annotations. Some description is to be found here: