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

[Feature] Optional fields #815

Merged
merged 3 commits into from
Sep 8, 2022
Merged

[Feature] Optional fields #815

merged 3 commits into from
Sep 8, 2022

Conversation

yannham
Copy link
Member

@yannham yannham commented Aug 31, 2022

Optional fields

Closes #373.

Overview

This PR adds the keyword optional (subject to bikeshed) to the metadata syntax to describe a field that may be filled, but is not required.

The general guideline for the semantics of optional fields is that an optional field without definition should be transparent to all record operations, and serialization. Once it get merged with a definition, or with the same field without definition but that is not optional, it acts like a normal field.

In particular, an empty optional field won't appear when calling to record.fields and will return false when calling to record.has_field. The rationale is that we don't want record.has_field "foo" r to return true on an empty optional field and then have r.foo to fail. We may want functions like fields_all or fields_optional that include empty optional fields later, but I don't see an obvious usage for now (dynamic contract inspection/building, maybe?), so let's wait for someone to actually ask for them.

Deciding "is an empty optional"

The way we handle metadata in Nickel is by nature very dynamic: any value can have metadata, and in principle we can't fetch the full metadata of an expression, or decide properties like "is it defined" without performing a form of evaluation, which can involve arbitrary computations. We can have nested metadata, etc.

This means that deciding "is this field an empty optional" is potentially arbitrary long, and causes the evaluation of the field, which sounds bad. For example, record.fields requires to decide "is empty optional" on each field, to filter the list. It feels more natural to store this information at the field level, as a simple flag, and update upon merge, such that we have a direct access in constant time to this property for any given field.

Solution implemented in this PR

However, this would make optional fields quite special, without a good reason. The same dilemma applies to many other metadata. This is why I think we should have a general reflection for metadata (should they only be allowed on record fields and stored there, basically), instead of special casing optional. In the meantime, this PR uses an heuristics to try to detect quickly an empty optional, which I hope will be more than enough for most concrete cases (indeed, while it may be arbitrarily complex to detect in theory, the natural use of optional fields should stay rather simple). The lack of optional fields have been limiting in several experimentation already, when trying to write contracts for concrete use-cases, so in my humble opinion, it's better than nothing to have them with some limitations now, and potentially make them better later, rather than waiting for a restructuring of metadata.

Prepare the feature of optional fields by adding an `opt` field to the
MetaValue structure (representing metadata + value). This commit doesn't
make use of it, doesn't add the corresponding syntax in the grammar nor
does it patch (as it should) various record functions, etc. It just add
the fields and do the bare minimum of fixes to get the code to compile
and to pass tests. The optional field feature is incomplete and inactive
at this point.
@github-actions github-actions bot temporarily deployed to pull request August 31, 2022 17:14 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 31, 2022 17:24 Inactive
@yannham yannham marked this pull request as draft September 1, 2022 08:14
@yannham yannham marked this pull request as draft September 1, 2022 08:14
@yannham yannham marked this pull request as draft September 1, 2022 08:14
@yannham yannham marked this pull request as draft September 1, 2022 08:14
@github-actions github-actions bot temporarily deployed to pull request September 1, 2022 15:18 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 5, 2022 15:39 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 6, 2022 08:38 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 6, 2022 08:48 Inactive
@yannham yannham changed the title Feature/optional fields [Feature] Optional fields Sep 6, 2022
@yannham yannham marked this pull request as ready for review September 6, 2022 09:32
This commit actally makes use of optional fields, previously added to
metadata. It adds the `optional` keyword in the parser, and patches all
record operations to ignore optional fields that don't have a
definition.

Because of the way metavalues current work, "optional not having a
definition" is not a trivial property to decide. Thus, we approximate it
using simple heuristics, than we hope will be good enough in practice.
@github-actions github-actions bot temporarily deployed to pull request September 6, 2022 09:35 Inactive
@thufschmitt
Copy link
Contributor

It feels a bit weird to have optional be a metadata field and not a property of the structure of the record itself. What's the reason for that? You mention that “this would make optional fields quite special, without a good reason”, but it intuitively seems to me that they have to be a first-class construct for records 🤔

@yannham
Copy link
Member Author

yannham commented Sep 7, 2022

@thufschmitt to be clear, I think having optional be a property of the record itself is eventually the right solution. But to me, it seems that this is not specific to optional: this is the same dilemma for e.g. doc, the priority (default, etc.), possibly the future hidden, serialize_with, custom merge function, etc. The rationale is then not to introduce an ad-hoc parameter for optionals right now, but to do like we've done until now temporarily, and to think about basically moving all metadata but annotations to be attached directly to record fields, at once.

I'm just not 100% convinced the somehow hacky solution of this PR is fine, even temporarily. But still a bit convinced.

@thufschmitt
Copy link
Contributor

Fair point. It's less obvious (to me at least) for stuff like doc, but it makes sense. Let's 👍 this and wait for #819 to have a better representation

@thufschmitt thufschmitt merged commit 4bba9f0 into master Sep 8, 2022
@thufschmitt thufschmitt deleted the feature/optional-fields branch September 8, 2022 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional fields in contracts
3 participants