review feedback round 2

Author: HT154

pkl-core/src/main/java/org/pkl/core/ast/internal/ToStringNode.java

@@ -17,11 +17,11 @@
 
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.Cached;
+import com.oracle.truffle.api.dsl.Cached.Shared;
 import com.oracle.truffle.api.dsl.Fallback;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.source.SourceSection;
-import org.pkl.core.ast.ConstantValueNode;
 import org.pkl.core.ast.ExpressionNode;
 import org.pkl.core.ast.MemberLookupMode;
 import org.pkl.core.ast.expression.member.InvokeMethodVirtualNode;
@@ -61,14 +61,17 @@ protected String evalBoolean(boolean value) {
   protected String evalTyped(
       VirtualFrame frame,
       VmTyped value,
-      @Cached(value = "createInvokeNode()", neverDefault = true)
+      @Shared("invokeToString") @Cached(value = "createInvokeNode()", neverDefault = true)
           InvokeMethodVirtualNode invokeNode) {
     return (String) invokeNode.executeWith(frame, value, value.getVmClass());
   }
 
   @Specialization
-  protected String evalReference(VirtualFrame frame, VmReference value, @Cached(value = "createInvokeNode()", neverDefault = true)
-  InvokeMethodVirtualNode invokeNode) {
+  protected String evalReference(
+      VirtualFrame frame,
+      VmReference value,
+      @Shared("invokeToString") @Cached(value = "createInvokeNode()", neverDefault = true)
+          InvokeMethodVirtualNode invokeNode) {
     return (String) invokeNode.executeWith(frame, value, value.getVmClass());
   }
 

pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java

@@ -2137,26 +2137,11 @@ public ReferenceTypeNode(
       this.domainTypeNode = domainTypeNode;
       this.referentTypeNode = referentTypeNode;
       this.getModuleNode = new GetModuleNode(sourceSection);
+      validateTypeArguments(sourceSection);
     }
 
     @Specialization(guards = "value.getVmClass() == getReferenceClass()")
     protected Object eval(VirtualFrame frame, VmReference value) {
-      // constraints may not be used in Reference type annotation referents
-      // walk the type and throw if any part of the referent is constrained
-      // perform this check at eval-time instead of init-time
-      // because type aliases can replace nodes between init and eval
-      referentTypeNode.acceptTypeNode(
-          true,
-          (typeNode) -> {
-            if (typeNode instanceof ConstrainedTypeNode) {
-              throw exceptionBuilder()
-                  .evalError("invalidReferenceTypeAnnotationWithConstraint")
-                  .withLocation(this)
-                  .build();
-            }
-            return true;
-          });
-
       if (referentTypeNode.isNoopTypeCheck()) {
         return value;
       }
@@ -2178,6 +2163,30 @@ protected Object eval(VirtualFrame frame, VmReference value) {
       throw new VmTypeMismatchException.Reference(sourceSection, value, domainType, referentType);
     }
 
+    public void validateTypeArguments(@Nullable SourceSection aliasSourceSection) {
+      // constraints may not be used in Reference type annotation referents
+      // walk the type and throw if any part of the referent is constrained
+
+      // TODO improve error message when this type node and/or referent constraint are behind type
+      // aliases
+      referentTypeNode.acceptTypeNode(
+          true,
+          (typeNode) -> {
+            if (typeNode instanceof ConstrainedTypeNode) {
+              CompilerDirectives.transferToInterpreter();
+              var err =
+                  exceptionBuilder()
+                      .evalError("invalidReferenceTypeAnnotationWithConstraint")
+                      .withLocation(this);
+              if (aliasSourceSection != null) {
+                err.withSourceSection(aliasSourceSection);
+              }
+              throw err.build();
+            }
+            return true;
+          });
+    }
+
     @Fallback
     protected Object fallback(Object value) {
       throw typeMismatch(value, RefModule.getReferenceClass());
@@ -2680,7 +2689,7 @@ public TypeAliasTypeNode(
 
       this.typeAlias = typeAlias;
       this.typeArgumentNodes = typeArgumentNodes;
-      aliasedTypeNode = typeAlias.instantiate(typeArgumentNodes);
+      aliasedTypeNode = typeAlias.instantiate(typeArgumentNodes, sourceSection);
     }
 
     public TypeNode getAliasedTypeNode() {

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

@@ -15,6 +15,8 @@
  */
 package org.pkl.core.runtime;
 
+import com.oracle.truffle.api.frame.VirtualFrame;
+import com.oracle.truffle.api.source.SourceSection;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
@@ -27,6 +29,9 @@
 import org.pkl.core.PType;
 import org.pkl.core.Reference;
 import org.pkl.core.TypeAlias;
+import org.pkl.core.ast.ExpressionNode;
+import org.pkl.core.ast.MemberLookupMode;
+import org.pkl.core.ast.expression.member.InvokeMethodVirtualNodeGen;
 import org.pkl.core.stdlib.VmObjectFactory;
 import org.pkl.core.util.Nullable;
 
@@ -226,7 +231,7 @@ private static void getCandidateSubscriptType(PType type, Object key, Set<PType>
       for (var kt : keyTypes) {
         if (kt == PType.UNKNOWN
             || (kt instanceof PType.Class klazz
-                && klazz.getPClass().getInfo() == PClassInfo.forValue(key))
+                && klazz.getPClass().getInfo() == PClassInfo.forValue(VmValue.export(key)))
             || (kt instanceof PType.StringLiteral stringLiteral
                 && stringLiteral.getLiteral().equals(key))) {
           normalizeTypes(typeArgs.get(1), clazz.getPClass().getModuleClass(), result);
@@ -414,4 +419,23 @@ public int hashCode() {
     result = 31 * result + candidateTypes.hashCode();
     return result;
   }
+
+  public String toPklString(@Nullable VirtualFrame frame, @Nullable SourceSection sourceSection) {
+    if (frame == null) {
+      frame = VmUtils.createEmptyMaterializedFrame();
+    }
+    if (sourceSection == null) {
+      sourceSection = VmUtils.unavailableSourceSection();
+    }
+
+    return (String)
+        InvokeMethodVirtualNodeGen.create(
+                sourceSection,
+                Identifier.TO_STRING,
+                new ExpressionNode[] {},
+                MemberLookupMode.EXPLICIT_RECEIVER,
+                null,
+                null)
+            .executeWith(frame, this, getVmClass());
+  }
 }

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

@@ -31,6 +31,7 @@
 import org.pkl.core.ast.VmModifier;
 import org.pkl.core.ast.type.TypeNode;
 import org.pkl.core.ast.type.TypeNode.ConstrainedTypeNode;
+import org.pkl.core.ast.type.TypeNode.ReferenceTypeNode;
 import org.pkl.core.ast.type.TypeNode.TypeVariableNode;
 import org.pkl.core.ast.type.TypeNode.UnknownTypeNode;
 import org.pkl.core.util.LateInit;
@@ -177,7 +178,8 @@ public Frame getEnclosingFrame() {
   }
 
   @TruffleBoundary
-  public TypeNode instantiate(TypeNode[] typeArgumentNodes) {
+  public TypeNode instantiate(
+      TypeNode[] typeArgumentNodes, SourceSection typeAliasTypeNodeSourceSection) {
     // Cloning the type node means that the entire type check remains within a single root node,
     // which should be good for interpreted and compiled performance alike:
     // * Fewer root nodes to call
@@ -199,6 +201,13 @@ public TypeNode instantiate(TypeNode[] typeArgumentNodes) {
           }
           return true;
         });
+    clone.accept(
+        node -> {
+          if (node instanceof ReferenceTypeNode referenceTypeNode) {
+            referenceTypeNode.validateTypeArguments(typeAliasTypeNodeSourceSection);
+          }
+          return true;
+        });
 
     return clone;
   }

pkl-core/src/main/java/org/pkl/core/stdlib/AbstractStringRenderer.java

@@ -17,6 +17,7 @@
 
 import org.pkl.core.runtime.VmClass;
 import org.pkl.core.runtime.VmFunction;
+import org.pkl.core.runtime.VmReference;
 import org.pkl.core.runtime.VmTypeAlias;
 
 /** Base class for renderers that are part of the standard library. */
@@ -51,6 +52,11 @@ protected void decreaseIndent() {
     currIndent.setLength(currIndent.length() - indent.length());
   }
 
+  @Override
+  public void visitReference(VmReference value) {
+    visitString(value.toPklString(null, currSourceSection));
+  }
+
   // override these to mark them final
 
   @Override

pkl-core/src/test/files/LanguageSnippetTests/input/api/reference.pkl

@@ -27,8 +27,8 @@ class A extends Resource {
 /// Test doc comment
 class AProperties {
   foo: Int
-  someMapping: Mapping<String, Int>
-  someMap: Map<Int, Int>
+  someMapping: Mapping<String, Int?>
+  someMap: Map<MapKey, Int>
   someListing: Listing<Int>
   someList: List<Int>
   nonInt: String
@@ -42,20 +42,26 @@ class B extends Resource {
 
 class BProperties {
   foo: String
-  someMapping: Mapping<String, String>
-  someMap: Map<Int, String>
+  someMapping: Mapping<String, String?>
+  someMap: Map<MapKey, String>
   someListing: Listing<String>
   someList: List<String>
   nonString: Int
 }
 
+class MapKey {
+  k: Int
+  
+  function toString(): String = "key:\(k)"
+}
+
 class K {
   aId: String | *Ref<String>?
   bId: String | *Ref<String>?
   aProperties: AProperties | *Ref<AProperties>?
   bProperties: BProperties | *Ref<BProperties>?
-  aValues: Listing<Int | Ref<Int>>?
-  bValues: Listing<String | Ref<String>>?
+  aValues: Listing<Int | Ref<Int?>>?
+  bValues: Listing<String | Ref<String?>>?
   splitUnion: Ref<Listing<String> | Listing<Int>>?
 }
 
@@ -84,15 +90,15 @@ k: K = new {
   aValues {
     aRef.outputs.foo
     aRef.outputs.someMapping["key"]
-    aRef.outputs.someMap[123]
+    aRef.outputs.someMap[new MapKey { k = 123 }]
     aRef.outputs.someListing[0]
     aRef.outputs.someList[math.maxInt]
     bRef.outputs.nonString
   }
   bValues {
     bRef.outputs.foo
     bRef.outputs.someMapping["key"]
-    bRef.outputs.someMap[123]
+    bRef.outputs.someMap[new MapKey { k = 123 }]
     bRef.outputs.someListing[0]
     bRef.outputs.someList[math.maxInt]
     aRef.outputs.nonInt
@@ -105,16 +111,5 @@ j: K = new {
   splitUnion = unknownRef.outputs.someListing
 }
 
-local x: UInt = 0
-local y = ref.Reference(d, UInt, x)
-local z = y
-
 refInterpolation = "\(aRef.outputs.someListing[1])"
-
-output {
-  renderer {
-    converters {
-      [ref.Reference] = (it) -> it.toString()
-    }
-  }
-}
+kInterpolation = "\(k)"

pkl-core/src/test/files/LanguageSnippetTests/input/api/reference2.pkl

@@ -0,0 +1,9 @@
+import "pkl:ref"
+
+class D extends ref.Domain {
+  function referenceToString(data: Any, path: List<ref.Access>): String = throw("not supported")
+}
+local d: D = new {}
+typealias Ref<T> = ref.Reference<D, T>
+
+test = ref.Reference(d, String, "") as Ref<Int>

pkl-core/src/test/files/LanguageSnippetTests/input/api/reference3.pkl

@@ -0,0 +1,9 @@
+import "pkl:ref"
+
+class D extends ref.Domain {
+  function referenceToString(data: Any, path: List<ref.Access>): String = throw("not supported")
+}
+typealias Ref<T> = ref.Reference<D, T>
+local d: D = new {}
+
+test = ref.Reference(d, String, "") as Ref<Listing<String(true)>>