Fix crashes (#316)

* Fix NPE on inspecting non-existent files

FileStore#packageName and FileStore#modified used to raise NPE when a
specified file does not exist. This patch makes them return null so that
callers can handle them.

* Fix leaks of ReusableCompiler.Borrow

ReusableCompiler.Borrow used to be leaked when a runtime exception is
thrown in CompileBatch.needsAdditionalSources. This patch ensures to
release it. It should fix "Compiler is already in-use!" errors that
repeat after a runtime exception.

Author: nya3jp

src/main/java/org/javacs/Cache.java

@@ -53,6 +53,7 @@ boolean needs(Path file, K k) {
         // If key was loaded before file was last modified, it needs to be reloaded
         var value = map.get(key);
         var modified = FileStore.modified(file);
+        if (modified == null) return true;
         // TODO remove all keys associated with file when file changes
         return value.created.isBefore(modified);
     }

src/main/java/org/javacs/CompileBatch.java

@@ -58,9 +58,11 @@ Set<Path> needsAdditionalSources() {
             if (!isValidFileRange(err)) continue;
             var className = errorText(err);
             var packageName = packageName(err);
-            var location = findPackagePrivateClass(packageName, className);
-            if (location != FILE_NOT_FOUND) {
-                addFiles.add(location);
+            if (packageName != null) {
+                var location = findPackagePrivateClass(packageName, className);
+                if (location != FILE_NOT_FOUND) {
+                    addFiles.add(location);
+                }
             }
         }
         return addFiles;

src/main/java/org/javacs/FileStore.java

@@ -138,7 +138,11 @@ static Instant modified(Path file) {
             readInfoFromDisk(file);
         }
         // Look up modified time from cache
-        return javaSources.get(file).modified;
+        var info = javaSources.get(file);
+        if (info == null) {
+            return null;
+        }
+        return info.modified;
     }
 
     static String packageName(Path file) {
@@ -147,7 +151,11 @@ static String packageName(Path file) {
             readInfoFromDisk(file);
         }
         // Look up package name from cache
-        return javaSources.get(file).packageName;
+        var info = javaSources.get(file);
+        if (info == null) {
+            return null;
+        }
+        return info.packageName;
     }
 
     public static String suggestedPackageName(Path file) {
@@ -157,7 +165,7 @@ public static String suggestedPackageName(Path file) {
             for (var sibling : javaSourcesIn(dir)) {
                 if (sibling.equals(file)) continue;
                 var packageName = packageName(sibling);
-                if (packageName.isBlank()) continue;
+                if (packageName == null || packageName.isBlank()) continue;
                 var relativePath = dir.relativize(file.getParent());
                 var relativePackage = relativePath.toString().replace(File.separatorChar, '.');
                 if (!relativePackage.isEmpty()) {
@@ -337,7 +345,7 @@ private static String patch(String sourceText, TextDocumentContentChangeEvent ch
                 int lines = change.range.end.line - change.range.start.line;
                 int chars = change.range.end.character;
                 for (int lineSkip = 0; lineSkip < lines; lineSkip++) {
-                    reader.readLine();                    
+                    reader.readLine();
                 }
                 reader.skip(chars);
             }

src/main/java/org/javacs/JavaCompilerService.java

@@ -64,6 +64,7 @@ private void loadCompile(Collection<? extends JavaFileObject> sources) {
             }
             cachedCompile.borrow.close();
         }
+        cachedCompile = null;
         cachedCompile = doCompile(sources);
         cachedModified.clear();
         for (var f : sources) {
@@ -74,7 +75,14 @@ private void loadCompile(Collection<? extends JavaFileObject> sources) {
     private CompileBatch doCompile(Collection<? extends JavaFileObject> sources) {
         if (sources.isEmpty()) throw new RuntimeException("empty sources");
         var firstAttempt = new CompileBatch(this, sources);
-        var addFiles = firstAttempt.needsAdditionalSources();
+        Set<Path> addFiles;
+        try {
+            addFiles = firstAttempt.needsAdditionalSources();
+        } catch (RuntimeException e) {
+            firstAttempt.close();
+            firstAttempt.borrow.close();
+            throw e;
+        }
         if (addFiles.isEmpty()) return firstAttempt;
         // If the compiler needs additional source files that contain package-private files
         LOG.info("...need to recompile with " + addFiles);
@@ -190,7 +198,7 @@ public List<String> publicTopLevelTypes() {
             if (!fileName.endsWith(".java")) continue;
             var className = fileName.substring(0, fileName.length() - ".java".length());
             var packageName = FileStore.packageName(file);
-            if (!packageName.isEmpty()) {
+            if (packageName != null && !packageName.isEmpty()) {
                 className = packageName + "." + className;
             }
             all.add(className);
@@ -207,7 +215,8 @@ public List<String> packagePrivateTopLevelTypes(String packageName) {
 
     private boolean containsImport(Path file, String className) {
         var packageName = packageName(className);
-        if (FileStore.packageName(file).equals(packageName)) return true;
+        // Note: FileStore.packageName may return null.
+        if (packageName.equals(FileStore.packageName(file))) return true;
         var star = packageName + ".*";
         for (var i : readImports(file)) {
             if (i.equals(className) || i.equals(star)) return true;

src/main/java/org/javacs/SourceFileManager.java

@@ -44,7 +44,7 @@ public String inferBinaryName(Location location, JavaFileObject file) {
             var source = (SourceFileObject) file;
             var packageName = FileStore.packageName(source.path);
             var className = removeExtension(source.path.getFileName().toString());
-            if (!packageName.isEmpty()) className = packageName + "." + className;
+            if (packageName != null && !packageName.isEmpty()) className = packageName + "." + className;
             return className;
         } else {
             return super.inferBinaryName(location, file);

src/main/java/org/javacs/SourceFileObject.java

@@ -119,7 +119,9 @@ public long getLastModified() {
         if (contents != null) {
             return modified.toEpochMilli();
         }
-        return FileStore.modified(path).toEpochMilli();
+        var fileModified = FileStore.modified(path);
+        if (fileModified == null) return 0;
+        return fileModified.toEpochMilli();
     }
 
     @Override

src/test/java/org/javacs/FileStoreTest.java

@@ -1,6 +1,6 @@
 package org.javacs;
 
-import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.*;
 import static org.junit.Assert.assertThat;
 
 import java.util.Set;
@@ -19,4 +19,11 @@ public void packageName() {
         var file = FindResource.path("/org/javacs/example/Goto.java");
         assertThat(FileStore.suggestedPackageName(file), equalTo("org.javacs.example"));
     }
+
+    @Test
+    public void missingFile() {
+        var file = FindResource.path("/org/javacs/example/NoSuchFile.java");
+        assertThat(FileStore.packageName(file), nullValue());
+        assertThat(FileStore.modified(file), nullValue());
+    }
 }