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

Local variables inside for loops yield an error #549

Open
EugZol opened this issue Jun 24, 2024 · 5 comments
Open

Local variables inside for loops yield an error #549

EugZol opened this issue Jun 24, 2024 · 5 comments

Comments

@EugZol
Copy link

EugZol commented Jun 24, 2024

Code:

hidden xs = new Listing { 1 2 3 }

object {
  for (x in xs) {
    local plusOne = x + 1
    plusOne
  }
}

On evaluation yields the following error:

A for-generator cannot generate object properties (only entries and elements).

Instead, expected to produce object with entries 2 3 4.

Version:

% pkl --version
Pkl 0.26.0 (macOS 14.3.1, native)
@holzensp
Copy link
Contributor

This is a short-coming we have at the moment. local is a modifier for a property; not a "variable." It really behaves like a property, which is why you can't have one in a for generator (because it's the same as generating the same property - possibly with different values - over and over).

If, as in this case, you only use it in one expression, you can use let;

object {
  for (x in XS) {
    let (plusOne = x + 1)
      plusOne
  }
}

If you do need it in multiple places, a (admittedly sub-optimal) workaround is to use a for generator again;

object {
  for (x in XS) {
    for (plusOne in List(x + 1)) {
      plusOne
    }
  }
}

@EugZol
Copy link
Author

EugZol commented Jun 25, 2024

My case was initially trying to group objects and map each subgroup under different properties of the result, but don't add the result property if the corresponding subgroup is empty. And another loop on top of that.

It went like this

Attempt 1: local variable in for-loop

class Entity {
  selectors: Listing<Selector>
}

class Selector {
  enabled: Boolean
  index: Int
}

hidden objects = new Listing<Entity> {
  new {
    selectors {
      new {
        enabled = false
        index = 1
      }
      new {
        enabled = true
        index = 2
      }
    }
  }
  new {
    selectors {
      new {
        enabled = false
        index = 3
      }
      new {
        enabled = true
        index = 4
      }
    }
  }
}

result: Listing = new {
  for (o in objects) {
    local grouped = o.selectors.toList().groupBy((x) -> x.enabled)

    new {
      when (grouped.containsKey(true)) {
        all_enabled {
          for (a in grouped[true]) {
            a.index
          }
        }
      }

      when (grouped.containsKey(false)) {
        all_disabled {
          for (a in grouped[false]) {
            a.index
          }
        }
      }
    }
  }
}

Error:

A for-generator cannot generate object properties (only entries and elements).

Attempt 2: local variable inside leaf object

result: Listing = new {
  for (o in objects) {
-
    new {
+      local grouped = o.selectors.toList().groupBy((x) -> x.enabled)

      when (grouped.containsKey(true)) {

Error:

Cannot find property `grouped`.

42 | when (grouped.containsKey(true)) {
           ^^^^^^^

Attempt 3: instance methods

I eventually was able to solve the problem with some instance methods on Entity.

However both errors above were rather unexpected for me. Curiously, removing whens second piece of code starts to work.

@EugZol
Copy link
Author

EugZol commented Jun 25, 2024

If you do need it in multiple places, a (admittedly sub-optimal) workaround is to use a for generator again;

I actually tried that as well! Doesn't work either, apparently because all_enabled/all_enabled are properties as well, and that artificial for-loop would need to generate them.

@holzensp
Copy link
Contributor

holzensp commented Jun 25, 2024

Works for me with that for-as-let, because all_enabled/all_disabled live inside that new {...} (which, btw, do you realise/intend that to be Dynamic)?

result: Listing = new {
  for (o in objects) {
    for (grouped in List(o.selectors.toList().groupBy((x) -> x.enabled))) {
      new {
        when (grouped.containsKey(true)) {
          all_enabled {
            for (a in grouped[true]) {
              a.index
            }
          }
        }

        when (grouped.containsKey(false)) {
          all_disabled {
            for (a in grouped[false]) {
              a.index
            }
          }
        }
      }
    }
  }
}

but also... this pattern of splitting a collection in two sub-collections based on a property is commonly called partitioning. Pkl has a partition method Collection, so I'd

result: Listing = new {
  for (o in objects) {
    let (partitionedSelectors = o.selectors.toList().partition((it) -> it.enabled))
      new {
        all_enabled {
          ...partitionedSelectors.first.map((it) -> it.index)
        }
        all_disabled {
          ...partitionedSelectors.second.map((it) -> it.index)
        }
      }
  }
}

(I'm omitting the whens around all_enabled and all_disabled, assuming that them being empty and them being null are equivalent; if not; you'd need to bring some whens back)

@EugZol
Copy link
Author

EugZol commented Jun 25, 2024

Works for me with that for-as-let, because all_enabled/all_disabled live inside that new {...}

Should've tried that! I've put it inside new before.

which, btw, do you realise/intend that to be Dynamic?

That's an example. Does it matter?

but also... this pattern of splitting a collection in two sub-collections based on a property is commonly called partitioning

That's nice! Thanks for the hint.

To summarize what I felt was inconvenient:

  • for refusing to produce properties even if it is executed once: should be regular "double declaration error" instead
  • local being quasi-property instead of syntax sugar... maybe block form of let statement which could yield multiple members could be introduced instead
  • when not seeing members which are defined at the same level

Hope will be addressed in the future versions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants