fix: resolve nullable outer property access and semicolons by jhult · Pull Request #54 · jdx/pklr

jhult · 2026-04-07T01:03:51Z

Original error

❯ RUST_BACKTRACE=1 hk run pre-commit

thread 'main' (49584) panicked at src/settings.rs:90:30:
Failed to load configuration: Failed to read config file: /Users/jonathan/development/inko-tantivy/hk.pkl

Caused by:
    Eval error: field not found: checkFail

Location:
    src/config.rs:389:18
stack backtrace:
   0: __rustc::rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::result::unwrap_failed
   3: hk::settings::<impl hk::settings::generated::settings::Settings>::get
   4: hk::cli::run::{{closure}}
   5: tokio::runtime::park::CachedParkThread::block_on
   6: tokio::runtime::runtime::Runtime::block_on
   7: hk::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

Summary

Arc-wrap Lambda captures so cloning a Lambda is O(1)
Seed Null into eval_scope for nullable-no-default base class
properties so outer.optProp resolves to Null instead of erroring
Treat ; as whitespace so Pkl's semicolon property separator is accepted

Details

`outer.before` — field not found

hk's helpers.pkl TestMaker class uses this pattern:

class TestMaker {
  before: String?
  local function makeTest(...): Config.StepTest = new Config.StepTest {
    before = outer.before
    ...
  }
}

When outer.before was evaluated and before had no default value and was not set on the TestMaker instance, outer (a snapshot of the scope before evaluating the object body) didn't contain before at all, causing a "field not found: before" error.

Fix: in eval_amended_object, after building eval_scope from base_scope + current_scope, iterate over the base entries and insert Null for any nullable-no-default property absent from eval_scope. Since eval_scope becomes outer in the child eval_entries call, outer.before now resolves to Null.

O(2^n) Lambda capture cloning

TestMaker defines five local functions in sequence: makeTest, checkPass, checkFail, fixPass, fixFail. Each is a deferred lambda that captures child_scope.flatten() at evaluation time, which includes all previously-defined lambdas. With Lambda captures stored as a bare IndexMap, cloning a Lambda deep-copied its entire capture map — causing each successive lambda's clone cost to double, resulting in O(2^n) total work when scope.flatten() was called across 128 hk builtins.

Fix: change Value::Lambda's captures from IndexMap<String, Value> to Arc<IndexMap<String, Value>>. Clone is now O(1).

Semicolons as property separators

Pkl allows ; as a separator between properties in object bodies (e.g., new { file = "package.json"; contains = "..." }). The lexer was rejecting them as unexpected characters.

Fix: skip ; alongside whitespace in skip_whitespace_and_comments.

(checkFail was missing because its function definition used a dotted return type Config.StepTest, fixed in the previous commit. After that fix, the error became field not found: before.)

Test plan

All 266 existing tests continue to pass
HK_PKL_BACKEND=pklr hk run check succeeds in inko-tantivy

Type annotations like `Config.StepTest` were not parsed correctly. The parser consumed only the first identifier (`Config`) and treated the dot as an unexpected token, causing function definitions with dotted return types to be silently dropped. This affected the hk builtins TestMaker helper class where functions like `checkFail`, `checkPass`, `fixPass`, and `fixFail` were all lost during parsing.

Pkl allows `;` as a property separator in object bodies (e.g., `new { file = "foo"; contains = "bar" }`). The lexer was rejecting them as unexpected characters. Now `;` is skipped along with other whitespace in `skip_whitespace_and_comments`.

Two changes work together to fix `outer.optionalProp` resolution in object amendments (the `outer.before` pattern in hk builtins): 1. Arc-wrap Lambda captures (`Value::Lambda` third field changed from `IndexMap` to `Arc<IndexMap>`). Previously, cloning a Lambda deep-copied its entire capture map, causing O(2^n) cost when successive local functions (makeTest, checkPass, checkFail, fixPass, fixFail) each captured the previous ones. With Arc the clone is O(1). 2. In `eval_amended_object`, seed `Null` into `eval_scope` for any nullable-no-default base class properties not already present. This makes `outer.before` (and similar optional properties) resolve to `Null` rather than failing with "field not found" when the property was never assigned in the class or overlay. Together these allow all 128+ hk builtins to evaluate successfully, including those using the `TestMaker` pattern with `outer.before`.

