Fix caching of globbed reads

Problem:
The result of a globbed read depends not only on the glob pattern but also on the current module URI.
However, ResourceManager does not take this into account when caching globbed reads, causing wrong results.

Changes:
- Cache globbed reads per read expression.
  This is also how globbed imports are cached, except that import URIs are constant.
- Make ReadGlobNode and ImportGlobNode code as similar as possible.
- Reduce code duplication by inheriting from AbstractReadNode.
- Add some tests.

Author: odenix

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/AbstractImportNode.java

@@ -18,16 +18,19 @@
 import com.oracle.truffle.api.source.SourceSection;
 import java.net.URI;
 import org.pkl.core.ast.ExpressionNode;
+import org.pkl.core.module.ResolvedModuleKey;
 
 public abstract class AbstractImportNode extends ExpressionNode {
+  protected final ResolvedModuleKey currentModule;
   protected final URI importUri;
 
-  AbstractImportNode(SourceSection sourceSection, URI importUri) {
+  AbstractImportNode(SourceSection sourceSection, ResolvedModuleKey currentModule, URI importUri) {
     super(sourceSection);
+    this.currentModule = currentModule;
     this.importUri = importUri;
   }
 
-  public URI getImportUri() {
+  public final URI getImportUri() {
     return importUri;
   }
 }

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/AbstractReadNode.java

@@ -30,30 +30,33 @@
 import org.pkl.core.util.Nullable;
 
 public abstract class AbstractReadNode extends UnaryExpressionNode {
-  private final ModuleKey moduleKey;
+  protected final ModuleKey currentModule;
 
-  protected AbstractReadNode(SourceSection sourceSection, ModuleKey moduleKey) {
+  protected AbstractReadNode(SourceSection sourceSection, ModuleKey currentModule) {
     super(sourceSection);
-    this.moduleKey = moduleKey;
+    this.currentModule = currentModule;
   }
 
   @TruffleBoundary
-  protected @Nullable Object doRead(String resourceUri, VmContext context, Node readNode) {
-    var resolvedUri = resolveResource(moduleKey, resourceUri);
-    return context.getResourceManager().read(resolvedUri, readNode).orElse(null);
-  }
-
-  private URI resolveResource(ModuleKey moduleKey, String resourceUri) {
-    URI parsedUri;
+  protected final URI parseUri(String resourceUri) {
     try {
-      parsedUri = IoUtils.toUri(resourceUri);
+      return IoUtils.toUri(resourceUri);
     } catch (URISyntaxException e) {
       throw exceptionBuilder()
           .evalError("invalidResourceUri", resourceUri)
           .withHint(e.getReason())
           .build();
     }
+  }
 
+  @TruffleBoundary
+  protected final @Nullable Object doRead(String resourceUri, VmContext context, Node readNode) {
+    var resolvedUri = resolveResource(currentModule, resourceUri);
+    return context.getResourceManager().read(resolvedUri, readNode).orElse(null);
+  }
+
+  private URI resolveResource(ModuleKey moduleKey, String resourceUri) {
+    var parsedUri = parseUri(resourceUri);
     var context = VmContext.get(this);
     URI resolvedUri;
     try {

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ImportGlobElementNode.java

@@ -36,7 +36,8 @@ public final class ImportGlobElementNode extends ExpressionNode {
   private final VmLanguage language;
   private final ResolvedModuleKey currentModule;
 
-  public ImportGlobElementNode(SourceSection sourceSection, VmLanguage language, ResolvedModuleKey currentModule) {
+  public ImportGlobElementNode(
+      SourceSection sourceSection, VmLanguage language, ResolvedModuleKey currentModule) {
     super(sourceSection);
     this.language = language;
     this.currentModule = currentModule;
@@ -48,7 +49,7 @@ public Object executeGeneric(VirtualFrame frame) {
     var path = (String) VmUtils.getMemberKey(frame);
     return importModule(mapping, path);
   }
-  
+
   @TruffleBoundary
   private VmTyped importModule(VmObjectLike mapping, String path) {
     @SuppressWarnings("unchecked")

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ImportGlobNode.java

@@ -38,20 +38,18 @@
 
 @NodeInfo(shortName = "import*")
 public class ImportGlobNode extends AbstractImportNode {
-  private final ResolvedModuleKey currentModule;
   private final String globPattern;
   private final SharedMemberNode importGlobElementNode;
 
-  @CompilationFinal @LateInit private VmMapping importsMapping;
+  @CompilationFinal @LateInit private VmMapping cachedResult;
 
   public ImportGlobNode(
       VmLanguage language,
       SourceSection sourceSection,
       ResolvedModuleKey currentModule,
       URI importUri,
       String globPattern) {
-    super(sourceSection, importUri);
-    this.currentModule = currentModule;
+    super(sourceSection, currentModule, importUri);
     this.globPattern = globPattern;
     importGlobElementNode =
         new SharedMemberNode(
@@ -65,42 +63,41 @@ public ImportGlobNode(
 
   @Override
   public Object executeGeneric(VirtualFrame frame) {
-    if (importsMapping == null) {
-      CompilerDirectives.transferToInterpreterAndInvalidate();
-      var context = VmContext.get(this);
-      try {
-        var moduleKey = context.getModuleResolver().resolve(importUri);
-        var securityManager = VmContext.get(this).getSecurityManager();
-        if (!moduleKey.isGlobbable()) {
-          throw exceptionBuilder()
-              .evalError("cannotGlobUri", importUri, importUri.getScheme())
-              .build();
-        }
-        var resolvedElements =
-            GlobResolver.resolveGlob(
-                securityManager,
-                moduleKey,
-                currentModule.getOriginal(),
-                currentModule.getUri(),
-                globPattern);
-        var builder = new VmObjectBuilder();
-        for (var entry : resolvedElements.entrySet()) {
-          builder.addEntry(entry.getKey(), importGlobElementNode);
-        }
-        importsMapping = builder.toMapping(resolvedElements);
-      } catch (IOException e) {
-        throw exceptionBuilder().evalError("ioErrorResolvingGlob", importUri).withCause(e).build();
-      } catch (SecurityManagerException | HttpClientInitException e) {
-        throw exceptionBuilder().withCause(e).build();
-      } catch (PackageLoadError e) {
-        throw exceptionBuilder().adhocEvalError(e.getMessage()).build();
-      } catch (InvalidGlobPatternException e) {
+    if (cachedResult != null) return cachedResult;
+
+    CompilerDirectives.transferToInterpreterAndInvalidate();
+    var context = VmContext.get(this);
+    try {
+      var moduleKey = context.getModuleResolver().resolve(importUri);
+      if (!moduleKey.isGlobbable()) {
         throw exceptionBuilder()
-            .evalError("invalidGlobPattern", globPattern)
-            .withHint(e.getMessage())
+            .evalError("cannotGlobUri", importUri, importUri.getScheme())
             .build();
       }
+      var resolvedElements =
+          GlobResolver.resolveGlob(
+              context.getSecurityManager(),
+              moduleKey,
+              currentModule.getOriginal(),
+              currentModule.getUri(),
+              globPattern);
+      var builder = new VmObjectBuilder();
+      for (var entry : resolvedElements.entrySet()) {
+        builder.addEntry(entry.getKey(), importGlobElementNode);
+      }
+      cachedResult = builder.toMapping(resolvedElements);
+      return cachedResult;
+    } catch (IOException e) {
+      throw exceptionBuilder().evalError("ioErrorResolvingGlob", importUri).withCause(e).build();
+    } catch (SecurityManagerException | HttpClientInitException e) {
+      throw exceptionBuilder().withCause(e).build();
+    } catch (PackageLoadError e) {
+      throw exceptionBuilder().adhocEvalError(e.getMessage()).build();
+    } catch (InvalidGlobPatternException e) {
+      throw exceptionBuilder()
+          .evalError("invalidGlobPattern", globPattern)
+          .withHint(e.getMessage())
+          .build();
     }
-    return importsMapping;
   }
 }

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ImportNode.java

@@ -33,7 +33,6 @@
 @NodeInfo(shortName = "import")
 public final class ImportNode extends AbstractImportNode {
   private final VmLanguage language;
-  private final ResolvedModuleKey currentModule;
 
   @CompilationFinal @LateInit private VmTyped importedModule;
 
@@ -42,9 +41,8 @@ public ImportNode(
       SourceSection sourceSection,
       ResolvedModuleKey currentModule,
       URI importUri) {
-    super(sourceSection, importUri);
+    super(sourceSection, currentModule, importUri);
     this.language = language;
-    this.currentModule = currentModule;
 
     assert importUri.isAbsolute();
   }

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ReadGlobElementNode.java

@@ -37,7 +37,7 @@ public Object executeGeneric(VirtualFrame frame) {
     var path = (String) VmUtils.getMemberKey(frame);
     return readResource(mapping, path);
   }
-  
+
   @TruffleBoundary
   private Object readResource(VmObjectLike mapping, String path) {
     @SuppressWarnings("unchecked")

pkl-core/src/main/java/org/pkl/core/ast/expression/unary/ReadGlobNode.java

@@ -20,24 +20,27 @@
 import com.oracle.truffle.api.frame.FrameDescriptor;
 import com.oracle.truffle.api.nodes.NodeInfo;
 import com.oracle.truffle.api.source.SourceSection;
-import java.net.URI;
-import java.net.URISyntaxException;
+import java.io.IOException;
+import org.graalvm.collections.EconomicMap;
+import org.pkl.core.SecurityManagerException;
 import org.pkl.core.ast.member.SharedMemberNode;
+import org.pkl.core.http.HttpClientInitException;
 import org.pkl.core.module.ModuleKey;
 import org.pkl.core.runtime.VmContext;
 import org.pkl.core.runtime.VmLanguage;
+import org.pkl.core.runtime.VmMapping;
 import org.pkl.core.runtime.VmObjectBuilder;
-import org.pkl.core.util.IoUtils;
+import org.pkl.core.util.GlobResolver;
+import org.pkl.core.util.GlobResolver.InvalidGlobPatternException;
 
 @NodeInfo(shortName = "read*")
-public abstract class ReadGlobNode extends UnaryExpressionNode {
-  private final ModuleKey currentModule;
+public abstract class ReadGlobNode extends AbstractReadNode {
   private final SharedMemberNode readGlobElementNode;
+  private final EconomicMap<String, VmMapping> cachedResults = EconomicMap.create();
 
   protected ReadGlobNode(
       VmLanguage language, SourceSection sourceSection, ModuleKey currentModule) {
-    super(sourceSection);
-    this.currentModule = currentModule;
+    super(sourceSection, currentModule);
     readGlobElementNode =
         new SharedMemberNode(
             sourceSection,
@@ -51,30 +54,43 @@ protected ReadGlobNode(
   @Specialization
   @TruffleBoundary
   public Object read(String globPattern) {
-    var context = VmContext.get(this);
-    var globUri = toUri(globPattern);
-    var resolvedElements =
-        context
-            .getResourceManager()
-            .resolveGlob(globUri, currentModule.getUri(), currentModule, this, globPattern);
-    var builder = new VmObjectBuilder();
-    for (var entry : resolvedElements.entrySet()) {
-      builder.addEntry(entry.getKey(), readGlobElementNode);
-    }
-    return builder.toMapping(resolvedElements);
-  }
+    var cachedResult = cachedResults.get(globPattern);
+    if (cachedResult != null) return cachedResult;
 
-  private URI toUri(String globPattern) {
+    // use same check as for globbed imports (see AstBuilder)
+    if (globPattern.startsWith("...")) {
+      throw exceptionBuilder().evalError("cannotGlobTripleDots").build();
+    }
+    var globUri = parseUri(globPattern);
+    var context = VmContext.get(this);
     try {
-      var globUri = IoUtils.toUri(globPattern);
-      if (IoUtils.parseTripleDotPath(globUri) != null) {
-        throw exceptionBuilder().evalError("cannotGlobTripleDots").build();
+      var resolvedUri = currentModule.resolveUri(globUri);
+      var reader = context.getResourceManager().getReader(resolvedUri, this);
+      if (!reader.isGlobbable()) {
+        throw exceptionBuilder().evalError("cannotGlobUri", globUri, globUri.getScheme()).build();
+      }
+      var resolvedElements =
+          GlobResolver.resolveGlob(
+              context.getSecurityManager(),
+              reader,
+              currentModule,
+              currentModule.getUri(),
+              globPattern);
+      var builder = new VmObjectBuilder();
+      for (var entry : resolvedElements.entrySet()) {
+        builder.addEntry(entry.getKey(), readGlobElementNode);
       }
-      return globUri;
-    } catch (URISyntaxException e) {
+      cachedResult = builder.toMapping(resolvedElements);
+      cachedResults.put(globPattern, cachedResult);
+      return cachedResult;
+    } catch (IOException e) {
+      throw exceptionBuilder().evalError("ioErrorResolvingGlob", globPattern).withCause(e).build();
+    } catch (SecurityManagerException | HttpClientInitException e) {
+      throw exceptionBuilder().withCause(e).build();
+    } catch (InvalidGlobPatternException e) {
       throw exceptionBuilder()
-          .evalError("invalidResourceUri", globPattern)
-          .withHint(e.getReason())
+          .evalError("invalidGlobPattern", globPattern)
+          .withHint(e.getMessage())
           .build();
     }
   }