[ObjC] Mark NSObject init / new as unavailable for records with any fields

[jgavris]

Blocked by #27 for full test suite, but passes.

jgavris in ~/code/djinni on objc-record-initializers
λ bazel test //test-suite:djinni-java-tests  //test-suite:djinni-objc-tests
INFO: Analyzed 2 targets (0 packages loaded, 0 targets configured).
INFO: Found 2 test targets...
INFO: Elapsed time: 0.371s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
//test-suite:djinni-java-tests                                  (cached) PASSED in 2.7s
//test-suite:djinni-objc-tests                                  (cached) PASSED in 14.9s

Executed 0 out of 2 tests: 2 tests pass.
INFO: Build completed successfully, 1 total action

• Records with non-null fields will be improperly constructed if a default initializer ([NSObject init] or [NSObject new]) is called (on any record with optional fields), which can cause an unexpectedly unwrapped optional nil in Swift code. Mark these as unavailable to hide them from Swift.
• Adds ./ci/generate.sh step as an intermediate check to make sure that all the example test files are 'clean' before running the test suite.

[finalpatch]

Hi @jgavris thanks for the PR. I don't completely understand the proposed changes.

We currently do not generate default (no parameter) init method, this makes sure the caller initializes records with proper initial values.

Are you proposing that we should add a default init method, but hide it from swift if it has nonnull members? Wouldn't this still expose potential invalid objects to ObjC uses?

[finalpatch]

My main concern is that this is a breaking change and it may break some existing code. However, if I understand correctly, if any code is broken by this change, then that code itself is problematic and should be fixed.

[jgavris]

There will likely be code that is 'accidentally' using the NSObject constructors (ex; TSXItemList() in Swift) and will have to migrate to the designated initializer, but they are not safe usages currently, no.

[finalpatch]

looks good once you re-run codegen in the other directories.

[finalpatch]

@jgavris do you still intend to merge this PR? Like I said above, we need to re-run codegen in all sub-project directories and fix the documentation otherwise I think this is good to go.

[jgavris]

@finalpatch Yes, thanks for the feedback. I was going to suggest adding your comment in #28 (comment) to the CI workflow (and test step that ensures they are not dirty), so that future contributions are 'clean'.

[finalpatch]

@finalpatch Yes, thanks for the feedback. I was going to suggest adding your comment in #28 (comment) to the CI workflow (and test step that ensures they are not dirty), so that future contributions are 'clean'.

Thanks. Yeah it's a bit annoying we have to check in the codegen changes. I'm not sure if the CI workflow is the right place for it. We are exploring way to run the codegen as a part of the bazel build process, hopefully this will make the manual codegen step no longer necessary.

[jgavris]

We are exploring way to run the codegen as a part of the bazel build process, hopefully this will make the manual codegen step no longer necessary.

SGTM. It's a bit tricky to get Bazel to modify / generate files as part of the build https://shantanugoel.com/2020/05/03/bazel-rule-auto-generate-files-compile-time/