Fix globbed read/import bugs (#449)

* Split MemberNode into (Regular/Shared)MemberNode

SharedMemberNode enables generating non-constant object members
at run time without generating an unbounded number of Truffle root nodes.

* Invert shouldRunTypeCheck to match its name

* Introduce VmObjectBuilder

Introduce VmObjectBuilder, a uniform way to build `VmObject`s
whose `ObjectMember`s are determined at run time.
Replace some manual object building code with VmObjectBuilder.
Add some assertions to fix IntelliJ warnings.

* Improve implementation of globbed read/import nodes

- Leverage SharedMemberNode to have a single Truffle root node per globbed
  read/import expression instead of one root node per resolved glob element.
- Remove caching in ReadGlobNode because it only works correctly if glob pattern is a string *constant*.
- Remove caching in StaticReadNode (now ReadGlobElementNode/ImportGlobElementNode)
  because it seems unnecessary and the implementation doesn't look quite right.

* Simplify code

* Fix ClassCastException when reflecting on globbed import

* 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/MemberNode.java

@@ -20,35 +20,22 @@
 import com.oracle.truffle.api.source.SourceSection;
 import java.util.function.Function;
 import org.pkl.core.ast.member.DefaultPropertyBodyNode;
-import org.pkl.core.ast.member.Member;
 import org.pkl.core.runtime.VmExceptionBuilder;
 import org.pkl.core.runtime.VmLanguage;
 import org.pkl.core.runtime.VmUtils;
 import org.pkl.core.util.Nullable;
 
 public abstract class MemberNode extends PklRootNode {
-  protected final Member member;
   @Child protected ExpressionNode bodyNode;
 
   protected MemberNode(
-      @Nullable VmLanguage language,
-      FrameDescriptor descriptor,
-      Member member,
-      ExpressionNode bodyNode) {
+      @Nullable VmLanguage language, FrameDescriptor descriptor, ExpressionNode bodyNode) {
 
     super(language, descriptor);
-    this.member = member;
     this.bodyNode = bodyNode;
   }
 
-  @Override
-  public final SourceSection getSourceSection() {
-    return member.getSourceSection();
-  }
-
-  public final SourceSection getHeaderSection() {
-    return member.getHeaderSection();
-  }
+  public abstract SourceSection getHeaderSection();
 
   public final SourceSection getBodySection() {
     return bodyNode.getSourceSection();
@@ -58,11 +45,6 @@ public final ExpressionNode getBodyNode() {
     return bodyNode;
   }
 
-  @Override
-  public final String getName() {
-    return member.getQualifiedName();
-  }
-
   public final void replaceBody(Function<ExpressionNode, ExpressionNode> replacer) {
     bodyNode = insert(replacer.apply(bodyNode));
   }
@@ -72,7 +54,7 @@ protected final Object executeBody(VirtualFrame frame) {
   }
 
   protected final VmExceptionBuilder exceptionBuilder() {
-    return new VmExceptionBuilder().withSourceSection(member.getHeaderSection());
+    return new VmExceptionBuilder().withSourceSection(getHeaderSection());
   }
 
   /**
@@ -83,9 +65,9 @@ protected final VmExceptionBuilder exceptionBuilder() {
    * org.pkl.core.runtime.VmUtils#SKIP_TYPECHECK_MARKER}. IDEA: might be more appropriate to only
    * skip constraints check
    */
-  protected final boolean shouldRunTypecheck(VirtualFrame frame) {
-    return frame.getArguments().length == 4
-        && frame.getArguments()[3] == VmUtils.SKIP_TYPECHECK_MARKER;
+  protected final boolean shouldRunTypeCheck(VirtualFrame frame) {
+    return frame.getArguments().length != 4
+        || frame.getArguments()[3] != VmUtils.SKIP_TYPECHECK_MARKER;
   }
 
   public boolean isUndefined() {

pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java

@@ -2218,8 +2218,7 @@ public Object visitReadExpr(ReadExprContext ctx) {
       return ReadOrNullNodeGen.create(createSourceSection(ctx), moduleKey, visitExpr(exprCtx));
     }
     assert tokenType == PklLexer.READ_GLOB;
-    return ReadGlobNodeGen.create(
-        language, createSourceSection(ctx), moduleKey, visitExpr(exprCtx));
+    return ReadGlobNodeGen.create(createSourceSection(ctx), moduleKey, visitExpr(exprCtx));
   }
 
   @Override
@@ -2660,8 +2659,7 @@ private AbstractImportNode doVisitImport(
     }
     var resolvedUri = resolveImport(importUri, importUriCtx);
     if (isGlobImport) {
-      return new ImportGlobNode(
-          language, section, moduleInfo.getResolvedModuleKey(), resolvedUri, importUri);
+      return new ImportGlobNode(section, moduleInfo.getResolvedModuleKey(), resolvedUri, importUri);
     }
     return new ImportNode(language, section, moduleInfo.getResolvedModuleKey(), resolvedUri);
   }

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/ImportGlobMemberBodyNode.java

@@ -0,0 +1,67 @@
+/**
+ * Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.pkl.core.ast.expression.unary;
+
+import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+import com.oracle.truffle.api.frame.VirtualFrame;
+import com.oracle.truffle.api.source.SourceSection;
+import java.util.Map;
+import org.pkl.core.SecurityManagerException;
+import org.pkl.core.ast.ExpressionNode;
+import org.pkl.core.http.HttpClientInitException;
+import org.pkl.core.module.ResolvedModuleKey;
+import org.pkl.core.packages.PackageLoadError;
+import org.pkl.core.runtime.VmContext;
+import org.pkl.core.runtime.VmLanguage;
+import org.pkl.core.runtime.VmObjectLike;
+import org.pkl.core.runtime.VmTyped;
+import org.pkl.core.runtime.VmUtils;
+import org.pkl.core.util.GlobResolver.ResolvedGlobElement;
+
+/** Used by {@link ReadGlobNode}. */
+public final class ImportGlobMemberBodyNode extends ExpressionNode {
+  private final VmLanguage language;
+  private final ResolvedModuleKey currentModule;
+
+  public ImportGlobMemberBodyNode(
+      SourceSection sourceSection, VmLanguage language, ResolvedModuleKey currentModule) {
+    super(sourceSection);
+    this.language = language;
+    this.currentModule = currentModule;
+  }
+
+  @Override
+  public Object executeGeneric(VirtualFrame frame) {
+    var mapping = VmUtils.getObjectReceiver(frame);
+    var path = (String) VmUtils.getMemberKey(frame);
+    return importModule(mapping, path);
+  }
+
+  @TruffleBoundary
+  private VmTyped importModule(VmObjectLike mapping, String path) {
+    @SuppressWarnings("unchecked")
+    var globElements = (Map<String, ResolvedGlobElement>) mapping.getExtraStorage();
+    var importUri = globElements.get(path).getUri();
+    var context = VmContext.get(this);
+    try {
+      context.getSecurityManager().checkImportModule(currentModule.getUri(), importUri);
+      var moduleToImport = context.getModuleResolver().resolve(importUri, this);
+      return language.loadModule(moduleToImport, this);
+    } catch (SecurityManagerException | PackageLoadError | HttpClientInitException e) {
+      throw exceptionBuilder().withCause(e).build();
+    }
+  }
+}

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

@@ -17,114 +17,93 @@
 
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
-import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.frame.FrameDescriptor;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.nodes.NodeInfo;
 import com.oracle.truffle.api.source.SourceSection;
 import java.io.IOException;
 import java.net.URI;
-import java.util.List;
-import org.graalvm.collections.EconomicMap;
 import org.pkl.core.SecurityManagerException;
-import org.pkl.core.ast.VmModifier;
-import org.pkl.core.ast.member.ObjectMember;
-import org.pkl.core.ast.member.UntypedObjectMemberNode;
+import org.pkl.core.ast.member.SharedMemberNode;
 import org.pkl.core.http.HttpClientInitException;
 import org.pkl.core.module.ResolvedModuleKey;
 import org.pkl.core.packages.PackageLoadError;
-import org.pkl.core.runtime.BaseModule;
 import org.pkl.core.runtime.VmContext;
 import org.pkl.core.runtime.VmLanguage;
 import org.pkl.core.runtime.VmMapping;
-import org.pkl.core.runtime.VmUtils;
-import org.pkl.core.util.EconomicMaps;
+import org.pkl.core.runtime.VmObjectBuilder;
 import org.pkl.core.util.GlobResolver;
 import org.pkl.core.util.GlobResolver.InvalidGlobPatternException;
-import org.pkl.core.util.GlobResolver.ResolvedGlobElement;
 import org.pkl.core.util.LateInit;
 
 @NodeInfo(shortName = "import*")
 public class ImportGlobNode extends AbstractImportNode {
-  private final VmLanguage language;
-
-  private final ResolvedModuleKey currentModule;
-
   private final String globPattern;
-
-  @CompilationFinal @LateInit private VmMapping importedMapping;
+  @Child @LateInit private SharedMemberNode memberNode;
+  @CompilationFinal @LateInit private VmMapping cachedResult;
 
   public ImportGlobNode(
-      VmLanguage language,
       SourceSection sourceSection,
       ResolvedModuleKey currentModule,
       URI importUri,
       String globPattern) {
-    super(sourceSection, importUri);
-    this.language = language;
-    this.currentModule = currentModule;
+    super(sourceSection, currentModule, importUri);
     this.globPattern = globPattern;
   }
 
-  @TruffleBoundary
-  private EconomicMap<Object, ObjectMember> buildMembers(
-      FrameDescriptor frameDescriptor, List<ResolvedGlobElement> uris) {
-    var members = EconomicMaps.<Object, ObjectMember>create();
-    for (var entry : uris) {
-      var readNode =
-          new ImportNode(
-              language, VmUtils.unavailableSourceSection(), currentModule, entry.getUri());
-      var member =
-          new ObjectMember(
-              VmUtils.unavailableSourceSection(),
-              VmUtils.unavailableSourceSection(),
-              VmModifier.ENTRY,
-              null,
-              "");
-      var memberNode = new UntypedObjectMemberNode(language, frameDescriptor, member, readNode);
-      member.initMemberNode(memberNode);
-      EconomicMaps.put(members, entry.getPath(), member);
+  private SharedMemberNode getMemberNode() {
+    if (memberNode == null) {
+      CompilerDirectives.transferToInterpreterAndInvalidate();
+      var language = VmLanguage.get(this);
+      memberNode =
+          new SharedMemberNode(
+              sourceSection,
+              sourceSection,
+              "",
+              language,
+              new FrameDescriptor(),
+              new ImportGlobMemberBodyNode(sourceSection, language, currentModule));
     }
-    return members;
+    return memberNode;
   }
 
   @Override
   public Object executeGeneric(VirtualFrame frame) {
-    if (importedMapping == 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 uris =
-            GlobResolver.resolveGlob(
-                securityManager,
-                moduleKey,
-                currentModule.getOriginal(),
-                currentModule.getUri(),
-                globPattern);
-        var members = buildMembers(frame.getFrameDescriptor(), uris);
-        importedMapping =
-            new VmMapping(
-                frame.materialize(), BaseModule.getMappingClass().getPrototype(), members);
-      } 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(resolvedElements.size());
+      for (var entry : resolvedElements.entrySet()) {
+        builder.addEntry(entry.getKey(), getMemberNode());
+      }
+      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 importedMapping;
   }
 }