Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Java @Nullable annotations in generated classes #1115

Open
roded opened this issue Jun 29, 2024 · 7 comments
Open

Java @Nullable annotations in generated classes #1115

roded opened this issue Jun 29, 2024 · 7 comments

Comments

@roded
Copy link

roded commented Jun 29, 2024

We use Checkerframework to ensure a project's null-safety.

In our case, this is done by assuming that all fields and parameters are annotated by @NotNull implicitly while nullable fields and parameters are explicitly annotated by a @Nullable annotations.

The generated Kaitai classes do have some nullable constructor parameters which fail the null-safty check in a project in which Checkerframework is enabled.

It would be nice to be able to instruct Kaitai to add a (customizable) @Nullable annotation where appropriate.

@GreyCat
Copy link
Member

GreyCat commented Jun 29, 2024

Unless I'm missing something, nullable was never standardized in Java? JSR305 was shot down, and then we've never got any official support for these anywhere in vendor-provided Java classes?

If that's true, this complicates generation of such annotations quite a bit — i.e. instead of following one standard (which will still probably need at less on/off flag), we'll need to support X different ways, e.g.:

@GreyCat
Copy link
Member

GreyCat commented Jun 30, 2024

Searching GitHub for all Nullable in Java, we can find that there are ~3.2M files using it. We can also explore how popular are specific implementations:

Implementation GH hits
javax.annotation.Nullable 1.1M
org.jetbrains.annotations.Nullable 922k
androidx.annotation.Nullable 481k
android.support.annotation.Nullable 416k
org.springframework.lang.Nullable 159k
org.eclipse.jdt.annotation.Nullable 62.5k
com.sun.istack.internal.Nullable 3.9k
others 55.6k

@roded
Copy link
Author

roded commented Jun 30, 2024

Yeah, what libraries usually do is allow configuring what annotation class to use as a nullable annotation. That way we, for example, could specify org.jetbrains.annotations.Nullable since that's what our Checkerframework configuration is configured for. Thanks for considering this.

@GreyCat
Copy link
Member

GreyCat commented Jun 30, 2024

Overall, sounds good then. We could add something like --java-nullable-class=... argument to CLI. The main challenge will be likely adding tests that prove that it works. Maybe something akin to what we do with C++ and valgrind — i.e. after the execution of tests runs, we add extra pass of static checking tool (?) which generates some kind of result which we interpret to add extra result statuses in our CI?

@generalmimon
Copy link
Member

@GreyCat:

We could add something like --java-nullable-class=... argument to CLI.

I agree, although calling it "class" seems a bit technically inaccurate (apparently, the correct Java term is "annotation type"). I'd rather call it --java-nullable-annotation.

@GreyCat
Copy link
Member

GreyCat commented Jun 30, 2024

Ok, so the action plan roughly seems like is:

  • Compiler code:
    • Add --java-nullable-annotation CLI argument parsing
    • Inject @Nullable and/or @NotNull annotations when this is activated in code generated for Java
  • Testing code:
    • Pick some classes implementing Nullable/NotNull
    • Adjust tests CI to fetch these new dependencies
    • Adjust builds-formats to pass new CLI arguments to compiler to force generation of dependencies (probably we can get away with always having them enabled)
    • Add a testing pass for Java only (akin to C++ Valgrind) that will execute static checker on top of regular Java code.
    • Adjust test aggregation to have parser for static checker (akin to Valgrind XML parser) and make sure it signals which tests are broken this way

@roded Do you think you'll be interested in contributing any of these?

@roded
Copy link
Author

roded commented Jun 30, 2024

@GreyCat Theoretically I'd be glad to. But I'm swamped for the next few months and I don't think I'll be able to find the time to contribute unfortunately. So long as this does not pick up more votes, feel free to keep this open till I do though.
Thanks for the attention on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants