rescript-lang / rescript

ReScript is a robustly typed language that compiles to efficient and human-readable JavaScript.
https://rescript-lang.org
Other
6.76k stars 449 forks source link

Stop emitting unused `param` argument #5281

Closed TheSpyder closed 1 year ago

TheSpyder commented 3 years ago

Thank you for filing! Check list:

Inspired by the fix for #4498, I was wondering if a similar change could improve emitted code for unit parameters. For a long time ReScript has emitted an unnecessary param parameter on functions that take a single unit argument. I can understand why, the unit still needs to be represented in the type system, but up until now I've been happy relying on JS bundling tools to remove it. However I have now hit a problem having it in emitted code.

Here's an example of what I mean: https://rescript-lang.org/try?code=AIWw9gJgrgNgpgCgETgMYAsCGSCUACOADwBc4AnAO0xjwEtiAuPBAZ2LNooHMAaPKCvTwBeAHz9BxfGIlDheJPSQAoZfGJ4QATwAqcNiObTxAKRYA6GGC7JSB1GAqknLXKvV4yAvQfkJjeADeynh0xLb6GsT08Eh82j5SygC+QA

Both functions defined in this code do not use their param argument emitted to JS. This doesn't matter too much for the runTest function, but including it in the myTest function breaks mocha. I recently discovered mocha's it function relies on fn.length to determine what the test is expecting: https://github.com/mochajs/mocha/blob/master/lib/runnable.js#L38

In this case, if fn.length is not zero it passes a "done callback" as the first argument rather than using the return value of the test function.

The same thing happens if the test is defined inline, which is how I normally use mocha. https://rescript-lang.org/try?code=AIWw9gJgrgNgpgCgETgMYAsCGSCUACOADwBc4AnAO0xjwEtiAuPBAZ2LNooHMAaPKCvTwBeAHz9BxfGIlDheJPSQAoZfGJ4QATwAqcNiObTxAb2V46xZKQPF68JHwTG8AKRYA6GGC7X9G1DAKUmCWXBxlAF8gA

Is it possible to remove the unused param parameter?

TheSpyder commented 3 years ago

I took a closer look at how bs-mocha works and realised it uses @this with a wrapper function to get around the problem: https://rescript-lang.org/try?code=C4TwDgpgBAtg9gYwBYEMBQaAC8AmBXAGwgAoAieZFUgSjQgA9gIAnAOxQKgEtgAuKYgGdgzLqwDmAGiiZgSLoIEVUUALwA+KHlY9q1NZu081UUj1IYiwbtdUDgPItIBmrfRpvEHwJzLkKoAH0DKFdiPUsIaxgQABUIYRNwkJ4yJkTvIlJpZI8AKUEAOgI4cTSE6wQ4ViYawRpaIA

So I'm doing that, now, but it would be nice if I didn't have to.

bobzhang commented 3 years ago

Is it possible to remove the unused param parameter?

We could eliminate it, but the only reliable way of writing bindings is using uncurried function

TheSpyder commented 3 years ago

Ah yeah I understand. I'm not going to be able to convert all of my code to uncurried functions, but I could use an uncurried wrapper instead of a @this wrapper. That's a bit cleaner.

fhammerschmidt commented 1 year ago

This is fixed now in ReScript 11 with activated uncurried mode. @cristianoc @TheSpyder

cristianoc commented 1 year ago

This is fixed now in ReScript 11 with activated uncurried mode. @cristianoc @TheSpyder

That's true. What changed is:

OK to close this?

TheSpyder commented 1 year ago

I don't have time to check right now, but I believe you it's fixed and am happy to close