starred/pkl
1
0
Fork 0
mirror of https://github.com/apple/pkl.git synced 2026-09-03 08:17:14 +02:00
Code Issues 198 Packages Projects Releases 10 Wiki Activity

[PR #844] [MERGED] Overhaul implementation of for-generators #768

New Issue
Closed
opened 2025-12-30 01:26:37 +01:00 by adam · 0 comments
adam commented 2025-12-30 01:26:37 +01:00
Owner
Copy Link
Copy Source

📋 Pull Request Information

Original PR: https://github.com/apple/pkl/pull/844
Author: @odenix
Created: 12/10/2024
Status: ✅ Merged
Merged: 1/28/2025
Merged by: @bioball

Base: main ← Head: for-generator

📝 Commits (5)

📊 Changes

57 files changed (+909 additions, -1139 deletions)

View changed files

📝 pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java (+1 -6)
📝 pkl-core/src/main/java/org/pkl/core/ast/VmModifier.java (+1 -8)
📝 pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java (+126 -155)
📝 pkl-core/src/main/java/org/pkl/core/ast/builder/SymbolTable.java (+73 -44)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/binary/LetExprNode.java (+3 -4)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorElementNode.java (+13 -20)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorEntryNode.java (+5 -15)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorForNode.java (+55 -102)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorMemberNode.java (+35 -77)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorObjectLiteralNode.java (+33 -44)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorPredicateMemberNode.java (+5 -13)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorPropertyNode.java (+19 -26)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorSpreadNode.java (+62 -107)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorWhenNode.java (+2 -2)
➕ pkl-core/src/main/java/org/pkl/core/ast/expression/generator/ObjectData.java (+93 -0)
➕ pkl-core/src/main/java/org/pkl/core/ast/expression/generator/RestoreForBindingsNode.java (+46 -0)
➖ pkl-core/src/main/java/org/pkl/core/ast/expression/generator/WriteForVariablesNode.java (+0 -52)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/literal/AmendFunctionNode.java (+5 -43)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/literal/FunctionLiteralNode.java (+2 -3)
📝 pkl-core/src/main/java/org/pkl/core/ast/expression/literal/ObjectLiteralNode.java (+2 -2)

...and 37 more files

📄 Description

Based on: https://github.com/apple/pkl/pull/837

Motivation:

  • fix known bugs and limitations of for-generators
  • improve code health by removing complex workarounds

Changes:

  • simplify AstBuilder code related to for-generators
    • track for-generators via SymbolTable.enterForGenerator()
    • add RestoreForBindingsNode during initial AST construction
      instead of calling MemberNode.replaceBody() later on
    • simplify unnecessarily complex code such as objectMemberInserter
  • remove workarounds and band-aids such as:
    • isInIterable
    • executeAndSetEagerly
    • adding dummy slots in AmendFunctionNode
  • overhaul implementation of for-generators
    • store keys and values of for-generator iterations in regular instead of auxiliary frame slots
      • set them via TypeNode.executeAndSet()
      • ResolveVariableNode no longer needs to search auxiliary slots
      • Read(Enclosing)AuxiliarySlot is no longer needed
    • at the start of each for-generator iteration, create a new VirtualFrame
      that is a copy of the current frame (arguments + slots)
      and stores the iteration key and value in additional slots.
    • execute for-generator iteration with the newly created frame
      • childNode.execute(newFrame)
      • Pkl objects created during the iteration will materialize this frame
    • store newly created frames in owner.extraStorage
      if their for-generator slots may be accessed when a generated member is executed
      • resolving variable names to for-generator variables at parse time would make this analysis more precise
    • when a generated member is executed,
      * retrieve the corresponding frame stored in owner.extraStorage
      * copy the retrieved frame's for-generator slots into slots of the current frame

Result:

  • for-generators are implemented in a correct, reasonably simple, and reasonably efficient way
    • complexity is fully contained within package generator and AstBuilder
  • for-generator keys and values can be accessed from all nested scopes:
    • key and value expressions of generated members
    • condition expressions of nested when-generators
    • iterable expressions of nested for-generators
  • for-generator keys and values can be accessed from within objects created by the expressions listed above
  • sibling for-generators can use the same key/value variable names
  • parent/child for-generators can use the same key/value variable names
  • fixes https://github.com/apple/pkl/issues/741

Limitations not addressed in this PR:

  • object spreading is eager in values
    This should be easy to fix.
  • for-generators are eager in values
    I think this could be fixed by:
    • resolving variable names to for-generator variables at parse time
    • replacing every access to a for-generator's value with iterable[key]
  • for/when-generator bodies can't have local properties/methods
    I think this could be fixed by:
    • resolving variable names to local properties/methods at parse time
    • internally renaming generated local properties/methods to avoid name clashes

🔄 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/844 **Author:** [@odenix](https://github.com/odenix) **Created:** 12/10/2024 **Status:** ✅ Merged **Merged:** 1/28/2025 **Merged by:** [@bioball](https://github.com/bioball) **Base:** `main` ← **Head:** `for-generator` --- ### 📝 Commits (5) - [`43ddb6a`](https://github.com/apple/pkl/commit/43ddb6aa1efb730e764275643c59ee9b2d69c977) Overhaul implementation of for-generators - [`f5fcc70`](https://github.com/apple/pkl/commit/f5fcc70174af559ae0f0e0433f76d64ff8ea255c) Polish code - [`7be3e44`](https://github.com/apple/pkl/commit/7be3e44cc4a33034a8c3095bb85dff15fdcda3c3) Reformat code - [`80c7319`](https://github.com/apple/pkl/commit/80c731969de031eb00aea7589b2f9c7db0bef8ed) Address review feedback - [`636a2b8`](https://github.com/apple/pkl/commit/636a2b85f3426d86e5c669c7f82b8fdc1102d1f7) Address review feedback ### 📊 Changes **57 files changed** (+909 additions, -1139 deletions) <details> <summary>View changed files</summary> 📝 `pkl-core/src/main/java/org/pkl/core/ast/MemberNode.java` (+1 -6) 📝 `pkl-core/src/main/java/org/pkl/core/ast/VmModifier.java` (+1 -8) 📝 `pkl-core/src/main/java/org/pkl/core/ast/builder/AstBuilder.java` (+126 -155) 📝 `pkl-core/src/main/java/org/pkl/core/ast/builder/SymbolTable.java` (+73 -44) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/binary/LetExprNode.java` (+3 -4) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorElementNode.java` (+13 -20) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorEntryNode.java` (+5 -15) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorForNode.java` (+55 -102) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorMemberNode.java` (+35 -77) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorObjectLiteralNode.java` (+33 -44) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorPredicateMemberNode.java` (+5 -13) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorPropertyNode.java` (+19 -26) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorSpreadNode.java` (+62 -107) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/GeneratorWhenNode.java` (+2 -2) ➕ `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/ObjectData.java` (+93 -0) ➕ `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/RestoreForBindingsNode.java` (+46 -0) ➖ `pkl-core/src/main/java/org/pkl/core/ast/expression/generator/WriteForVariablesNode.java` (+0 -52) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/literal/AmendFunctionNode.java` (+5 -43) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/literal/FunctionLiteralNode.java` (+2 -3) 📝 `pkl-core/src/main/java/org/pkl/core/ast/expression/literal/ObjectLiteralNode.java` (+2 -2) _...and 37 more files_ </details> ### 📄 Description Based on: https://github.com/apple/pkl/pull/837 Motivation: * fix known bugs and limitations of for-generators * improve code health by removing complex workarounds Changes: * simplify AstBuilder code related to for-generators * track for-generators via `SymbolTable.enterForGenerator()` * add `RestoreForBindingsNode` during initial AST construction instead of calling `MemberNode.replaceBody()` later on * simplify unnecessarily complex code such as `objectMemberInserter` * remove workarounds and band-aids such as: * `isInIterable` * `executeAndSetEagerly` * adding dummy slots in `AmendFunctionNode` * overhaul implementation of for-generators * store keys and values of for-generator iterations in regular instead of auxiliary frame slots * set them via `TypeNode.executeAndSet()` * `ResolveVariableNode` no longer needs to search auxiliary slots * `Read(Enclosing)AuxiliarySlot` is no longer needed * at the start of each for-generator iteration, create a new `VirtualFrame` that is a copy of the current frame (arguments + slots) and stores the iteration key and value in additional slots. * execute for-generator iteration with the newly created frame * `childNode.execute(newFrame)` * Pkl objects created during the iteration will materialize this frame * store newly created frames in `owner.extraStorage` if their for-generator slots may be accessed when a generated member is executed * resolving variable names to for-generator variables at parse time would make this analysis more precise * when a generated member is executed, * retrieve the corresponding frame stored in `owner.extraStorage` * copy the retrieved frame's for-generator slots into slots of the current frame Result: * for-generators are implemented in a correct, reasonably simple, and reasonably efficient way * complexity is fully contained within package `generator` and `AstBuilder` * for-generator keys and values can be accessed from all nested scopes: * key and value expressions of generated members * condition expressions of nested when-generators * iterable expressions of nested for-generators * for-generator keys and values can be accessed from within objects created by the expressions listed above * sibling for-generators can use the same key/value variable names * parent/child for-generators can use the same key/value variable names * fixes https://github.com/apple/pkl/issues/741 Limitations not addressed in this PR: * object spreading is eager in values This should be easy to fix. * for-generators are eager in values I think this could be fixed by: * resolving variable names to for-generator variables at parse time * replacing every access to a for-generator's `value` with `iterable[key]` * for/when-generator bodies can't have local properties/methods I think this could be fixed by: * resolving variable names to local properties/methods at parse time * internally renaming generated local properties/methods to avoid name clashes --- <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:37 +01:00
adam closed this issue 2025-12-30 01:26:37 +01:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
bug
duplicate
enhancement
good first issue
help wanted
pull-request
Mirrored from GitHub Pull Request
No labels pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
adam (Adam Melkus)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/pkl#768