fix(eval): make @Deprecated lazy — only warn on access by jdx · Pull Request #58 · jdx/pklr

jdx · 2026-04-26T20:44:02Z

Summary

pkl-jvm warns about @Deprecated properties when they are referenced, not when their containing module is loaded. pklr previously walked the module body and emitted [pklr] WARNING: property '...' is deprecated for every @Deprecated property at evaluation time, even when the user never touched the property.

Concretely: importing a module that happens to define one or more @Deprecated aliases (e.g. hk's Builtins.pkl, with its long-deprecated check_byte_order_marker / fix_byte_order_marker aliases) printed warnings on every load — see jdx/hk#878.

Change

ObjectSource now carries a deprecated: IndexMap<String, Option<String>> populated from @Deprecated annotations on entries.
eval_module attaches a minimal ObjectSource to its return value only when the module declares deprecated properties, so behavior is unchanged for the common case (modules with no @Deprecated still return Value::Object(_, None)).
Expr::Field and Expr::NullSafeField consult the source's deprecated map and emit the warning at access time.
The eager check_deprecated calls in module/object body iteration are removed.

Verified end-to-end against hk

// library.pkl
import "pkl:test"
@Deprecated { message = "use newName instead" }
oldName = "value"
newName = "newvalue"

Caller	Before	After
`import "library.pkl"; result = library.newName`	warns	silent ✓
`import "library.pkl"; result = library.oldName`	warns	warns ✓
Just evaluating `library.pkl` directly (no access)	warns	silent ✓

Tests

cargo test: all 238 existing tests pass. (No stderr-capturing harness in tests/pkl_features.rs, so a regression test for the warning text would require restructuring; verified behaviorally via the cases above.)

🤖 Generated with Claude Code

Note

Medium Risk
Changes core evaluation semantics around @Deprecated by moving warnings from module/object evaluation time to field access time and propagating deprecation metadata through amend/merge paths; potential for missed/extra warnings or subtle ObjectSource behavior changes.

Overview
Switches @Deprecated warnings from eager emission during module/object evaluation to lazy emission only when a deprecated property is accessed via Expr::Field/Expr::NullSafeField, with per-(field,message) dedup to avoid stderr spam.

Extends ObjectSource with a deprecated map populated by new collect_deprecated/merge_deprecated helpers, and threads that metadata through module evaluation and object/class amend/merge code paths so deprecations survive overlays and late-binding re-evaluation.

^{Reviewed by Cursor Bugbot for commit 2ee1fc3. Bugbot is set up for automated code reviews on this repo. Configure here.}

@deprecated

pkl-jvm warns about @deprecated properties when they are referenced, not when their containing module is loaded. pklr previously walked the module body and emitted "[pklr] WARNING: property '...' is deprecated" for every @deprecated property at evaluation time, even when the user never touched that property. Concretely, importing a module that happens to define one or more @deprecated aliases (e.g. hk's `Builtins.pkl`) printed warnings on every load — see jdx/hk discussion #878. Move the warning to field-access expressions: - `ObjectSource` now carries a `deprecated: IndexMap<String, Option<String>>` populated from `@Deprecated` annotations on entries. - `eval_module` attaches a minimal `ObjectSource` to its return value *only* when the module declares deprecated properties, so behavior is unchanged for the common case. - Field access (`obj.field`) and null-safe field access (`obj?.field`) consult the source's `deprecated` map and emit the warning then. - The eager `check_deprecated` calls in module/object body iteration are removed. All 238 existing tests pass.

gemini-code-assist · 2026-04-26T20:44:05Z

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

greptile-apps · 2026-04-26T20:46:45Z

Greptile Summary

This PR converts @Deprecated warnings from eager (on module load) to lazy (on field access), storing a deprecated map on ObjectSource and emitting via warn_if_deprecated_access with per-(field, message) deduplication. The core approach is sound and matches pkl-jvm semantics.

P1 – null-safe access misses null-valued deprecated properties: Expr::NullSafeField checks !matches!(val, Value::Null) before warning, but unwrap_or(Value::Null) collapses both "field absent" and "field = null" into the same Value::Null, so a deprecated property whose value is null is silently accessed without a warning.

Confidence Score: 4/5

Safe to merge with the null-value edge-case fix applied; core lazy-warning semantics are correct.

One P1 logic bug (null-valued deprecated property skips warning via null-safe access) pulls the ceiling to 4. The P2 gap in merge_values does not lower the score further. All 238 tests pass and the happy-path behaviour matches pkl-jvm.

src/eval.rs — specifically the Expr::NullSafeField branch and the merge_values function.

Important Files Changed

Filename	Overview
src/eval.rs	Core change: replaces eager check_deprecated calls with lazy collect_deprecated/warn_if_deprecated_access, adds dedup via warned_deprecated HashSet. One confirmed logic bug: null-safe field access suppresses the warning when a deprecated property's value is null. A secondary gap exists in merge_values where overlay deprecated metadata is dropped.
src/value.rs	Adds deprecated: IndexMap<String, Option> field to ObjectSource. Clean structural change; Value::merge predates this PR and its source-dropping behavior is a separate pre-existing gap.

_{Reviews (3): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile}

Greptile review on PR #58: - P0: Remove leftover `[pklr-debug]` eprintln in `collect_deprecated` that fired for every annotation on every property in production. - P1: Propagate the deprecated map through merge/amend code paths (eval_amended_object's type-name attachment, the extends-with-type fallback, the eager-merge fallback, and eval_mapping_entries_with_type_default's type-name attachment) so warnings still fire on field access against amended/merged objects. - P2: Deduplicate per (property name, message) on the Evaluator so a deprecated property referenced inside a loop or template doesn't flood stderr — matches pkl-jvm's per-class-per-property dedup.

When the synthesized-fallback `ObjectSource` was built for the type-name attach paths and the extends-with-type fallback, only the base's deprecation map was propagated. If the overlay entries introduced new `@Deprecated` properties, those were silently dropped and field access against the merged result would skip the warning. Add `merge_deprecated(base, overlay_entries)` and use it at the three synthesized-fallback sites so coverage matches the inline-object path (`eval_entries`), which already collects from its entries.

greptile-apps · 2026-04-26T20:58:49Z

+                    Value::Object(map, source) => {
+                        let val = map.get(field).cloned().unwrap_or(Value::Null);
+                        if !matches!(val, Value::Null) {
+                            self.warn_if_deprecated_access(source, field);
+                        }
+                        Ok(val)
+                    }
                    _ => Err(Error::Eval(format!(


Null-value deprecated property silently skips warning

map.get(field).cloned().unwrap_or(Value::Null) returns Value::Null both when the field is absent from the map and when it is present with a null value. The !matches!(val, Value::Null) guard was meant to suppress warnings for absent fields, but it also suppresses the warning for a deprecated property that legitimately holds null. Any PKL property declared both @Deprecated and nullable (or simply set to null) will be silently accessed via ?. without the expected stderr warning.

Fix by checking presence separately from value:

Suggested change

Value::Object(map, source) => {

let val = map.get(field).cloned().unwrap_or(Value::Null);

if !matches!(val, Value::Null) {

self.warn_if_deprecated_access(source, field);

}

Ok(val)

}

_ => Err(Error::Eval(format!(

Value::Object(map, source) => {

if map.contains_key(field) {

self.warn_if_deprecated_access(source, field);

}

let val = map.get(field).cloned().unwrap_or(Value::Null);

Ok(val)

}

## Summary The `deprecated` map added in [#58](#58) is only read by the evaluator's private field-access helper. Marking the field `pub(crate)` keeps that working and removes it from pklr's public API. cargo-semver-checks will still flag this as a break in the strict sense (`constructible_struct_adds_private_field` — `ObjectSource` is no longer externally constructible via a struct literal). In practice no one downstream constructs `ObjectSource` literally — it's an internal-ish struct describing evaluator state — so the plan is to override the release-plz auto-bump and ship 0.4.2 manually instead of 0.5.0 from [#59](#59). ## Test plan - `cargo test`: 238 passed. - `cargo clippy --all-targets -- -D warnings`: clean. - The lazy-on-access warning behavior added in #58 is unchanged (the helper is in the same crate). 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Low Risk** > Low risk, single-field visibility change with no runtime behavior impact; only potential risk is breaking external code that constructed or accessed `ObjectSource` directly. > > **Overview** > Makes `ObjectSource.deprecated` `pub(crate)` so deprecation metadata remains usable by the evaluator but is no longer part of the public API (and can’t be read/constructed via struct literals outside the crate). > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit e960737. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

@deprecated

## 🤖 Release * `pklr`: 0.4.1 → 0.4.2 The `deprecated` field added in [#58](#58) is scoped to `pub(crate)` ([#61](#61)) so `ObjectSource` no longer exposes new public surface. cargo-semver-checks still flags the struct as no-longer-externally-constructible-via-literal, but in practice nothing downstream constructs `ObjectSource` literally — it's an internal-ish struct describing evaluator state — so this ships as a patch release. The release-plz auto-detected major bump is overridden here manually. ### Changelog #### Fixed - *(eval)* make @deprecated lazy — only warn on access ([#58](#58)) - *(value)* scope `ObjectSource.deprecated` field to `pub(crate)` ([#61](#61)) --- This PR was originally opened by [release-plz](https://github.com/release-plz/release-plz/) as 0.5.0; manually retargeted to 0.4.2.  --- > [!NOTE] > **Low Risk** > Low risk: this PR only updates release metadata (version numbers and changelog) with no functional code changes. > > **Overview** > Publishes release `v0.4.2` by bumping the crate version from `0.4.1` to `0.4.2` in `Cargo.toml`/`Cargo.lock`. > > Updates `CHANGELOG.md` with the `0.4.2` entry noting the fix to make `@Deprecated` warnings lazy (warn only on access). > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5ebfa23. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

@deprecated

Closes hk discussion #878. The auto-generated pkl/Builtins.pkl had bindings of the form check_byte_order_marker = Builtins["builtins/check_byte_order_marker.pkl"].check_byte_order_marker whose RHS reads a property annotated @deprecated. With pklr's old eager handling those warnings fired on every load; even after pklr 0.4.2's lazy @deprecated, the field-access on the deprecated stub still warns at module-evaluation time because the Builtins.pkl binding itself performs that access. Two coordinated changes: - bump pklr to 0.4.2 (lazy @deprecated, warning fires only on field access — see jdx/pklr#58, jdx/pklr#61). - have scripts/gen_builtins.py special-case the deprecated aliases: skip them in the auto-generated loop and emit them as @deprecated bindings that point at the canonical byte_order_marker step. The Builtins.pkl bindings no longer access a deprecated property at load, so the warning only fires when a user references Builtins.check_byte_order_marker / .fix_byte_order_marker directly — preserving the migration nudge. End-to-end with HK_PKL_BACKEND=pklr: - import Builtins.pkl, no use of deprecated aliases → silent. - use Builtins.check_byte_order_marker → warning fires.

…#880) ## Summary - Closes [discussion #878](#878). - With `HK_PKL_BACKEND=pklr`, every load of `Builtins.pkl` printed `[pklr] WARNING: property 'check_byte_order_marker' is deprecated` and the same for `fix_byte_order_marker`, even when the user's `hk.pkl` did not reference those aliases. - Combine the upstream pklr fix with a generator change so the warning only fires when a user actually references the deprecated aliases. ## Two coordinated changes 1. **Bump `pklr` 0.4.1 → 0.4.2** ([jdx/pklr#58](jdx/pklr#58), [jdx/pklr#61](jdx/pklr#61)). pklr now treats `@Deprecated` lazily — the warning fires on field access, not on module evaluation. 2. **`scripts/gen_builtins.py`**: special-case the deprecated aliases. Skip them in the auto-loop and emit them as `@Deprecated` bindings that point at the canonical `byte_order_marker` step. With this, `Builtins.pkl`'s own bindings no longer access a deprecated property at load, so even the lazy field-access warning stays silent until the user explicitly references `Builtins.check_byte_order_marker` / `.fix_byte_order_marker`. The deprecated stub files (`pkl/builtins/check_byte_order_marker.pkl`, `pkl/builtins/fix_byte_order_marker.pkl`) keep their `@Deprecated` annotations untouched — the migration nudge is preserved for direct importers. ## Test plan - [x] `mise run test:cargo` — 145 passed. - [x] End-to-end with `HK_PKL_BACKEND=pklr`: - import `Builtins.pkl` without using deprecated aliases → **silent** (was 2× warnings per load). - access `Builtins.check_byte_order_marker` → warning fires once per evaluation. - [x] `hk` pre-commit hooks pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Low Risk** > Low risk: changes are limited to a dependency patch bump and generation/mapping of builtin aliases to reduce deprecation warnings, with no impact on core hook execution logic beyond alias resolution. > > **Overview** > Stops `Builtins.pkl` from triggering deprecation warnings on load when using the `pklr` backend. > > This bumps `pklr` from `0.4.1` to `0.4.2` and updates `scripts/gen_builtins.py` to *skip* generating deprecated builtin bindings in the main loop, then re-add them as explicit `@Deprecated` aliases pointing to the canonical `byte_order_marker` builtin. > > The pre-commit migration map is updated so `check-byte-order-marker` and `fix-byte-order-marker` now resolve directly to `Builtins.byte_order_marker` instead of the deprecated alias names. > > Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 549c070. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).

[autofix.ci] apply automated fixes

84da4ed

greptile-apps Bot reviewed Apr 26, 2026

View reviewed changes

Comment thread src/eval.rs Outdated

Comment thread src/eval.rs

Comment thread src/eval.rs Outdated

jdx and others added 3 commits April 26, 2026 15:50

[autofix.ci] apply automated fixes

bf34324

jdx merged commit 91ebdfa into main Apr 26, 2026
6 checks passed

jdx deleted the fix/deprecated-lazy-evaluation branch April 26, 2026 20:58

greptile-apps Bot reviewed Apr 26, 2026

View reviewed changes

This was referenced Apr 26, 2026

chore: release v0.4.2 #59

Merged

fix(eval): drop @Deprecated warnings to avoid SemVer break #60

Closed

chore(value): scope ObjectSource.deprecated field to pub(crate) #61

Merged

jdx mentioned this pull request Apr 26, 2026

chore: release v0.4.3 #62

Open

jdx mentioned this pull request Apr 26, 2026

fix(builtins): silence pklr deprecation warnings on Builtins.pkl load jdx/hk#880

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eval): make @Deprecated lazy — only warn on access#58

fix(eval): make @Deprecated lazy — only warn on access#58
jdx merged 5 commits intomainfrom
fix/deprecated-lazy-evaluation

jdx commented Apr 26, 2026 •

edited by cursor Bot

Loading

Uh oh!

gemini-code-assist Bot commented Apr 26, 2026

Uh oh!

greptile-apps Bot commented Apr 26, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot Apr 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

jdx commented Apr 26, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change

Verified end-to-end against hk

Tests

Uh oh!

gemini-code-assist Bot commented Apr 26, 2026

Uh oh!

greptile-apps Bot commented Apr 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greptile-apps Bot Apr 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

jdx commented Apr 26, 2026 •

edited by cursor Bot

Loading

greptile-apps Bot commented Apr 26, 2026 •

edited

Loading