apple / pkl

A configuration as code language with rich validation and tooling.
https://pkl-lang.org
Apache License 2.0
10.15k stars 270 forks source link

Type constraints on `typealias`es are evaluated eagerly #446

Open sin-ack opened 5 months ago

sin-ack commented 5 months ago

Consider the following (the required typealias redirection is another issue):

// foo.pkl
typealias FooMapping = Mapping<String, String>
foo: FooMapping(toMap().every((k, v) -> bar.containsKey(k))

bar: Mapping<String, Boolean>

// bar.pkl
amends "./foo.pkl"

bar {
  "a" = true
  "b" = false
}

foo {
  "a" = "aaa"
}
$ pkl eval bar.pkl
foo {
  ["a"] = "aaa"
}
bar {
  ["a"] = true
  ["b"] = false
}

As expected. However, if we change foo's definition to:

typealias FooMapping1 = Mapping<String, String>
typealias FooMapping = FooMapping1(toMap().every((k, v) -> bar.containsKey(k))
foo: FooMapping

Then I get:

$ pkl eval bar.pkl
–– Pkl Error ––
Type constraint `toMap().every((k, v) -> bar.containsKey(k))` violated.
Value: new Mapping { ["a"] = "aaa" }

2 | typealias FooMapping = FooMapping1(toMap().every((k, v) -> bar.containsKey(k)))
                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
at foo#foo (file:///.../foo.pkl, line 2)

8 | foo {
    ^^^^^
at bar#foo (file:///.../bar.pkl, line 8)

Placing a trace() on bar reveals that bar is a Mapping {} during the evaluation of the constraint, which is unexpected (to me at least). Is this intended behavior?

translatenix commented 5 months ago

My understanding is that accessing bar from within a type constraint isn’t allowed because it isn’t const. If you don’t get an error for that it’s probably a bug.

holzensp commented 5 months ago

I can't reproduce this (but also; the snippets you included are not syntactically valid Pkl, so I think something got lost in reduction of the example).

There is a known bug that typealiases resolved names in their definition scope, instead of their use scope. This has been addressed in #373 and will be part of 0.26 (hopefully June).

By the way, toMap() already forces the entire Mapping (keys and values). If you want to keep things lazy, consider keys.intersect(bar.keys) == keys instead.

bioball commented 5 months ago

I think @sin-ack is using 0.26.0-dev, which reproduces for me.

We currently have an issue in the latest dev version where typealias bodies are not late-bound (changed a fix for a scoping bug in https://github.com/apple/pkl/pull/144). But, at the same time, they aren't required to be const.

We're thinking through whether typealias bodies should be late-bound, or if this is actually the correct behavior. If this is the correct behavior, then we will add an additional rule that references must be const. If not, then we need to adjust the behavior here so that those references are late-bound.