Minimize for-generator scoping bug

There is currently a bug around resolving variables within the iterable of a for generator or spread syntax (apple#741) Normally, mappings/listings are type-checked lazily. However, this results in the said bug getting widened, for any object members declared in the iterable. As a workaround for now, prevent the bug from being any worse by ensuring that these object members are eagerly typechecked.
bioball · Oct 30, 2024 · c2f7879 · c2f7879
1 parent 466ae6f
commit c2f7879
Show file tree

Hide file tree

Showing 17 changed files with 192 additions and 32 deletions.
diff --git a/pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java b/pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java
@@ -313,7 +313,8 @@ public ObjectMember visitClazz(ClazzContext ctx) {
                   headerSection,
                   isLocal ? VmModifier.LOCAL_CLASS_OBJECT_MEMBER : VmModifier.CLASS_OBJECT_MEMBER,
                   scope.getName(),
-                  scope.getQualifiedName());
+                  scope.getQualifiedName(),
+                  false);
 
           result.initMemberNode(
               new UntypedObjectMemberNode(
@@ -366,7 +367,8 @@ public ObjectMember visitTypeAlias(TypeAliasContext ctx) {
                       ? VmModifier.LOCAL_TYPEALIAS_OBJECT_MEMBER
                       : VmModifier.TYPEALIAS_OBJECT_MEMBER,
                   scopeName,
-                  scope.getQualifiedName());
+                  scope.getQualifiedName(),
+                  false);
 
           result.initMemberNode(
               new UntypedObjectMemberNode(
@@ -697,7 +699,8 @@ private ObjectMember doVisitObjectMethod(
                   createSourceSection(headerCtx),
                   modifiers,
                   scope.getName(),
-                  scope.getQualifiedName());
+                  scope.getQualifiedName(),
+                  false);
           var body = visitExpr(exprCtx);
           var node =
               new ObjectMethodNode(
@@ -808,7 +811,8 @@ private ObjectMember doVisitObjectProperty(
                   scope.buildFrameDescriptor(),
                   modifiers,
                   bodyNode,
-                  visitTypeAnnotation(typeAnnCtx))
+                  visitTypeAnnotation(typeAnnCtx),
+                  scope.isVisitingIterable())
               : VmUtils.createObjectProperty(
                   language,
                   sourceSection,
@@ -818,7 +822,8 @@ private ObjectMember doVisitObjectProperty(
                   scope.buildFrameDescriptor(),
                   modifiers,
                   bodyNode,
-                  null);
+                  null,
+                  scope.isVisitingIterable());
         });
   }
 
