Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-exported record fields #1132

Merged
merged 2 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lsp/nls/src/requests/completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,7 @@ mod tests {
contracts: Vec::new(),
},
opt: false,
not_exported: false,
priority: MergePriority::Neutral,
};

Expand Down
2 changes: 2 additions & 0 deletions src/eval/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,8 @@ fn merge_fields<'a, C: Cache, I: DoubleEndedIterator<Item = &'a Ident> + Clone>(
// If one of the record requires this field, then it musn't be optional. The
// resulting field is optional iff both are.
opt: metadata1.opt && metadata2.opt,
// 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,
Comment on lines +435 to +436
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the correct semantics, especially when considering a program like

{
  a = 1, b = 2
} | {
  a | Num,
  b | not_exported
    | Num,
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

priority,
};

Expand Down
5 changes: 5 additions & 0 deletions src/parser/grammar.lalrpop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,10 @@ SimpleFieldAnnotAtom<TypeRule>: FieldMetadata = {
opt: true,
..Default::default()
},
"|" "not_exported" => FieldMetadata {
not_exported: true,
..Default::default()
},
}

// A single field metadata annotation.
Expand Down Expand Up @@ -1009,6 +1013,7 @@ extern {
"doc" => Token::Normal(NormalToken::Doc),
"optional" => Token::Normal(NormalToken::Optional),
"priority" => Token::Normal(NormalToken::Priority),
"not_exported" => Token::Normal(NormalToken::NotExported),

"hash" => Token::Normal(NormalToken::OpHash),
"serialize" => Token::Normal(NormalToken::Serialize),
Expand Down
2 changes: 2 additions & 0 deletions src/parser/lexer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ pub enum NormalToken<'input> {
Priority,
#[token("force")]
Force,
#[token("not_exported")]
NotExported,

#[token("%hash%")]
OpHash,
Expand Down
1 change: 1 addition & 0 deletions src/parser/uniterm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ impl UniRecord {
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
Expand Down
9 changes: 7 additions & 2 deletions src/serialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ where
S: Serializer,
{
let mut entries = record
.iter_without_opts()
.iter_serializable()
.collect::<Result<Vec<_>, _>>()
.map_err(|missing_def_err| {
Error::custom(format!(
Expand Down Expand Up @@ -195,7 +195,7 @@ pub fn validate(format: ExportFormat, t: &RichTerm) -> Result<(), SerializationE
Null => Err(SerializationError::UnsupportedNull(format, t.clone())),
Bool(_) | Num(_) | Str(_) | Enum(_) => Ok(()),
Record(record) => {
record.iter_without_opts().try_for_each(|binding| {
record.iter_serializable().try_for_each(|binding| {
// unwrap(): terms must be fully evaluated before being validated for
// serialization. Otherwise, it's an internal error.
let (_, rt) = binding.unwrap_or_else(|err| panic!("encountered field without definition `{}` during pre-serialization validation", err.id));
Expand Down Expand Up @@ -426,6 +426,11 @@ mod tests {
"{baz | default = {subfoo | default = !false} & {subbar | default = 1 - 1}}",
json!({"baz": {"subfoo": true, "subbar": 0}})
);

assert_json_eq!(
"{a = {b | default = {}} & {b.c | not_exported = false} & {b.d = true}}",
json!({"a": {"b": {"d": true}}})
);
}

#[test]
Expand Down
25 changes: 15 additions & 10 deletions src/term/record.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ pub struct FieldMetadata {
pub annotation: TypeAnnotation,
/// If the field is optional.
pub opt: bool,
/// If the field is serialized.
pub not_exported: bool,
pub priority: MergePriority,
}

Expand Down Expand Up @@ -133,6 +135,7 @@ impl FieldMetadata {
contracts: outer.annotation.contracts,
},
opt: outer.opt || inner.opt,
not_exported: outer.not_exported || inner.not_exported,
priority,
}
}
Expand Down Expand Up @@ -359,22 +362,24 @@ impl RecordData {
})
}

/// Return an iterator over the fields' values, ignoring optional fields without
/// definition. Fields that aren't optional but yet don't have a definition are mapped to the
/// error `MissingFieldDefError`.
pub fn iter_without_opts(
/// 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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about iter_fields_with_value ? 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(late to the party) Naming idea: iter_exported_fields

&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,
}
})
Comment on lines +365 to +382
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/// Get the value of a field. Ignore optional fields without value: trying to get their value
Expand Down