diff --git a/src/error.rs b/src/error.rs index 3f4d89b6b9..7514f17abc 100644 --- a/src/error.rs +++ b/src/error.rs @@ -385,6 +385,26 @@ pub enum ParseError { /// - a variable is used as both a record and enum row variable, e.g. in the /// signature `forall r. [| ; r |] -> { ; r }`. TypeVariableKindMismatch { ty_var: Ident, span: RawSpan }, + /// A record literal, which isn't a record type, has a field with a type annotation but without + /// a definition. While we could technically handle this situation, this is most probably an + /// error from the user, because this type annotation is useless and, maybe non-intuitively, + /// won't have any effect as part of a larger contract: + /// + /// ```nickel + /// let MixedContract = {foo : String, bar | Number} in + /// { foo = 1, bar = 2} | MixedContract + /// ``` + /// + /// This example works, because the `foo : String` annotation doesn't propagate, and contract + /// application is mostly merging, which is probably not the intent. It might become a warning + /// in a future version, but we don't have warnings for now, so we rather forbid such + /// constructions. + TypedFieldWithoutDefinition { + /// The position of the field definition. + field_span: RawSpan, + /// The position of the type annotation. + annot_span: RawSpan, + }, } /// An error occurring during the resolution of an import. @@ -550,6 +570,13 @@ impl ParseError { InternalParseError::TypeVariableKindMismatch { ty_var, span } => { ParseError::TypeVariableKindMismatch { ty_var, span } } + InternalParseError::TypedFieldWithoutDefinition { + field_span, + annot_span, + } => ParseError::TypedFieldWithoutDefinition { + field_span, + annot_span, + }, }, } } @@ -1561,6 +1588,23 @@ impl IntoDiagnostics for ParseError { String::from("Type variables may be used either as types, polymorphic record tails, or polymorphic enum tails."), String::from("Using the same variable as more than one of these is not permitted.") ]), + ParseError::TypedFieldWithoutDefinition { field_span, annot_span } => { + Diagnostic::error() + .with_message("statically typed field without a definition") + .with_labels(vec![ + primary(&field_span).with_message("this field doesn't have a definition"), + secondary(&annot_span).with_message("but it has a type annotation"), + ]) + .with_notes(vec![ + String::from("A static type annotation must be attached to an expression but \ + this field doesn't have a definition."), + String::from("Did you mean to use `|` instead of `:`, for example when defining a record contract?"), + String::from("Typed fields without definitions are only allowed inside \ + record types, but the enclosing record literal doesn't qualify as a \ + record type. Please refer to the manual for the defining conditions of a \ + record type."), + ]) + } }; vec![diagnostic] diff --git a/src/eval/fixpoint.rs b/src/eval/fixpoint.rs index f664e98602..5b1daa2cce 100644 --- a/src/eval/fixpoint.rs +++ b/src/eval/fixpoint.rs @@ -85,7 +85,7 @@ pub fn rec_env<'a, I: Iterator, C: Cache>( let id_value = Ident::fresh(); final_env.insert(id_value, idx); - let with_ctr_applied = PendingContract::apply_all( + let with_ctr_applied = RuntimeContract::apply_all( RichTerm::new(Term::Var(id_value), value.pos), field.pending_contracts.iter().cloned().flat_map(|ctr| { // This operation is the heart of our preliminary fix for @@ -112,7 +112,7 @@ pub fn rec_env<'a, I: Iterator, C: Cache>( } else { vec![ ctr.clone(), - PendingContract { + RuntimeContract { contract: ctr.contract, label: Label { polarity: ctr.label.polarity.flip(), diff --git a/src/eval/merge.rs b/src/eval/merge.rs index e8e40d5a68..b2860af47d 100644 --- a/src/eval/merge.rs +++ b/src/eval/merge.rs @@ -646,24 +646,24 @@ impl RevertClosurize for Field { } } -impl RevertClosurize for PendingContract { +impl RevertClosurize for RuntimeContract { fn revert_closurize( self, cache: &mut C, env: &mut Environment, with_env: Environment, - ) -> PendingContract { + ) -> RuntimeContract { self.map_contract(|ctr| ctr.revert_closurize(cache, env, with_env)) } } -impl RevertClosurize for Vec { +impl RevertClosurize for Vec { fn revert_closurize( self, cache: &mut C, env: &mut Environment, with_env: Environment, - ) -> Vec { + ) -> Vec { self.into_iter() .map(|pending_contract| pending_contract.revert_closurize(cache, env, with_env.clone())) .collect() diff --git a/src/eval/mod.rs b/src/eval/mod.rs index 1395e78068..e71119dd46 100644 --- a/src/eval/mod.rs +++ b/src/eval/mod.rs @@ -87,7 +87,7 @@ use crate::{ array::ArrayAttrs, make as mk_term, record::{Field, RecordData}, - BinaryOp, BindingType, LetAttrs, PendingContract, RichTerm, StrChunk, Term, UnaryOp, + BinaryOp, BindingType, LetAttrs, RichTerm, RuntimeContract, StrChunk, Term, UnaryOp, }, transform::Closurizable, }; @@ -253,7 +253,7 @@ impl VirtualMachine { value_next = value_next .map(|value| -> Result { let pos_value = value.pos; - let value_with_ctr = PendingContract::apply_all( + let value_with_ctr = RuntimeContract::apply_all( value, pending_contracts.into_iter(), pos_value, @@ -649,7 +649,7 @@ impl VirtualMachine { .pending_contracts .iter() .map(|ctr| { - PendingContract::new( + RuntimeContract::new( ctr.contract.clone().closurize( &mut self.cache, &mut local_env, @@ -688,10 +688,10 @@ impl VirtualMachine { // avoiding repeated contract application. Annotations could then be a good way of // remembering which contracts have been applied to a value. Term::Annotated(annot, inner) => { - let contracts = annot.as_pending_contracts()?; + let contracts = annot.pending_contracts()?; let pos = inner.pos; let inner_with_ctr = - PendingContract::apply_all(inner.clone(), contracts.into_iter(), pos); + RuntimeContract::apply_all(inner.clone(), contracts.into_iter(), pos); Closure { body: inner_with_ctr, diff --git a/src/eval/operation.rs b/src/eval/operation.rs index 75c8e01344..583f12ad46 100644 --- a/src/eval/operation.rs +++ b/src/eval/operation.rs @@ -27,8 +27,8 @@ use crate::{ make as mk_term, record::{self, Field, FieldMetadata, RecordData}, string::NickelString, - BinaryOp, CompiledRegex, IndexMap, MergePriority, NAryOp, Number, PendingContract, - RecordExtKind, RichTerm, SharedTerm, StrChunk, Term, UnaryOp, + BinaryOp, CompiledRegex, IndexMap, MergePriority, NAryOp, Number, RecordExtKind, RichTerm, + RuntimeContract, SharedTerm, StrChunk, Term, UnaryOp, }, transform::Closurizable, }; @@ -526,7 +526,7 @@ impl VirtualMachine { let ts = ts .into_iter() .map(|t| { - let t_with_ctrs = PendingContract::apply_all( + let t_with_ctrs = RuntimeContract::apply_all( t, attrs.pending_contracts.iter().cloned(), pos.into_inherited(), @@ -695,7 +695,7 @@ impl VirtualMachine { let terms = seq_terms( ts.into_iter().map(|t| { - let t_with_ctr = PendingContract::apply_all( + let t_with_ctr = RuntimeContract::apply_all( t, attrs.pending_contracts.iter().cloned(), pos.into_inherited(), @@ -1043,7 +1043,7 @@ impl VirtualMachine { .map(|t| { mk_term::op1( UnaryOp::Force { ignore_not_exported }, - PendingContract::apply_all( + RuntimeContract::apply_all( t, attrs.pending_contracts.iter().cloned(), pos.into_inherited(), @@ -1725,12 +1725,12 @@ impl VirtualMachine { .filter(|ctr| !ctrs_left.contains(ctr) && !ctrs_common.contains(ctr)); ts.extend(ts1.into_iter().map(|t| - PendingContract::apply_all(t, ctrs_left.iter().cloned(), pos1) + RuntimeContract::apply_all(t, ctrs_left.iter().cloned(), pos1) .closurize(&mut self.cache, &mut env, env1.clone()) )); ts.extend(ts2.into_iter().map(|t| - PendingContract::apply_all(t, ctrs_right.clone(), pos2) + RuntimeContract::apply_all(t, ctrs_right.clone(), pos2) .closurize(&mut self.cache, &mut env, env2.clone()) )); @@ -1763,7 +1763,7 @@ impl VirtualMachine { return Err(EvalError::Other(format!("elem_at: index out of bounds. Expected an index between 0 and {}, got {}", ts.len(), n), pos_op)); } - let elem_with_ctr = PendingContract::apply_all( + let elem_with_ctr = RuntimeContract::apply_all( ts.get(n_as_usize).unwrap().clone(), attrs.pending_contracts.iter().cloned(), pos1.into_inherited(), @@ -1973,7 +1973,7 @@ impl VirtualMachine { let array_with_ctr = Closure { body: RichTerm::new( - Term::Array(ts, attrs.with_extra_contracts([PendingContract::new(rt3, lbl)])), + Term::Array(ts, attrs.with_extra_contracts([RuntimeContract::new(rt3, lbl)])), pos2, ), env: env2, @@ -2020,7 +2020,7 @@ impl VirtualMachine { }; for (id, field) in record_data.fields.iter_mut() { - field.pending_contracts.push(PendingContract { + field.pending_contracts.push(RuntimeContract { contract: contract_at_field(*id), label: label.clone(), }); @@ -2989,12 +2989,12 @@ fn eq( let pos1 = value1.pos; let pos2 = value2.pos; - let value1_with_ctr = PendingContract::apply_all( + let value1_with_ctr = RuntimeContract::apply_all( value1, pending_contracts1.into_iter(), pos1, ); - let value2_with_ctr = PendingContract::apply_all( + let value2_with_ctr = RuntimeContract::apply_all( value2, pending_contracts2.into_iter(), pos2, @@ -3049,14 +3049,14 @@ fn eq( .into_iter() .map(|t| { let pos = t.pos.into_inherited(); - PendingContract::apply_all(t, a1.pending_contracts.iter().cloned(), pos) + RuntimeContract::apply_all(t, a1.pending_contracts.iter().cloned(), pos) .closurize(cache, &mut shared_env1, env1.clone()) }) .collect::>() .into_iter() .zip(l2.into_iter().map(|t| { let pos = t.pos.into_inherited(); - PendingContract::apply_all(t, a2.pending_contracts.iter().cloned(), pos) + RuntimeContract::apply_all(t, a2.pending_contracts.iter().cloned(), pos) .closurize(cache, &mut shared_env2, env2.clone()) })) .collect::>(); @@ -3140,7 +3140,7 @@ where .value .map(|value| { let pos = value.pos; - let value_with_ctrs = PendingContract::apply_all( + let value_with_ctrs = RuntimeContract::apply_all( value, field.pending_contracts.iter().cloned(), pos, diff --git a/src/parser/error.rs b/src/parser/error.rs index 27d63eef36..c4ff8d6240 100644 --- a/src/parser/error.rs +++ b/src/parser/error.rs @@ -44,4 +44,24 @@ pub enum ParseError { /// - a variable is used as both a record and enum row variable, e.g. in the /// signature `forall r. [| ; r |] -> { ; r }`. TypeVariableKindMismatch { ty_var: Ident, span: RawSpan }, + /// A record literal, which isn't a record type, has a field with a type annotation but without + /// a definition. While we could technically handle this situation, this is most probably an + /// error from the user, because this type annotation is useless and, maybe non-intuitively, + /// won't have any effect as part of a larger contract: + /// + /// ```nickel + /// let MixedContract = {foo : String, bar | Number} in + /// { foo = 1, bar = 2} | MixedContract + /// ``` + /// + /// This example works, because the `foo : String` annotation doesn't propagate, and contract + /// application is mostly merging, which is probably not the intent. It might become a warning + /// in a future version, but we don't have warnings for now, so we rather forbid such + /// constructions. + TypedFieldWithoutDefinition { + /// The position of the field definition (the identifier only). + field_span: RawSpan, + /// The position of the type annotation. + annot_span: RawSpan, + }, } diff --git a/src/parser/uniterm.rs b/src/parser/uniterm.rs index a29811a3cc..0412abe5f4 100644 --- a/src/parser/uniterm.rs +++ b/src/parser/uniterm.rs @@ -1,6 +1,7 @@ //! Additional AST nodes for the common UniTerm syntax (see RFC002 for more details). use super::*; use error::ParseError; +use indexmap::{map::Entry, IndexMap}; use utils::{build_record, FieldDef, FieldPathElem}; use crate::{ @@ -169,11 +170,151 @@ pub struct UniRecord { /// Error indicating that a construct is not allowed when trying to interpret an `UniRecord` as a /// record type in a strict way. See [`UniRecord::into_type_strict`]. Hold the position of the /// illegal construct. +#[derive(Debug, Copy, Clone)] pub struct InvalidRecordTypeError(pub TermPos); impl UniRecord { - /// Try to convert a `UniRecord` to a type. The strict part means that the `UniRecord` must be - /// a plain record type, uniquely containing fields of the form `fields: Type`. Currently, it + /// Check if a field definition has a type annotation but no definition. This is currently + /// forbidden for record literals that aren't record types. In that case, raise the + /// corresponding parse error. + pub fn check_typed_field_without_def(&self) -> Result<(), ParseError> { + enum FieldState { + // A field with a type annotation but without a definition was encountered. Still, we + // might find a definition later, because of piecewise definitions + Candidate((RawSpan, RawSpan)), + // Marker to indicate that a field has been defined before, and can no longer raise an + // error. + Defined, + } + + // We have to be a bit careful because of piecewise definitions. That is, we still want to + // accept record literals such as: + // + // ``` + // { + // map : forall a b. (a -> b) -> Array a -> Array b, + // map = fun f array => ... + // } + // ``` + // + // On the other hand, it's a bit too complex to handle the case of piecewise definitions: + // + // ``` + // { + // foo.bar.baz : Num, + // foo.bar.baz = 1, + // } + // ``` + // + // This is arguably much less common and useful. In this case, we are more restrictive and + // reject such an example, although it would theoretically be acceptable as it's elaborated + // as a record literal that is accepted: + // + // ``` + // { foo = { bar = {baz : Num = 1 } } } + // ``` + // We're using an index map because this map impacts the determinism of error reporting. + let mut candidate_fields = IndexMap::new(); + + let first_without_def = self.fields.iter().find_map(|field_def| { + let path_as_ident = field_def.path_as_ident(); + + match &field_def.field { + Field { + value: None, + metadata: + FieldMetadata { + annotation: + TypeAnnotation { + types: Some(labeled_ty), + .. + }, + .. + }, + .. + } => { + // If the path is a single identifier, we don't error out right away, because + // we might already have found a definition for this field, or might do later + // in the loop. + if let Some(ident) = path_as_ident { + match candidate_fields.entry(ident) { + // If the hashmap is occupied, we've met this field before. Either + // there is another definition without annotation, in which case + // there's no need to replace it, or there is a `Defined` element, + // which means this is false positive that we can ignore. In both cases, + // we don't have anytning more to do + Entry::Occupied(_) => None, + Entry::Vacant(vacant_entry) => { + vacant_entry.insert(FieldState::Candidate(( + ident.pos.unwrap(), + labeled_ty.label.span, + ))); + None + } + } + } + // We don't do anything smart for composite paths: we raise an error right way + else { + Some((field_def.pos.unwrap(), labeled_ty.label.span)) + } + } + field => { + if let (Some(ident), Some(_)) = (path_as_ident, &field.value) { + candidate_fields.insert(ident, FieldState::Defined); + } + + None + } + } + }); + + let first_without_def = + first_without_def.or(candidate_fields.into_iter().find_map(|(_, field_state)| { + if let FieldState::Candidate(spans) = field_state { + Some(spans) + } else { + None + } + })); + + if let Some((ident_span, annot_span)) = first_without_def { + Err(ParseError::TypedFieldWithoutDefinition { + field_span: ident_span, + annot_span, + }) + } else { + Ok(()) + } + } + + /// Checks if this record qualifies as a record type. If this function returns true, then + /// `into_type_strict()` must succeed. + pub fn is_record_type(&self) -> bool { + self.fields.iter().all(|field_def| { + // Warning: this pattern must stay in sync with the corresponding pattern in `into_type_strict`. + matches!(&field_def.field, + Field { + value: None, + metadata: + FieldMetadata { + doc: None, + annotation: + TypeAnnotation { + types: Some(_), + contracts, + }, + opt: false, + not_exported: false, + priority: MergePriority::Neutral, + }, + // At this stage, this field should always be empty. It's a run-time thing, and + // is only filled during program transformation. + pending_contracts: _, + } if contracts.is_empty()) + }) + } + + /// a plain record type, uniquely containing fields of the form `fields: Type`. Currently, this /// doesn't support the field path syntax: `{foo.bar.baz : Type}.into_type_strict()` returns an /// `Err`. pub fn into_type_strict(self) -> Result { @@ -185,6 +326,8 @@ impl UniRecord { // At parsing stage, all `Rc`s must be 1-counted. We can thus call // `into_owned()` without risking to actually clone anything. match field_def.field { + // Warning: this pattern must stay in sync with the corresponding pattern in + // `is_record_type`. Field { value: None, metadata: @@ -289,29 +432,42 @@ impl UniRecord { impl TryFrom for RichTerm { type Error = ParseError; - /// Convert a `UniRecord` to a term. If the `UniRecord` has a tail, it is first interpreted as - /// a type and then converted to a contract. Otherwise it is interpreted as a record directly. - /// Fail if the `UniRecord` has a tail but isn't syntactically a record type either. Elaborate - /// field paths `foo.bar = value` to the expanded form `{foo = {bar = value}}`. + /// Convert a `UniRecord` to a term. If the `UniRecord` is syntactically a record type or it + /// has a tail, it is first interpreted as a type and then converted to a contract. One + /// exception is the empty record, which behaves the same both as a type and a contract, and + /// turning an empty record literal to an opaque function would break everything. + /// + /// Otherwise it is interpreted as a record directly. Fail if the `UniRecord` has a tail but + /// isn't syntactically a record type either. Elaborate field paths `foo.bar = value` to the + /// expanded form `{foo = {bar = value}}`. /// /// We also fix the type variables of the type appearing inside annotations (see /// [`FixTypeVars::fix_type_vars`]). fn try_from(ur: UniRecord) -> Result { let pos = ur.pos; - let result = if let Some((_, tail_pos)) = ur.tail { - ur.into_type_strict() + // First try to interpret this record as a type. + let result = if ur.tail.is_some() || (ur.is_record_type() && !ur.fields.is_empty()) { + let mut ty = if let Some((_, tail_pos)) = ur.tail { // We unwrap all positions: at this stage of the parsing, they must all be set - .map_err(|InvalidRecordTypeError(pos)| { - ParseError::InvalidUniRecord(pos.unwrap(), tail_pos.unwrap(), pos.unwrap()) - }) - .and_then(|mut ty| { - ty.fix_type_vars(pos.unwrap())?; - ty.contract().map_err(|UnboundTypeVariableError(id)| { - ParseError::UnboundTypeVariables(vec![id], pos.unwrap()) - }) - }) + ur.into_type_strict() + .map_err(|InvalidRecordTypeError(pos)| { + ParseError::InvalidUniRecord(pos.unwrap(), tail_pos.unwrap(), pos.unwrap()) + })? + } else { + // As per the condition of the enclosing if-then-else, `ur.is_record_type()` must + // be `true` in this branch, and it is an invariant of this function that then + // `ur.into_type_strict()` must succeed + ur.into_type_strict().unwrap() + }; + + ty.fix_type_vars(pos.unwrap())?; + ty.contract().map_err(|UnboundTypeVariableError(id)| { + ParseError::UnboundTypeVariables(vec![id], pos.unwrap()) + }) } else { + ur.check_typed_field_without_def()?; + let UniRecord { fields, attrs, .. } = ur; let elaborated = fields .into_iter() @@ -321,13 +477,15 @@ impl TryFrom for RichTerm { }) .collect::, _>>()?; - Ok(RichTerm::from(build_record(elaborated, attrs))) + let record_term = RichTerm::from(build_record(elaborated, attrs)); + Ok(record_term) }; result.map(|rt| rt.with_pos(pos)) } } +/// Try to convert a `UniRecord` to a type. The strict part means that the `UniRecord` must be impl TryFrom for Types { type Error = ParseError; diff --git a/src/parser/utils.rs b/src/parser/utils.rs index 2eadfd54ad..504fac4704 100644 --- a/src/parser/utils.rs +++ b/src/parser/utils.rs @@ -183,6 +183,18 @@ impl FieldDef { (fst, content) } + + /// Returns the identifier corresponding to this definition if the path is composed of exactly one element which is a static identifier. Returns `None` otherwise. + pub fn path_as_ident(&self) -> Option { + if self.path.len() > 1 { + return None; + } + + self.path.first().and_then(|path_elem| match path_elem { + FieldPathElem::Expr(_) => None, + FieldPathElem::Ident(ident) => Some(*ident), + }) + } } /// The last field of a record, that can either be a normal field declaration or an ellipsis. diff --git a/src/term/array.rs b/src/term/array.rs index d3a318ea1a..0836ed1a06 100644 --- a/src/term/array.rs +++ b/src/term/array.rs @@ -12,7 +12,7 @@ pub struct ArrayAttrs { pub closurized: bool, /// List of lazily-applied contracts. /// These are only observed when data enters or leaves the array. - pub pending_contracts: Vec, + pub pending_contracts: Vec, } impl ArrayAttrs { @@ -41,7 +41,7 @@ impl ArrayAttrs { /// future pub fn with_extra_contracts(mut self, iter: I) -> Self where - I: IntoIterator, + I: IntoIterator, { for ctr in iter { if !self.pending_contracts.contains(&ctr) { diff --git a/src/term/mod.rs b/src/term/mod.rs index aad938f938..f2c0761224 100644 --- a/src/term/mod.rs +++ b/src/term/mod.rs @@ -251,18 +251,19 @@ pub enum BindingType { Revertible(FieldDeps), } -/// A contract with its associated data. +/// A runtime representation of a contract, as a term ready to be applied via `Assume()` together +/// with its label. #[derive(Debug, PartialEq, Clone)] -pub struct PendingContract { +pub struct RuntimeContract { /// The pending contract, can be a function or a record. pub contract: RichTerm, /// The blame label. pub label: Label, } -impl PendingContract { +impl RuntimeContract { pub fn new(contract: RichTerm, label: Label) -> Self { - PendingContract { contract, label } + RuntimeContract { contract, label } } /// Map a function over the term representing the underlying contract. @@ -270,44 +271,47 @@ impl PendingContract { where F: FnOnce(RichTerm) -> RichTerm, { - PendingContract { + RuntimeContract { contract: f(self.contract), ..self } } - /// Apply a series of pending contracts to a given term. + /// Apply this contract to a term. + pub fn apply(self, rt: RichTerm, pos: TermPos) -> RichTerm { + use crate::mk_app; + + mk_app!( + make::op2(BinaryOp::Assume(), self.contract, Term::Lbl(self.label)).with_pos(pos), + rt + ) + .with_pos(pos) + } + + /// Apply a series of contracts to a term, in order. pub fn apply_all(rt: RichTerm, contracts: I, pos: TermPos) -> RichTerm where I: Iterator, { - use crate::mk_app; - - contracts.fold(rt, |acc, ctr| { - mk_app!( - make::op2(BinaryOp::Assume(), ctr.contract, Term::Lbl(ctr.label)).with_pos(pos), - acc - ) - .with_pos(pos) - }) + contracts.fold(rt, |acc, ctr| ctr.apply(acc, pos)) } } -impl Traverse for PendingContract { +impl Traverse for RuntimeContract { fn traverse(self, f: &F, state: &mut S, order: TraverseOrder) -> Result where F: Fn(RichTerm, &mut S) -> Result, { let contract = self.contract.traverse(f, state, order)?; - Ok(PendingContract { contract, ..self }) + Ok(RuntimeContract { contract, ..self }) } } -impl std::convert::TryFrom for PendingContract { +impl std::convert::TryFrom for RuntimeContract { type Error = UnboundTypeVariableError; fn try_from(labeled_ty: LabeledType) -> Result { - Ok(PendingContract::new( + Ok(RuntimeContract::new( labeled_ty.types.contract()?, labeled_ty.label, )) @@ -454,7 +458,7 @@ impl TypeAnnotation { /// Return the main annotation, which is either the type annotation if any, or the first /// contract annotation. pub fn first(&self) -> Option<&LabeledType> { - self.iter().next() + self.types.iter().chain(self.contracts.iter()).next() } /// Iterate over the annotations, starting by the type and followed by the contracts. @@ -479,11 +483,26 @@ impl TypeAnnotation { }) } - /// Build a list of pending contracts from this type annotation. - pub fn as_pending_contracts(&self) -> Result, UnboundTypeVariableError> { - self.iter() + /// Build a list of pending contracts from this annotation, to be stored alongside the metadata + /// of a field. Similar to [all_contracts], but including the contracts from `self.contracts` + /// only, while `types` is excluded. Contracts derived from type annotations aren't treated the + /// same since they don't propagate through merging. + pub fn pending_contracts(&self) -> Result, UnboundTypeVariableError> { + self.contracts + .iter() + .cloned() + .map(RuntimeContract::try_from) + .collect::, _>>() + } + + /// Convert all the contracts of this annotation, including the potential type annotation as + /// the first element, to a runtime representation. + pub fn all_contracts(&self) -> Result, UnboundTypeVariableError> { + self.types + .iter() + .chain(self.contracts.iter()) .cloned() - .map(PendingContract::try_from) + .map(RuntimeContract::try_from) .collect::, _>>() } } @@ -1264,7 +1283,7 @@ pub enum BinaryOp { /// more principled primop. DynExtend { metadata: FieldMetadata, - pending_contracts: Vec, + pending_contracts: Vec, ext_kind: RecordExtKind, }, /// Remove a field from a record. The field name is given as an arbitrary Nickel expression. diff --git a/src/term/record.rs b/src/term/record.rs index a0cd3d2f94..fc7c397f53 100644 --- a/src/term/record.rs +++ b/src/term/record.rs @@ -146,7 +146,7 @@ pub struct Field { pub metadata: FieldMetadata, /// List of contracts yet to be applied. /// These are only observed when data enter or leave the record. - pub pending_contracts: Vec, + pub pending_contracts: Vec, } impl From for Field { @@ -348,7 +348,7 @@ impl RecordData { let pos = v.pos; Some(Ok(( id, - PendingContract::apply_all(v, field.pending_contracts.into_iter(), pos), + RuntimeContract::apply_all(v, field.pending_contracts.into_iter(), pos), ))) } None if !field.metadata.opt => Some(Err(MissingFieldDefError { @@ -405,7 +405,7 @@ impl RecordData { .. }) => { let pos = value.pos; - Ok(Some(PendingContract::apply_all( + Ok(Some(RuntimeContract::apply_all( value.clone(), pending_contracts.iter().cloned(), pos, diff --git a/src/transform/gen_pending_contracts.rs b/src/transform/gen_pending_contracts.rs index 04d38c261b..6248b32eb1 100644 --- a/src/transform/gen_pending_contracts.rs +++ b/src/transform/gen_pending_contracts.rs @@ -1,26 +1,47 @@ //! Generate contract applications from annotations. //! -//! Since RFC005, contracts aren't "pre-applied" during the apply contract transformation, but lazy -//! applied during evaluation. This phase still exists, just to generate the pending contracts (the -//! only ones that the interpreter will care about at runtime) from the static annotations. +//! Since RFC005, contracts aren't "pre-applied" during the apply contract transformation, but +//! lazily applied during evaluation. This phase still exists, just to generate the pending +//! contracts (the only ones that the interpreter will care about at runtime) from the static +//! annotations. //! -//! It must be run before `share_normal_form` so that newly generated pending contracts are -//! transformed as well. +//! However, contracts generated from a type annotation (see +//! [#1228](https://github.com/tweag/nickel/issues/1228)) are an exception, because they shouldn't +//! propagate. Such contracts are generated and applied once and for all during this phase. +//! +//! The `gen_pending_contracts` phase implemented by this module must be run before +//! `share_normal_form` so that newly generated pending contracts are transformed as well. use crate::{ identifier::Ident, match_sharedterm, term::{ record::{Field, RecordData}, - IndexMap, RichTerm, Term, + IndexMap, RichTerm, RuntimeContract, Term, }, types::UnboundTypeVariableError, }; pub fn transform_one(rt: RichTerm) -> Result { fn attach_to_field(field: Field) -> Result { - let pending_contracts = field.metadata.annotation.as_pending_contracts()?; + // We simply add the contracts to the pending contract fields + let pending_contracts = field.metadata.annotation.pending_contracts()?; + // Type annotations are different: the contract is generated statically, because as opposed + // to contract annotations, type anntotations don't propagate. + let value = field + .value + .map(|v| -> Result { + if let Some(labeled_ty) = &field.metadata.annotation.types { + let pos = v.pos; + let contract = RuntimeContract::try_from(labeled_ty.clone())?; + Ok(contract.apply(v, pos)) + } else { + Ok(v) + } + }) + .transpose()?; Ok(Field { + value, pending_contracts, ..field }) diff --git a/src/transform/mod.rs b/src/transform/mod.rs index 7f3b2ed987..7379b4379d 100644 --- a/src/transform/mod.rs +++ b/src/transform/mod.rs @@ -3,7 +3,7 @@ use crate::{ cache::ImportResolver, eval::{cache::Cache, Closure, Environment, IdentKind}, identifier::Ident, - term::{record::Field, BindingType, PendingContract, RichTerm, Term, Traverse, TraverseOrder}, + term::{record::Field, BindingType, RichTerm, RuntimeContract, Term, Traverse, TraverseOrder}, typecheck::Wildcards, types::UnboundTypeVariableError, }; @@ -165,24 +165,24 @@ impl Closurizable for RichTerm { } } -impl Closurizable for PendingContract { +impl Closurizable for RuntimeContract { fn closurize( self, cache: &mut C, env: &mut Environment, with_env: Environment, - ) -> PendingContract { + ) -> RuntimeContract { self.map_contract(|ctr| ctr.closurize(cache, env, with_env)) } } -impl Closurizable for Vec { +impl Closurizable for Vec { fn closurize( self, cache: &mut C, env: &mut Environment, with_env: Environment, - ) -> Vec { + ) -> Vec { self.into_iter() .map(|pending_contract| pending_contract.closurize(cache, env, with_env.clone())) .collect() diff --git a/tests/integration/pass/types_dont_propagate.ncl b/tests/integration/pass/types_dont_propagate.ncl new file mode 100644 index 0000000000..6a397da34c --- /dev/null +++ b/tests/integration/pass/types_dont_propagate.ncl @@ -0,0 +1,23 @@ +let {check, ..} = import "lib/assert.ncl" in + +[ + # check that a record type literal is indeed converted to the corresponding + # contract, which shouldn't be a record literal +# std.typeof {foo : String, bar : Number} != `Record, + + # check_types_dont_propagate + + # TODO: restore the test below. The PR which added it is not at fault: the + # test is failing on master. The issue is that contracts derived from record + # type seem to erase metadata, while they shouldn't. + #({foo | default = 5} | {foo : Number}) & {foo = "a"} == {foo = "a"}, + + let swap + : forall a b. {foo : a, bar : b} -> {foo : b, bar : a } + = fun {foo=prev_foo, bar=prev_bar} => {bar = prev_foo, foo = prev_bar} + in + ((swap {foo = 1, bar = "a"}) + & {foo | force = false, bar | force = true}) + == {foo = false, bar = true}, +] +|> check diff --git a/tests/integration/records_fail.rs b/tests/integration/records_fail.rs index 15041298f7..4af5a5877d 100644 --- a/tests/integration/records_fail.rs +++ b/tests/integration/records_fail.rs @@ -74,7 +74,7 @@ fn missing_field() { Err(Error::EvalError(EvalError::MissingFieldDef { id, ..})) if id.to_string() == "foo" ); assert_matches!( - eval("{foo : Number, bar = foo + 1}.foo"), + eval("{foo | not_exported, bar = foo + 1}.foo"), Err(Error::EvalError(EvalError::MissingFieldDef {id, ..})) if id.to_string() == "foo" ); assert_matches!( diff --git a/tests/integration/typecheck_fail.rs b/tests/integration/typecheck_fail.rs index 74442fc538..680bb07579 100644 --- a/tests/integration/typecheck_fail.rs +++ b/tests/integration/typecheck_fail.rs @@ -12,6 +12,7 @@ fn type_check(rt: &RichTerm) -> Result<(), TypecheckError> { typecheck::type_check(rt, Context::new(), &DummyResolver {}).map(|_| ()) } +#[track_caller] fn type_check_expr(s: impl std::string::ToString) -> Result<(), TypecheckError> { let s = s.to_string(); let id = Files::new().add("", s.clone()); diff --git a/tests/snapshot/inputs/docs/types.ncl b/tests/snapshot/inputs/docs/types.ncl index df59333eb7..01d7e39987 100644 --- a/tests/snapshot/inputs/docs/types.ncl +++ b/tests/snapshot/inputs/docs/types.ncl @@ -3,4 +3,5 @@ : Number | std.string.Stringable | doc "World!" + = 5, } diff --git a/tests/snapshot/inputs/errors/typed_field_without_annotation.ncl b/tests/snapshot/inputs/errors/typed_field_without_annotation.ncl new file mode 100644 index 0000000000..484f2e8654 --- /dev/null +++ b/tests/snapshot/inputs/errors/typed_field_without_annotation.ncl @@ -0,0 +1,5 @@ +{ + foo : Number + | doc "foo", + bar : String, +} diff --git a/tests/snapshot/snapshots/snapshot__error_typed_field_without_annotation.ncl.snap b/tests/snapshot/snapshots/snapshot__error_typed_field_without_annotation.ncl.snap new file mode 100644 index 0000000000..101fa258fd --- /dev/null +++ b/tests/snapshot/snapshots/snapshot__error_typed_field_without_annotation.ncl.snap @@ -0,0 +1,17 @@ +--- +source: tests/snapshot/main.rs +expression: snapshot +--- +error: statically typed field without a definition + ┌─ [INPUTS_PATH]/errors/typed_field_without_annotation.ncl:2:3 + │ +2 │ foo : Number + │ ^^^ ------ but it has a type annotation + │ │ + │ this field doesn't have a definition + │ + = A static type annotation must be attached to an expression but this field doesn't have a definition. + = Did you mean to use `|` instead of `:`, for example when defining a record contract? + = Typed fields without definitions are only allowed inside record types, but the enclosing record literal doesn't qualify as a record type. Please refer to the manual for the defining conditions of a record type. + +