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

Is there a way to reverse the default assumption about nullability? #94

Open
jonnermut opened this issue Dec 21, 2017 · 8 comments
Open

Comments

@jonnermut
Copy link

Couldn't find this anywhere in the doco, so I'm assuming no... but thought I'd ask the question. I have a couple of quite large code bases that pretty much assume all fields and parameters are nullable. Eg they have null checks already for everything. There have lots of hibernate and json model objects while have to have empty constructors and null initialised fields.
For these existing code bases, it seems it would be a lot less annotation effort to reverse the assumption and assume @nullable by default. Then the checker would pick up non checked uses straight away, and we could gradually add @nonnull annotations.
As it is, its going to be too big a task (currently 5k+ combined NullAway and ErrorProne warnings on one of the code bases) to get this happening.

@JakeWharton
Copy link

I don't think it's supported yet, but you can annotate packages with Checker Framework's @DefaultQualifier annotation to make the default nullable.

@msridhar
Copy link
Collaborator

Hi @jonnermut! We don't support this reversal at the moment, though with some work we could probably do it in the future. For the fields, are they in fact nullable and your code needs to have defensive null checks everywhere? Or is it that they are not null after initialization, but NullAway doesn't understand the initialization pattern? If it's the latter, and if there is some annotation on the fields or classes to indicate they are getting initialized in a special way, we could probably add support for that pretty easily. Parameters are a different story, and probably require something like flipping the default.

For adopting Error Prone gradually, you can try to use the new -XepExcludedPaths option in EP 2.1.3 to exclude large chunks of your code base from all Error Prone checks, and then gradually allow checks on more code as it is ready. It'll still be painful, but less painful then trying to get all code passing the checks at once 😄

@jonnermut
Copy link
Author

jonnermut commented Dec 21, 2017

Thanks @JakeWharton , I don't thinks its supported yet.

Thanks @msridhar .
It's a real mix, as you'd expect in a large code base. I have a ton of Spring beans it's tripping on that are annotated with @Autowired and @PostConstruct and I need to add those annotations to the config and see how much that pairs things down.

Most of really nullable fields are in model objects which model JSON (where they are almost all nullable), or Hibernate database entities (where they are a mix). In both of these cases every field is private with a public getter/setter pair which would also need annotation. In both cases the fields have to start null and have empty constructors, and they may or may not be initialised inside the frameworks (Hibernate & Jackson).

Parameters are also a mix. But the vast majority of our methods are written defensively with null guards (due to bitter experience of NPEs!), so it would be useful to set the reverse default there for sure.

Lately the team has been writing a lot of Swift, Kotlin & (recent versions of) Typescript, and the non null enforcing in those languages really shows up how primitive the null checking is in Java (and our C++ code for that matter...). So I'm keen to make this happen.

Do you think that the code is encapsulated enough that changing the logic in nullnessFromAnnotations in Nullness.java would be enough? If so I might have a go at a fork to see if it works:

  private static Nullness nullnessFromAnnotations(Element element) {
    for (AnnotationMirror anno : NullabilityUtil.getAllAnnotations(element)) {
      String annotStr = anno.getAnnotationType().toString();
      if (isNullableAnnotation(annotStr)) {
        return Nullness.NULLABLE;
      }
    }
    return Nullness.NONNULL;
  }


  public static boolean hasNullableAnnotation(Element element) {
    return nullnessFromAnnotations(element) == Nullness.NULLABLE;
  }

@msridhar
Copy link
Collaborator

@jonnermut feel free to give it a shot! I think the key issue is how NullAway should figure out whether a class has opted in to the default nullable treatment. I think we'll need to add support for something like @DefaultQualifier, both at the class and package level. And also this should work whether a class is present as source code or in a jar file. Depending on how expensive the @DefaultQualifier search is, we may need some caching as well. And we'll also have to figure out which non-null annotations to support 😄

FYI, I probably won't be able to look too much at this until after the holidays.

@caseykulm
Copy link

Could this be solved by passing a custom annotation to the -XepOpt:NullAway:ExcludedFieldAnnotations complication flag to not check certain fields?

To extend upon this, I was hoping to find a way to completely exclude a field marked with this custom annotation from any NullAway analysis, and not just initialization. I have a field that is both late initialized, and is nulled out after being used in many different functions. I would ideally like for it to not be nullable, but in the meantime it would be nice to not have to add a @SupressWarning("NullAway") annotation to the method that nulls out this object (since it might have other fields I do want NullAway to run on).

@caseykulm
Copy link

After discussing the extended use case above with a co-worker, we decided that wrapping the value in Optional can handle the extended case.

@caseykulm
Copy link

To help solve the original problem in this thread, and to even match Kotlin, a pre-baked @LateInitVar annotation could be added to NullAway as an ExcludedFieldAnnotation.

@msridhar
Copy link
Collaborator

msridhar commented Apr 5, 2018

Hi @caseykulm, I won't be able to look more until next week, but could you open a separate issue in the @LateInitVar support? That sounds like a good idea, but I think it's separate from this issue.

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

4 participants