uber / NullAway

A tool to help eliminate NullPointerExceptions (NPEs) in your Java code with low build-time overhead
MIT License
3.63k stars 293 forks source link

jdk Files false positive/wrong line #912

Closed xenoterracide closed 8 months ago

xenoterracide commented 8 months ago

According to javadoc File.getParent should be @Nullable and according to intellij, Files.isDirectory( first parameter is @Nonnull, javadoc there doesn't say. So it would seem to me that the wrong line is reported, or this a false positive entirely. Either isDirectory is going to blow, or it's going to return false, either way findGitDir won't even be called if getParent() is null.

/home/xeno/IdeaProjects/gradle-semver/src/main/java/com/xenoterracide/gradle/semver/SemVerPlugin.java:68: error: [NullAway] passing @Nullable parameter 'current.getParent()' where @NonNull is required
      return findGitDir(current.getParent());
                                         ^
    (see http://t.uber.com/nullaway )
  private static boolean isGitDirHeuristic(@NonNull Path path) {
    var dotGit = path.resolve(Path.of(".git"));
    var head = dotGit.resolve(Path.of("HEAD"));
    var config = dotGit.resolve(Path.of("config"));
    var refs = dotGit.resolve(Path.of("refs"));
    var objects = dotGit.resolve(Path.of("objects"));
    return (
      Files.isDirectory(dotGit) &&
      Files.isRegularFile(head) &&
      Files.isRegularFile(config) &&
      Files.isDirectory(refs) &&
      Files.isDirectory(objects)
    );
  }

  private static Optional<Path> findGitDir(@NonNull Path current) {
    var dotGit = current.resolve(Path.of(".git"));
    if (isGitDirHeuristic(dotGit)) {
      return Optional.of(dotGit);
    } else if (Files.isDirectory(current.getParent())) {
      return findGitDir(current.getParent()); // ERROR!
    }
    return Optional.empty();
  }
com.google.errorprone:error_prone_core:2.24.1=annotationProcessor,errorprone,testAnnotationProcessor
com.uber.nullaway:nullaway:0.10.23=annotationProcessor,errorprone,testAnnotationProcessor
org.checkerframework:dataflow-nullaway:3.40.0=annotationProcessor,errorprone,testAnnotationProcessor

note: I tried giving parent it's own variable (var parent = current.getParent()), but that didn't work either.

msridhar commented 8 months ago

Thanks for the report. I've put up #913 to fix this issue. After that lands, you should get an error at the call to isDirectory().

xenoterracide commented 8 months ago

I haven't dug into that file. You might want to look at the other similar methods though? Like is regular file, just to make sure they are also properly annotated.

PS on my phone at the moment

msridhar commented 8 months ago

Our JDK models are quite incomplete, so we end up doing these one-off patches. We are currently working on a way to import the annotations from https://github.com/jspecify/jdk so we have much more complete coverage. In light of that, I'll probably continue doing one-off fixes for the time being rather than trying to systemically pull in the right models manually.