protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

[Java] [Request] Add @Nullable annotations to java code.

Open usbpc opened this issue 6 years ago • 13 comments

The addition of info if a value can be null or not would make working with protobuf in Kotlin way easier. Since Kotlin offers a pretty good null safety when using pure Kotlin or just the java standard library.

It also takes into account when there is a @Nullable annotation present so this would be a nice addition and would prevent bugs when using code generated by protoc with Kotlin.

usbpc avatar Mar 05 '18 05:03 usbpc

@usbpc I would be willing to own this ! Please advise next steps. How can I assign this project to myself?

tanvijaywant31 avatar Mar 05 '18 23:03 tanvijaywant31

@usbpc which of the annotations ( required, optional or repeated ) correspond to @nullable ? Would it be optional ? Thanks.

tanvijaywant31 avatar Mar 06 '18 01:03 tanvijaywant31

@usbpc I would be willing to own this ! Please advise next steps. How can I assign this project to myself?

I'm myself only a user of protobuf and have not personally contributed to this project. But looking on the official protobuf website they have a section about contributing in the FAQ.

@usbpc which of the annotations ( required, optional or repeated ) correspond to @nullable ? Would it be optional ? Thanks.

I'm not sure what exactly you mean with this question. The annotation @Nullable would only be present in the generated java code for values that can have the value null. As to what annotation exactly would be used I'm not sure as there seem to be at least three versions when I look in my IDE.

How I want it to work is when I define a field in protobuf like this:

syntax = "proto3";

import "google/protobuf/wrappers.proto";
message NullableString {
    google.protobuf.StringValue some_var = 1;
}

That the getter for the field someVar in the generated java class for NullableString would have a @Nullable annotation so that some analytic tools can tell you if you forget to deal with a maybe null to prevent NullPointerExceptions.

Since a normal string defined like this string some_other_var = 2; has a default value beeing the empty string this can never be null.

usbpc avatar Mar 06 '18 05:03 usbpc

@usbpc Thanks for clarrifying.

tanvijaywant31 avatar Mar 07 '18 00:03 tanvijaywant31

Protobuf Java API never returns null. If a field is absent, the accessor will just return the default value. For this reason I think @nullable isn't useful for protobuf.

xfxyjwf avatar Mar 21 '18 00:03 xfxyjwf

I think there might be a bit of confusion here. There are (roughly) two kinds of nullability annotations:

  1. value can sometimes be null, aka "nullable"
  2. value can never be null, aka "not-null"

If the Protobuf Java API never returns null then it doesn't need to use the former, but it would still be very helpful if it used the latter.

Kotlin currently uses "platform types" for values going into or out of protobuf methods. Kotlin doesn't do any compile time null checking when dealing with platform types (ie: it behaves just like Java).

However, if the Kotlin compiler sees annotations that say a value "can never be null", then it will use the a more strict type. That means that if the parameters and return types of these methods were annotated one way or the other, then Kotlin could do compile time null checks for code that uses these methods.

More details can be found here.

tl;dr: Since Protobuf Java API never returns null then it should be annotated as never returning null. Likewise, parameters of methods that should never be null (like the parameters to the generated setters) should be annotated as not-null.

xenomachina avatar May 21 '18 19:05 xenomachina

@xenomachina As far as I know there is no such not-null annotation in Java 7 and protobuf does not take additional dependencies.

xfxyjwf avatar May 21 '18 20:05 xfxyjwf

Not even JSR-305 annotations?

I've found a workaround that may be useful to others: place a package-info.java in the same package that the proto-compiler is generating into that looks something like this:

@javax.annotation.ParametersAreNonnullByDefault
package com.example.mypackage;

The Kotlin compiler will then behave as though all parameters to methods in that package are annotated as not null, except those that are explicitly annotated otherwise.

xenomachina avatar May 22 '18 02:05 xenomachina

What about annotating arguments of setters with @NotNull? It throws NPE when null is passed anyway.

mustafin avatar Apr 17 '19 06:04 mustafin

Protobuf Java API never returns null.

This may have been true in 2018 (can't tell for sure), but it's unfortunately not 100% true today. :grimacing: Given the ffollowing .proto file:

syntax = "proto3";

enum SomeEnum {
    VALUE_ONE = 0;
    VALUE_TWO = 1;
}

message SomeMessage {
    SomeEnum some_enum = 1;
}

...with Java code being generated using protoc test.proto --java_out=java (version 3.20.1) will have an output Test.java file which contains a method defined like this:

    public static SomeEnum forNumber(int value) {
      switch (value) {
        case 0: return VALUE_ONE;
        case 1: return VALUE_TWO;
        default: return null;
      }
    }

Is there any way to disable this, so that the forNumber() method can throw exceptions in cases like this instead? :thinking:

I discovered this since I tried to enable the hack suggested by @xenomachina in our code base, but SpotBugs discovered that we now have methods defined with a non-null annotation (a custom one in our case which has @TypeQualifierDefault( { ElementType.METHOD, ElementType.PARAMETER } ), i.e. enforcing non-nullability for both return values and parameters) which do indeed return null... :grin:

perlun avatar May 04 '22 12:05 perlun

Luckily for us, https://github.com/protocolbuffers/protobuf/issues/5526 was recently reopened. Let's hope it gets implemented eventually. :pray:

perlun avatar Oct 14 '22 09:10 perlun

We should be using the javax.annotation.Nullable

googleberg avatar Oct 14 '22 17:10 googleberg

FYI: There's another method that can accept and return null values: a map field's getFooOrDefault method. And of course equals is an example of a method that accepts a null parameter.

[edit: And I think protobuf might still generate valueOf as a deprecated equivalent to forNumber?]

I don't want to claim with absolute 100% confidence that those are the only other such methods, but we've been getting by pretty well with our temporary Google-internal nullness annotations (which are recognized by our Kotlin and Checker Framework users), and I think those are the only ones in the generated code that I see nullness annotations on.


Jerry, if you get to the point that you think the protobuf team may want to investigate this soon, let me know. There have been some past discussions about dependencies (including on javax.annotation specifically), and my team has been working through some similar issues.

cpovirk avatar Oct 14 '22 19:10 cpovirk

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

github-actions[bot] avatar Jun 09 '24 10:06 github-actions[bot]

I still expect to make this happen eventually. (And it helps that there's been a renewed push toward JSpecify 1.0.)

The actual Protobuf team can step in if they'd like this to be closed, but I'm at least not letting the bot close it :)

cpovirk avatar Jun 10 '24 11:06 cpovirk