Investigate getting rid of option nesting

[cknitt]

#7054 and rescript-lang/rescript-core#217 triggered a discussion about whether it is actually worth wrapping/boxing options at all (to be able to distinguish between None and Some(None)).

IMO it would be nice to get rid of this if possible.

Looking at the compiler output of a typical project at our company, I can currently see a lot of Caml_option.some, all of them for JSX props or children, where they are unwanted and only cause unnecessary runtime overhead.

[cometkim]

I don't expect any additional runtime overhead to do that. At least in JavaScript, nullable values can be handled efficiently than the custom share option API, which is being surely de-opted at any time.

But it breaks stuff any codebase that actually distinguishes Some(None) / None. As nested options have usuable information anyway, I guess there are real usecases.

[cometkim]

For example, Map API in JavaScript.

Users can actually put nullable values there and use it by has() then get() combination.

It ReScript we can provide more convenient way because of the nested option

let map = Map.make()

@val external maybeValue: option<string> = "maybeValue"

map->set(key, maybeValue)

let _ = switch map->getOption(key) {
| Some(Some(value)) => Some(value) // item exists, non-null value
| Some(None) => Some(defaultValue) // item exists, null value
| None => None // not exists
}

[cknitt]

True, we'd lose the distinction between Some(None) and None in your example. But in the example, I think it would anyway be better design/clearer code to define a variant for the stuff that you put into the map, like Initialized(value) | NotInitialized or whatever naming makes sense in the context of your app.

Getting rid of nesting, we'd simplify things a lot and get less weird and more efficient JS output. It could also be one step to eventually get to "zero-runtime" JS output.

In a large project I checked this against, I did not find a single case where the wrapping actually made sense. Only unnecessary wrapping of React props taking an optional React.element (as that is an abstract type, so the compiler can't know whether boxing would be needed or not).

[cometkim]

Yes, I can avoid using the option type if I really want to do that.

Even without the syntax, Some(None) is a widely used concept in practice. It's so common, like a nullable column in a database.

People in TypeScript prefer to use only .get() without .has() because for making their compiler happy. That is a really bad practice to modeling and handling data that TypeScript leads to do. They eventually lose control over data unexpectedly. I saw a few times in multiple productions.

"Zero-runtime" means "just follow JavaScript," which doesn't always make sense. We already have pretty minimal runtime to make option option.

Define a new nullable type will be just ok and viable. But the defining option as just a nullable value will confuse users who expect the real option. I don't get it if I should have to be aware of Some(None) in option type that doesn't work as expect. No, it is not an option then. It's not about simplifying things. It's just a different one that should have a different name.

[cometkim]

You know, the option is about boxing. It essentially adds some runtime cost, just like when I use my own data type and variants. What we really need to consider is how less cost we can add, not how not to.

I learned it in a hard way. I made a boilerplate code library for it. That didn't make my project any more efficient.

So now I do expect that kind of cost for handling my data correctly. Calling any nullish value an option is not an honest way. It's weird.

[cometkim]

Example and some extra explanation here

https://github.com/option-t/option-t

Why we're trying to remove our own advantages?

[cometkim]

The current option implementation is almost optimal due to the compile time specialization.

It only adds less than a ns per access. Because the compiler handles values in mostly unboxed representation and only nested none values have custom structure but without boxing. This makes all cases to be cachable and efficient.

The slowest part is scavenger GC for temporary objects. Not the nested option itself.

So, if we remove this kind of specialization, make users have their own polymorphic option utilities or variants per each optional data, then that makes real perf problem. Because there is no way to achieve the same level of optimization without writing raw JavaScript. In terms of hidden class and GC pressure.

Less job doesn't mean always faster. Access patterns are usually more important in non-trivial codebases.

I think the only downside we currently have is a little bloat in the output, but it still gives many benefits, even users are not aware of it.

[cristianoc]

Here's a PR for quick testing on existing projects: #7442

Just to get some more real world data points.

[cknitt]

Thanks a lot! Will try that against a real-world project to see if anything goes 💥.

[cknitt]

Tested, here is a typical output change (which is quite nice):

My app does not run 100% correctly though. The UI comes up and looks good, but there is some serialization error (I think rescript-schema/sury will probably need to be adapted), and also an issue with the sync logic of my app starting but not terminating, will need to investigate in detail what the issue is here.

[cknitt]

BTW, in case this turns out to be too invasive/not feasible, here is another idea:

The main issue that I have with the current nested option handling are the unnecessary Primitive_option.some calls that can be seen in the screenshot in the previous comment.

My understanding is that this happens because both React.element and ReactNative.Style.t are abstract types, so the compiler does not know their shape and therefore cannot be sure that they do not contain an option that needs to be wrapped.

For example, in

module A = {
  @react.component
  let make = (
    ~i as _: option<int>=?,
    ~s as _: option<string>=?,
    ~element as _: option<React.element>=?,
  ) => React.null
}

let _ = <A i=1 s="test" element={<div />} />

Primitive_option.some is emitted for element, but not for i or s:

JsxRuntime.jsx(Playground$A, {
      i: 1,
      s: "test",
      element: Caml_option.some(JsxRuntime.jsx("div", {}))
    });

Would it be possible to tag an abstract type definition like e.g. React.element and ReactNative.Style.t with an attribute that tells the compiler "I know that these will never contain an option", so that the call to Primitive_option.some can be omitted for them?

[nojaf]

Your element example might be related to a weird thing that happens in the ppx:

rescript/compiler/syntax/src/jsx_v4.ml

Lines 1194 to 1207 in a87e656

	let expr =
	(* I don't quite know why fragment and uppercase don't do this additional ReactDOM.someElement wrapping *)
	match component_description with
	| FragmentComponent | UppercasedComponent -> mapper.expr mapper child
	| LowercasedComponent ->
	let element_binding =
	match config.module_ |> String.lowercase_ascii with
	| "react" -> Lident "ReactDOM"
	| _generic -> module_access_name config "Elements"
	in
	Exp.apply
	(Exp.ident
	{txt = Ldot (element_binding, "someElement"); loc = Location.none})
	[(Nolabel, mapper.expr mapper child)]

(An additional wrap of ReacDOM.someElement)

Originally introduced in rescript-lang/syntax#667

@cristianoc any idea if that still holds today?

[glennsl]

My understanding is that this happens because both React.element and ReactNative.Style.t are abstract types, so the compiler does not know their shape and therefore cannot be sure that they do not contain an option that needs to be wrapped.

What about simply defining element as private {}? Seems slightly better than breaking the type system at least.

[cknitt]

What about simply defining element as private {}? Seems slightly better than breaking the type system at least.

This actually seems to work. 🤯 🎉

The printer formats the private in private {} away though, will create an issue for that.

[cknitt]

It seems the downsides of the nested option handling can be mitigated well with #7458 and similar optimizations.

So it's doubtful whether it still makes sense to pursue getting rid of option nesting altogether, I am therefore closing this issue.

For reference, here is a previous issue where @cristianoc was exploring some ideas about flattening promise and option types on a more theoretical level: #5707