[PR #747] [CLOSED] Fix resolution of for-generator vars within iteratee #707

New Issue

adam · 2025-12-30T01:26:18+01:00

adam commented

2025-12-30 01:26:18 +01:00

📋 Pull Request Information

Original PR: https://github.com/apple/pkl/pull/747
Author: @bioball
Created: 10/29/2024
Status: ❌ Closed

Base: main ← Head: for-generator-scope-fix

📝 Commits (1)

252249f Fix resolution of for-generator vars within iteratee

📊 Changes

11 files changed (+378 additions, -15 deletions)

View changed files

📝 pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java (+22 -2)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorForNode.java (+4 -6)
➕ pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorIterableNode.java (+80 -0)
➕ pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorIteratingNode.java (+63 -0)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorMemberNode.java (+4 -0)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorSpreadNode.java (+4 -7)
➕ pkl-core/src/main/java/org/pkl/core/ast/frame/WriteNthFrameSlotNode.java (+65 -0)
➕ pkl-core/src/test/files/LanguageSnippetTests/input/generators/forGeneratorNestedReference.pkl (+31 -0)
➕ pkl-core/src/test/files/LanguageSnippetTests/input/generators/spreadSyntaxInForGenerator.pkl (+57 -0)
➕ pkl-core/src/test/files/LanguageSnippetTests/output/generators/forGeneratorNestedReference.pcf (+20 -0)
➕ pkl-core/src/test/files/LanguageSnippetTests/output/generators/spreadSyntaxInForGenerator.pcf (+28 -0)

📄 Description

This is one attempt to fix #741. The approach here is to wrap the iterable in a new root node; that is:

res {
  for (i in List(1, 2)) {
//          ^^^^^^^^^^  now a root node
    i
  }
}

We then store any existing for-generator vars as auxiliary slots inside this root node.

I'm not entirely happy with this approach; one quirk here is that we are creating a deeper stack frame, but cloning the parent stack frame. We need to do this so that we can resolve variables that might be on the outer frame.

Going deeper: the reason we need to copy outer frame slot values is because our lexical scope is all based off of VmObjectLike; we don't have a mechanism for resolving a variable on the outer frame if there is no VmObject to attach a frame to.

However, we don't have this pattern anywhere in the codebase. It's a one-off and feels quite hacky.

Also, creating a new root node adds more interpreter overhead. But the cost there seems palatable; it's unclear to me how much this actually hurts our real world performance.

I can think of two alternatives to this approach:

Deep-force the iterable prior to executing each loop. However:
- This will introduce other regressions (deep-forcing might throw, but for-generators today may not necessarily execute those paths).
- This is likely terrible for performance (think: iterating over a glob import)
Resolve variables at parse-time, within AstBuilder. I think we want to do this eventually, but it's a big effort.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/apple/pkl/pull/747 **Author:** [@bioball](https://github.com/bioball) **Created:** 10/29/2024 **Status:** ❌ Closed **Base:** `main` ← **Head:** `for-generator-scope-fix` --- ### 📝 Commits (1) - [`252249f`](https://github.com/apple/pkl/commit/252249fc87525e30a13719e9ba1fdddbd4b0eec4) Fix resolution of for-generator vars within iteratee ### 📊 Changes **11 files changed** (+378 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java` (+22 -2) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorForNode.java` (+4 -6) ➕ `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorIterableNode.java` (+80 -0) ➕ `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorIteratingNode.java` (+63 -0) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorMemberNode.java` (+4 -0) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorSpreadNode.java` (+4 -7) ➕ `pkl-core/src/main/java/org/pkl/core/ast/frame/WriteNthFrameSlotNode.java` (+65 -0) ➕ `pkl-core/src/test/files/LanguageSnippetTests/input/generators/forGeneratorNestedReference.pkl` (+31 -0) ➕ `pkl-core/src/test/files/LanguageSnippetTests/input/generators/spreadSyntaxInForGenerator.pkl` (+57 -0) ➕ `pkl-core/src/test/files/LanguageSnippetTests/output/generators/forGeneratorNestedReference.pcf` (+20 -0) ➕ `pkl-core/src/test/files/LanguageSnippetTests/output/generators/spreadSyntaxInForGenerator.pcf` (+28 -0) </details> ### 📄 Description This is one attempt to fix #741. The approach here is to wrap the iterable in a new root node; that is: ```pkl res { for (i in List(1, 2)) { // ^^^^^^^^^^ now a root node i } } ``` We then store any existing for-generator vars as auxiliary slots inside this root node. I'm not entirely happy with this approach; one quirk here is that we are creating a deeper stack frame, but cloning the parent stack frame. We need to do this so that we can resolve variables that might be on the outer frame. Going deeper: the reason we need to copy outer frame slot values is because our lexical scope is all based off of `VmObjectLike`; we don't have a mechanism for resolving a variable on the outer frame if there is _no_ VmObject to attach a frame to. However, we don't have this pattern anywhere in the codebase. It's a one-off and feels quite hacky. Also, creating a new root node adds more interpreter overhead. But the cost there seems palatable; it's unclear to me how much this actually hurts our real world performance. I can think of two alternatives to this approach: 1. Deep-force the iterable prior to executing each loop. However: - This will introduce _other_ regressions (deep-forcing might throw, but for-generators today may not necessarily execute those paths). - This is likely terrible for performance (think: iterating over a glob import) 2. Resolve variables at parse-time, within AstBuilder. I think we want to do this eventually, but it's a big effort. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

adam added the pull-request label 2025-12-30 01:26:18 +01:00

adam closed this issue

2025-12-30 01:26:18 +01:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/pkl#707