Enable error-prone check for GuardedBy, fix errors (#1621)

.idea/inspectionProfiles/Project_Default.xml

@@ -33,6 +33,7 @@
         </list>
       </option>
     </inspection_tool>
+    <inspection_tool class="FieldAccessNotGuarded" enabled="true" level="WARNING" enabled_by_default="true" />
     <inspection_tool class="FieldMayBeFinal" enabled="true" level="INFORMATION" enabled_by_default="true">
       <scope name="AllExceptTruffleAst" level="WARNING" enabled="true" />
     </inspection_tool>

build-logic/src/main/kotlin/pklJSpecify.gradle.kts

@@ -38,6 +38,7 @@ nullaway { onlyNullMarked = true }
 tasks.withType<JavaCompile>().configureEach {
   options.errorprone.disableAllChecks = true
   options.errorprone.check("StringCaseLocaleUsage", CheckSeverity.ERROR)
+  options.errorprone.check("GuardedBy", CheckSeverity.ERROR)
   options.errorprone.nullaway {
     error()
     onlyNullMarked = true

pkl-core/src/main/java/org/pkl/core/externalreader/ExternalReaderProcessImpl.java

@@ -34,7 +34,6 @@ import org.pkl.core.messaging.MessageTransport;
 import org.pkl.core.messaging.MessageTransports;
 import org.pkl.core.messaging.ProtocolException;
 import org.pkl.core.util.ErrorMessages;
-import org.pkl.core.util.LateInit;
 
 final class ExternalReaderProcessImpl implements ExternalReaderProcess {
 
@@ -51,13 +50,11 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess {
   private final Object lock = new Object();
   private @GuardedBy("lock") boolean closed = false;
 
-  @LateInit
   @GuardedBy("lock")
-  private Process process;
+  private @Nullable Process process;
 
-  @LateInit
   @GuardedBy("lock")
-  private MessageTransport transport;
+  private @Nullable MessageTransport transport;
 
   private void log(String msg) {
     if (logPrefix != null) {
@@ -96,9 +93,9 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess {
               ErrorMessages.create("externalReaderAlreadyTerminated"));
         }
 
+        assert transport != null;
         return transport;
       }
-    }
 
       // This relies on Java/OS behavior around PATH resolution, absolute/relative paths, etc.
       var command = new ArrayList<String>();
@@ -120,19 +117,24 @@ final class ExternalReaderProcessImpl implements ExternalReaderProcess {
               new ExternalReaderMessagePackEncoder(process.getOutputStream()),
               this::log);
 
-    var rxThread = new Thread(this::runTransport, "ExternalReaderProcessImpl rxThread for " + spec);
+      var myTransport = transport;
+      var rxThread =
+          new Thread(
+              () -> this.runTransport(myTransport),
+              "ExternalReaderProcessImpl rxThread for " + spec);
       rxThread.setDaemon(true);
       rxThread.start();
 
       return transport;
     }
+  }
 
   /**
    * Runs the underlying message transport so it can receive responses from the child process.
    *
    * <p>Blocks until the underlying transport is closed.
    */
-  private void runTransport() {
+  private void runTransport(MessageTransport transport) {
     try {
       transport.start(
           (msg) -> {

pkl-core/src/main/java/org/pkl/core/module/ModulePathResolver.java

@@ -65,6 +65,7 @@ public final class ModulePathResolver implements AutoCloseable {
     this.modulePath = modulePath;
   }
 
+  @GuardedBy("lock")
   private void populateCaches() throws IOException {
     fileCache = new HashMap<>();
     zipFileSystems = new ArrayList<>();
@@ -139,19 +140,18 @@ public final class ModulePathResolver implements AutoCloseable {
     }
   }
 
+  @GuardedBy("lock")
   private void populateFileCache(Path basePath) throws IOException {
     try (var stream =
         Files.find(
             basePath,
             Integer.MAX_VALUE,
             // reduce file cache size by filtering out .class files
-            // (currently `read()` only supports text resources, no known use case for accessing
+            // (no known use case for accessing .class files from Pkl code)
-            // .class files from Pkl code)
             (path, attributes) ->
                 attributes.isRegularFile() && !path.toString().endsWith(".class"))) {
       // in case of duplicate path, first entry wins (cf. class loader)
-      stream.forEach(
+      for (var path : stream.toList()) {
-          (path) -> {
         var relativized = IoUtils.relativize(path, basePath);
         assert fileCache != null;
         fileCache.putIfAbsent(IoUtils.toNormalizedPathString(relativized), path);
@@ -162,7 +162,7 @@ public final class ModulePathResolver implements AutoCloseable {
           var isDirectory = i < (relativized.getNameCount() - 1);
           element = element.putIfAbsent(name, new TreePathElement(name, isDirectory));
         }
-          });
+      }
     }
   }
 

pkl-core/src/main/java/org/pkl/core/module/ProjectDependenciesManager.java

@@ -96,6 +96,7 @@ public final class ProjectDependenciesManager {
     }
   }
 
+  @GuardedBy("lock")
   private void ensureLocalProjectDependencyInitialized(
       DeclaredDependencies localProjectDependencies, ProjectDeps projectDeps) {
     // turn `package:` scheme into `projectpackage`: scheme
@@ -162,17 +163,23 @@ public final class ProjectDependenciesManager {
     return ret;
   }
 
+  // `ensureDependenciesInitialized` makes `myDependencies` safe to access
+  @SuppressWarnings({"FieldAccessNotGuarded", "GuardedBy"})
   public Map<String, Dependency> getDependencies() {
     ensureDependenciesInitialized();
     assert myDependencies != null;
     return myDependencies;
   }
 
+  // `ensureDependenciesInitialized` makes `localPackageDependencies` safe to access
+  @SuppressWarnings({"FieldAccessNotGuarded", "GuardedBy"})
   public boolean isLocalPackage(PackageUri packageUri) {
     ensureDependenciesInitialized();
     return localPackageDependencies.containsKey(packageUri);
   }
 
+  // `ensureDependenciesInitialized` makes `localPackageDependencies` safe to access
+  @SuppressWarnings({"FieldAccessNotGuarded", "GuardedBy"})
   public Map<String, Dependency> getLocalPackageDependencies(PackageUri packageUri) {
     ensureDependenciesInitialized();
     var dep = localPackageDependencies.get(packageUri);

pkl-core/src/main/java/org/pkl/core/packages/PackageResolvers.java

@@ -87,6 +87,8 @@ final class PackageResolvers {
       checkNotClosed();
       synchronized (lock) {
         var metadata = cachedDependencyMetadata.get(uri);
+        // incorrect "condition is always false" inspection
+        //noinspection ConstantValue
         if (metadata == null) {
           metadata = doGetDependencyMetadata(uri, checksums);
           cachedDependencyMetadata.put(uri, metadata);
@@ -318,7 +320,10 @@ final class PackageResolvers {
         throws IOException, SecurityManagerException {
       var packageUri = uri.getPackageUri();
       ensurePackageDownloaded(packageUri, checksums);
-      var elem = cachedTreePathElementRoots.get(uri.getPackageUri()).getElement(uri.getAssetPath());
+      TreePathElement elem;
+      synchronized (lock) {
+        elem = cachedTreePathElementRoots.get(uri.getPackageUri()).getElement(uri.getAssetPath());
+      }
       if (elem == null) {
         throw new FileNotFoundException();
       } else if (elem.isDirectory()) {
@@ -332,7 +337,10 @@ final class PackageResolvers {
         }
         throw fileIsADirectory();
       }
-      var entries = cachedEntries.get(packageUri);
+      EconomicMap<String, ByteBuffer> entries;
+      synchronized (lock) {
+        entries = cachedEntries.get(packageUri);
+      }
       // need to normalize here but not in `doListElements` nor `doHasElement` because
       // `TreePathElement.getElement` does normalization already.
       var path = IoUtils.toNormalizedPathString(Path.of(uri.getAssetPath()).normalize());
@@ -344,7 +352,10 @@ final class PackageResolvers {
         throws IOException, SecurityManagerException {
       var packageUri = uri.getPackageUri();
       ensurePackageDownloaded(packageUri, checksums);
-      var element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath());
+      TreePathElement element;
+      synchronized (lock) {
+        element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath());
+      }
       if (element == null) {
         return Collections.emptyList();
       }
@@ -356,9 +367,11 @@ final class PackageResolvers {
         throws IOException, SecurityManagerException {
       var packageUri = uri.getPackageUri();
       ensurePackageDownloaded(packageUri, checksums);
+      synchronized (lock) {
         var element = cachedTreePathElementRoots.get(packageUri).getElement(uri.getAssetPath());
         return element != null;
       }
+    }
 
     @Override
     protected DependencyMetadata doGetDependencyMetadata(
@@ -570,6 +583,8 @@ final class PackageResolvers {
       var packageUri = uri.getPackageUri();
       synchronized (lock) {
         var fs = fileSystems.get(packageUri);
+        // incorrect "condition is always false" inspection
+        //noinspection ConstantValue
         if (fs == null) {
           var metadata = getDependencyMetadata(packageUri, checksums);
           var zipFilePath = getZipFilePath(packageUri, metadata);

pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java

@@ -156,10 +156,14 @@ public final class VmClass extends VmValue {
     EconomicMaps.put(declaredProperties, property.getName(), property);
 
     if (!property.isLocal()) {
+      synchronized (allPropertiesLock) {
+        synchronized (allHiddenPropertyNamesLock) {
           __allProperties = null;
           __allHiddenPropertyNames = null;
         }
       }
+    }
+  }
 
   @TruffleBoundary
   public void addProperties(Iterable<ClassProperty> properties) {
@@ -173,9 +177,11 @@ public final class VmClass extends VmValue {
     EconomicMaps.put(declaredMethods, method.getName(), method);
 
     if (!method.isLocal()) {
+      synchronized (allMethodsLock) {
         __allMethods = null;
       }
     }
+  }
 
   @TruffleBoundary
   public void addMethods(Iterable<ClassMethod> methods) {
@@ -471,6 +477,7 @@ public final class VmClass extends VmValue {
 
   public VmTyped getMirror() {
     synchronized (mirrorLock) {
+      //noinspection ConstantValue
       if (__mirror == null) {
         __mirror = MirrorFactories.classFactory.create(this);
       }
@@ -481,6 +488,7 @@ public final class VmClass extends VmValue {
   @TruffleBoundary
   public EconomicMap<Object, ObjectMember> getTypedToDynamicMembers() {
     synchronized (typedToDynamicMembersLock) {
+      //noinspection ConstantValue
       if (__typedToDynamicMembers == null) {
         __typedToDynamicMembers =
             createDelegatingMembers(
@@ -495,6 +503,7 @@ public final class VmClass extends VmValue {
   @TruffleBoundary
   public EconomicMap<Object, ObjectMember> getDynamicToTypedMembers() {
     synchronized (dynamicToTypedMembersLock) {
+      //noinspection ConstantValue
       if (__dynamicToTypedMembers == null) {
         __dynamicToTypedMembers =
             createDelegatingMembers(
@@ -512,6 +521,7 @@ public final class VmClass extends VmValue {
   @TruffleBoundary
   public EconomicMap<Object, ObjectMember> getMapToTypedMembers() {
     synchronized (mapToTypedMembersLock) {
+      //noinspection ConstantValue
       if (__mapToTypedMembers == null) {
         __mapToTypedMembers =
             createDelegatingMembers(
@@ -610,6 +620,7 @@ public final class VmClass extends VmValue {
   @TruffleBoundary
   public PClass export() {
     synchronized (pClassLock) {
+      //noinspection ConstantValue
       if (__pClass == null) {
         var exportedAnnotations = new ArrayList<PObject>();
         var properties =