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

New Issue

adam · 2025-12-30T01:23:22+01:00

adam commented

2025-12-30 01:23:22 +01:00

Originally created by @HT154 on GitHub (May 7, 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.

Originally created by @HT154 on GitHub (May 7, 2025). Repro: ```pkl 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`.

adam commented

2025-12-30 01:23:23 +01:00

@bioball commented on GitHub (May 7, 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)

@bioball commented on GitHub (May 7, 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)

adam commented

2025-12-30 01:23:23 +01:00

@Raja-fn commented on GitHub (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

@Raja-fn commented on GitHub (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

adam commented

2025-12-30 01:23:23 +01:00

@odenix commented on GitHub (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.

@odenix commented on GitHub (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.

adam commented

2025-12-30 01:23:24 +01:00

@bioball commented on GitHub (May 22, 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!

@bioball commented on GitHub (May 22, 2025): I think amending is orthogonal here; normal properties will default to the type's default value. ```pkl a: A ``` Is effectively the same as: ```pkl a: A = new A {} ``` It's strange that this works: ```pkl a: A b = a ``` But this suddenly breaks: ```pkl 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!

adam commented

2025-12-30 01:23:24 +01:00

@odenix commented on GitHub (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.

@odenix commented on GitHub (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.

adam commented

2025-12-30 01:23:24 +01:00

@HT154 commented on GitHub (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.

@HT154 commented on GitHub (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: ```pkl // 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: ```pkl 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.

adam commented

2025-12-30 01:23:25 +01:00

@bioball commented on GitHub (May 22, 2025):

Our Kubernetes package has fixed properties without explicitly assigned values: 65907044bc/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:

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.

@bioball commented on GitHub (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](https://github.com/user-attachments/assets/d225eb79-c00d-49a9-8f0c-45d6bc9d6340) > 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.

adam commented

2025-12-30 01:23:25 +01:00

@odenix commented on GitHub (May 31, 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.

@odenix commented on GitHub (May 31, 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.

adam commented

2025-12-30 01:23:25 +01:00

@bioball commented on GitHub (May 31, 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).

@bioball commented on GitHub (May 31, 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).

adam commented

2025-12-30 01:23:26 +01:00

@stackoverflow commented on GitHub (Sep 22, 2025):

I don't have a super strong opinion here, but I agree with odenix that it's better to be explicit in this case. As an empty local might be a legitimate error from the user side, but for normal properties you can't be sure.
We could change IntelliJ's and tree-sitter's grammar to not allow an empty local, as, right now, this will always throw in the ast builder, regardless if the variable is read or not.

@stackoverflow commented on GitHub (Sep 22, 2025): I don't have a super strong opinion here, but I agree with odenix that it's better to be explicit in this case. As an empty local might be a legitimate error from the user side, but for normal properties you can't be sure. We could change IntelliJ's and tree-sitter's grammar to not allow an empty local, as, right now, this will always throw in the ast builder, regardless if the variable is read or not.

adam commented

2025-12-30 01:23:26 +01:00

@RedCMD commented on GitHub (Sep 25, 2025):

even the syntax highlighter doesn't like it

@RedCMD commented on GitHub (Sep 25, 2025): even the syntax highlighter doesn't like it

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: starred/pkl#309