From 781eea632f68c478c0a96495f7f4822ba6f7873d Mon Sep 17 00:00:00 2001 From: +merlan #flirora Date: Sat, 22 Jun 2024 04:59:52 -0400 Subject: [PATCH] Add message when trying to access a field that is not set (#4399) Co-authored-by: Laurenz --- crates/typst-cli/src/query.rs | 2 +- crates/typst-ide/src/analyze.rs | 3 +- crates/typst-ide/src/complete.rs | 2 +- crates/typst-macros/src/elem.rs | 46 +++++++++------ crates/typst/src/eval/code.rs | 2 +- crates/typst/src/foundations/content.rs | 71 +++++++++++++++++------- crates/typst/src/foundations/element.rs | 24 +++++--- crates/typst/src/foundations/selector.rs | 2 +- tests/suite/foundations/content.typ | 14 ++++- tests/suite/styling/show.typ | 2 +- 10 files changed, 114 insertions(+), 54 deletions(-) diff --git a/crates/typst-cli/src/query.rs b/crates/typst-cli/src/query.rs index a80127779..ef3c79514 100644 --- a/crates/typst-cli/src/query.rs +++ b/crates/typst-cli/src/query.rs @@ -88,7 +88,7 @@ fn format(elements: Vec, command: &QueryCommand) -> StrResult { let mapped: Vec<_> = elements .into_iter() .filter_map(|c| match &command.field { - Some(field) => c.get_by_name(field), + Some(field) => c.get_by_name(field).ok(), _ => Some(c.into_value()), }) .collect(); diff --git a/crates/typst-ide/src/analyze.rs b/crates/typst-ide/src/analyze.rs index b43d87cde..ecb73dcec 100644 --- a/crates/typst-ide/src/analyze.rs +++ b/crates/typst-ide/src/analyze.rs @@ -92,7 +92,8 @@ pub fn analyze_labels(document: &Document) -> (Vec<(Label, Option)>, let Some(label) = elem.label() else { continue }; let details = elem .get_by_name("caption") - .or_else(|| elem.get_by_name("body")) + .or_else(|_| elem.get_by_name("body")) + .ok() .and_then(|field| match field { Value::Content(content) => Some(content), _ => None, diff --git a/crates/typst-ide/src/complete.rs b/crates/typst-ide/src/complete.rs index 608b14730..cdcac956d 100644 --- a/crates/typst-ide/src/complete.rs +++ b/crates/typst-ide/src/complete.rs @@ -441,7 +441,7 @@ fn field_access_completions( if let Some((elem, styles)) = func.element().zip(styles.as_ref()) { for param in elem.params().iter().filter(|param| !param.required) { if let Some(value) = elem.field_id(param.name).and_then(|id| { - elem.field_from_styles(id, StyleChain::new(styles)) + elem.field_from_styles(id, StyleChain::new(styles)).ok() }) { ctx.value_completion( Some(param.name.into()), diff --git a/crates/typst-macros/src/elem.rs b/crates/typst-macros/src/elem.rs index 35dc8b75a..a3666e1d8 100644 --- a/crates/typst-macros/src/elem.rs +++ b/crates/typst-macros/src/elem.rs @@ -390,13 +390,13 @@ fn create_fields_enum(element: &Elem) -> TokenStream { } impl ::std::convert::TryFrom for Fields { - type Error = (); + type Error = #foundations::FieldAccessError; fn try_from(value: u8) -> Result { #(const #consts: u8 = Fields::#variants as u8;)* match value { #(#consts => Ok(Self::#variants),)* - _ => Err(()), + _ => Err(#foundations::FieldAccessError::Internal), } } } @@ -877,9 +877,9 @@ fn create_fields_impl(element: &Elem) -> TokenStream { let Field { enum_ident, ident, .. } = field; let expr = if field.required { - quote! { Some(#into_value(self.#ident.clone())) } + quote! { Ok(#into_value(self.#ident.clone())) } } else { - quote! { self.#ident.clone().map(#into_value) } + quote! { self.#ident.clone().map(#into_value).ok_or(#foundations::FieldAccessError::Unset) } }; quote! { Fields::#enum_ident => #expr } @@ -890,9 +890,9 @@ fn create_fields_impl(element: &Elem) -> TokenStream { let Field { enum_ident, ident, .. } = field; let expr = if field.required { - quote! { Some(#into_value(self.#ident.clone())) } + quote! { Ok(#into_value(self.#ident.clone())) } } else if field.synthesized { - quote! { self.#ident.clone().map(#into_value) } + quote! { self.#ident.clone().map(#into_value).ok_or(#foundations::FieldAccessError::Unset) } } else { let value = create_style_chain_access( field, @@ -900,7 +900,7 @@ fn create_fields_impl(element: &Elem) -> TokenStream { if field.ghost { quote!(None) } else { quote!(self.#ident.as_ref()) }, ); - quote! { Some(#into_value(#value)) } + quote! { Ok(#into_value(#value)) } }; quote! { Fields::#enum_ident => #expr } @@ -911,10 +911,10 @@ fn create_fields_impl(element: &Elem) -> TokenStream { let Field { enum_ident, .. } = field; let expr = if field.required || field.synthesized { - quote! { None } + quote! { Err(#foundations::FieldAccessError::Unknown) } } else { let value = create_style_chain_access(field, false, quote!(None)); - quote! { Some(#into_value(#value)) } + quote! { Ok(#into_value(#value)) } }; quote! { Fields::#enum_ident => #expr } @@ -962,6 +962,10 @@ fn create_fields_impl(element: &Elem) -> TokenStream { let Elem { ident, .. } = element; + let result = quote! { + Result<#foundations::Value, #foundations::FieldAccessError> + }; + quote! { impl #foundations::Fields for #ident { type Enum = Fields; @@ -977,27 +981,33 @@ fn create_fields_impl(element: &Elem) -> TokenStream { } } - fn field(&self, id: u8) -> Option<#foundations::Value> { - let id = Fields::try_from(id).ok()?; + fn field(&self, id: u8) -> #result { + let id = Fields::try_from(id)?; match id { #(#field_arms,)* - _ => None, + // This arm might be reached if someone tries to access an + // internal field. + _ => Err(#foundations::FieldAccessError::Unknown), } } - fn field_with_styles(&self, id: u8, styles: #foundations::StyleChain) -> Option<#foundations::Value> { - let id = Fields::try_from(id).ok()?; + fn field_with_styles(&self, id: u8, styles: #foundations::StyleChain) -> #result { + let id = Fields::try_from(id)?; match id { #(#field_with_styles_arms,)* - _ => None, + // This arm might be reached if someone tries to access an + // internal field. + _ => Err(#foundations::FieldAccessError::Unknown), } } - fn field_from_styles(id: u8, styles: #foundations::StyleChain) -> Option<#foundations::Value> { - let id = Fields::try_from(id).ok()?; + fn field_from_styles(id: u8, styles: #foundations::StyleChain) -> #result { + let id = Fields::try_from(id)?; match id { #(#field_from_styles_arms,)* - _ => None, + // This arm might be reached if someone tries to access an + // internal field. + _ => Err(#foundations::FieldAccessError::Unknown), } } diff --git a/crates/typst/src/eval/code.rs b/crates/typst/src/eval/code.rs index 8542c846e..dcbc5b69d 100644 --- a/crates/typst/src/eval/code.rs +++ b/crates/typst/src/eval/code.rs @@ -315,7 +315,7 @@ impl Eval for ast::FieldAccess<'_> { if let Some(element) = func.element(); if let Some(id) = element.field_id(&field); let styles = vm.context.styles().at(field.span()); - if let Some(value) = element.field_from_styles( + if let Ok(value) = element.field_from_styles( id, styles.as_ref().map(|&s| s).unwrap_or_default(), ); diff --git a/crates/typst/src/foundations/content.rs b/crates/typst/src/foundations/content.rs index 5a22734b5..c3ab38518 100644 --- a/crates/typst/src/foundations/content.rs +++ b/crates/typst/src/foundations/content.rs @@ -196,10 +196,14 @@ impl Content { /// This is the preferred way to access fields. However, you can only use it /// if you have set the field IDs yourself or are using the field IDs /// generated by the `#[elem]` macro. - pub fn get(&self, id: u8, styles: Option) -> Option { + pub fn get( + &self, + id: u8, + styles: Option, + ) -> Result { if id == 255 { if let Some(label) = self.label() { - return Some(label.into_value()); + return Ok(label.into_value()); } } match styles { @@ -212,13 +216,13 @@ impl Content { /// /// If you have access to the field IDs of the element, use [`Self::get`] /// instead. - pub fn get_by_name(&self, name: &str) -> Option { + pub fn get_by_name(&self, name: &str) -> Result { if name == "label" { if let Some(label) = self.label() { - return Some(label.into_value()); + return Ok(label.into_value()); } } - let id = self.elem().field_id(name)?; + let id = self.elem().field_id(name).ok_or(FieldAccessError::Unknown)?; self.get(id, None) } @@ -229,7 +233,7 @@ impl Content { /// generated by the `#[elem]` macro. pub fn field(&self, id: u8) -> StrResult { self.get(id, None) - .ok_or_else(|| missing_field(self.elem().field_name(id).unwrap())) + .map_err(|e| e.message(self, self.elem().field_name(id).unwrap())) } /// Get a field by name, returning a missing field error if it does not @@ -238,7 +242,7 @@ impl Content { /// If you have access to the field IDs of the element, use [`Self::field`] /// instead. pub fn field_by_name(&self, name: &str) -> StrResult { - self.get_by_name(name).ok_or_else(|| missing_field(name)) + self.get_by_name(name).map_err(|e| e.message(self, name)) } /// Resolve all fields with the styles and save them in-place. @@ -568,8 +572,8 @@ impl Content { default: Option, ) -> StrResult { self.get_by_name(&field) - .or(default) - .ok_or_else(|| missing_field_no_default(&field)) + .or_else(|e| default.ok_or(e)) + .map_err(|e| e.message_no_default(self, &field)) } /// Returns the fields of this content. @@ -968,18 +972,43 @@ pub trait PlainText { fn plain_text(&self, text: &mut EcoString); } -/// The missing field access error message. -#[cold] -fn missing_field(field: &str) -> EcoString { - eco_format!("content does not contain field {}", field.repr()) +/// An error arising when trying to access a field of content. +#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)] +pub enum FieldAccessError { + Unknown, + Unset, + Internal, } -/// The missing field access error message when no default value was given. -#[cold] -fn missing_field_no_default(field: &str) -> EcoString { - eco_format!( - "content does not contain field {} and \ - no default value was specified", - field.repr() - ) +impl FieldAccessError { + /// Formats the error message given the content and the field name. + #[cold] + pub fn message(self, content: &Content, field: &str) -> EcoString { + let elem_name = content.elem().name(); + match self { + FieldAccessError::Unknown => { + eco_format!("{elem_name} does not have field {}", field.repr()) + } + FieldAccessError::Unset => { + eco_format!( + "field {} in {elem_name} is not known at this point", + field.repr() + ) + } + FieldAccessError::Internal => { + eco_format!( + "internal error when accessing field {} in {elem_name} – this is a bug", + field.repr() + ) + } + } + } + + /// Formats the error message for an `at` calls without a default value. + #[cold] + pub fn message_no_default(self, content: &Content, field: &str) -> EcoString { + let mut msg = self.message(content, field); + msg.push_str(" and no default was specified"); + msg + } } diff --git a/crates/typst/src/foundations/element.rs b/crates/typst/src/foundations/element.rs index 6ffeadb90..e578d2094 100644 --- a/crates/typst/src/foundations/element.rs +++ b/crates/typst/src/foundations/element.rs @@ -11,8 +11,8 @@ use smallvec::SmallVec; use crate::diag::SourceResult; use crate::engine::Engine; use crate::foundations::{ - cast, Args, Content, Dict, Func, ParamInfo, Repr, Scope, Selector, StyleChain, - Styles, Value, + cast, Args, Content, Dict, FieldAccessError, Func, ParamInfo, Repr, Scope, Selector, + StyleChain, Styles, Value, }; use crate::text::{Lang, Region}; use crate::utils::Static; @@ -123,8 +123,12 @@ impl Element { (self.0.field_name)(id) } - /// Extract the field name for the given field ID. - pub fn field_from_styles(&self, id: u8, styles: StyleChain) -> Option { + /// Extract the value of the field for the given field ID and style chain. + pub fn field_from_styles( + &self, + id: u8, + styles: StyleChain, + ) -> Result { (self.0.field_from_styles)(id, styles) } @@ -223,13 +227,17 @@ pub trait Fields { fn has(&self, id: u8) -> bool; /// Get the field with the given field ID. - fn field(&self, id: u8) -> Option; + fn field(&self, id: u8) -> Result; /// Get the field with the given ID in the presence of styles. - fn field_with_styles(&self, id: u8, styles: StyleChain) -> Option; + fn field_with_styles( + &self, + id: u8, + styles: StyleChain, + ) -> Result; /// Get the field with the given ID from the styles. - fn field_from_styles(id: u8, styles: StyleChain) -> Option + fn field_from_styles(id: u8, styles: StyleChain) -> Result where Self: Sized; @@ -282,7 +290,7 @@ pub struct NativeElementData { /// Gets the name of a field by its numeric index. pub field_name: fn(u8) -> Option<&'static str>, /// Get the field with the given ID in the presence of styles (see [`Fields`]). - pub field_from_styles: fn(u8, StyleChain) -> Option, + pub field_from_styles: fn(u8, StyleChain) -> Result, /// Gets the localized name for this element (see [`LocalName`][crate::text::LocalName]). pub local_name: Option) -> &'static str>, pub scope: Lazy, diff --git a/crates/typst/src/foundations/selector.rs b/crates/typst/src/foundations/selector.rs index 6802a1502..55fd95559 100644 --- a/crates/typst/src/foundations/selector.rs +++ b/crates/typst/src/foundations/selector.rs @@ -128,7 +128,7 @@ impl Selector { Self::Elem(element, dict) => { target.func() == *element && dict.iter().flat_map(|dict| dict.iter()).all(|(id, value)| { - target.get(*id, styles).as_ref() == Some(value) + target.get(*id, styles).as_ref().ok() == Some(value) }) } Self::Label(label) => target.label() == Some(*label), diff --git a/tests/suite/foundations/content.typ b/tests/suite/foundations/content.typ index afecc124f..31ef1c54c 100644 --- a/tests/suite/foundations/content.typ +++ b/tests/suite/foundations/content.typ @@ -13,7 +13,7 @@ - C --- content-field-missing --- -// Error: 25-28 content does not contain field "fun" +// Error: 25-28 heading does not have field "fun" #show heading: it => it.fun = A @@ -118,3 +118,15 @@ } = Hello, world! + +--- content-fields-unset --- +// Error: 10-15 field "block" in raw is not known at this point +#raw("").block + +--- content-fields-unset-no-default --- +// Error: 2-21 field "block" in raw is not known at this point and no default was specified +#raw("").at("block") + +--- content-try-to-access-internal-field --- +// Error: 9-15 hide does not have field "hidden" +#hide[].hidden diff --git a/tests/suite/styling/show.typ b/tests/suite/styling/show.typ index aa121bffb..05f545c60 100644 --- a/tests/suite/styling/show.typ +++ b/tests/suite/styling/show.typ @@ -78,7 +78,7 @@ Another text. = Heading --- show-unknown-field --- -// Error: 25-29 content does not contain field "page" +// Error: 25-29 heading does not have field "page" #show heading: it => it.page = Heading