greptile-apps · 2026-04-07T01:07:14Z

Greptile Summary

This PR fixes three distinct bugs in pklr: outer.optionalProp now resolves to Null instead of panicking for nullable class properties with no default; Value::Lambda and Value::Object captures are Arc-wrapped for O(1) clone (eliminating O(2^n) deep-copy on large scopes); and semicolons are now treated as whitespace in the lexer so Pkl optional property separators are accepted.

Confidence Score: 5/5

Safe to merge; all three fixes are well-scoped and backed by regression tests.

Remaining findings are P2 style notes that do not affect correctness on the described code paths.

src/eval.rs nullable seeding (union-type gap is P2)

Important Files Changed

Filename	Overview
src/eval.rs	Arc-wraps Object/Lambda captures for O(1) clone; seeds Null for nullable properties so outer.optProp resolves correctly
src/lexer.rs	Adds semicolon to skip_whitespace_and_comments so Pkl semicolons are accepted as optional property separators
src/parser.rs	Extends parse_type to handle dotted type names like Config.StepTest
src/value.rs	Arc-wraps Object IndexMap and Lambda captures; as_object_mut and merge use Arc::make_mut correctly
tests/integration.rs	Adds nested class amend and outer.before regression tests; network tests are properly marked ignore

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[eval_amended_object] --> B[Build eval_scope from base + current scope]
    B --> C{For each base Property}
    C -->|nullable, no default, absent from scope| D[Seed Null into eval_scope]
    C -->|otherwise| E[Skip]
    D --> F[eval_entries with seeded scope]
    E --> F
    F --> G[set outer = flatten of seeded scope]
    G --> H[outer.optProp resolves to Null]

_{Reviews (1): Last reviewed commit: "fix(eval): resolve nullable outer proper..." | Re-trigger Greptile}

greptile-apps · 2026-04-07T01:07:17Z

+        for entry in base_entries {
+            if let Entry::Property(prop) = entry
+                && prop.value.is_none()
+                && prop.body.is_none()
+                && !has_modifier(&prop.modifiers, Modifier::Local)
+                && eval_scope.get(&prop.name).is_none()
+                && matches!(prop.type_ann, Some(crate::parser::TypeExpr::Nullable(_)))
+            {
+                eval_scope.set(prop.name.clone(), Value::Null);
+            }


Nullable seeding misses union-type nullable annotations

The guard matches!(prop.type_ann, Some(TypeExpr::Nullable(_))) catches the common Prop? spelling, but Pkl also supports expressing optionality as a union, e.g. prop: (String|Null). Those produce a TypeExpr::Union variant and are skipped here, so outer.prop on such a property would still fail with "field not found". The same gap exists in the parallel seeding in eval_entries. Narrow edge case not present in the reported bug, but worth tracking.

greptile-apps · 2026-04-07T01:07:18Z

+  j: String?
+  k: String?
+  l: String?
+  m: String?
+  n: String?
+  o: String?
+  p: String?
+  q: String?
+  r: String?
+  s: String?
+  t: String?
+  env: Mapping<String, String> = new Mapping<String, String> {}
+  tests: Mapping<String, StepTest> = new Mapping<String, StepTest> {}
+}
+
+class Hook {
+  fix: Boolean?
+  stash: String?
+  env: Mapping<String, String> = new Mapping<String, String> {}
+  steps: Mapping<String, Step> = new Mapping<String, Step> {}
+}
+
+hooks: Mapping<String, Hook> = new Mapping<String, Hook> {}
+"#;
+    let mut evaluator = pklr::Evaluator::new();
+    let result = evaluator
+        .eval_source(src, std::path::Path::new("test_nested.pkl"))
+        .await;
+    eprintln!("eval completed, is_ok={}", result.is_ok());
+    if let Err(ref e) = result {
+        eprintln!("error: {e}");
+    }
+    assert!(result.is_ok());
+}
+
+#[tokio::test]


Local-path tests silently pass when fixture is absent

