-
Notifications
You must be signed in to change notification settings - Fork 468
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
Comments
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. |
For example, Map API in JavaScript. Users can actually put nullable values there and use it by 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
} |
True, we'd lose the distinction between 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 |
Yes, I can avoid using the option type if I really want to do that. Even without the syntax, People in TypeScript prefer to use only "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. |
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. |
Example and some extra explanation here https://github.com/option-t/option-t Why we're trying to remove our own advantages? |
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. |
#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
andSome(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.The text was updated successfully, but these errors were encountered: