Non-exported record fields #1132
Conversation
| // The resulting field will be suppressed from serialization if either of the fields to be merged is. | ||
| not_exported: metadata1.not_exported || metadata2.not_exported, |
There was a problem hiding this comment.
This seems to be the correct semantics, especially when considering a program like
{
a = 1, b = 2
} | {
a | Num,
b | not_exported
| Num,
}
There was a problem hiding this comment.
Interesting, because it means it actually gives a way to "remove" fields via merging, at least as far as serialization is concerned. I don't know if it should be used that way, or if it is a problem at all, but just wanted to point it out. It's probably also morally better than plain removal of a field, because you can still query it, see it when not exporting, etc.
| /// Return an iterator over the fields' values, ignoring optional fields | ||
| /// without definition and fields marked as not_exported. Fields that | ||
| /// aren't optional but yet don't have a definition are mapped to the error | ||
| /// `MissingFieldDefError`. | ||
| pub fn iter_serializable( | ||
| &self, | ||
| ) -> impl Iterator<Item = Result<(&Ident, &RichTerm), MissingFieldDefError>> { | ||
| self.fields | ||
| .iter() | ||
| .filter_map(|(id, field)| match field.value { | ||
| Some(ref v) => Some(Ok((id, v))), | ||
| self.fields.iter().filter_map(|(id, field)| { | ||
| debug_assert!(field.pending_contracts.is_empty()); | ||
| match field.value { | ||
| Some(ref v) if !field.metadata.not_exported => Some(Ok((id, v))), | ||
| None if !field.metadata.opt => Some(Err(MissingFieldDefError { | ||
| id: *id, | ||
| metadata: field.metadata.clone(), | ||
| })), | ||
| None => None, | ||
| }) | ||
| _ => None, | ||
| } | ||
| }) |
There was a problem hiding this comment.
This function is only used in src/serialize.rs to decide which fields are serialized. I renamed it to make this more explicit and I added a debug_assert! to ensure that no pending contracts are encountered. This seems to be an important invariant in the codebase: "On accessing record fields, pending contracts are applied". The into_ counterpart into_iter_without_opts of this function is able to apply pending contracts since it takes ownership of the field values. But iter_serializable only works on immutable references, and is called only after fully evaluating a term. Therefore, just asserting that no pending contracts are left seemed to be the most reasonable course of action.
| /// without definition and fields marked as not_exported. Fields that | ||
| /// aren't optional but yet don't have a definition are mapped to the error | ||
| /// `MissingFieldDefError`. | ||
| pub fn iter_serializable( |
There was a problem hiding this comment.
Very nitpicky nitpick, but iter_serializable could make it sound like it only iter through serializable fields (avoiding the one that aren't). I don't have great suggestions unfortunately. It's probably iter_to_serialize but that doesn't sound great either. Feel free to ignore 🙃
There was a problem hiding this comment.
Funnily enough, I had iter_to_serialize in a previous iteration, but it did indeed sound too bad for my tastes. I'm open to suggestions 😄
There was a problem hiding this comment.
How about iter_fields_with_value ? 😅
There was a problem hiding this comment.
How about iter_fields_with_value ? sweat_smile
I fear it might be misleading, but it does iter on fields that don't have values (where value is None). What it does is that it excludes fields without value and with the optional attribute set, together with fields with the not_exported attribute.
There was a problem hiding this comment.
@vkleen it's not a huge deal either, let's not keep this PR open for that. I think iter_serializable is not terrible, iter_to_serialize is also okish. Pick your poison 😛
There was a problem hiding this comment.
I'll leave it the way it is (the path of least resistance 😆). The next time someone touches that code, they can see if another name fits better.
There was a problem hiding this comment.
(late to the party) Naming idea: iter_exported_fields
This PR adds a
not_exportedfield to record field metadata along with syntax to set it to true. The field is set tofalseby default and a metadata annotation in a record likewill set it to
true. Upon merging it infects everything, i.e. when a field marked asnot_exportedis merged with any other field, the result will benot_exported.