Several tests depending on /tmp/hk-extracted/ do an early return when the file is missing, so cargo test -- --ignored reports them as passed even without the fixture. Consistently relying on #[ignore = "..."] (already used on some tests) and removing the early-return guard would make the skip intent explicit.

gemini-code-assist

Code Review

This pull request optimizes the evaluator by wrapping object and lambda maps in Arc for O(1) cloning and switching Scope to use Rc. It also enhances the parser to support dotted type names, updates the lexer to treat semicolons as whitespace, and implements logic to seed Null for nullable properties without defaults. Feedback highlights that the current nullability check is too restrictive and the logic is duplicated, suggesting a refactor into a more comprehensive helper method.

gemini-code-assist · 2026-04-07T01:09:23Z

+        for entry in entries {
+            if let Entry::Property(prop) = entry
+                && prop.value.is_none()
+                && prop.body.is_none()
+                && !has_modifier(&prop.modifiers, Modifier::Local)
+                && !outer_map.contains_key(&prop.name)
+                && matches!(prop.type_ann, Some(crate::parser::TypeExpr::Nullable(_)))
+            {
+                outer_map.insert(prop.name.clone(), Value::Null);
+            }
+        }


The logic for seeding Null for nullable properties is duplicated here and in eval_amended_object (lines 1247-1257). Furthermore, the current check matches!(prop.type_ann, Some(crate::parser::TypeExpr::Nullable(_))) is too restrictive. In Pkl, properties are also nullable if they are of type Any, if they are untyped (which defaults to Any), or if they are a Union type that includes Null (e.g., String | Null).

Consider refactoring this into a helper method that performs a more comprehensive nullability check to ensure consistency across the evaluator.

## 🤖 New release * `pklr`: 0.4.0 -> 0.4.1 (✓ API compatible changes) <details><summary>Changelog</summary> <blockquote> ## [0.4.1](v0.4.0...v0.4.1) - 2026-04-07 ### Fixed - resolve nullable outer property access and semicolons ([#54](#54)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).  --- > [!NOTE] > [Cursor Bugbot](https://cursor.com/bugbot) is generating a summary for commit f211a5c. Configure [here](https://www.cursor.com/dashboard/bugbot).

Includes this fix: jdx/pklr#54

jhult added 3 commits April 6, 2026 14:28

fix(lexer): treat semicolons as whitespace

c1cb4f0

Pkl allows `;` as a property separator in object bodies (e.g., `new { file = "foo"; contains = "bar" }`). The lexer was rejecting them as unexpected characters. Now `;` is skipped along with other whitespace in `skip_whitespace_and_comments`.

greptile-apps Bot reviewed Apr 7, 2026

View reviewed changes

gemini-code-assist Bot reviewed Apr 7, 2026

View reviewed changes

jdx merged commit 5e84d7f into jdx:main Apr 7, 2026
7 checks passed

jdx mentioned this pull request Apr 7, 2026

chore: release v0.4.1 #55

Merged

jhult deleted the fix/nullable-outer-property-access-and-semicolons branch April 8, 2026 02:22

jhult mentioned this pull request Apr 8, 2026

fix(deps): update rust crate pklr to 0.4.1 jdx/hk#805

Merged

jdx pushed a commit to jdx/hk that referenced this pull request Apr 8, 2026

fix(deps): update rust crate pklr to 0.4.1 (#805)

c9836be

Includes this fix: jdx/pklr#54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: resolve nullable outer property access and semicolons#54

fix: resolve nullable outer property access and semicolons#54
jdx merged 3 commits intojdx:mainfrom
jhult:fix/nullable-outer-property-access-and-semicolons

jhult commented Apr 7, 2026

Uh oh!

greptile-apps Bot commented Apr 7, 2026

Uh oh!

greptile-apps Bot Apr 7, 2026

Uh oh!

greptile-apps Bot Apr 7, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Apr 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jhult commented Apr 7, 2026

Original error

Summary

Details

outer.before — field not found

O(2^n) Lambda capture cloning

Semicolons as property separators

Test plan

Uh oh!

greptile-apps Bot commented Apr 7, 2026

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`outer.before` — field not found