JSpecify and annotated packages option

[msridhar]

Once we land support for @NullMarked, it seems the AnnotatedPackages option would no longer be absolutely necessary. If we end up adding a flag for "JSpecify mode", we could say that at least one of the JSpecify or AnnotatedPackages options should be given (and giving both would of course be supported). We could instead just make AnnotatedPackages optional, but I'm concerned that could lead to confusion for users who are also not using @NullMarked.

[bannmann]

I started adopting JSpecify annotations in a 20K LOC code base. In line with the recommendations in the JSpecify user guide, I planned to annotate one package after another, adding @NullMarked to each package once I finished it.

Having learned that NullAway does not yet support @NullMarked, once I finish annotating a package, I will now add it to the AnnotatedPackages option in addition to flagging the package itself as @NullMarked. The eventual goal would of course be to put @NullMarked on the module level.

Regarding configuration, I share @msridhar's concerns about just making AnnotatedPackages optional.

I suggest adding a second option to control this behavior. It could be called e.g. NullMarkedMode and its values would look something like this:

• UsePackageAllowList makes NullAway require the AnnotatedPackages option, just like today (the default value)
• OnlyAnnotatedCode limits NullAway to those packages / modules that have JSpecify @NullMarked annotations. The AnnotatedPackages option would not be required.
  • Whether that mode can be further restricted by AnnotatedPackages is up for debate. Personally, I don't see a point in that and would want to avoid the confusion by having AnnotatedPackages throw an error if it's used together with this mode.

[msridhar]

@bannmann NullAway should support @NullMarked and @NullUmarked now! Can you open a separate bug if you're seeing that it's not supported? The only place where support likely won't work yet is on modules, but I would expect packages to work.

[msridhar]

Regarding your suggested configurations options @bannmann I like them! We are going to have a messy and probably somewhat lengthy transition over to JSpecify checking, with an unfortunate amount of configuration, in order to not break existing users. I think your options fit well with such a transition plan. If, in your experimentation, it would be beneficial to have a flag that forbids AnnotatedPackages so NullAway relies entirely on @NullMarked, let us know and we can try to add it.

[msridhar]

@pemistahl @beatbrot continuing from #1042, see the discussion above. We now have a JSpecify mode flag, and we also support @NullMarked and @NullUnmarked.

@pemistahl if you are seeing a case where @NullMarked is not working, please open a separate issue on that. It should work on packages, classes, and methods in NullAway.

Note that we still require the AnnotatedPackages flag right now and crash without it. I am open to adding a flag like OnlyAnnotatedCode (I might call it OnlyNullMarkedCode) and then requiring either AnnotatedPackages or OnlyAnnotatedCode (this could be independent of JSpecify mode actually). I think AnnotatedPackages is still quite useful since it treats packages as hierarchical, whereas JSpecify does not. So if you have packages com.foo.p1 and com.foo.p2, you can specify AnnotatedPackages to be com.foo and both will be treated as annotated. With @NullMarked you would need to add two package-info.java files, one in each package. Some companies have dozens or hundreds of packages so this could make a big difference. (If you're using Java modules you could make the whole module @NullMarked but NullAway doesn't support that yet and adoption seems to be low.)

Any thoughts?

[beatbrot]

Yeah, I agree with your take @msridhar :) At my company, we have several hundreds of packages and annotating each with @NullMarked would be a huge pain for us. But since AnnotatedPackages is hierachical , it is enough for us to just specify com.<companyname> and everything counts as NullMarked.

Some parts of our codebase however is exported as API. And for this small part, we want to use @NullMarked, so that our API consumers don't have to configure their tools at all. So the ideal solution for us would be if NullAway would understand both :)

Edit: Also, I don't see us ever adopting Modules, so while the idea is nice to NullMark a module, it won't help us :)

[pemistahl]

So the ideal solution for us would be if NullAway would understand both.

Exactly, that's what I have in mind, too. I want to be able to decide whether to check on package level or on class level. We have an application with a very large code base without any static null checks. We would like to gradually add static null checks to this application but we don't want to be enforced to do this for an entire package at a time. Class by class would be much more feasible for us. @msridhar Can you please make this possible?

[msridhar]

Exactly, that's what I have in mind, too. I want to be able to decide whether to check on package level or on class level. We have an application with a very large code base without any static null checks. We would like to gradually add static null checks to this application but we don't want to be enforced to do this for an entire package at a time. Class by class would be much more feasible for us. @msridhar Can you please make this possible?

This should already be possible. @NullMarked on classes is supported, modulo any remaining bugs. So, for now, you could do the following:

1. Set AnnotatedPackages to be some dummy value, not corresponding to any real package in your code or libraries
2. Write @NullMarked on packages or classes as desired, and they should be checked by NullAway.

This issue is about getting rid of 1 and instead providing an option explicitly indicating you don't want to use AnnotatedPackages.

@pemistahl if the above does not work, could you please file a separate issue, ideally with a standalone reproducer? We will take a look.

EDIT: also note that you should not need to pass the JSpecify mode flag for the above to work; @NullMarked should be recognized out of the box.

[pemistahl]

You are right @msridhar, it works to set AnnotatedPackages to the empty string. Then I can work with @NullMarked alone to enable the checks on class level. Thanks for your help.

[sdeleuze]

@msridhar The Spring team is interested by this issue as currently we annotate only packages where null-safety is defined with @NullMarked but we have to duplicate that information with NullAway package configuration. As we plan to extend JSpecify and NullAway to other Spring projects, having a unique source of truth would be great.

Configuring NullAway:AnnotatedPackages to an empty string works as expected, and I will use that in Spring Framework for now, but that looks a bit hackish to be promoted as a general guideline, so a proper flag to enable this (allowing the user to choose between specifying packages or analysis of @NullMarked / @NullUnmaked would be a useful enhancement IMO.

We would also be ok with enabling @NullMarked detection if no package has been configured. I think you consider this is maybe too surprising for users and given the fact that support for this is not complete (Spring @NonNullApi is not supported due to #633 for example), maybe that's indeed better to have an explicit configuration for now. But middle/long term when hopefully JSpecify will be more ubiquitious and/or #633 fixed, would make sense from my POV as a default behavior.

[msridhar]

@sdeleuze thanks for the feedback. I can prioritize adding a flag like OnlyNullMarkedCode that, if provided, makes AnnotatedPackages optional. And, once JSpecify is more ubiquitous, maybe we can even make OnlyNullMarkedCode optional.