Skip to content

Behavior differs between local and non-local properties with no default value #1064

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

Open
HT154 opened this issue May 6, 2025 · 9 comments
Open

Comments

@HT154
Copy link
Contributor

HT154 commented May 6, 2025

Repro:

class A {}
a: A
// result: a {}
local aLocal: A
// error: Missing property value.

I would expect aLocal to be initialized to the default value of A here instead.
This error is thrown during parsing, so it's not even necessary to have aLocal in the evaluation path to trigger it.
Also pkl-intellij does not register this as an error, only pkl eval.

@bioball
Copy link
Member

bioball commented May 6, 2025

I'm actually not sure why it behaves this way. I can't think of a good reason for the language to do this; might okay to make this behave like a normal property (default to the type's default value)

@Raja-fn
Copy link

Raja-fn commented May 18, 2025

I think that you are just putting the value without just explaining the old value which is not providing the first value any point

@odenix
Copy link
Contributor

odenix commented May 20, 2025

@HT154 What’s the use case? Unlike a regular property, a local property cannot be amended in place. I can’t think of many valid reasons to set a local property to the default object value, so it might pay off to be explicit.

@bioball
Copy link
Member

bioball commented May 21, 2025

I think amending is orthogonal here; normal properties will default to the type's default value.

a: A

Is effectively the same as:

a: A = new A {}

It's strange that this works:

a: A

b = a

But this suddenly breaks:

local a: A

b = a

Also, const and fixed properties also cannot be amended either, but they, like other properties, default to the type's default value.

By the way, welcome back!

@odenix
Copy link
Contributor

odenix commented May 22, 2025

normal properties will default to the type's default value.

True, except that many types don’t have default values and always need to be assigned explicitly.

Also, const and fixed properties also cannot be amended either, but they, like other properties, default to the type's default value.

Under the assumption that default values are rarely useful for local/fixed/const properties, requiring explicit assignment will avoid mistakes. I don’t have a strong opinion on this issue, but being selectively more restrictive can sometimes be a tolerable/beneficial form of inconsistency.

@HT154
Copy link
Contributor Author

HT154 commented May 22, 2025

My use case here is, once again, abusing the language to get more namespacing without more modules. Sorry!

I have a bunch of similar looking but unrelated types and I want to build sets of mixins for them. To that end, I have a module that look like this:

// mixins.pkl
class MixinsForA {
  const mix1: Mixin<A> = new { ... }
  const function mix2(arg): Mixin<A> = new { ... }
}

class MixinsForB {
  const mix1: Mixin<B> = new { ... }
  const function mix2(arg): Mixin<B> = new { ... }
}

Usage looks like this:

import "mixins.pkl"

local a: mixins.MixinsForA = new {}

local b: mixins.MixinsForB = new {}

// someA |> a.mix1 |> a.mix2("foo")

A couple key goals here are having a single import point and reducing the amount of code needed to use/reference each mixin.

@bioball
Copy link
Member

bioball commented May 22, 2025

Our Kubernetes package has fixed properties without explicitly assigned values: https://github.com/apple/pkl-k8s/blob/65907044bcf76ff0347abbb84e307fd291e0ad70/generated-package/api/core/v1/Pod.pkl#L29-L31

I'm not sure what errors we might be preventing here; you still get an error if the property's type has no default, but only when accessed. I think that better follows Pkl's lazy ethos. Also, parse-time checks like this can't be caught using test.catch().

We can (and probably should) add an IDE error if a local property's type has no default value, which is the same as what we do with const and fixed:

Image

I don’t have a strong opinion on this issue, but being selectively more restrictive can sometimes be a tolerable/beneficial form of inconsistency.

Yeah, makes sense, good food for thought.

@odenix
Copy link
Contributor

odenix commented May 30, 2025

I'm not sure what errors we might be preventing here

When I see local a: A, I immediately wonder if the author forgot to set a value (and they might have). To me it looks strange that a property that can’t be set anywhere else doesn’t have an explicit value. The analogy that comes to mind is immutable locals in GPLs, which typically require an explicit value.

For module properties, defaulting to new {} avoids a lot of noise. For locals, it’s not clear why new {} is a useful default.

We can (and probably should) add an IDE error if a local property's type has no default value

I can’t tell if you were implying otherwise, but I believe that IntelliJ should only give an error if parsing/evaluation is guaranteed to produce an error. Otherwise, IntelliJ should give a warning.

@bioball
Copy link
Member

bioball commented May 30, 2025

I can’t tell if you were implying otherwise, but I believe that IntelliJ should only give an error if parsing/evaluation is guaranteed to produce an error. Otherwise, IntelliJ should give a warning.

We only show an error annotation if we know it will error during eval. And, a local property whose type has no default, will indeed raise an error during eval (as long as the property is executed).

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