Specialize uses of new dict literal to direct JS object creation

[zth]

A Dict is obviously a JS object at runtime, but there has been no way to create a dict in a way that compiles to a regular JS object creation. This can be a bit confusing when looking at the generated code, as well as hurt performance (albeit slightly in most cases) as creating dict objects are done dynamically via an array.

This changes so that any usage of the new dict{} literal gets compiled to an actual JS object creation.

Related issues:

• Feature suggestion: syntax for Js.Dict.t literals #6301
• specialize Js.Dict.fromArray #2320

[cknitt]

What about RescriptCore.Dict.fromArray?

[zth]

Yeah that's a good point, we should make sure this PR handles both.

[zth]

Dict should be promoted to a built in type and specialized that way instead.

[zth]

@cristianoc please have a look at whether this approach is good and if the optimization is in the correct place. Maybe it could be put somewhere else?

When Core moves into the compiler (#6761) then we'll change this to account for Dict.fromArray as well.

[zth]

Got a ton of unrelated changes here rebasing, including to test JS files. Not sure why. Also changes to build.ninja, not sure why either.

[cknitt]

Got a ton of unrelated changes here rebasing, including to test JS files. Not sure why. Also changes to build.ninja, not sure why either.

build.ninja has changed because DictTest.res was added.

The line number changes in the JS output must be from a previous PR, maybe I forgot to check those in when reformatting the tests.

[zth]

@cknitt could you check if that's the case, with the reformatting in master?

[cknitt]

Yes, I currently get the same JS output files changed on master.

[cknitt]

Shall I create a separate PR for committing those JS output changes?

[cknitt]

Shall I create a separate PR for committing those JS output changes?

@zth #6992

[cristianoc]

Looks good.
Left comment on robustness of path detection.

[zth]

@cristianoc check again please. I changed the approach to use a builtin primitive instead, and it now only works on the new dict{} literal, which I think makes sense.

[cristianoc]

Looks great

[cristianoc]

The only minor comment is the syntax does not let us remove Js.Dict in future.
Just adding a reminder issue should be enough I think. Or in the issue about moving core into the compiler.

[zth]

The only minor comment is the syntax does not let us remove Js.Dict in future. Just adding a reminder issue should be enough I think. Or in the issue about moving core into the compiler.

Why is that? This is in the syntax so we should be able to just change the path the dict literal produces, no?

Regardless, maybe there's a better module to put this and other internal-only functions like for await? Several benefits would come from that:

• Never accidentally trigger autocomplete for this function if you pipe a dict
• No issue like the one above stated where we can't move modules around. They can all just remain in the internal one

What do you think?

Another observation on this change - previously there was a wip PR that added spreads to dicts, which is quite nice. That's probably a bit harder to achieve with this structure. And it's a nice feature, so it'd be cool to have it at some point.

[zth]

Trying to figure out why the syntax roundtrip tests still are broken.

[cristianoc]

The only minor comment is the syntax does not let us remove Js.Dict in future. Just adding a reminder issue should be enough I think. Or in the issue about moving core into the compiler.

Why is that? This is in the syntax so we should be able to just change the path the dict literal produces, no?

Regardless, maybe there's a better module to put this and other internal-only functions like for await? Several benefits would come from that:

• Never accidentally trigger autocomplete for this function if you pipe a dict
• No issue like the one above stated where we can't move modules around. They can all just remain in the internal one

What do you think?

Another observation on this change - previously there was a wip PR that added spreads to dicts, which is quite nice. That's probably a bit harder to achieve with this structure. And it's a nice feature, so it'd be cool to have it at some point.

Sounds like a great idea.
My only comment was trivial: if we remove the path, the trick does not work anymore. So we'll need to remember to change the path when that happens. And check that the trick works today when one opens core by default.

[cristianoc]

Trying to figure out why the syntax roundtrip tests still are broken.

you did not check in the changes -- les's see if CI works now

[zth]

@cristianoc @cknitt look again, the creator fn is now in its own runtime module.

[cristianoc]

can you check that the dict syntax is not formatted away?
that's what I interpret the print tests to be saying at the moment

[cknitt]

Maybe also add a Dict<int> for testing?

[zth]

Sure, adding.

[cristianoc]

ship it

[nojaf]

ship it

Any timeline on the next alpha?

[zth]

@nojaf not sure, but probably not too far away. We can release the alphas pretty much whenever we want to.