Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Late-bound values of iteratees within nested for/spread fail to resolve for-generator variables #741

Open
bioball opened this issue Oct 25, 2024 · 0 comments · May be fixed by #844
Open
Labels
bug Something isn't working

Comments

@bioball
Copy link
Contributor

bioball commented Oct 25, 2024

This code throws:

bar = new {}

foo {
  for (i in List(1)) {
    ...(bar) {
      baz {
        new { i }
      }
    }.baz
  }
}

This also throws (logically the same code):

bar = new {}

foo {
  for (i in List(1)) {
    for (elem in (bar) {
      baz {
        new { i }
      }
    }.baz) {
      elem
    }
  }
}

Throws:

–– Pkl Error ––
Cannot find property `i`.

7 | new { i }
          ^
bioball added a commit to bioball/pkl that referenced this issue Oct 30, 2024
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 bioball added the bug Something isn't working label Oct 31, 2024
bioball added a commit that referenced this issue Oct 31, 2024
There is currently a bug around resolving variables within the iterable of a for
generator or spread syntax (#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.
odenix added a commit to odenix/pkl that referenced this issue Dec 10, 2024
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 some unnecessarily complex code
* 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 apple#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
@odenix odenix linked a pull request Dec 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants
@bioball and others