tweag/nickel
1
1
Fork 0
mirror of https://github.com/tweag/nickel.git synced 2024-10-06 08:07:37 +03:00
Code Issues Projects Releases Wiki Activity

Fix record.update by making record.insert act consistently (#1669)

Browse Source 
* Add variant of has_field that considers non-empty opt ones

* Extend record_op_kind to insertion and deletion

* Fix compilation errors

* Add regression test

* Pretty-printing of record operator variants

* Formatting

* Update core/src/term/mod.rs

Co-authored-by: jneem <joeneeman@gmail.com>

---------

Co-authored-by: jneem <joeneeman@gmail.com>
This commit is contained in:
Yann Hamdaoui 2023-10-10 09:38:26 +02:00 committed by GitHub
parent 91d63ed611
commit 94c25735d4
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 117 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
core/src/eval/mod.rs
View File

@ -85,7 +85,8 @@ use crate::{
array::ArrayAttrs, array::ArrayAttrs,
make as mk_term, make as mk_term,
record::{Field, RecordData}, record::{Field, RecordData},
BinaryOp, BindingType, LetAttrs, RichTerm, RuntimeContract, StrChunk, Term, UnaryOp, BinaryOp, BindingType, LetAttrs, RecordOpKind, RichTerm, RuntimeContract, StrChunk, Term,
UnaryOp,
}, },
transform::Closurizable, transform::Closurizable,
}; };
@ -608,6 +609,7 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
metadata: metadata.clone(), metadata: metadata.clone(),
pending_contracts: pending_contracts.clone(), pending_contracts: pending_contracts.clone(),
ext_kind, ext_kind,
op_kind: RecordOpKind::ConsiderAllFields,
}, },
name_as_term.clone(), name_as_term.clone(),
acc, acc,

30
core/src/eval/operation.rs
View File

