Support Bindgen opt-in list #6639

elle-j · 2023-05-17T10:00:13Z

What, How & Why?

Adds initial support for an opt-in list for the Bindgen spec. This allows SDKs to use only some of the classes and records/structs of the general spec and generate bindings only for the items in the opt-in list.

Added Behavior:

Command option
- The realm-bindgen command now accepts an optional --opt-in option specifying the path of the opt-in list.
- If omitted, none of the bound methods or fields will be marked as opted in to.
- Examples (showing only the opt-in option):

# Command line
realm-bindgen --opt-in path/to/opt/in/spec.yml

# CMake
bindgen(OPTIN path/to/opt/in/spec.yml)

Spec/yaml opt-in file
- The opt-in file should list:
  - The records along with their fields (names).
  - The classes along with their methods (unique names).
- Example:

records:
  Property:
    fields:
      - name
      - public_name
      - type
  ObjectSchema:
    fields:
      - name
      - persisted_properties
      - computed_properties

classes:
  Results:
    methods:
      - from_table
      - is_valid
      - sort_by_names
  Realm:
    methods:
      - get_shared_realm
      - config
      - begin_transaction

SDK Requirements:

Applying the opt-in list
- The SDK's generate() function will receive a BoundSpec containing all of the entries of the general spec.
- Since the SDK may add its own custom methods/fields onto the BoundSpec, the SDK is responsible for calling the instance method BoundSpec.applyOptInList() if an opt-in list was provided.
- Once applied, the methods and fields on the BoundSpec that also appear in the opt-in list will have the property isOptedInTo = true, otherwise it will be set to false.
- (See usage examples below.)

Updated Type:

The SDK's generate() function accepts an object as an argument which now also contains the BoundSpec.
- The object properties have been renamed:

// Argument passed to `generate()`
export type TemplateContext = {
  rawSpec: Spec;      // Previously called `spec`
  spec: BoundSpec;    // Newly added
  file: (path: string, formatter?: Formatter) => Outputter;  // Unchanged
};

SDK Usage Examples:

Example using CMake:

When calling the bindgen() function for each of your templates in CMakeLists.txt, pass the path of the opt-in list.

set(OPT_IN_FILE ${SDK_DIR}/path/to/opt/in/spec.yml)

bindgen(
    # ...
    OPTIN ${OPT_IN_FILE}
)

Example of the `generate()` function:

The methods and fields of the BoundSpec have an isOptedInTo boolean property that the consumer can handle accordingly.
- TypeScript example:

export function generate({ rawSpec, spec, file }: TemplateContext): void {
  const out = file("output/file/path");
  // ...
  spec.applyOptInList();
  for (const cls of spec.classes) {
    // ...
    for (const method of cls.methods) {
      if (!method.isOptedInTo) {
        out(`/** @deprecated */`); // Or: continue;
      }
      // ...
    }
  }
}

For a full example of how Realm JS applied the opt-in list, see Realm JS PR #5820.

Potential Additions in Upcoming Iterations:

Opt in to all methods/fields of a class/record without listing them.
- Current behavior:
  - Can only opt in to methods and fields. So in order to opt in to all methods of a class, you'd need to explicitly list them all.
- Alternative:
  - Provide an easy way to indicate that all of its methods/fields are opted in to.
Opt out of an entire class/record.
- Current behavior:
  - Even if a class/record is omitted from the opt-in list, it will still be generated, but w/o its methods/fields.
- Alternative:
  - Provide isOptedInTo on the bound class/record itself so that it can be assigned false if omitted.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

papafe · 2023-05-17T10:47:00Z

@elle-j Just to be sure, with this we are giving a way for sdk to define which part of the whole spec they need? So instead of creating bindings for everything it will create bindings only for methods/properties in the opt-in file?

RedBeard0531

nice work

bindgen/src/bound-model.ts

RedBeard0531 · 2023-05-17T10:37:32Z

bindgen/src/bound-model.ts

