Issue witht lombok @Builder

[jockech]

This the current issue. No matter how I try(adding @nullable or @NotNull) the error will still pop out. It seems that it has something to do with the lombok @builder

[Garciat]

Any updates on this, @lazaroclapp ? We are running into this constantly in our projects.

[msridhar]

The way Lombok Builders work (Lombok goes in and modifies the AST of Email to generate the Builder code), I think properly supporting them in NullAway will take some significant work. Making @Builder an excluded class annotation might be a good solution here for now (and possibly for the long-term).

[Garciat]

It could also implement a sub-checker/thingamajig that is aware of Lombok's semantics and makes use of its annotations to determine nullability. Assuming that it runs at a compilation stage where those are available.

[msridhar]

Sure that’s what will take work 🙂 Note that with excluded class annotations, the Builder class itself won’t get checked, but the nullability annotations should still be used when checking code using the builder.

[Garciat]

Ah yes of course. I was just thinking it wouldn't be necessary to observe Builder classes themselves; instead just being aware of Lombok semantics.

[xenoterracide]

hmm... I don't understand why this is happening, this is the decompiled class... there's no way a null should be settable, they're checked... is there a workaround? can we make NullAway aware of the lombok annotation? maybe we could ask lombok to generate code with the correct annotations?

public class FoleyConnectionEvent {
    @NonNull
    private final Monitor monitor;
    @NonNull
    private final Foley foley;
    @NonNull
    private final String gatewayId;
    @NonNull
    private final SoftwareVersions softwareVersions;
    private final Instant connectTime;

    String rfid() {
        return this.foley.getRfid();
    }

    Foley toFoley(GatewayProperties gateway) {
        String site = (String)Objects.requireNonNull(this.foley.getSite() != null ? this.foley.getSite() : gateway.getSite(), "must specify site.id in either payload or in spring Env");
        return this.foley.withSite(site);
    }

    MonitorFoleyConnection toConnection() {
        Objects.requireNonNull(this.monitor);
        Objects.requireNonNull(this.foley);
        MonitorFoleyConnection mfc = MonitorFoleyConnection.builder().connectTime(this.connectTime).gatewayId(this.gatewayId).rfid(this.foley.getRfid()).monitorSn(this.monitor.getSerialNumber()).hardwareVersion(this.monitor.getHardwareVersion()).build();
        if (this.softwareVersions != null) {
            this.softwareVersions.copyTo(mfc);
        }

        return mfc;
    }

    private static Instant $default$connectTime() {
        return Instant.now();
    }

    FoleyConnectionEvent(@NonNull final Monitor monitor, @NonNull final Foley foley, @NonNull final String gatewayId, @NonNull final SoftwareVersions softwareVersions, final Instant connectTime) {
        if (monitor == null) {
            throw new NullPointerException("monitor is marked non-null but is null");
        } else if (foley == null) {
            throw new NullPointerException("foley is marked non-null but is null");
        } else if (gatewayId == null) {
            throw new NullPointerException("gatewayId is marked non-null but is null");
        } else if (softwareVersions == null) {
            throw new NullPointerException("softwareVersions is marked non-null but is null");
        } else {
            this.monitor = monitor;
            this.foley = foley;
            this.gatewayId = gatewayId;
            this.softwareVersions = softwareVersions;
            this.connectTime = connectTime;
        }
    }

    public static FoleyConnectionEvent.FoleyConnectionEventBuilder builder() {
        return new FoleyConnectionEvent.FoleyConnectionEventBuilder();
    }

    public static class FoleyConnectionEventBuilder {
        private Monitor monitor;
        private Foley foley;
        private String gatewayId;
        private SoftwareVersions softwareVersions;
        private boolean connectTime$set;
        private Instant connectTime$value;

        FoleyConnectionEventBuilder() {
        }

        public FoleyConnectionEvent.FoleyConnectionEventBuilder monitor(@NonNull final Monitor monitor) {
            if (monitor == null) {
                throw new NullPointerException("monitor is marked non-null but is null");
            } else {
                this.monitor = monitor;
                return this;
            }
        }

        public FoleyConnectionEvent.FoleyConnectionEventBuilder foley(@NonNull final Foley foley) {
            if (foley == null) {
                throw new NullPointerException("foley is marked non-null but is null");
            } else {
                this.foley = foley;
                return this;
            }
        }

        public FoleyConnectionEvent.FoleyConnectionEventBuilder gatewayId(@NonNull final String gatewayId) {
            if (gatewayId == null) {
                throw new NullPointerException("gatewayId is marked non-null but is null");
            } else {
                this.gatewayId = gatewayId;
                return this;
            }
        }

        public FoleyConnectionEvent.FoleyConnectionEventBuilder softwareVersions(@NonNull final SoftwareVersions softwareVersions) {
            if (softwareVersions == null) {
                throw new NullPointerException("softwareVersions is marked non-null but is null");
            } else {
                this.softwareVersions = softwareVersions;
                return this;
            }
        }

        public FoleyConnectionEvent.FoleyConnectionEventBuilder connectTime(final Instant connectTime) {
            this.connectTime$value = connectTime;
            this.connectTime$set = true;
            return this;
        }

        public FoleyConnectionEvent build() {
            Instant connectTime$value = this.connectTime$value;
            if (!this.connectTime$set) {
                connectTime$value = FoleyConnectionEvent.$default$connectTime();
            }

            return new FoleyConnectionEvent(this.monitor, this.foley, this.gatewayId, this.softwareVersions, connectTime$value);
        }

