Support Optionals as Optional Parameters in the Constructor

znehrenberg-sc commented 9 months ago

Problem Statement

Required optionals within the Djinni constructors presents a twofold problem.

Firstly, the developer experience when interacting with Djinni is inconsistent. Optionals are not required in typescript constructors, but are required in all other constructors. Fields are settable in C++ and typescript, but not in Java and Objective C. We should strive to have a consistent developer experience when working with optionals across any language-barrier code.

Secondly, as records become larger and used in more places, each additional optional parameter creates an increasingly problematic cascade of build errors. This can be partly mitigated by the use of builders for individual records, but builders still require updating with every record change, whether optional or not.

Changes to heavily touched records delay projects by cascades of changes required to support any single Djinni record change. These cascading changes should not be required. This PR does the following:

Changes at a High Level

By default, records are mutable
By default, optionals will be omitted from the constructor in Djinni
The Djinni code generator will generate two constructors for a record with optionals: one with all values and one with optionals omitted. This will minimize code disruption
Legacy behavior can be returned via a generation flag for each platform
All mutable parameters will have the copy property for objC
A new deriving method specifier will be implemented so that individual records can still require all parameters

Legacy Record Behavior

The following compiler flags will return record behavior to legacy (readonly, all parameters required) for C++/ObjC/Java:

--cpp-legacy-records
--java-legacy-records
--objc-legacy-records

Deriving Record

Any record can be made to have all parameters be required by specifying it as a req deriving record:

MyClass = record {
  required: string;
  optional: optional<string>;
} deriving(req)

Omitting Convenience Constructors

Extra convenience constructors which require all parameters can be removed from optional ObjC records with a new compiler flag:

--objc-omit-full-convenience-constructor

Tests

New tests are added to ensure optional behavior is correct. These tests generate new Djinni records which show how code can be generated for different permutations of optional and required parameters in a Djinni record.

Most changes in this PR come from new record file generation, and current optional record updates.

znehrenberg-sc commented 9 months ago

Regeneration of Djinni records causes most of the changes in this PR. The actual changes are in the following:

README.md
src/source/*
test-suite/djinni/optionals.djinni
test-suite/handwritten-src/*
test-suit/run_djinni.sh

li-feng-sc commented 9 months ago

The test failure is because we have a CI step that checks when code generator is modified, all generated code is regenerated in test suite, example and benchmark.

znehrenberg-sc commented 9 months ago

Let a few comments. The main things to look again is

Java setters need to respect nonnull

Check whether we are double copying mutable objc members

Currently Composer is still an internal project so no need to mention it in the PR description.

I also don't see any changes in the TypeScript generator, does that mean our TS output is already good?

For the last point, yes typescript already operates like we want. When the parameter is marked as optional, it isn't required for initialization

Snapchat / djinni