+  }
+
+  addField(field: Field) {
+    assert(!(field.name in this._fields), `Duplicate field name on record/struct '${this.name}': '${field.name}'.`);


I'm pretty sure it is impossible[1] to express duplicate fields in the spec file syntax right now, but I suppose it is fine to leave it incase that changes.

[1] You could have duplicates in the spec.yaml file, but the yaml spec requires that keys in a mapping are unique, so such a file isn't valid yaml. And even if the yaml parser doesn't detect that, since it parses into a Plain Old JS Object, any later field will completely replace the prior duplicate, so by the time you get to this code, you won't have any trace of the in-file duplication.

Good point!

bindgen/src/context.ts

bindgen/src/program.ts

bindgen/src/spec/model.ts

bindgen/src/bound-model.ts

elle-j · 2023-05-17T11:25:56Z

So instead of creating bindings for everything it will create bindings only for methods/properties in the opt-in file?

@papafe, yes that's correct. I've added this to the description now, thanks!

kraenhansen · 2023-05-17T12:54:44Z

My initial assumption, was that the part of the spec not opted into, would be completely left out of the parsed spec, this would make it simpler for the generator as it doesn't have to check for the isOptedInTo.
I'm curious if you thought about that approach and if yes, why you didn't choose to go that route?

I'm curious if you actively choose "opt-in" instead of "allow", "include" or "expose"?
I personally think isAllowed, isIncluded or isExposed might make equal amount of sense and be more direct.

elle-j · 2023-05-17T13:39:24Z

bindgen/src/generator.ts

@@ -16,12 +16,13 @@
 //
 ////////////////////////////////////////////////////////////////////////////

-import { Spec } from "./spec";
+import { OptInSpec, Spec } from "./spec";


Starting a thread here to facilitate discussing this previous comment from @kraenhansen:

(A) My initial assumption, was that the part of the spec not opted into, would be completely left out of the parsed spec, this would make it simpler for the generator as it doesn't have to check for the isOptedInTo.
I'm curious if you thought about that approach and if yes, why you didn't choose to go that route?

(B) I'm curious if you actively choose "opt-in" instead of "allow", "include" or "expose"?
I personally think isAllowed, isIncluded or isExposed might make equal amount of sense and be more direct.

Regarding (A), we may want to handle the isOptedInTo differently. For instance, currently in our TS template (see RealmJS PR) we add deprecation annotations to the generated types with a message saying what to add to the opt-in list to be able to use it. This way we can still see the available methods and props when working with the binding, rather than switching to the spec to see if it's available. What's your take on this?

Regarding (B), there were some discussions regarding the word "allow" and how it may be ambiguous in the sense that everything in the general spec is essentially allowed (and I guess "exposed" as well) to be used. "included" is also on the table and the current name "opt-in" is definitely easy to change 👍 I thought it made the intent a bit clearer than the other alternatives, but I'm very open to changing it to something that everyone finds suitable 🙂

What's your take on this?

I agree that seems valuable 👍

a bit clearer than the other alternatives

My main gripe is that "opt-in" is two words and "opted in to" is three 🙈 But ... I'm not strongly opposed 🙂

Thanks for the input, I'll leave the conversation unresolved for now to see if others want to weigh in on this 👍

I have a slight preference for "includeList"/"isIncluded", but not enough to overrule the patch author. Just stating my preference in case it comes to a vote.

kneth

Thank you for preparing the path of other SDKs to adopt the binding generator!

bindgen/src/program.ts

nhachicha · 2023-05-29T09:25:40Z

Hi @elle-j
Does this support exclude scenarios? For the browser target, we want to consume the default spec except for some unused classes (see diff)

elle-j · 2023-05-30T07:21:33Z

@nhachicha, do you mean if there's an option to only list things to exclude rather than include? If so, then no, at least not at this time. Everything is excluded by default and needs to be explicitly opted in to via the opt-in list. The main reason for this is to be able to add things to the general spec in Core without silently adding it to all consumers of the spec as well.

So for the commented out parts of your diff example, you'd just not include them in the opt-in list 👍 (You could start by copying the Realm JS opt-in list. It includes most things of the general spec but not everything. There's also an earlier commit that includes everything from the general spec before removing unused parts if that's more helpful for you.)

Does this answer your question?

RedBeard0531 · 2023-05-30T07:41:40Z

@nhachicha You can always do your own skipping in the template file if you need to. This is just attempting to bake in a particularly common form into the bindgen lib, but since each generator is Just Code, you are free to do whatever logic you need there.

That said we considered adding something that would help with this case, but decided not to add it at this time. Right now we only support a single opt-in yaml file, but we could fairly easily add support for multiple yaml files. That would allow 1 file for "realm-js common" stuff used by all engines, as well as an engine-specific file. We weren't sure if any other SDKs would need this, and the space savings for JS would be minimal, so we decided to do the simplest thing for now. We can revisit that decision once the main functionality lands.

nhachicha · 2023-05-30T10:22:02Z

@elle-j @RedBeard0531

The main reason for this is to be able to add things to the general spec in Core without silently adding it to all consumers of the spec as well.

This makes sense. WASM target will use the Opt-in approach for now 👍

RedBeard0531

Please revert c985657. Other than that, LGTM

RedBeard0531 · 2023-06-01T11:09:29Z

bindgen/src/generator.ts

@@ -16,12 +16,13 @@
 //
 ////////////////////////////////////////////////////////////////////////////

-import { Spec } from "./spec";
+import { OptInSpec, Spec } from "./spec";


I have a slight preference for "includeList"/"isIncluded", but not enough to overrule the patch author. Just stating my preference in case it comes to a vote.

bindgen/src/spec.ts

kraenhansen · 2023-06-09T12:44:01Z

I know you've sunk a great deal of time in this already, but I just realised that a lot of the code could perhaps be simplified if we just included an optional optIn object in the original Spec which could be merged across all provided specs. That way the js_spec.yml maintained by our team could be expanded to include the optIn information, instead of having to pass a separate parameter and file through the CLI and we wouldn't need separate parse logic either.

elle-j · 2023-06-15T11:41:47Z

@kraenhansen, as we discussed on Slack, I'll go ahead and note your suggestion as a potential change for an upcoming iteration and merge this as is 👍

…e it.

We'll instead have the binding generator generate the opt-in spec schema file from spec.yaml in order to list all classes and records with their corresponding methods and fields. The validation would then also provide autocomplete in the opt-in spec file.

* bindgen: Ran clang-format [bindgen] Upgrade to core v13.15.1 (#6726) Support Bindgen opt-in list (#6639)

* Add initial support for a Bindgen opt-in list. * Update CMakeLists with opt-in option. * Move calling of 'bindModel()' to Bindgen. * Remove 'Readonly' return types from getters. * Add comment to 'opt in' command option. * Move logic of applying the opt-in list and require consumers to invoke it. * Update doc comments. * Validate parsed yaml file. * Update name in 'OptInSpec' type. * Revert 'Validate parsed yaml file'. We'll instead have the binding generator generate the opt-in spec schema file from spec.yaml in order to list all classes and records with their corresponding methods and fields. The validation would then also provide autocomplete in the opt-in spec file. * Update pointer to 'external/catch'. * Update error message for missing method/field.

elle-j requested review from RedBeard0531, kneth, kraenhansen, nirinchev and papafe May 17, 2023 10:00

cla-bot bot added the cla: yes label May 17, 2023

github-actions bot assigned elle-j May 17, 2023

RedBeard0531 approved these changes May 17, 2023

View reviewed changes

elle-j commented May 17, 2023

View reviewed changes

elle-j mentioned this pull request May 17, 2023

Utilize Bindgen opt-in list realm/realm-js#5820

Merged

1 task

kneth approved these changes May 21, 2023

View reviewed changes

bindgen/src/program.ts Show resolved Hide resolved

elle-j marked this pull request as ready for review May 30, 2023 09:00

RedBeard0531 requested changes Jun 1, 2023

View reviewed changes

kraenhansen approved these changes Jun 9, 2023

View reviewed changes

elle-j added 7 commits June 15, 2023 14:02

Add initial support for a Bindgen opt-in list.

c229976

Update CMakeLists with opt-in option.

dcf0618

Move calling of 'bindModel()' to Bindgen.

354f07d

Remove 'Readonly' return types from getters.

bbd81c2

Add comment to 'opt in' command option.

e9683bf

Move logic of applying the opt-in list and require consumers to invok…

cc73ada

…e it.

Update doc comments.

7338f3f

elle-j added 4 commits June 15, 2023 14:02

Validate parsed yaml file.

a70af87

Update name in 'OptInSpec' type.

05a618f

Update pointer to 'external/catch'.

3b888b4

elle-j force-pushed the lj/bindgen-opt-in branch from 3bca1cc to 3b888b4 Compare June 15, 2023 12:59

Update error message for missing method/field.

420cd99

elle-j merged commit b0a1e94 into bindgen Jun 16, 2023

elle-j deleted the lj/bindgen-opt-in branch June 16, 2023 13:34

papafe added a commit that referenced this pull request Jun 20, 2023

Merge branch 'bindgen' into fp/geospatial

5ec2f81

* bindgen: Ran clang-format [bindgen] Upgrade to core v13.15.1 (#6726) Support Bindgen opt-in list (#6639)

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Bindgen opt-in list #6639

Support Bindgen opt-in list #6639

elle-j commented May 17, 2023 •

edited

Loading

papafe commented May 17, 2023

RedBeard0531 left a comment

RedBeard0531 May 17, 2023

elle-j May 17, 2023

elle-j commented May 17, 2023

kraenhansen commented May 17, 2023 •

edited

Loading

elle-j May 17, 2023 •

edited

Loading

kraenhansen May 22, 2023 •

edited

Loading

elle-j May 22, 2023

RedBeard0531 Jun 1, 2023

kneth left a comment

nhachicha commented May 29, 2023

elle-j commented May 30, 2023 •

edited

Loading

RedBeard0531 commented May 30, 2023

nhachicha commented May 30, 2023

RedBeard0531 left a comment

RedBeard0531 Jun 1, 2023

kraenhansen commented Jun 9, 2023 •

edited

Loading

elle-j commented Jun 15, 2023

Support Bindgen opt-in list #6639

Support Bindgen opt-in list #6639

Conversation

elle-j commented May 17, 2023 • edited Loading

What, How & Why?

Added Behavior:

SDK Requirements:

Updated Type:

SDK Usage Examples:

Example using CMake:

Example of the generate() function:

Potential Additions in Upcoming Iterations:

☑️ ToDos

papafe commented May 17, 2023

RedBeard0531 left a comment

Choose a reason for hiding this comment

RedBeard0531 May 17, 2023

Choose a reason for hiding this comment

elle-j May 17, 2023

Choose a reason for hiding this comment

elle-j commented May 17, 2023

kraenhansen commented May 17, 2023 • edited Loading

elle-j May 17, 2023 • edited Loading

Choose a reason for hiding this comment

kraenhansen May 22, 2023 • edited Loading

Choose a reason for hiding this comment

elle-j May 22, 2023

Choose a reason for hiding this comment

RedBeard0531 Jun 1, 2023

Choose a reason for hiding this comment

kneth left a comment

Choose a reason for hiding this comment

nhachicha commented May 29, 2023

elle-j commented May 30, 2023 • edited Loading

RedBeard0531 commented May 30, 2023

nhachicha commented May 30, 2023

RedBeard0531 left a comment

Choose a reason for hiding this comment

RedBeard0531 Jun 1, 2023

Choose a reason for hiding this comment

kraenhansen commented Jun 9, 2023 • edited Loading

elle-j commented Jun 15, 2023

elle-j commented May 17, 2023 •

edited

Loading

Example of the `generate()` function:

kraenhansen commented May 17, 2023 •

edited

Loading

elle-j May 17, 2023 •

edited

Loading

kraenhansen May 22, 2023 •

edited

Loading

elle-j commented May 30, 2023 •

edited

Loading

kraenhansen commented Jun 9, 2023 •

edited

Loading