@@ -899,8 +904,16 @@ public GeneratorMemberNode visitObjectEntry(ObjectEntryContext ctx) {
 
   @Override
   public GeneratorMemberNode visitObjectSpread(ObjectSpreadContext ctx) {
+    var scope = symbolTable.getCurrentScope();
+    var visitingIterable = scope.isVisitingIterable();
+    scope.setVisitingIterable(true);
+    var expr = visitExpr(ctx.expr());
+    scope.setVisitingIterable(visitingIterable);
     return GeneratorSpreadNodeGen.create(
-        createSourceSection(ctx), visitExpr(ctx.expr()), ctx.QSPREAD() != null);
+        createSourceSection(ctx),
+        expr,
+        ctx.QSPREAD() != null,
+        symbolTable.getCurrentScope().isVisitingIterable());
   }
 
   private void insertWriteForGeneratorVarsToFrameSlotsNode(@Nullable MemberNode memberNode) {
@@ -991,7 +1004,11 @@ public GeneratorForNode visitForGenerator(ForGeneratorContext ctx) {
           ignoreT1 ? null : visitTypeAnnotation(ctx.t1.typedIdentifier().typeAnnotation());
     }
 
+    var scope = symbolTable.getCurrentScope();
+    var visitingIterable = scope.isVisitingIterable();
+    scope.setVisitingIterable(true);
     var iterableNode = visitExpr(ctx.e);
+    scope.setVisitingIterable(visitingIterable);
     var memberNodes = doVisitForWhenBody(ctx.objectBody());
     if (keyVariableSlot != -1) {
       currentScope.popForGeneratorVariable();
@@ -1202,7 +1219,8 @@ private ObjectMember doVisitObjectElement(ObjectElementContext ctx) {
                   elementNode.getSourceSection(),
                   VmModifier.ELEMENT,
                   null,
-                  scope.getQualifiedName());
+                  scope.getQualifiedName(),
+                  scope.isVisitingIterable());
 
           if (elementNode instanceof ConstantNode constantNode) {
             member.initConstantValue(constantNode);
@@ -1257,7 +1275,8 @@ private Function<EntryScope, Pair<ExpressionNode, ObjectMember>> objectMemberIns
               keyNode.getSourceSection(),
               VmModifier.ENTRY,
               null,
-              scope.getQualifiedName());
+              scope.getQualifiedName(),
+              scope.isVisitingIterable());
 
       if (valueCtx != null) { // ["key"] = value
         var valueNode = visitExpr(valueCtx);
@@ -1786,7 +1805,8 @@ public ObjectMember visitImportClause(ImportClauseContext ctx) {
                   importNode.getSourceSection(),
                   modifiers,
                   scope.getName(),
-                  scope.getQualifiedName());
+                  scope.getQualifiedName(),
+                  false);
 
           result.initMemberNode(
               new UntypedObjectMemberNode(

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/builder/SymbolTable.java b/pkl-core/src/main/java/org/pkl/core/ast/builder/SymbolTable.java
@@ -171,6 +171,7 @@ public abstract static class Scope {
     private int entryCount = 0;
     private final FrameDescriptor.Builder frameDescriptorBuilder;
     private final ConstLevel constLevel;
+    private boolean isVisitingIterable;
 
     private Scope(
         @Nullable Scope parent,
@@ -187,6 +188,7 @@ private Scope(
           parent != null && parent.constLevel.biggerOrEquals(constLevel)
               ? parent.constLevel
               : constLevel;
+      this.isVisitingIterable = parent != null && parent.isVisitingIterable;
     }
 
     public final @Nullable Scope getParent() {
@@ -337,6 +339,14 @@ public final boolean isLexicalScope() {
     public ConstLevel getConstLevel() {
       return constLevel;
     }
+
+    public void setVisitingIterable(boolean isVisitingIterable) {
+      this.isVisitingIterable = isVisitingIterable;
+    }
+
+    public boolean isVisitingIterable() {
+      return isVisitingIterable;
+    }
   }
 
   private interface LexicalScope {}

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorSpreadNode.java b/pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorSpreadNode.java
@@ -48,12 +48,17 @@
 public abstract class GeneratorSpreadNode extends GeneratorMemberNode {
   @Child private ExpressionNode iterableNode;
   private final boolean nullable;
+  private final boolean isInIterable;
 
   public GeneratorSpreadNode(
-      SourceSection sourceSection, ExpressionNode iterableNode, boolean nullable) {
+      SourceSection sourceSection,
+      ExpressionNode iterableNode,
+      boolean nullable,
+      boolean isInIterable) {
     super(sourceSection);
     this.iterableNode = iterableNode;
     this.nullable = nullable;
+    this.isInIterable = isInIterable;
   }
 
   protected abstract void executeWithIterable(
@@ -326,7 +331,8 @@ private ObjectMember createMember(ObjectMember prototype, Object value) {
             prototype.getHeaderSection(),
             prototype.getModifiers(),
             prototype.getNameOrNull(),
-            prototype.getQualifiedName());
+            prototype.getQualifiedName(),
+            isInIterable);
     result.initConstantValue(value);
     return result;
   }

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMember.java b/pkl-core/src/main/java/org/pkl/core/ast/member/ObjectMember.java
@@ -27,6 +27,9 @@
 import org.pkl.core.util.Nullable;
 
 public final class ObjectMember extends Member {
+
+  private final boolean isInIterable;
+
   @CompilationFinal private @Nullable Object constantValue;
   @CompilationFinal private @Nullable MemberNode memberNode;
 
@@ -35,9 +38,11 @@ public ObjectMember(
       SourceSection headerSection,
       int modifiers,
       @Nullable Identifier name,
-      String qualifiedName) {
+      String qualifiedName,
+      boolean isInIterable) {
 
     super(sourceSection, headerSection, modifiers, name, qualifiedName);
+    this.isInIterable = isInIterable;
   }
 
   public void initConstantValue(ConstantNode node) {
@@ -152,4 +157,21 @@ public boolean equals(@Nullable Object obj) {
   public int hashCode() {
     return System.identityHashCode(this);
   }
+
+  /**
+   * Tells if this member is declared inside the iterable of a for-generator, or an object spread.
+   *
+   * This is <p>{@code true} for {@code new {}} within:
+   *
+   * <pre>
+   * {@code
+   * for (x in new Listing { new {} }) {
+   *                         ^^^^^^
+   *   // etc
+   * }
+   * </pre>
+   */
+  public boolean isInIterable() {
+    return isInIterable;
+  }
 }
diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/PropertyTypeNode.java
@@ -61,9 +61,30 @@ public String getName() {
     return qualifiedPropertyName;
   }
 
+  private boolean isInIterable(VirtualFrame frame) {
+    if (frame.getArguments().length < 4) {
+      return false;
+    }
+    if (frame.getArguments()[3] instanceof Boolean isInIterable) {
+      return isInIterable;
+    }
+    return false;
+  }
+
   @Override
   public Object execute(VirtualFrame frame) {
     try {
+      if (isInIterable(frame)) {
+        // There is currently a bug around resolving variables within the iterable of a for
+        // generator or spread syntax (https://github.com/apple/pkl/issues/741)
+        //
+        // Normally, mappings/listings are type-checked lazily. However, this results in the said
+        // bug getting widened, for any object members declared in the iterable.
+        //
+        // As a workaround for now, prevent the bug from being any worse by ensuring that these
+        // object members are eagerly typechecked.
+        return typeNode.executeEagerly(frame, frame.getArguments()[2]);
+      }
       return typeNode.execute(frame, frame.getArguments()[2]);
     } catch (VmTypeMismatchException e) {
       CompilerDirectives.transferToInterpreter();

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/TypeCheckedPropertyNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/TypeCheckedPropertyNode.java
@@ -31,6 +31,8 @@
 public abstract class TypeCheckedPropertyNode extends RegularMemberNode {
   @Child @Executed protected ExpressionNode ownerNode = new GetOwnerNode();
 
+  private final ObjectMember member;
+
   protected TypeCheckedPropertyNode(
       @Nullable VmLanguage language,
       FrameDescriptor descriptor,
@@ -40,6 +42,7 @@ protected TypeCheckedPropertyNode(
     super(language, descriptor, member, bodyNode);
 
     assert member.isProp();
+    this.member = member;
   }
 
   @SuppressWarnings("unused")
@@ -55,7 +58,8 @@ protected Object evalTypedObjectCached(
 
     // TODO: propagate SUPER_CALL_MARKER to disable constraint (but not type) check
     if (callNode != null && VmUtils.shouldRunTypeCheck(frame)) {
-      return callNode.call(VmUtils.getReceiverOrNull(frame), property.getOwner(), result);
+      return callNode.call(
+          VmUtils.getReceiverOrNull(frame), property.getOwner(), result, member.isInIterable());
     }
 
     return result;
@@ -75,7 +79,8 @@ protected Object eval(
             typeAnnNode.getCallTarget(),
             VmUtils.getReceiverOrNull(frame),
             property.getOwner(),
-            result);
+            result,
+            member.isInIterable());
       }
     }
 

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/TypedPropertyNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/TypedPropertyNode.java
@@ -26,6 +26,7 @@
 /** A property definition that has a type annotation. */
 public final class TypedPropertyNode extends RegularMemberNode {
   @Child private DirectCallNode typeCheckCallNode;
+  private final ObjectMember member;
 
   @TruffleBoundary
   public TypedPropertyNode(
@@ -38,6 +39,7 @@ public TypedPropertyNode(
     super(language, descriptor, member, bodyNode);
 
     assert member.isProp();
+    this.member = member;
 
     typeCheckCallNode = DirectCallNode.create(typeNode.getCallTarget());
   }
@@ -47,7 +49,10 @@ public Object execute(VirtualFrame frame) {
     var propertyValue = executeBody(frame);
     if (VmUtils.shouldRunTypeCheck(frame)) {
       return typeCheckCallNode.call(
-          VmUtils.getReceiver(frame), VmUtils.getOwner(frame), propertyValue);
+          VmUtils.getReceiver(frame),
+          VmUtils.getOwner(frame),
+          propertyValue,
+          member.isInIterable());
     }
     return propertyValue;
   }

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/member/UnresolvedPropertyNode.java b/pkl-core/src/main/java/org/pkl/core/ast/member/UnresolvedPropertyNode.java
@@ -187,7 +187,8 @@ public ClassProperty execute(VirtualFrame frame, VmClass clazz) {
             descriptor,
             modifiers,
             effectiveBodyNode,
-            typeNode);
+            typeNode,
+            false);
 
     return new ClassProperty(
         sourceSection,

diff --git a/pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java b/pkl-core/src/main/java/org/pkl/core/ast/type/TypeNode.java
@@ -1571,7 +1571,8 @@ public final Object createDefaultValue(
               headerSection,
               VmModifier.HIDDEN,
               Identifier.DEFAULT,
-              qualifiedName + ".default");
+              qualifiedName + ".default",
+              false);
 
       var defaultMemberValue =
           valueTypeNode.createDefaultValue(language, headerSection, qualifiedName);

diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmClass.java
@@ -544,7 +544,8 @@ private EconomicMap<Object, ObjectMember> createDelegatingMembers(
               VmUtils.unavailableSourceSection(),
               VmModifier.NONE,
               name,
-              name.toString());
+              name.toString(),
+              false);
       member.initMemberNode(memberNodeFactory.apply(member));
       result.put(name, member);
     }

diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/VmObjectBuilder.java b/pkl-core/src/main/java/org/pkl/core/runtime/VmObjectBuilder.java
@@ -49,7 +49,12 @@ public VmObjectBuilder addEntry(Object key, Object value) {
   public VmObjectBuilder addEntry(Object key, SharedMemberNode valueNode) {
     var entry =
         new ObjectMember(
-            valueNode.getSourceSection(), valueNode.getHeaderSection(), VmModifier.ENTRY, null, "");
+            valueNode.getSourceSection(),
+            valueNode.getHeaderSection(),
+            VmModifier.ENTRY,
+            null,
+            "",
+            false);
     entry.initMemberNode(valueNode);
     EconomicMaps.put(members, key, entry);
     return this;