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

Type constraints on typealiases are evaluated eagerly #446

Open
sin-ack opened this issue Apr 22, 2024 · 3 comments
Open

Type constraints on typealiases are evaluated eagerly #446

sin-ack opened this issue Apr 22, 2024 · 3 comments

Comments

@sin-ack
Copy link

sin-ack commented Apr 22, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

bioball commented Apr 25, 2024

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 #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.

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

4 participants