@ -30,8 +30,7 @@ use crate::{
make as mk_term, make as mk_term,
record::{self, Field, FieldMetadata, RecordData}, record::{self, Field, FieldMetadata, RecordData},
string::NickelString, string::NickelString,
BinaryOp, CompiledRegex, IndexMap, MergePriority, NAryOp, Number, RecordExtKind, RichTerm, *,
RuntimeContract, SharedTerm, StrChunk, Term, UnaryOp,
}, },
transform::Closurizable, transform::Closurizable,
}; };
@ -1661,6 +1660,7 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
metadata, metadata,
pending_contracts, pending_contracts,
ext_kind, ext_kind,
op_kind,
} => { } => {
if let Term::Str(id) = &*t1 { if let Term::Str(id) = &*t1 {
match_sharedterm! {t2, with { match_sharedterm! {t2, with {
@ -1691,9 +1691,9 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
LocIdent::from(id), LocIdent::from(id),
Field { value, metadata, pending_contracts } Field { value, metadata, pending_contracts }
) { ) {
//TODO: what to do on insertion where an empty optional field Some(t) if matches!(op_kind, RecordOpKind::ConsiderAllFields)
//exists? Temporary: we fail with existing field exception || !t.is_empty_optional() =>
Some(t) => Err(EvalError::Other(format!( Err(EvalError::Other(format!(
"record_insert: \ "record_insert: \
tried to extend a record with the field {id}, \ tried to extend a record with the field {id}, \
but it already exists" but it already exists"
@ -1712,18 +1712,18 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
Err(mk_type_error!("record_insert", "String", 1, t1, pos1)) Err(mk_type_error!("record_insert", "String", 1, t1, pos1))
} }
} }
BinaryOp::DynRemove() => match_sharedterm! {t1, with { BinaryOp::DynRemove(op_kind) => match_sharedterm! {t1, with {
Term::Str(id) => match_sharedterm! {t2, with { Term::Str(id) => match_sharedterm! {t2, with {
Term::Record(record) => { Term::Record(record) => {
let mut fields = record.fields; let mut fields = record.fields;
let fetched = fields.remove(&LocIdent::from(&id)); let fetched = fields.remove(&LocIdent::from(&id));
match fetched { if fetched.is_none()
None || matches!((op_kind, fetched), (RecordOpKind::IgnoreEmptyOpt, Some(Field {
| Some(Field {
value: None, value: None,
metadata: FieldMetadata { opt: true, ..}, metadata: FieldMetadata { opt: true, ..},
.. ..
}) => match record.sealed_tail.as_ref() { }))) {
match record.sealed_tail.as_ref() {
Some(t) if t.has_dyn_field(&id) => Some(t) if t.has_dyn_field(&id) =>
Err(EvalError::IllegalPolymorphicTailAccess { Err(EvalError::IllegalPolymorphicTailAccess {
action: IllegalPolymorphicTailAction::RecordRemove { action: IllegalPolymorphicTailAction::RecordRemove {
@ -1745,7 +1745,8 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
pos_op, pos_op,
)), )),
} }
_ => { }
else {
Ok(Closure { Ok(Closure {
body: RichTerm::new( body: RichTerm::new(
Term::Record(RecordData { fields, ..record }), Term::Record(RecordData { fields, ..record }),
@ -1755,22 +1756,21 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
}) })
} }
} }
}
} else { } else {
Err(mk_type_error!("record_remove", "Record", 2, t2, pos2)) Err(mk_type_error!("record_remove", "Record", 2, t2, pos2))
} }
}, }
} else { } else {
Err(mk_type_error!("record_remove", "String", 1, t1, pos1)) Err(mk_type_error!("record_remove", "String", 1, t1, pos1))
} }
}, },
BinaryOp::HasField() => match_sharedterm! {t1, with { BinaryOp::HasField(op_kind) => match_sharedterm! {t1, with {
Term::Str(id) => { Term::Str(id) => {
if let Term::Record(record) = &*t2 { if let Term::Record(record) = &*t2 {
Ok(Closure::atomic_closure(RichTerm::new( Ok(Closure::atomic_closure(RichTerm::new(
Term::Bool(matches!( Term::Bool(matches!(
record.fields.get(&LocIdent::from(id.into_inner())), record.fields.get(&LocIdent::from(id.into_inner())),
Some(field) if !field.is_empty_optional() Some(field) if matches!(op_kind, RecordOpKind::ConsiderAllFields) || !field.is_empty_optional()
)), )),
pos_op_inh, pos_op_inh,
))) )))

16
core/src/parser/grammar.lalrpop
View File

@ -856,7 +856,8 @@ BOpPre: BinaryOp = {
"unseal" => BinaryOp::Unseal(), "unseal" => BinaryOp::Unseal(),
"seal" => BinaryOp::Seal(), "seal" => BinaryOp::Seal(),
"go_field" => BinaryOp::GoField(), "go_field" => BinaryOp::GoField(),
"has_field" => BinaryOp::HasField(), "has_field" => BinaryOp::HasField(RecordOpKind::IgnoreEmptyOpt),
"has_field_all" => BinaryOp::HasField(RecordOpKind::ConsiderAllFields),
"elem_at" => BinaryOp::ArrayElemAt(), "elem_at" => BinaryOp::ArrayElemAt(),
"hash" => BinaryOp::Hash(), "hash" => BinaryOp::Hash(),
"serialize" => BinaryOp::Serialize(), "serialize" => BinaryOp::Serialize(),
@ -868,8 +869,16 @@ BOpPre: BinaryOp = {
ext_kind: RecordExtKind::WithValue, ext_kind: RecordExtKind::WithValue,
metadata: Default::default(), metadata: Default::default(),
pending_contracts: Default::default(), pending_contracts: Default::default(),
op_kind: RecordOpKind::IgnoreEmptyOpt,
}, },
"record_remove" => BinaryOp::DynRemove(), "record_insert_all" => BinaryOp::DynExtend {
ext_kind: RecordExtKind::WithValue,
metadata: Default::default(),
pending_contracts: Default::default(),
op_kind: RecordOpKind::ConsiderAllFields,
},
"record_remove" => BinaryOp::DynRemove(RecordOpKind::IgnoreEmptyOpt),
"record_remove_all" => BinaryOp::DynRemove(RecordOpKind::ConsiderAllFields),
"label_with_message" => BinaryOp::LabelWithMessage(), "label_with_message" => BinaryOp::LabelWithMessage(),
"label_with_notes" => BinaryOp::LabelWithNotes(), "label_with_notes" => BinaryOp::LabelWithNotes(),
"label_append_note" => BinaryOp::LabelAppendNote(), "label_append_note" => BinaryOp::LabelAppendNote(),
@ -1051,7 +1060,9 @@ extern {
"record_map" => Token::Normal(NormalToken::RecordMap), "record_map" => Token::Normal(NormalToken::RecordMap),
"record_empty_with_tail" => Token::Normal(NormalToken::RecordEmptyWithTail), "record_empty_with_tail" => Token::Normal(NormalToken::RecordEmptyWithTail),
"record_insert" => Token::Normal(NormalToken::RecordInsert), "record_insert" => Token::Normal(NormalToken::RecordInsert),
"record_insert_all" => Token::Normal(NormalToken::RecordInsertAll),
"record_remove" => Token::Normal(NormalToken::RecordRemove), "record_remove" => Token::Normal(NormalToken::RecordRemove),
"record_remove_all" => Token::Normal(NormalToken::RecordRemoveAll),
"record_seal_tail" => Token::Normal(NormalToken::RecordSealTail), "record_seal_tail" => Token::Normal(NormalToken::RecordSealTail),
"record_unseal_tail" => Token::Normal(NormalToken::RecordUnsealTail), "record_unseal_tail" => Token::Normal(NormalToken::RecordUnsealTail),
"seq" => Token::Normal(NormalToken::Seq), "seq" => Token::Normal(NormalToken::Seq),
@ -1067,6 +1078,7 @@ extern {
"lookup_type_variable" => Token::Normal(NormalToken::LookupTypeVar), "lookup_type_variable" => Token::Normal(NormalToken::LookupTypeVar),
"has_field" => Token::Normal(NormalToken::HasField), "has_field" => Token::Normal(NormalToken::HasField),
"has_field_all" => Token::Normal(NormalToken::HasFieldAll),
"map" => Token::Normal(NormalToken::Map), "map" => Token::Normal(NormalToken::Map),
"generate" => Token::Normal(NormalToken::ArrayGen), "generate" => Token::Normal(NormalToken::ArrayGen),
"elem_at" => Token::Normal(NormalToken::ElemAt), "elem_at" => Token::Normal(NormalToken::ElemAt),

6
core/src/parser/lexer.rs
View File

@ -221,8 +221,12 @@ pub enum NormalToken<'input> {
RecordMap, RecordMap,
#[token("%record_insert%")] #[token("%record_insert%")]
RecordInsert, RecordInsert,
#[token("%record_insert_all%")]
RecordInsertAll,
#[token("%record_remove%")] #[token("%record_remove%")]
RecordRemove, RecordRemove,
#[token("%record_remove_all%")]
RecordRemoveAll,
#[token("%record_empty_with_tail%")] #[token("%record_empty_with_tail%")]
RecordEmptyWithTail, RecordEmptyWithTail,
#[token("%record_seal_tail%")] #[token("%record_seal_tail%")]
@ -248,6 +252,8 @@ pub enum NormalToken<'input> {
#[token("%has_field%")] #[token("%has_field%")]
HasField, HasField,
#[token("%has_field_all%")]
HasFieldAll,
#[token("%map%")] #[token("%map%")]
Map, Map,
#[token("%elem_at%")] #[token("%elem_at%")]

33
core/src/term/mod.rs
View File

@ -1346,6 +1346,19 @@ pub enum RecordExtKind {
WithoutValue, WithoutValue,
} }
/// A flavor for record operations. By design, we want empty optional values to be transparent for
/// record operations, because they would otherwise make many operations fail spuriously (e.g.
/// trying to map over such an empty value). So they are most of the time silently ignored.
///
/// However, it's sometimes useful and even necessary to take them into account. This behavior is
/// controlled by [RecordOpKind].
#[derive(Clone, Debug, PartialEq, Eq, Copy, Default)]
pub enum RecordOpKind {
#[default]
IgnoreEmptyOpt,
ConsiderAllFields,
}
/// Primitive binary operators /// Primitive binary operators
#[derive(Clone, Debug, PartialEq)] #[derive(Clone, Debug, PartialEq)]
pub enum BinaryOp { pub enum BinaryOp {
@ -1418,16 +1431,17 @@ pub enum BinaryOp {
metadata: FieldMetadata, metadata: FieldMetadata,
pending_contracts: Vec<RuntimeContract>, pending_contracts: Vec<RuntimeContract>,
ext_kind: RecordExtKind, ext_kind: RecordExtKind,
op_kind: RecordOpKind,
}, },
/// Remove a field from a record. The field name is given as an arbitrary Nickel expression. /// Remove a field from a record. The field name is given as an arbitrary Nickel expression.
DynRemove(), DynRemove(RecordOpKind),
/// Access the field of record. The field name is given as an arbitrary Nickel expression. /// Access the field of record. The field name is given as an arbitrary Nickel expression.
DynAccess(), DynAccess(),
/// Test if a record has a specific field. /// Test if a record has a specific field.
HasField(), HasField(RecordOpKind),
/// Concatenate two arrays. /// Concatenate two arrays.
ArrayConcat(), ArrayConcat(),
@ -1510,10 +1524,19 @@ impl fmt::Display for BinaryOp {
ApplyContract() => write!(f, "apply_contract"), ApplyContract() => write!(f, "apply_contract"),
Unseal() => write!(f, "unseal"), Unseal() => write!(f, "unseal"),
GoField() => write!(f, "go_field"), GoField() => write!(f, "go_field"),
DynExtend { .. } => write!(f, "record_insert"), DynExtend {
DynRemove() => write!(f, "record_remove"), op_kind: RecordOpKind::IgnoreEmptyOpt,
..
} => write!(f, "record_insert"),
DynExtend {
op_kind: RecordOpKind::ConsiderAllFields,
..
} => write!(f, "record_insert_all"),
DynRemove(RecordOpKind::IgnoreEmptyOpt) => write!(f, "record_remove"),
DynRemove(RecordOpKind::ConsiderAllFields) => write!(f, "record_remove_all"),
DynAccess() => write!(f, "dyn_access"), DynAccess() => write!(f, "dyn_access"),
HasField() => write!(f, "has_field"), HasField(RecordOpKind::IgnoreEmptyOpt) => write!(f, "has_field"),
HasField(RecordOpKind::ConsiderAllFields) => write!(f, "has_field_all"),
ArrayConcat() => write!(f, "array_concat"), ArrayConcat() => write!(f, "array_concat"),
ArrayElemAt() => write!(f, "elem_at"), ArrayElemAt() => write!(f, "elem_at"),
Merge(_) => write!(f, "merge"), Merge(_) => write!(f, "merge"),

17
core/src/transform/desugar_destructuring.rs
View File

@ -33,9 +33,12 @@
use crate::destructuring::{FieldPattern, Match, RecordPattern}; use crate::destructuring::{FieldPattern, Match, RecordPattern};
use crate::identifier::LocIdent; use crate::identifier::LocIdent;
use crate::match_sharedterm; use crate::match_sharedterm;
use crate::term::make::{op1, op2}; use crate::term::{
use crate::term::{BinaryOp::DynRemove, RichTerm, Term, TypeAnnotation, UnaryOp::StaticAccess}; make::{op1, op2},
use crate::term::{BindingType, LetAttrs}; BinaryOp::DynRemove,
BindingType, LetAttrs, RecordOpKind, RichTerm, Term, TypeAnnotation,
UnaryOp::StaticAccess,
};
/// Entry point of the patterns desugaring. /// Entry point of the patterns desugaring.
/// It desugar a `RichTerm` if possible (the term is a let pattern or a function with patterns in /// It desugar a `RichTerm` if possible (the term is a let pattern or a function with patterns in
@ -156,9 +159,11 @@ fn bind_open_field(x: LocIdent, pat: &RecordPattern, body: RichTerm) -> RichTerm
Term::Let( Term::Let(
var, var,
matches.iter().fold(Term::Var(x).into(), |x, m| match m { matches.iter().fold(Term::Var(x).into(), |x, m| match m {
Match::Simple(i, _) | Match::Assign(i, _, _) => { Match::Simple(i, _) | Match::Assign(i, _, _) => op2(
op2(DynRemove(), Term::Str((*i).into()), x) DynRemove(RecordOpKind::default()),
} Term::Str((*i).into()),
x,
),
}), }),
body, body,
Default::default(), Default::default(),

4
core/src/typecheck/operation.rs
View File

@ -295,7 +295,7 @@ pub fn get_bop_type(
) )
} }
// forall a. Str -> { _ : a } -> { _ : a} // forall a. Str -> { _ : a } -> { _ : a}
BinaryOp::DynRemove() => { BinaryOp::DynRemove(_) => {
let res = state.table.fresh_type_uvar(var_level); let res = state.table.fresh_type_uvar(var_level);
( (
mk_uniftype::str(), mk_uniftype::str(),
@ -304,7 +304,7 @@ pub fn get_bop_type(
) )
} }
// forall a. Str -> {_: a} -> Bool // forall a. Str -> {_: a} -> Bool
BinaryOp::HasField() => { BinaryOp::HasField(_) => {
let ty_elt = state.table.fresh_type_uvar(var_level); let ty_elt = state.table.fresh_type_uvar(var_level);
( (
mk_uniftype::str(), mk_uniftype::str(),

15
core/tests/integration/pass/records/record_insert.ncl Normal file
View File

@ -0,0 +1,15 @@
# test.type = 'pass'
# regression test for record insertion and record removal not being consistent
# with respect to optional fields
let my_insert = fun field content r =>
let r =
if %has_field% field r then
%record_remove% field r
else
r
in
%record_insert% field r content
in
let foo | { value | optional, .. } = { bar = "hello" } in
my_insert "value" "world" foo == { bar = "hello", value = "world" }