diff --git a/lib/crates/fabro-cli/tests/it/cmd/validate.rs b/lib/crates/fabro-cli/tests/it/cmd/validate.rs index 8a7e019a6..190c2817b 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/validate.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/validate.rs @@ -190,6 +190,27 @@ fn bare_fabro_with_unbound_inputs_in_imported_prompt_validates_structurally_with "); } +/// Regression: https://github.com/fabro-sh/fabro/issues/330 +/// +/// Undefined template variables in partials included by an imported prompt must +/// surface as structural validation warnings, matching direct `@file` prompts. +#[test] +fn bare_fabro_with_unbound_inputs_in_template_partial_validates_structurally_with_warning() { + let context = test_context!(); + let mut cmd = context.validate(); + cmd.arg(fixture("templated_unbound_partial/workflow.fabro")); + fabro_snapshot!(context.filters(), cmd, @" + success: true + exit_code: 0 + ----- stdout ----- + ----- stderr ----- + Workflow: TemplatedUnboundPartial (3 nodes, 2 edges) + Graph: [FIXTURES]/templated_unbound_partial/workflow.fabro + warning: [FIXTURES]/templated_unbound_partial/test-include.partial.md:1:4: undefined template variable `inputs.hello` in node `test_imported_include` attribute `prompt` [node: test_imported_include] (template_undefined_variable) + Validation: OK + "); +} + #[test] fn bare_fabro_picks_up_sibling_workflow_toml_inputs() { let context = test_context!(); diff --git a/lib/crates/fabro-template/src/lib.rs b/lib/crates/fabro-template/src/lib.rs index b85447a06..a3cc2175f 100644 --- a/lib/crates/fabro-template/src/lib.rs +++ b/lib/crates/fabro-template/src/lib.rs @@ -18,7 +18,8 @@ pub use dependency::{ }; pub use store::{ BundleTemplateStore, CachedTemplateStore, FilesystemTemplateStore, RecordingTemplateStore, - TemplateIncludeResolver, TemplateLoadError, TemplateSource, TemplateStore, + TemplateIncludeResolver, TemplateLoadError, TemplateSource, TemplateSourceOrigin, + TemplateStore, }; pub type TemplateLoader = Arc Option + Send + Sync>; @@ -38,6 +39,15 @@ impl TemplateRenderMode { } } +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub struct TemplateErrorLocation { + pub source_name: Option, + pub line: Option, + pub column: Option, + pub span_start: Option, + pub span_len: Option, +} + #[derive(Debug, Default, Clone)] pub struct TemplateContext { goal: Option, @@ -159,26 +169,26 @@ pub enum TemplateError { Syntax { line: Option, source_name: Option, - source_text: Option, + source_text: Option>, span: Option, - source_code: Option>>, + source_code: Option>>>, source: Box, }, UndefinedVariable { expression: Option, line: Option, source_name: Option, - source_text: Option, + source_text: Option>, span: Option, - source_code: Option>>, + source_code: Option>>>, source: Box, }, Render { line: Option, source_name: Option, - source_text: Option, + source_text: Option>, span: Option, - source_code: Option>>, + source_code: Option>>>, source: Box, }, } @@ -239,54 +249,158 @@ fn extract_expression(error: &minijinja::Error) -> Option { Some(source.get(range)?.trim().to_owned()) } -impl From for TemplateError { - fn from(error: minijinja::Error) -> Self { - let line = error.line().and_then(|n| u32::try_from(n).ok()); +struct MiniJinjaErrorDetails { + line: Option, + source_name: Option, + source_text: Option>, + span: Option, + source_code: Option>>>, +} + +impl MiniJinjaErrorDetails { + fn from_error(error: &minijinja::Error, origin: Option<&TemplateSourceOrigin>) -> Self { let source_name = error.name().map(str::to_owned); - let source_text = error.template_source().map(str::to_owned); - let span = error.range().and_then(|range| { + let mut source_text = error.template_source().map(Arc::::from); + let mut line = error.line().and_then(|n| u32::try_from(n).ok()); + let mut span_start = None; + let mut span_len = None; + + if let Some(range) = error.range() { let start = range.start; - let len = range.end.checked_sub(range.start)?; - Some((start, len).into()) - }); + let len = range.end.checked_sub(range.start); + if let Some(len) = len { + span_start = Some(start); + span_len = Some(len); + } + } + + if let (Some(origin), Some(fragment_start), Some(len)) = (origin, span_start, span_len) { + if let Some(start) = origin.fragment_start().checked_add(fragment_start) { + if let Some((origin_line, _)) = source_position(origin.source_text(), start) { + source_text = Some(origin.clone_source_text()); + line = Some(origin_line); + span_start = Some(start); + span_len = Some(len); + } + } + } + + let span = span_start + .zip(span_len) + .map(|(start, len)| (start, len).into()); let source_code = source_name .as_ref() .zip(source_text.as_ref()) - .map(|(name, source)| Box::new(NamedSource::new(name.clone(), source.clone()))); - match error.kind() { - ErrorKind::SyntaxError => Self::Syntax { - line, - source_name, - source_text, - span, - source_code, - source: Box::new(error), - }, - ErrorKind::UndefinedError => { - let expression = extract_expression(&error); - Self::UndefinedVariable { - expression, - line, - source_name, - source_text, - span, - source_code, - source: Box::new(error), - } - } - _ => Self::Render { - line, - source_name, - source_text, - span, - source_code, - source: Box::new(error), - }, + .map(|(name, source)| Box::new(NamedSource::new(name.clone(), Arc::clone(source)))); + Self { + line, + source_name, + source_text, + span, + source_code, } } } +fn source_position(source_text: &str, offset: usize) -> Option<(u32, u32)> { + if offset > source_text.len() || !source_text.is_char_boundary(offset) { + return None; + } + let line = source_text[..offset] + .bytes() + .filter(|byte| *byte == b'\n') + .count() + + 1; + let line_start = source_text[..offset] + .rfind('\n') + .map_or(0, |index| index + 1); + let column = source_text[line_start..offset].chars().count() + 1; + Some((u32::try_from(line).ok()?, u32::try_from(column).ok()?)) +} + +fn primary_template_error(error: &minijinja::Error) -> &minijinja::Error { + let mut selected = matches!( + error.kind(), + ErrorKind::SyntaxError | ErrorKind::UndefinedError + ) + .then_some(error); + + let mut current = error as &(dyn std::error::Error + 'static); + while let Some(source) = current.source() { + if let Some(template_error) = source.downcast_ref::() { + if matches!( + template_error.kind(), + ErrorKind::SyntaxError | ErrorKind::UndefinedError + ) { + selected = Some(template_error); + } + } + current = source; + } + + selected.unwrap_or(error) +} + +/// Converts a MiniJinja error into Fabro's template boundary error. +/// +/// MiniJinja wraps semantic failures with operation-specific errors for +/// include/import/extends rendering. Fabro classifies by the deepest semantic +/// MiniJinja cause while storing the original outer error as the source, so +/// renderers that walk the chain still show wrapper context. +impl From for TemplateError { + fn from(error: minijinja::Error) -> Self { + Self::from_minijinja(error, None) + } +} + impl TemplateError { + fn from_minijinja( + error: minijinja::Error, + origin: Option<(&str, &TemplateSourceOrigin)>, + ) -> Self { + let primary = primary_template_error(&error); + let primary_origin = origin.and_then(|(source_name, origin)| { + (primary.name() == Some(source_name)).then_some(origin) + }); + let details = MiniJinjaErrorDetails::from_error(primary, primary_origin); + match primary.kind() { + ErrorKind::SyntaxError => Self::Syntax { + line: details.line, + source_name: details.source_name, + source_text: details.source_text, + span: details.span, + source_code: details.source_code, + source: Box::new(error), + }, + ErrorKind::UndefinedError => { + let expression = extract_expression(primary); + Self::UndefinedVariable { + expression, + line: details.line, + source_name: details.source_name, + source_text: details.source_text, + span: details.span, + source_code: details.source_code, + source: Box::new(error), + } + } + _ => { + let render_origin = origin.and_then(|(source_name, origin)| { + (error.name() == Some(source_name)).then_some(origin) + }); + let details = MiniJinjaErrorDetails::from_error(&error, render_origin); + Self::Render { + line: details.line, + source_name: details.source_name, + source_text: details.source_text, + span: details.span, + source_code: details.source_code, + source: Box::new(error), + } + } + } + } + #[must_use] pub fn expression(&self) -> Option<&str> { match self { @@ -298,6 +412,18 @@ impl TemplateError { } } + #[must_use] + pub fn location(&self) -> TemplateErrorLocation { + let span = self.span(); + TemplateErrorLocation { + source_name: self.source_name().map(ToOwned::to_owned), + line: self.line(), + column: self.column(), + span_start: span.map(|span| span.offset()), + span_len: span.map(|span| span.len()), + } + } + #[must_use] pub fn line(&self) -> Option { match self { @@ -344,16 +470,10 @@ impl TemplateError { pub fn column(&self) -> Option { let source_text = self.source_text()?; let offset = self.span()?.offset(); - if offset > source_text.len() || !source_text.is_char_boundary(offset) { - return None; - } - let line_start = source_text[..offset] - .rfind('\n') - .map_or(0, |index| index + 1); - u32::try_from(source_text[line_start..offset].chars().count() + 1).ok() + source_position(source_text, offset).map(|(_, column)| column) } - fn source_code_ref(&self) -> Option<&NamedSource> { + fn source_code_ref(&self) -> Option<&NamedSource>> { match self { Self::LoaderDependentString { .. } | Self::Load { .. } => None, Self::Syntax { source_code, .. } @@ -411,7 +531,7 @@ fn is_plain_text(template: &str) -> bool { } pub fn render(template: &str, ctx: &TemplateContext) -> Result { - render_with(None, template, ctx, UndefinedBehavior::Strict, None) + render_with(None, template, ctx, UndefinedBehavior::Strict, None, None) } pub fn render_named( @@ -419,12 +539,39 @@ pub fn render_named( template: &str, ctx: &TemplateContext, ) -> Result { - render_with( - Some(name.into()), + render_named_with_origin(name, template, ctx, TemplateRenderMode::Strict, None) +} + +pub fn render_named_fragment( + name: impl Into, + template: &str, + origin: &TemplateSourceOrigin, + ctx: &TemplateContext, +) -> Result { + render_named_with_origin( + name, template, ctx, - UndefinedBehavior::Strict, + TemplateRenderMode::Strict, + Some(origin), + ) +} + +pub fn render_named_with_origin( + name: impl Into, + template: &str, + ctx: &TemplateContext, + mode: TemplateRenderMode, + origin: Option<&TemplateSourceOrigin>, +) -> Result { + let name = name.into(); + render_with( + Some(&name), + template, + ctx, + mode.undefined_behavior(), None, + origin, ) } @@ -434,12 +581,14 @@ pub fn render_named_with_loader( ctx: &TemplateContext, loader: &TemplateLoader, ) -> Result { + let name = name.into(); render_with( - Some(name.into()), + Some(&name), template, ctx, UndefinedBehavior::Strict, Some(loader), + None, ) } @@ -448,7 +597,14 @@ pub fn render_named_with_loader( /// passes (e.g. manifest scanning, `fabro validate` on a bare `.fabro`) where /// the user has not yet bound inputs — strict checking happens elsewhere. pub fn render_lenient(template: &str, ctx: &TemplateContext) -> Result { - render_with(None, template, ctx, UndefinedBehavior::Chainable, None) + render_with( + None, + template, + ctx, + UndefinedBehavior::Chainable, + None, + None, + ) } pub fn render_lenient_named( @@ -456,12 +612,21 @@ pub fn render_lenient_named( template: &str, ctx: &TemplateContext, ) -> Result { - render_with( - Some(name.into()), + render_named_with_origin(name, template, ctx, TemplateRenderMode::Lenient, None) +} + +pub fn render_lenient_named_fragment( + name: impl Into, + template: &str, + origin: &TemplateSourceOrigin, + ctx: &TemplateContext, +) -> Result { + render_named_with_origin( + name, template, ctx, - UndefinedBehavior::Chainable, - None, + TemplateRenderMode::Lenient, + Some(origin), ) } @@ -471,27 +636,30 @@ pub fn render_lenient_named_with_loader( ctx: &TemplateContext, loader: &TemplateLoader, ) -> Result { + let name = name.into(); render_with( - Some(name.into()), + Some(&name), template, ctx, UndefinedBehavior::Chainable, Some(loader), + None, ) } fn render_with( - name: Option, + name: Option<&str>, template: &str, ctx: &TemplateContext, undefined: UndefinedBehavior, loader: Option<&TemplateLoader>, + origin: Option<&TemplateSourceOrigin>, ) -> Result { if is_plain_text(template) { return Ok(template.to_owned()); } if loader.is_none() { - reject_loader_dependent_string(name.as_deref(), template)?; + reject_loader_dependent_string(name, template)?; } let mut env = Environment::new(); env.set_undefined_behavior(undefined); @@ -501,11 +669,12 @@ fn render_with( let loader = Arc::clone(loader); env.set_loader(move |name| Ok(loader(name))); } + let origin = name.zip(origin); match name { - Some(name) => env.render_named_str(&name, template, ctx.clone().into_value()), + Some(name) => env.render_named_str(name, template, ctx.clone().into_value()), None => env.render_str(template, ctx.clone().into_value()), } - .map_err(TemplateError::from) + .map_err(|error| TemplateError::from_minijinja(error, origin)) } pub fn render_source( @@ -555,25 +724,26 @@ fn render_rooted_source( } }); - env.render_named_str( - &source.path.to_string(), - &source.content, - ctx.clone().into_value(), - ) - .map_err(|error| { - if let Some(error) = load_error - .lock() - .expect("template load error mutex should not be poisoned") - .take() - { - TemplateError::Load { - source_name: Some(source.path.to_string()), - source: Box::new(error), + let source_name = source.path.to_string(); + let origin = source + .origin + .as_ref() + .map(|origin| (source_name.as_str(), origin)); + env.render_named_str(&source_name, &source.content, ctx.clone().into_value()) + .map_err(|error| { + if let Some(error) = load_error + .lock() + .expect("template load error mutex should not be poisoned") + .take() + { + TemplateError::Load { + source_name: Some(source.path.to_string()), + source: Box::new(error), + } + } else { + TemplateError::from_minijinja(error, origin) } - } else { - TemplateError::from(error) - } - }) + }) } fn joined_template_path(root: &ManifestPath, name: &str, parent: &str) -> String { @@ -601,6 +771,7 @@ mod tests { use std::collections::HashMap; use fabro_util::env::TestEnv; + use fabro_util::error; use toml::map::Map; use super::*; @@ -764,6 +935,162 @@ mod tests { assert_eq!(rendered, "included content"); } + fn assert_semantic_undefined_error( + err: &TemplateError, + expected_source: &str, + expected_source_text: &str, + ) { + let TemplateError::UndefinedVariable { expression, .. } = err else { + panic!("expected undefined variable error, got {err:?}"); + }; + assert_eq!(expression.as_deref(), Some("inputs.hello")); + + let location = err.location(); + assert_eq!(location.source_name.as_deref(), Some(expected_source)); + assert_eq!(location.line, Some(1)); + assert_eq!( + location.span_start, + expected_source_text.find("inputs.hello") + ); + assert_eq!(location.span_len, Some("inputs.hello".len())); + + let chain = error::collect_chain(err); + assert!( + chain.iter().any(|cause| cause.contains("undefined value")), + "missing undefined cause in source chain: {chain:?}" + ); + assert!( + chain + .iter() + .skip(1) + .any(|cause| cause.contains(expected_source)), + "missing source context in source chain: {chain:?}" + ); + } + + #[test] + fn render_source_reports_undefined_variable_from_include() { + let ctx = TemplateContext::new(); + let source = TemplateSource::new( + manifest_path("prompts/main.md"), + manifest_path("prompts"), + r#"{% include "partial.md" %}"#, + ); + + let err = render_source( + &source, + &ctx, + bundle_store(&[("prompts/partial.md", "{{ inputs.hello }}")]), + TemplateRenderMode::Strict, + ) + .unwrap_err(); + + assert_semantic_undefined_error(&err, "prompts/partial.md", "{{ inputs.hello }}"); + } + + #[test] + fn render_source_reports_undefined_variable_from_imported_macro() { + let ctx = TemplateContext::new(); + let source = TemplateSource::new( + manifest_path("prompts/main.md"), + manifest_path("prompts"), + r#"{% import "macros.md" as macros %}{{ macros.greet() }}"#, + ); + + let err = render_source( + &source, + &ctx, + bundle_store(&[( + "prompts/macros.md", + r"{% macro greet() %}{{ inputs.hello }}{% endmacro %}", + )]), + TemplateRenderMode::Strict, + ) + .unwrap_err(); + + assert_semantic_undefined_error( + &err, + "prompts/macros.md", + r"{% macro greet() %}{{ inputs.hello }}{% endmacro %}", + ); + } + + #[test] + fn render_source_reports_undefined_variable_from_from_imported_macro() { + let ctx = TemplateContext::new(); + let source = TemplateSource::new( + manifest_path("prompts/main.md"), + manifest_path("prompts"), + r#"{% from "macros.md" import greet %}{{ greet() }}"#, + ); + + let err = render_source( + &source, + &ctx, + bundle_store(&[( + "prompts/macros.md", + r"{% macro greet() %}{{ inputs.hello }}{% endmacro %}", + )]), + TemplateRenderMode::Strict, + ) + .unwrap_err(); + + assert_semantic_undefined_error( + &err, + "prompts/macros.md", + r"{% macro greet() %}{{ inputs.hello }}{% endmacro %}", + ); + } + + #[test] + fn render_source_reports_undefined_variable_from_extended_layout() { + let ctx = TemplateContext::new(); + let source = TemplateSource::new( + manifest_path("pages/main.md"), + manifest_path("pages"), + r#"{% extends "layout.md" %}{% block body %}Body{% endblock %}"#, + ); + + let err = render_source( + &source, + &ctx, + bundle_store(&[( + "pages/layout.md", + "{{ inputs.hello }}:{% block body %}{% endblock %}", + )]), + TemplateRenderMode::Strict, + ) + .unwrap_err(); + + assert_semantic_undefined_error( + &err, + "pages/layout.md", + "{{ inputs.hello }}:{% block body %}{% endblock %}", + ); + } + + #[test] + fn render_named_fragment_reports_location_in_full_source() { + let ctx = TemplateContext::new(); + let source_text = "digraph {\n plan [prompt=\"Hello {{ inputs.name }}\"]\n}\n"; + let fragment = "Hello {{ inputs.name }}"; + let origin = TemplateSourceOrigin::from_first_fragment_match(source_text, fragment) + .expect("fragment should be present in source"); + + let err = render_named_fragment("workflow.fabro", fragment, &origin, &ctx).unwrap_err(); + + let location = err.location(); + assert_eq!(location.source_name.as_deref(), Some("workflow.fabro")); + assert_eq!(location.line, Some(2)); + assert_eq!(location.column, Some(26)); + assert_eq!(location.span_start, source_text.find("inputs.name")); + assert_eq!(location.span_len, Some("inputs.name".len())); + assert_eq!( + err.span().map(|span| span.offset()), + source_text.find("inputs.name") + ); + } + #[test] fn render_source_supports_nested_include() { let ctx = TemplateContext::new(); diff --git a/lib/crates/fabro-template/src/store.rs b/lib/crates/fabro-template/src/store.rs index 397c8094d..e595a8b64 100644 --- a/lib/crates/fabro-template/src/store.rs +++ b/lib/crates/fabro-template/src/store.rs @@ -1,15 +1,54 @@ use std::collections::{HashMap, HashSet}; use std::path::{Component, Path, PathBuf}; -use std::sync::{Mutex, MutexGuard}; +use std::sync::{Arc, Mutex, MutexGuard}; use fabro_types::ManifestPath; use thiserror::Error; +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct TemplateSourceOrigin { + source_text: Arc, + fragment_start: usize, +} + +impl TemplateSourceOrigin { + #[must_use] + fn new(source_text: impl Into>, fragment_start: usize) -> Self { + Self { + source_text: source_text.into(), + fragment_start, + } + } + + #[must_use] + pub fn from_first_fragment_match(source_text: &str, fragment: &str) -> Option { + source_text + .find(fragment) + .map(|fragment_start| Self::new(source_text, fragment_start)) + } + + #[must_use] + pub(crate) fn source_text(&self) -> &str { + &self.source_text + } + + #[must_use] + pub(crate) fn clone_source_text(&self) -> Arc { + Arc::clone(&self.source_text) + } + + #[must_use] + pub(crate) fn fragment_start(&self) -> usize { + self.fragment_start + } +} + #[derive(Clone, Debug, PartialEq, Eq)] pub struct TemplateSource { pub path: ManifestPath, pub root: ManifestPath, pub content: String, + pub origin: Option, } impl TemplateSource { @@ -19,8 +58,15 @@ impl TemplateSource { path, root, content: content.into(), + origin: None, } } + + #[must_use] + pub fn with_origin(mut self, origin: TemplateSourceOrigin) -> Self { + self.origin = Some(origin); + self + } } pub trait TemplateStore: Send + Sync { diff --git a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs index e8f03dc88..1e71699b1 100644 --- a/lib/crates/fabro-workflow/src/transforms/file_inlining.rs +++ b/lib/crates/fabro-workflow/src/transforms/file_inlining.rs @@ -200,7 +200,7 @@ impl FileInliningTransform { node_id.clone(), "prompt", ) - .with_source_text(self.source_text.as_deref(), prompt) + .with_source_origin(self.source_text.as_deref(), prompt) .with_template_store(template_render_store( &self.current_dir, Arc::clone(&self.resolver), @@ -234,7 +234,7 @@ impl FileInliningTransform { }; let ctx = TemplateContext::for_input_scan(self.inputs.clone()); let target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") - .with_source_text(self.source_text.as_deref(), goal) + .with_source_origin(self.source_text.as_deref(), goal) .with_template_store(template_render_store( &self.current_dir, Arc::clone(&self.resolver), @@ -270,7 +270,7 @@ impl FileInliningTransform { let (source, store) = self.template_source_for_resolved_file(&resolved)?; let target = owner_target .with_source_name(resolved.path.display().to_string()) - .with_source_text(Some(&resolved.content), &resolved.content) + .with_source_origin(Some(&resolved.content), &resolved.content) .with_template_store(TemplateRenderStore::new(source, store)); Ok(Some(render_file_contents( &resolved, diff --git a/lib/crates/fabro-workflow/src/transforms/import.rs b/lib/crates/fabro-workflow/src/transforms/import.rs index 1734b7836..5127bedf8 100644 --- a/lib/crates/fabro-workflow/src/transforms/import.rs +++ b/lib/crates/fabro-workflow/src/transforms/import.rs @@ -675,7 +675,7 @@ impl ImportTransform { let path_ctx = TemplateContext::for_input_scan(self.inputs.clone()); let mut ignored_goal_diagnostics = Vec::new(); let goal_target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") - .with_source_text(self.source_text.as_deref(), graph.goal()) + .with_source_origin(self.source_text.as_deref(), graph.goal()) .with_template_store(template_render_store( &self.current_dir, Arc::clone(&self.resolver), @@ -700,7 +700,7 @@ impl ImportTransform { placeholder_id.clone(), "import", ) - .with_source_text(self.source_text.as_deref(), &import_path); + .with_source_origin(self.source_text.as_deref(), &import_path); let rendered_import_path = render_template_for_target( &import_path, &path_ctx, diff --git a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs index a29d159a3..1891e5a47 100644 --- a/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs +++ b/lib/crates/fabro-workflow/src/transforms/variable_expansion.rs @@ -4,8 +4,8 @@ use std::sync::Arc; use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_template::{ - TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateStore, - render_lenient_named, render_named, render_source, + TemplateContext, TemplateError, TemplateRenderMode, TemplateSource, TemplateSourceOrigin, + TemplateStore, render_named_with_origin, render_source, }; use fabro_util::error::collect_chain; use fabro_validate::{Diagnostic, Severity}; @@ -35,13 +35,12 @@ pub enum RenderMode { #[derive(Clone)] pub(crate) struct TemplateRenderTarget { - pub source_name: Option, - pub source_text: Option, - pub source_offset: Option, - pub node_id: Option, - pub edge: Option<(String, String)>, - pub owner: String, - template_store: Option, + pub source_name: Option, + pub node_id: Option, + pub edge: Option<(String, String)>, + pub owner: String, + source_origin: Option, + template_store: Option, } #[derive(Clone)] @@ -61,8 +60,12 @@ impl TemplateRenderStore { text: &str, ctx: &TemplateContext, mode: TemplateRenderMode, + origin: Option<&TemplateSourceOrigin>, ) -> Result { - let mut source = self.source.clone(); + let mut source = match origin { + Some(origin) => self.source.clone().with_origin(origin.clone()), + None => self.source.clone(), + }; text.clone_into(&mut source.content); render_source(&source, ctx, Arc::clone(&self.store), mode) } @@ -74,11 +77,10 @@ impl TemplateRenderTarget { let attr_name = attr_name.into(); Self { source_name, - source_text: None, - source_offset: None, node_id: None, edge: None, owner: format!("graph attribute `{attr_name}`"), + source_origin: None, template_store: None, } } @@ -93,11 +95,10 @@ impl TemplateRenderTarget { let attr_name = attr_name.into(); Self { source_name, - source_text: None, - source_offset: None, node_id: Some(node_id.clone()), edge: None, owner: format!("node `{node_id}` attribute `{attr_name}`"), + source_origin: None, template_store: None, } } @@ -114,11 +115,10 @@ impl TemplateRenderTarget { let attr_name = attr_name.into(); Self { source_name, - source_text: None, - source_offset: None, node_id: None, edge: Some((from.clone(), to.clone())), owner: format!("edge `{from} -> {to}` attribute `{attr_name}`"), + source_origin: None, template_store: None, } } @@ -130,9 +130,10 @@ impl TemplateRenderTarget { } #[must_use] - pub(crate) fn with_source_text(mut self, source_text: Option<&str>, value: &str) -> Self { - self.source_text = source_text.map(ToOwned::to_owned); - self.source_offset = source_text.and_then(|source_text| source_text.find(value)); + pub(crate) fn with_source_origin(mut self, source_text: Option<&str>, value: &str) -> Self { + self.source_origin = source_text.and_then(|source_text| { + TemplateSourceOrigin::from_first_fragment_match(source_text, value) + }); self } @@ -159,11 +160,16 @@ pub(crate) fn render_template_for_target( ) -> Result { let source_name = target.template_source_name(); let render_with_mode = |mode| match target.template_store.as_ref() { - Some(template_store) => template_store.render(text, ctx, mode), - None if matches!(mode, TemplateRenderMode::Strict) => { - render_named(source_name.clone(), text, ctx) + Some(template_store) => { + template_store.render(text, ctx, mode, target.source_origin.as_ref()) } - None => render_lenient_named(source_name.clone(), text, ctx), + None => render_named_with_origin( + source_name.clone(), + text, + ctx, + mode, + target.source_origin.as_ref(), + ), }; match render_mode { RenderMode::Strict => render_with_mode(TemplateRenderMode::Strict) @@ -197,16 +203,7 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> }; let _ = write!(message, " in {}", target.owner); - let source_location = target - .source_text - .as_deref() - .zip(target.source_offset) - .zip(error.span()) - .and_then(|((source_text, source_offset), span)| { - let absolute_offset = source_offset.checked_add(span.offset())?; - let (line, column) = source_position(source_text, absolute_offset)?; - Some((line, column, absolute_offset, span.len())) - }); + let location = error.location(); Diagnostic { rule: TEMPLATE_UNDEFINED_VARIABLE_RULE.to_owned(), @@ -217,42 +214,15 @@ fn template_diagnostic(error: &TemplateError, target: &TemplateRenderTarget) -> fix: Some(format!( "bind `{name}` via `[run.inputs]` in workflow.toml, or pass `--input {name}=`" )), - source_path: error - .source_name() - .map(ToOwned::to_owned) - .or_else(|| target.source_name.clone()), - line: source_location - .map(|(line, _, _, _)| line) - .or_else(|| error.line()), - column: source_location - .map(|(_, column, _, _)| column) - .or_else(|| error.column()), - span_start: source_location - .map(|(_, _, span_start, _)| span_start) - .or_else(|| error.span().map(|span| span.offset())), - span_len: source_location - .map(|(_, _, _, span_len)| span_len) - .or_else(|| error.span().map(|span| span.len())), + source_path: location.source_name.or_else(|| target.source_name.clone()), + line: location.line, + column: location.column, + span_start: location.span_start, + span_len: location.span_len, related: Vec::new(), } } -fn source_position(source_text: &str, offset: usize) -> Option<(u32, u32)> { - if offset > source_text.len() || !source_text.is_char_boundary(offset) { - return None; - } - let line = source_text[..offset] - .bytes() - .filter(|byte| *byte == b'\n') - .count() - + 1; - let line_start = source_text[..offset] - .rfind('\n') - .map_or(0, |index| index + 1); - let column = source_text[line_start..offset].chars().count() + 1; - Some((u32::try_from(line).ok()?, u32::try_from(column).ok()?)) -} - /// Expands `{{ goal }}` / `{{ inputs.* }}` across all string attributes. pub struct TemplateTransform { pub inputs: HashMap, @@ -285,7 +255,7 @@ impl TemplateTransform { } let ctx = TemplateContext::for_input_scan(self.inputs.clone()); let target = TemplateRenderTarget::graph_attr(self.source_name.clone(), "goal") - .with_source_text(self.source_text.as_deref(), goal); + .with_source_origin(self.source_text.as_deref(), goal); render_template_for_target(goal, &ctx, self.render_mode, &target, diagnostics) } @@ -314,7 +284,7 @@ impl TemplateTransform { } let target = owner_for_attr(attr_name) .with_source_name(source_name.cloned().unwrap_or_else(|| "workflow".into())) - .with_source_text(source_text, text); + .with_source_origin(source_text, text); *text = render_template_for_target(text, ctx, render_mode, &target, diagnostics)?; } } diff --git a/test/templated_unbound_partial/test-include.partial.md b/test/templated_unbound_partial/test-include.partial.md new file mode 100644 index 000000000..f740dbf5f --- /dev/null +++ b/test/templated_unbound_partial/test-include.partial.md @@ -0,0 +1 @@ +{{ inputs.hello }} diff --git a/test/templated_unbound_partial/test-include.prompt.md b/test/templated_unbound_partial/test-include.prompt.md new file mode 100644 index 000000000..54e6e7be4 --- /dev/null +++ b/test/templated_unbound_partial/test-include.prompt.md @@ -0,0 +1 @@ +{% include "test-include.partial.md" %} diff --git a/test/templated_unbound_partial/workflow.fabro b/test/templated_unbound_partial/workflow.fabro new file mode 100644 index 000000000..f4dfcb0d8 --- /dev/null +++ b/test/templated_unbound_partial/workflow.fabro @@ -0,0 +1,7 @@ +digraph TemplatedUnboundPartial { + start [shape=Mdiamond, label="Start"] + test_imported_include [label="Test", prompt="@test-include.prompt.md"] + exit [shape=Msquare, label="Exit"] + + start -> test_imported_include -> exit +}