From 0962bcaf07d66309e6276c317fe1d132d4ffad53 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Tue, 22 Mar 2022 18:24:25 +0100 Subject: [PATCH 1/2] Fix share normal form pass not acting inside types --- src/term.rs | 78 +++++++++++++++++++++++++--------------------------- src/types.rs | 69 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 106 insertions(+), 41 deletions(-) diff --git a/src/term.rs b/src/term.rs index 00bc6243..79f5245d 100644 --- a/src/term.rs +++ b/src/term.rs @@ -979,12 +979,12 @@ impl RichTerm { self, f: &mut F, state: &mut S, - method: TraverseOrder, + order: TraverseOrder, ) -> Result where F: FnMut(RichTerm, &mut S) -> Result, { - let rt = match method { + let rt = match order { TraverseOrder::TopDown => f(self, state)?, TraverseOrder::BottomUp => self, }; @@ -992,38 +992,38 @@ impl RichTerm { let result = match_sharedterm! {rt.term, with { Term::Fun(id, t) => { - let t = t.traverse(f, state, method)?; + let t = t.traverse(f, state, order)?; RichTerm::new( Term::Fun(id, t), pos, ) }, Term::FunPattern(id, d, t) => { - let t = t.traverse(f, state, method)?; + let t = t.traverse(f, state, order)?; RichTerm::new( Term::FunPattern(id, d, t), pos, ) }, Term::Let(id, t1, t2, btype) => { - let t1 = t1.traverse(f, state, method)?; - let t2 = t2.traverse(f, state, method)?; + let t1 = t1.traverse(f, state, order)?; + let t2 = t2.traverse(f, state, order)?; RichTerm::new( Term::Let(id, t1, t2, btype), pos, ) }, Term::LetPattern(id, pat, t1, t2) => { - let t1 = t1.traverse(f, state, method)?; - let t2 = t2.traverse(f, state, method)?; + let t1 = t1.traverse(f, state, order)?; + let t2 = t2.traverse(f, state, order)?; RichTerm::new( Term::LetPattern(id, pat, t1, t2), pos, ) }, Term::App(t1, t2) => { - let t1 = t1.traverse(f, state, method)?; - let t2 = t2.traverse(f, state, method)?; + let t1 = t1.traverse(f, state, order)?; + let t2 = t2.traverse(f, state, order)?; RichTerm::new( Term::App(t1, t2), pos, @@ -1035,12 +1035,12 @@ impl RichTerm { let cases_res: Result, E> = cases .into_iter() // For the conversion to work, note that we need a Result<(Ident,RichTerm), E> - .map(|(id, t)| t.traverse(f, state, method).map(|t_ok| (id.clone(), t_ok))) + .map(|(id, t)| t.traverse(f, state, order).map(|t_ok| (id.clone(), t_ok))) .collect(); - let default = default.map(|t| t.traverse(f, state, method)).transpose()?; + let default = default.map(|t| t.traverse(f, state, order)).transpose()?; - let t = t.traverse(f, state, method)?; + let t = t.traverse(f, state, order)?; RichTerm::new( Term::Switch(t, cases_res?, default), @@ -1048,15 +1048,15 @@ impl RichTerm { ) }, Term::Op1(op, t) => { - let t = t.traverse(f, state, method)?; + let t = t.traverse(f, state, order)?; RichTerm::new( Term::Op1(op, t), pos, ) }, Term::Op2(op, t1, t2) => { - let t1 = t1.traverse(f, state, method)?; - let t2 = t2.traverse(f, state, method)?; + let t1 = t1.traverse(f, state, order)?; + let t2 = t2.traverse(f, state, order)?; RichTerm::new(Term::Op2(op, t1, t2), pos, ) @@ -1064,7 +1064,7 @@ impl RichTerm { Term::OpN(op, ts) => { let ts_res: Result, E> = ts .into_iter() - .map(|t| t.traverse(f, state, method)) + .map(|t| t.traverse(f, state, order)) .collect(); RichTerm::new( Term::OpN(op, ts_res?), @@ -1072,7 +1072,7 @@ impl RichTerm { ) }, Term::Wrapped(i, t) => { - let t = t.traverse(f, state, method)?; + let t = t.traverse(f, state, order)?; RichTerm::new( Term::Wrapped(i, t), pos, @@ -1084,7 +1084,7 @@ impl RichTerm { let map_res: Result, E> = map .into_iter() // For the conversion to work, note that we need a Result<(Ident,RichTerm), E> - .map(|(id, t)| t.traverse(f, state, method).map(|t_ok| (id.clone(), t_ok))) + .map(|(id, t)| t.traverse(f, state, order).map(|t_ok| (id.clone(), t_ok))) .collect(); RichTerm::new( Term::Record(map_res?, attrs), @@ -1097,14 +1097,14 @@ impl RichTerm { let map_res: Result, E> = map .into_iter() // For the conversion to work, note that we need a Result<(Ident,RichTerm), E> - .map(|(id, t)| Ok((id, t.traverse(f, state, method)?))) + .map(|(id, t)| Ok((id, t.traverse(f, state, order)?))) .collect(); let dyn_fields_res: Result, E> = dyn_fields .into_iter() .map(|(id_t, t)| { Ok(( - id_t.traverse(f, state, method)?, - t.traverse(f, state, method)?, + id_t.traverse(f, state, order)?, + t.traverse(f, state, order)?, )) }) .collect(); @@ -1116,7 +1116,7 @@ impl RichTerm { Term::Array(ts) => { let ts_res: Result, E> = ts .into_iter() - .map(|t| t.traverse(f, state, method)) + .map(|t| t.traverse(f, state, order)) .collect(); RichTerm::new( @@ -1130,7 +1130,7 @@ impl RichTerm { .map(|chunk| match chunk { chunk @ StrChunk::Literal(_) => Ok(chunk), StrChunk::Expr(t, indent) => { - Ok(StrChunk::Expr(t.traverse(f, state, method)?, indent)) + Ok(StrChunk::Expr(t.traverse(f, state, order)?, indent)) } }) .collect(); @@ -1141,17 +1141,19 @@ impl RichTerm { ) }, Term::MetaValue(meta) => { + let mut f_on_type = |ty: Types, s: &mut S| { + match ty.0 { + AbsType::Flat(t) => t.traverse(f, s, order).map(|t| Types(AbsType::Flat(t))), + _ => Ok(ty), + } + }; + let contracts: Result, _> = meta .contracts .into_iter() .map(|ctr| { - let types = match ctr.types { - Types(AbsType::Flat(t)) => { - Types(AbsType::Flat(t.traverse(f, state, method)?)) - } - ty => ty, - }; - Ok(Contract { types, ..ctr }) + let Contract {types, label} = ctr; + types.traverse(&mut f_on_type, state, order).map(|types| Contract { types, label }) }) .collect(); let contracts = contracts?; @@ -1159,19 +1161,14 @@ impl RichTerm { let types = meta .types .map(|ctr| { - let types = match ctr.types { - Types(AbsType::Flat(t)) => { - Types(AbsType::Flat(t.traverse(f, state, method)?)) - } - ty => ty, - }; - Ok(Contract { types, ..ctr }) + let Contract {types, label} = ctr; + types.traverse(&mut f_on_type, state, order).map(|types| Contract { types, label }) }) .transpose()?; let value = meta .value - .map(|t| t.traverse(f, state, method)) + .map(|t| t.traverse(f, state, order)) .map_or(Ok(None), |res| res.map(Some))?; let meta = MetaValue { doc: meta.doc, @@ -1180,6 +1177,7 @@ impl RichTerm { priority: meta.priority, value, }; + RichTerm::new( Term::MetaValue(meta), pos, @@ -1187,7 +1185,7 @@ impl RichTerm { }} else rt }; - match method { + match order { TraverseOrder::TopDown => Ok(result), TraverseOrder::BottomUp => f(result, state), } diff --git a/src/types.rs b/src/types.rs index 2c8953d3..f12885b0 100644 --- a/src/types.rs +++ b/src/types.rs @@ -54,7 +54,7 @@ use crate::error::{ParseError, ParseErrors, TypecheckError}; use crate::identifier::Ident; use crate::term::make as mk_term; -use crate::term::{RichTerm, Term}; +use crate::term::{RichTerm, Term, TraverseOrder}; use crate::{mk_app, mk_fun, mk_switch}; use std::collections::HashMap; use std::fmt; @@ -367,6 +367,73 @@ impl Types { _ => false, } } + + /// Apply a transformation on a whole type by mapping a faillible function `f` on each node in + /// manner as prescribed by the order. + /// `f` may return a generic error `E` and use the state `S` which is passed around. + pub fn traverse( + self: Types, + f: &mut F, + state: &mut S, + order: TraverseOrder, + ) -> Result + where + F: FnMut(Types, &mut S) -> Result, + { + let ty = match order { + TraverseOrder::TopDown => f(self, state)?, + TraverseOrder::BottomUp => self, + }; + + let abs_ty = match ty.0 { + AbsType::Dyn() + | AbsType::Num() + | AbsType::Bool() + | AbsType::Str() + | AbsType::Sym() + | AbsType::Var(_) + | AbsType::RowEmpty() + | AbsType::Flat(_) => Ok(ty.0), + AbsType::Forall(id, ty_inner) => (*ty_inner) + .traverse(f, state, order) + .map(|ty| AbsType::Forall(id, Box::new(ty))), + AbsType::Enum(ty_inner) => (*ty_inner) + .traverse(f, state, order) + .map(Box::new) + .map(AbsType::Enum), + AbsType::StaticRecord(ty_inner) => (*ty_inner) + .traverse(f, state, order) + .map(Box::new) + .map(AbsType::StaticRecord), + AbsType::DynRecord(ty_inner) => (*ty_inner) + .traverse(f, state, order) + .map(Box::new) + .map(AbsType::DynRecord), + AbsType::Array(ty_inner) => (*ty_inner) + .traverse(f, state, order) + .map(Box::new) + .map(AbsType::Array), + AbsType::Arrow(ty1, ty2) => { + let ty1 = (*ty1).traverse(f, state, order)?; + let ty2 = (*ty2).traverse(f, state, order)?; + Ok(AbsType::Arrow(Box::new(ty1), Box::new(ty2))) + } + AbsType::RowExtend(id, ty_row, tail) => { + let ty_row = ty_row + .map(|ty| (*ty).traverse(f, state, order)) + .transpose()?; + let tail = (*tail).traverse(f, state, order)?; + Ok(AbsType::RowExtend(id, ty_row.map(Box::new), Box::new(tail))) + } + }?; + + let result = Types(abs_ty); + + match order { + TraverseOrder::TopDown => Ok(result), + TraverseOrder::BottomUp => f(result, state), + } + } } impl fmt::Display for Types { From f79b407efb5decef130c78e9cf01046b9d8dc475 Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Tue, 22 Mar 2022 19:14:39 +0100 Subject: [PATCH 2/2] Add regression test for #668 (serialization bug) --- tests/pass/serialize.ncl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/pass/serialize.ncl b/tests/pass/serialize.ncl index 366da6d5..1c09f9ef 100644 --- a/tests/pass/serialize.ncl +++ b/tests/pass/serialize.ncl @@ -28,5 +28,10 @@ let assertDeserInv = fun x => bar = ["str", true], baz = {subfoo = true, subbar = 0} }, + + # regression test for issue #668 (https://github.com/tweag/nickel/issues/668) + let base = {foo | {_: {bar | default = 2}}} in + let ext = {foo = {some = {}}} in + assertSerInv (base & ext), ] |> array.foldl (fun x y => (x | Assert) && y) true