        public String toString() {
            return "FoleyConnectionEvent.FoleyConnectionEventBuilder(monitor=" + this.monitor + ", foley=" + this.foley + ", gatewayId=" + this.gatewayId + ", softwareVersions=" + this.softwareVersions + ", connectTime$value=" + this.connectTime$value + ")";
        }
    }
}```

[xenoterracide]

opened an issue to add those annotations ^

[lazaroclapp]

Note that the issue as reported for the original code here is not that those fields can be set to null but that they aren't guaranteed to be set to something other than null during initialization. (A way to test if this is the only problem is to try @SuppressWarnings("NullAway.Init") instead of suppressing all NullAway warnings)

What is the exact error reported for the code in the FoleyConnectionEvent? (And, if possible, I'd also love to see the lombok annotated code previous to code generation)

[msridhar]

Yes, I'd also like to see the error message you're getting.

Also, note that there is no easy way for Error Prone / NullAway to analyze the de-Lombok'd code. So even if that code looks ok, I don't think it's going to help us.

[lazaroclapp]

Yes, I'd also like to see the error message you're getting.

Also, note that there is no easy way for Error Prone / NullAway to analyze the de-Lombok'd code. So even if that code looks ok, I don't think it's going to help us.

I think Uber is seeing the post-lombok AST, thought, at least some of the time (e.g. auto-fix sometimes fails to apply fixes internally because those are appending/prepending to AST nodes that don't exist in the source). The problem is, the AST transformations produced by Lombok don't always match what de-Lombok does when run source to source. In general, Lombok + EP is a big source of issues.

[xenoterracide]

Calebs-MBP:phg-model calebcushing$ ./gradlew compileJava

> Task :compileJava
/Users/calebcushing/IdeaProjects/phg/modules/phg-model/src/main/java/com/potreromed/phg/model/usecase/FoleyConnectionEvent.java:15: warning: [NullAway] initializer method does not guarantee @NonNull fields monitor, foley, gatewayId, softwareVersions, connectTime$value are initialized along all control-flow paths (remember to check for exceptions or early returns).
@Builder
^
    (see http://t.uber.com/nullaway )
1 warning

@Builder
public class FoleyConnectionEvent {

    @NonNull
    private final Monitor monitor;
    @NonNull
    private final Foley foley;
    @NonNull
    private final String gatewayId;
    @NonNull
    private final SoftwareVersions softwareVersions;
    @Builder.Default
    private final Instant connectTime = Instant.now();

    String rfid() {
        return foley.getRfid();
    }

    Foley toFoley( GatewayProperties gateway ) {
        var site = Objects.requireNonNull(
            foley.getSite() != null
            ? foley.getSite() : gateway.getSite(),
            "must specify site.id in either payload or in spring Env"
        );
        return foley.withSite( site );
    }

    MonitorFoleyConnection toConnection() {
        Objects.requireNonNull( monitor );
        Objects.requireNonNull( foley );
        var mfc = MonitorFoleyConnection.builder()
            .connectTime( connectTime )
            .gatewayId( gatewayId )
            .rfid( foley.getRfid() )
            .monitorSn( monitor.getSerialNumber() )
            .hardwareVersion( monitor.getHardwareVersion() )
            .build();
        if ( softwareVersions != null ) {
            softwareVersions.copyTo( mfc );
        }
        return mfc;
    }
}

note: didn't use de-lombok, just looked at the class using intellij's decompiler, and I don't see that @SuppressWarnings("NullAway.Init") does anything on the field/class

[xenoterracide]

In general, Lombok + EP is a big source of issues.

who do I have to pay to make that not true?

[msridhar]

who do I have to pay to make that not true?

Not me 😄 In general, the javac compilation model is not designed to handle annotation processors mutating ASTs, which is what Lombok does. For your example, I'm guessing Lombok generates a constructor that is invoked by the generated Builder, and injects that constructor into the FoleyConnectionEvent AST. But NullAway may check FoleyConnectionEvent before Lombok runs, so this doesn't help. I think the options here are either suppressions or building some serious smarts about how Lombok works into NullAway.

[xenoterracide]

is there a primer anywhere on how error prone works? how I could write my own plugin? I'd work on this problem if I had any clue where to begin. right now this is a very opaque problem to me.

[msridhar]

@xenoterracide what problem are you trying to solve? The specific problem of NullAway false positives with Lombok builders, or general incompatibility and flakiness between Error Prone and Gradle? The former is probably fixable with a bit of work. My impression is the latter will be much more difficult, and would probably be greatly helped by support / interest from the Lombok authors in compatibility.

[xenoterracide]

I presume you mean "error prone and lombok" not gradle, and yes I mean compatibility in general, as in some cases this is the only problem, in others I was crashing EP completely. Currently stuck between terse and ... strict, but if I could figure out how to fix that.

[msridhar]

I think your best bet is to just disable the Error Prone checks that are crashing.

[etki]

Just duplicating workaround from Lombok issue: you can safely declare builder inner class in your main class:

@Builder(builderClassName = "LombokBuilder")
class X {
    // ...
    @Foo
    public static class LombokBuilder {}
}

see how people make Jackson and Lombok builders work together: https://stackoverflow.com/a/48801237/2908793

[msridhar]

@etki have you tested that the workaround actually works for removing NullAway warnings? I don’t see how it would work.

[etki]

@msridhar It's not NullAway itself, it's a way to set SuppressWarnings on lombok builder

[msridhar]

I don’t think the warnings are on the builder. They are on the fields that look like they have no initialization.