Support Optionals as Optional Parameters in the Constructor #159

znehrenberg-sc · 2023-12-21T00:43:28Z

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 · 2023-12-22T21:29:35Z

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

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?

li-feng-sc · 2023-12-27T03:27:30Z

README.md

+- Optional behavior will be able to be switched via a usage flag for each platform. 
+- A new [deriving method](https://github.com/dropbox/djinni#derived-methods) specifier will be implemented so that individual records can still require all parameters
+
+### Deriving Record


Section title is a bit confusing. How about "Requiring optional parameters in individual records"?

li-feng-sc · 2023-12-27T03:48:31Z

test-suite/generated-src/java/com/dropbox/djinni/test/ClientReturnedRecord.java

    @Nonnull
    public String getContent() {
        return mContent;
    }

+    public void setContent(String content) {


content is not null, we need to guard it like in the constructor, otherwise we opens a loophole to allow nulls slipping into nonnull fields.

We didn't need to guard primitives earlier because they can't be null. But String can.

To be clear, @nonnull should be added to setters of required variables, however it is not necessary for setters of optionals

li-feng-sc · 2023-12-27T03:58:10Z

test-suite/generated-src/objc/DBClientReturnedRecord.h


-@property (nonatomic, readonly, nonnull) NSString * content;
+@property (nonatomic, copy, nonnull) NSString * content;


do we still need to generate [content copy]; in the constructor body now that the property itself is copy? We definitely don't want to double copy the string.

Did some research and TLDR yes.

The initializer sets variables directly and bypasses their properties. Therefore the copy annotation is required in both places.

li-feng-sc · 2023-12-27T04:05:36Z

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 · 2023-12-27T17:28:38Z

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

znehrenberg-sc added 3 commits December 15, 2023 22:47

Initial changes

8d4bd1b

Add new generated files, regenerate standard files, and add tests

f2145e6

Update readme

7feaf08

znehrenberg-sc changed the base branch from main to experimental-record December 22, 2023 20:40

znehrenberg-sc added 3 commits December 22, 2023 12:59

Update after design doc changes

d38123e

Cleanup

1c74239

Further cleanup

bec86b9

li-feng-sc reviewed Dec 27, 2023

View reviewed changes

Address PR Comments

da9e7b4

li-feng-sc approved these changes Dec 27, 2023

View reviewed changes

Missing file generation

ed45568

li-feng-sc merged commit 1025958 into Snapchat:experimental-record Dec 27, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Optionals as Optional Parameters in the Constructor #159

Support Optionals as Optional Parameters in the Constructor #159

znehrenberg-sc commented Dec 21, 2023 •

edited

Loading

znehrenberg-sc commented Dec 22, 2023

li-feng-sc left a comment

li-feng-sc Dec 27, 2023

li-feng-sc Dec 27, 2023

znehrenberg-sc Dec 27, 2023

li-feng-sc Dec 27, 2023

znehrenberg-sc Dec 27, 2023 •

edited

Loading

li-feng-sc commented Dec 27, 2023

znehrenberg-sc commented Dec 27, 2023


		@property (nonatomic, readonly, nonnull) NSString * content;
		@property (nonatomic, copy, nonnull) NSString * content;

Support Optionals as Optional Parameters in the Constructor #159

Support Optionals as Optional Parameters in the Constructor #159

Conversation

znehrenberg-sc commented Dec 21, 2023 • edited Loading

Problem Statement

Changes at a High Level

Legacy Record Behavior

Deriving Record

Omitting Convenience Constructors

Tests

znehrenberg-sc commented Dec 22, 2023

li-feng-sc left a comment

Choose a reason for hiding this comment

li-feng-sc Dec 27, 2023

Choose a reason for hiding this comment

li-feng-sc Dec 27, 2023

Choose a reason for hiding this comment

znehrenberg-sc Dec 27, 2023

Choose a reason for hiding this comment

li-feng-sc Dec 27, 2023

Choose a reason for hiding this comment

znehrenberg-sc Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

li-feng-sc commented Dec 27, 2023

znehrenberg-sc commented Dec 27, 2023

znehrenberg-sc commented Dec 21, 2023 •

edited

Loading

znehrenberg-sc Dec 27, 2023 •

edited

Loading