Skip to content

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

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.

Sign up for GitHub

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

Merged
jdx merged 5 commits into from
Apr 26, 2026
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7f129ec
fix(eval): make @Deprecated lazy — only warn on access
jdx Apr 26, 2026
84da4ed
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 26, 2026
a23d050
fix(eval): address PR review — remove debug print, propagate map, dedup
jdx Apr 26, 2026
2ee1fc3
fix(eval): include overlay @Deprecated in amend-path source map
jdx Apr 26, 2026
bf34324
[autofix.ci] apply automated fixes
autofix-ci[bot] Apr 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 78 additions & 27 deletions src/eval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ use std::path::{Path, PathBuf};

use crate::error::{Error, Result};
use crate::lexer;
use crate::parser::{
self, Annotation, BinOp, Entry, Expr, Modifier, Module, Property, StringInterpPart, UnOp,
};
use crate::parser::{self, BinOp, Entry, Expr, Modifier, Module, Property, StringInterpPart, UnOp};
use crate::value::{ObjectSource, Value};

/// Evaluates pkl source files to [`Value`].
Expand Down Expand Up @@ -674,7 +672,6 @@ impl Evaluator {
if has_modifier(mods, Modifier::Local) {
continue; // already collected
}
check_deprecated(&prop.annotations, &prop.name);
// Extract renderer converters from the `output` block AST,
// then skip it (it's not included in the output).
// Clear any base-inherited converters so child overrides take precedence.
Expand Down Expand Up @@ -748,7 +745,23 @@ impl Evaluator {
}
out.retain(|_, v| !matches!(v, Value::Lambda(..)));
}
Ok(Value::Object(Arc::new(out), None))
// If the module declares any `@Deprecated` properties, attach a
// minimal ObjectSource carrying just the deprecation map so field
// access can warn lazily. Modules without @Deprecated keep `None`
// source to avoid changing amend behavior in the common case.
let deprecated = collect_deprecated(&module.body);
let source = if deprecated.is_empty() {
None
} else {
Some(Arc::new(ObjectSource {
entries: Vec::new(),
scope: IndexMap::new(),
is_open: true,
type_name: None,
deprecated,
}))
};
Ok(Value::Object(Arc::new(out), source))
}

#[async_recursion(?Send)]
Expand Down Expand Up @@ -863,7 +876,6 @@ impl Evaluator {
if has_modifier(mods, Modifier::Local) {
continue;
}
check_deprecated(&prop.annotations, &prop.name);
// Skip the `default` property — it's a template, not an output entry
if prop.name == "default" && default_template.is_some() {
continue;
Expand Down Expand Up @@ -918,6 +930,7 @@ impl Evaluator {
scope: IndexMap::new(),
Comment thread
greptile-apps[bot] marked this conversation as resolved.
is_open: true,
type_name: Some(tn.clone()),
deprecated: IndexMap::new(),
},
};
*result_src = Some(std::sync::Arc::new(new_src));
Expand Down Expand Up @@ -985,6 +998,7 @@ impl Evaluator {
scope: child_scope.flatten(),
is_open: true, // default: allow new properties
type_name: None,
deprecated: collect_deprecated(entries),
};
Ok(Value::Object(Arc::new(map), Some(Arc::new(source))))
}
Expand Down Expand Up @@ -1379,11 +1393,13 @@ impl Evaluator {
let mut source_scope = scope.flatten();
source_scope.shift_remove("outer");
source_scope.shift_remove("this");
let deprecated = collect_deprecated(&src_entries);
let source = ObjectSource {
entries: src_entries,
scope: source_scope,
is_open: true,
type_name: None,
deprecated,
};
Ok(Value::Object(Arc::new(map), Some(Arc::new(source))))
}
Expand Down Expand Up @@ -1469,6 +1485,7 @@ impl Evaluator {
scope: IndexMap::new(),
is_open,
type_name: tn,
deprecated: IndexMap::new(),
}
};
*src_slot = Some(Arc::new(new_src));
Expand All @@ -1488,6 +1505,7 @@ impl Evaluator {
scope: IndexMap::new(),
is_open: true,
type_name: type_name.clone(),
deprecated: IndexMap::new(),
};
Ok(Value::Object(Arc::new(merged), Some(Arc::new(src))))
} else {
Expand Down Expand Up @@ -1540,10 +1558,14 @@ impl Evaluator {
_ => {}
}
match &obj {
Value::Object(map, _) => map
.get(field)
.cloned()
.ok_or_else(|| Error::Eval(format!("field not found: {field}"))),
Value::Object(map, source) => {
let val = map
.get(field)
.cloned()
.ok_or_else(|| Error::Eval(format!("field not found: {field}")))?;
warn_if_deprecated_access(source, field);
Ok(val)
}
_ => Err(Error::Eval(format!(
"cannot access field '{field}' on {}",
value_type_name(&obj)
Expand All @@ -1554,7 +1576,13 @@ impl Evaluator {
let obj = self.eval_expr(obj_expr, scope, depth + 1).await?;
match &obj {
Value::Null => Ok(Value::Null),
Value::Object(map, _) => Ok(map.get(field).cloned().unwrap_or(Value::Null)),
Value::Object(map, source) => {
let val = map.get(field).cloned().unwrap_or(Value::Null);
if !matches!(val, Value::Null) {
warn_if_deprecated_access(source, field);
}
Ok(val)
}
_ => Err(Error::Eval(format!(
Comment on lines +1615 to 1622

@greptile-apps greptile-apps Bot Apr 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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)
}

Fix in Claude Code

"cannot access field '{field}' on {}",
value_type_name(&obj)
Expand Down Expand Up @@ -2204,6 +2232,7 @@ impl Evaluator {
scope: IndexMap::new(),
is_open: true,
type_name: Some(tn.clone()),
deprecated: IndexMap::new(),
},
};
*result_src = Some(std::sync::Arc::new(new_src));
Expand Down Expand Up @@ -2472,26 +2501,48 @@ fn resolve_package_uri(uri: &str) -> Result<PackageSource> {
Err(Error::Eval(format!("unsupported package URI: {uri}")))
}

fn check_deprecated(annotations: &[Annotation], prop_name: &str) {
for ann in annotations {
if ann.name == "Deprecated" {
// Look for a "message" property in the annotation body
let mut message = None;
for entry in &ann.body {
if let Entry::Property(p) = entry
&& p.name == "message"
&& let Some(Expr::String(s)) = &p.value
{
message = Some(s.clone());
/// Collect `@Deprecated` annotations from a list of entries into a map of
/// property name → optional message. Used to populate `ObjectSource.deprecated`
/// so field access can warn lazily, instead of warning eagerly when a module
/// or object body is evaluated.
fn collect_deprecated(entries: &[Entry]) -> IndexMap<String, Option<String>> {
let mut out: IndexMap<String, Option<String>> = IndexMap::new();
for entry in entries {
if let Entry::Property(prop) = entry {
for ann in &prop.annotations {
eprintln!("[pklr-debug] ann {:?}", ann.name);
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated
if ann.name != "Deprecated" {
continue;
}
}
if let Some(msg) = message {
eprintln!("[pklr] WARNING: property '{prop_name}' is deprecated: {msg}");
} else {
eprintln!("[pklr] WARNING: property '{prop_name}' is deprecated");
let mut message = None;
for e in &ann.body {
if let Entry::Property(p) = e
&& p.name == "message"
&& let Some(Expr::String(s)) = &p.value
{
message = Some(s.clone());
}
}
out.insert(prop.name.clone(), message);
}
}
}
out
}

/// If `field` is marked `@Deprecated` in `source`, emit the warning.
/// Called from field-access expressions so the warning fires when a
/// deprecated property is *used*, not when its containing module loads.
fn warn_if_deprecated_access(source: &Option<Arc<ObjectSource>>, field: &str) {
let Some(src) = source else { return };
let Some(message) = src.deprecated.get(field) else {
return;
};
if let Some(msg) = message {
eprintln!("[pklr] WARNING: property '{field}' is deprecated: {msg}");
} else {
eprintln!("[pklr] WARNING: property '{field}' is deprecated");
}
}
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated

/// Resolve a potentially dotted name (e.g. "Foo.Bar") in scope.
Expand Down
5 changes: 5 additions & 0 deletions src/value.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ pub struct ObjectSource {
/// The pkl class name this object was instantiated from (e.g., "Step", "Group").
/// Used by `output.renderer.converters` to apply type-specific transforms.
pub type_name: Option<String>,
/// Map of property name → optional deprecation message for properties
/// annotated with `@Deprecated`. Consulted on field access so the
/// warning fires when a deprecated property is *used*, not when the
/// containing module is loaded.
pub deprecated: IndexMap<String, Option<String>>,
}

/// A pkl runtime value.
Expand Down
Loading