rescript-lang / rescript-compiler

The compiler for ReScript.
https://rescript-lang.org
Other
6.74k stars 448 forks source link

Ignore unsupplied optional arguments when passing a function as a parameter #6693

Closed cknitt closed 1 month ago

cknitt commented 7 months ago

This is a feature request.

Example of what I mean (using ReScript 11.1.0-rc.5, uncurried mode, ReScript Core, from my work on https://github.com/rescript-association/rescript-core/pull/201):

let values: array<JSON.t> = [String("test"), Number(42.)]

// Multiple bindings
@val external stringify: JSON.t => string = "JSON.stringify"
// etc.

// 1. this works fine
let x = values->Array.map(stringify)

// Single binding with optional parameters
@unboxed
type replacer = Array(array<string>) | Function((string, JSON.t) => JSON.t)
@val external stringify: (JSON.t, ~replacer: replacer=?, ~indent: int=?) => string = "JSON.stringify"

// 2. doesn't compile
let y = values->Array.map(stringify)

// 3. but this does
let z = values->Array.map(x => stringify(x))

Would it be possible to make 2. compile and work, too?

We will run into such situations more often when we change the standard library to follow this style for bindings (single binding for with optional arguments instead of multiple bindings for a JS function).

I already ran into this in practice with Core's Int.fromString which has an optional radix argument.

In TypeScript, this works fine BTW:

const x = ['test', 42].map(JSON.stringify);

It would be interesting to explore if we can make it work in ReScript, too.

cknitt commented 7 months ago

Actually in the case of RescriptCore.Int.fromString the optional argument radix is not trailing, but leading. Still, it would be good to be able Int.fromString over an array without having to supply the radix parameter.

cristianoc commented 7 months ago

The runtime representation of the two functions is different, so it would not be just a type cast. Instead, code would need to be generated to pass the relevant undefined in place of the optional arguments. There are no similar transformations in the compiler a type checking time, afaik.

fhammerschmidt commented 6 months ago

+1 I just added Core 1.3.0 in a project and it's very annoying.

It can be fixed by using the underscore

opt->Option.map(Int.toString(_))

but I would love if I had not to explain that to beginners

cknitt commented 6 months ago

I think

opt->Option.map(x => Int.toString(x))

will be a clearer workaround for beginners.

But yes, this is very unintuitive:

// Fine
let x = Int.toString(5)
let x = 5->Int.toString

// Not fine
let x = [5]->Array.map(Int.toString)
CarlOlson commented 5 months ago

This is going from a function with more parameters to less, but is the opposite any more doable?

module Array = {
  @send external map: (array<'a>, ('a, ~index: int=?) => 'b) => array<'b> = "map"
}

// Not fine
let x = [5]->Array.map(x => x + 1)
cknitt commented 1 month ago

Good point by @cometkim: This can do weird things in JS:

['1', '2', '3'].map(Number.parseInt)
> [1, NaN, NaN]

😱

zth commented 1 month ago

@cknitt what do you think, still something you'd want to pursue given the drawbacks @cometkim showed?

cknitt commented 1 month ago

It could work better in ReScript than in JS. But still maybe not a good idea to encourage this pattern if it is problematic in JS.

So I think I can let it go. 🙂