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

feat: Field extensions/updates to existing objects #3084

Merged
merged 135 commits into from Aug 23, 2022
Merged

Conversation

ggreif
Copy link
Contributor

@ggreif ggreif commented Jan 29, 2022

Related to #3072.

Functional record updates

This adds the ability to overwrite a set of fields (in a potentially type-changing manner) relative to a (set of) base(s).

The syntax is

{ base_1 and <more-bases> with a = val; <more-fields> }

The resulting object has type { a : val_ty; <more-fields> } and base_1_ty and <more-bases_ty> as long as the bases and the new fields are field-disjoint. In general the overriding fields are removed from the bases and the resulting type is formed from the overwritten fields' types plus the rump bases' field types.

The full syntax is a Swiss army knife of thing, allowing

  • record construction from fields
  • object field extension
  • object field type changing
  • object concatenation
  • (when opted-in with --experimental-field-aliasing) ability to create entangled var fields (internally: aliased mutable cells).

Some fine points

  • When the field list is omitted, { base_1 and base_2 and base_3 } syntax is object concatenation, analogous to the type-level and operator.
  • The and operator (when surrounded by braces) puts the bases into object context. This is a different flavour of product construction from the Bool (monoidal) operation (also a product).
  • When any two base is overlapping in a common field, then an explicit overwrite of that field is required to remove any ambiguity.
  • [EXPERIMENTAL, --experimental-field-aliasing must be specified] All non-overwritten var fields from bases will show up in the resulting object as var fields that alias the original field from its base. This behaviour can be disabled by adding an overwrite base with var field = base.field. Note that this is analogous to the aliasing behaviour of shared [var] arrays.

TODOs

  • how to exclude { in {} } in the parser? FIXED: 47ebf5a
  • do we want to exclude semi in { a = 25; in { b = 42 } }? Yes, FIXED: fbf5ade
  • field replacement and subtyping?
  • IN base=exp(ob) is too permitting (future?) — FIXED: 3120aa1
  • multi-value fetching from base?
  • documentation
  • constness inference should see bases
  • compiler tests for var field aliasing (works in interpreter)
  • effect ordering: fields vs. bases
  • consider module { M and N }; where M, N are modules
  • tests with await
  • bases that have bounded polymorphic types (see f4d8bbe and Type.as_obj crashes on bases #3405)
  • add tests ruling out Text etc. (the special member hacks) — See https://github.com/dfinity/motoko/pull/3084/files#r947909810, however
  • consolidate RefD to prim + VarD — write issue!
  • Changelog.md
  • Check interaction with type fields (claudio)
  • type fields aren't ambiguous when they are the same
  • don't let values overwrite types
  • test { t : type } and { t : Val } — what to do? They live in different namespaces!

@ggreif ggreif added DO-NOT-MERGE feature New feature or request language design Requires design work P2 medium priority, resolve within a couple of milestones typing Involves the type system labels Jan 29, 2022
@crusso
Copy link
Contributor

crusso commented Aug 22, 2022

Ok, I propose the following compromise.

We allow bases to have mutable fields but require them to be overridden in the with clause, with no support for aliasing.

If users complain that this is not enough, we can relax the requirement and introduce the aliasing semantics (or even implicit shallow copy, if that's what users actually expect). Any future relaxation would not be a breaking change.

I'm fine with the syntax as is - it's a bit late to complain about it.

@rossberg @nomeata sound reasonable?

@nomeata and I discussed this:

me: Gabor has implemented my suggestion of requiring users to override inherited var fields, preventing aliasing (in a way that we can relax later, sans breaking anyone). For or against? Indifferent?
nomeata: For. It means the syntax is just simple sugar around a plain record construction, with the inherited fields just copying values out of the appropriate parent, right?
me: Yep, but you need to be explicit about what to do with var fields, the only option being redefining them (or forgetting them via prior annotation)
nometa: Right

ggreif and others added 3 commits August 22, 2022 16:51
Make mutable field aliasing (when extending/merging records) an experimental opt-in feature, and hide it behind a feature flag. When the flag is not supplied, an error is emitted that instructs the user to overwrite all `var` fields from bases explicitly. When `--experimental-field-aliasing` is given no such error gets emitted, and the mutable cells of bases are aliased in the resulting record.

See discussion in #3084 why this is a necessity.

Also contains a few minor tweaks.

Co-authored-by: Claudio Russo <claudio@dfinity.org>
src/ir_def/ir.ml Outdated Show resolved Hide resolved
@ggreif ggreif changed the title Field extensions/updates to existing objects feat: Field extensions/updates to existing objects Aug 23, 2022
test/run/object-extend.mo Outdated Show resolved Hide resolved
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

See doc changes and #3417

@crusso crusso mentioned this pull request Aug 23, 2022
(The lowering phase also need to promote the type of bases, not just typing. Infer_exp_promote doesn't promote the annotation on the expression, so you need to do it manually in lowering again).

Fixes #3405.
doc/md/language-manual.md Outdated Show resolved Hide resolved
Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

But don't forget my suggestion to the doc. You can also put the other wording in an html comment for preservation.

ggreif and others added 2 commits August 23, 2022 21:15
This is now an opt-in experimental feature

Co-authored-by: Claudio Russo <claudio@dfinity.org>
Co-authored-by: Claudio Russo <claudio@dfinity.org>
@ggreif ggreif added the automerge-squash When ready, merge (using squash) label Aug 23, 2022
@mergify mergify bot merged commit f3e9136 into master Aug 23, 2022
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Aug 23, 2022
@rossberg
Copy link
Contributor

The restriction makes sense to me.

Still curious why you had to make this construct so complex, though. What can it express that a simple binary one can not?

@ggreif
Copy link
Contributor Author

ggreif commented Aug 24, 2022

Still curious why you had to make this construct so complex, though. What can it express that a simple binary one can not?

@rossberg a few reasons:

  • type-level and is unbiased, it is nice to have an unbiased value-level construct too
  • a binary with would have different performance characteristics when applied to three or more bases (depending on choice of association and size of the bases)
  • iterated binary merges would also create unnecessary garbage in heap
  • { base with field = val } has exactly the same character count as base with { field = val }

@ggreif ggreif deleted the gabor/record-ext branch August 24, 2022 16:00
@crusso
Copy link
Contributor

crusso commented Aug 24, 2022

The restriction makes sense to me.

Still curious why you had to make this construct so complex, though. What can it express that a simple binary one can not?

@rossberg Isn't the simplest example one where there are two fields that clash and you want to take the first from the left, but the second from the right record? Though I guess you could forget one field using subtyping before doing the merge (or, perhaps better, add a \ x operator for removing fields by name).

I agree the complexity is a bit high, especially when you try to describe it in prose.... The binary version isn't that much better in the sense that it still breaks subject reduction and can (equally) only be described as a type based elaboration.

@rossberg
Copy link
Contributor

@crusso:

Isn't the simplest example one where there are two fields that clash and you want to take the first from the left, but the second from the right record?

True, although you can achieve that with a type constraint or more orthogonal means, as you say. In general, bidirectional merging is only practically useful if you have actual mixins (records with abstract fields), short of that, it may not justify the extra complexity.

@ggreif, the performance and garbage arguments are a bit unconvincing. It should be fairly easy for a compiler to syntactically recognise the cases that correspond to the complex construct and compile them the same way.

As for syntax and symmetry, I'd have thought that the obvious incoherence between A and B on the type level and {A and B} on the term level would be more concerning. ;)

@nomeata
Copy link
Collaborator

nomeata commented Aug 25, 2022

I am confused to still see IR changes here. I thought the final design did not introduce aliasing of mutable fields? I understand that maybe we’ll get some form of aliasing in the future, but until then it’s dead code and hence dangerous. (unless I am missing something).

Maybe revert it in a separate PR, where we can easily take it from if we need it again?

@ggreif
Copy link
Contributor Author

ggreif commented Aug 25, 2022

@nomeata The aliasing feature is still available, but behind an opt-in flag --experimental-field-aliasing, which we don't disclose in the changelog. It is being tested. I left it in so that people can play with it, but at the time we retain all the options. But we certainly are thinking about (maybe second-class) mutable references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature New feature or request language design Requires design work opportunity More optimisation opportunities inside P2 medium priority, resolve within a couple of milestones typing Involves the type system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants