Skip to content

Investigate getting rid of option nesting #7430

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cknitt opened this issue May 5, 2025 · 7 comments
Open

Investigate getting rid of option nesting #7430

cknitt opened this issue May 5, 2025 · 7 comments
Assignees
Milestone

Comments

@cknitt
Copy link
Member

cknitt commented May 5, 2025

#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
Copy link
Member

cometkim commented May 6, 2025

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
Copy link
Member

cometkim commented May 6, 2025

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
Copy link
Member Author

cknitt commented May 6, 2025

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
Copy link
Member

cometkim commented May 6, 2025

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
Copy link
Member

cometkim commented May 6, 2025

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
Copy link
Member

cometkim commented May 6, 2025

Example and some extra explanation here

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

Why we're trying to remove our own advantages?

@cometkim
Copy link
Member

cometkim commented May 6, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants