Skip to content
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

curriing limit to 10 is low #3514

Closed
slyshykO opened this issue Aug 26, 2023 · 10 comments
Closed

curriing limit to 10 is low #3514

slyshykO opened this issue Aug 26, 2023 · 10 comments

Comments

@slyshykO
Copy link

Description

I've tried to create a form with 11 fields with Fable.Form .
When compiling to JavaScript Fable generates the wrong function call & export

import { createObj, curry11 } from "./fable_modules/fable-library.4.1.4/Util.js";
...
return append(extra0Field, append(extra0Field, append(extra0Field, append(extra0Field, append(extra0Field, append(extra0Field, append(extra1Field, append(extra0Field, append(rememberMe, append(passwordField, append(emailField, succeed(curry11(onSubmit)))))))))))));
...

Fable 4.0.1 generate the working code.

Repro code

https://github.com/slyshykO/big-form

Expected and actual results

Fable compile working code.

Related information

  • Fable version: 4.1.4
  • Operating system: win11 x64
@MangelMaxime
Copy link
Member

When running fable in non watch mode (dotnet fable src) the generated is correct and support infinite curry as requested in #2525

However, if running it in watch mode (dotnet fable watch src) then it tries to use curry11 which is invalid.

Fable 4.0.2 is the version which introduced this regression.

If you revert to Fable 4.0.1 then your application should works, you will have a few bug fixes missing but most of the changes are related to the others Fable target like TypeScript, Rust, Python, etc. So I hope for now it can be a workaround for your project.

@MangelMaxime
Copy link
Member

@ncave I am looking into fixing this issue because having infinite currying/uncurrying is really important for libraries like Thoth.Json and Fable.Form.

I found the previous implementation of the curry function:

fable-library/Util.ts

const CURRIED = Symbol("curried");

function _curry(args: any[],  arity: number, f: Function): (x: any) => any {
  return (arg: any) => arity === 1
    ? f(...args.concat([arg]))
    // Note it's important to generate a new args array every time
    // because a partially applied function can be run multiple times
    : _curry(args.concat([arg]), arity - 1, f);
}

export function curry(arity: number, f: Function): Function | undefined {
  if (f == null || f.length === 1) {
    return f;
  }
  else if (CURRIED in f) {
    return (f as any)[CURRIED];
  } else {
    return _curry([], arity, f);
  }
}

But when I try to apply it in place of this code

let partialApplyAtRuntime (com: Compiler) t arity (expr: Expr) (partialArgs: Expr list) =
match com.Options.Language with
| JavaScript | TypeScript | Dart | Python ->
match uncurryLambdaType -1 [] expr.Type with
| ([]|[_]), _ -> expr
| argTypes, returnType ->
let curriedType = makeLambdaType argTypes returnType
let curried = Helper.LibCall(com, "Util", $"curry{argTypes.Length}", curriedType, [expr])
match partialArgs with
| [] -> curried
| partialArgs -> curriedApply None t curried partialArgs
, it looks like the arity number is wrong (offset by 1).

When trying different approach I also had issues with mangling signature changing, etc. Probably because the code changed too much between Fable 4 and Fable 3, so I can naïvely patch it...

While reading through the code I found out that the new curry/uncurry implementation is using some kind of global storage.

const curried = new WeakMap<object, object>();

Can't this global storage be out of sync depending on the code order? I mean it seams like it store the last function called with uncurryX but what garantee that the next curryX is the one related to the correct function?

I suppose that storage is to work around "partial application and side effets" as if I replace it with an older implementation like:

export function curry2<T1, T2, TResult>(f: (a1: T1, a2: T2) => TResult) {
  return ((a1: T1) => (a2: T2) => f(a1, a2));
}

then 2 tests are failings.

Do you have any guidance to give me ?

@ncave
Copy link
Collaborator

ncave commented Oct 18, 2023

@MangelMaxime If you need more than 10 levels, perhaps just add extra curry/uncurry to fable-library/Utils.ts.

Here is when the first 10 curry/uncurry were introduced.

Here is a description of why those tests are failing.

To (mis)quote B.G., "20 curry/uncurry ought to be enough for anybody." ;)

@slyshykO
Copy link
Author

I've heard that typescript have variadic tuples. Maybe it helps.

https://instil.co/blog/crazy-powerful-typescript-tuple-types/

@MangelMaxime
Copy link
Member

@ncave Thank for the commit, this is what I was working with. However, I feel like others things moved in the code too and I can't easily revert this commit. Will give it another try.

If you need more than 10 levels, perhaps just add extra curry/uncurry to fable-library/Utils.ts.

This will increase the bundle size and is a temporary solution, nothing prevent users to have forms with more that 10 flats fields (bad UX probably but I see a lot of them), or JSON with more that 10 properties (in this case Decode.object should be a good workaround as it has no limit).

I will probably resort to this fix for now, if I can't make it work with infinite currying.


@ncave Do you know why we need to store uncurried function inside of a WeekMap and can't just return directly the function ?

@ncave
Copy link
Collaborator

ncave commented Oct 19, 2023

@MangelMaxime

Do you know why we need to store uncurried function inside of a WeekMap

Yes, it's described in #3378:

to resolve the problem of side effects running multiple times when partial applying a function that has been previously uncurried. The way this is currently solved for JS and Python is to add a property with a reference to the curried function (kept in a WeakMap) so when partial applying we can check if the curried version is already available.

@MangelMaxime
Copy link
Member

I will need to re-read the code because I don't understand how that WeakMap stuff works.

I don't understand how it know which functions to retrieve. To me it looks like if uncurry2 is called twice, then the last call override the previous stored value in the WeakMap.

Meaning that when curry2 is called, there is no guarantee that you get the function you wanted to have.

Unless, curry2 and uncurry2 are always right after one another which I don't think is possible to guarantee with async code.

@ncave
Copy link
Collaborator

ncave commented Oct 19, 2023

@MangelMaxime

I don't understand how it know which functions to retrieve

If I'm not mistaken, the WeakMap is just a storage, the key is stored in each function object as property.

@MangelMaxime
Copy link
Member

@ncave

Ohhh, I think you are right. A new function is created each time, and that function is what is used as a key. So each time, there is a new instance so it doesn't override the previous uncurryX.

@ncave
Copy link
Collaborator

ncave commented Oct 19, 2023

@MangelMaxime Right, we haven't implemented that for Rust yet, so in Rust partial application with side effects can run the side effects twice (or more).

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