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 452 forks source link

v12 alpha: incorrect compiler output #7023

Open cknitt opened 2 months ago

cknitt commented 2 months ago

When compiling rescript-schema with v12 alpha.3, I noticed invalid output. Simplified, the problem is this:

In v11, the following ReScript code:

type error

let reason = (error: error, ~nestedLevel as _ = ?): string => String.make(error)
let reason: error => string = Obj.magic(reason)

let message = (error: error) => `Failed reason: ${error->reason}`

compiles to

function reason(error, param) {
  return String(error);
}

function message(error) {
  return "Failed reason: " + reason(error);
}

whereas in v12 alpha, it compiles to the invalid code

function reason(error, param) {
  return String(error);
}

function message(error) {
  return "Failed reason: " + param => reason(error, param);
}

/cc @cristianoc @cometkim

cometkim commented 2 months ago

Hmm, that's confusing. Isn't this a change in type rather than a change in output? Let me find out.

cometkim commented 2 months ago

v12 alpha.1 output

function reason(error, param) {
  return String(error);
}

function message(error) {
  return "Failed reason: " + (function (param) {
    return reason(error, param);
  });
}
cknitt commented 2 months ago

So there seem to be two different issues:

  1. Since alpha.2: The output is invalid JS (parenthesis missing).
  2. Since alpha.1: The output is incorrect, we want to append reason(errror), not a function.

The latter may be related to some changes around removing curried mode?

cometkim commented 2 months ago

Since alpha.2: The output is invalid JS (parenthesis missing).

well, I think it shouldn't be able to produce without %raw, and %raw block will be wrapped correctly

cometkim commented 2 months ago

The latter may be related to some changes around removing curried mode?

This one: https://github.com/rescript-lang/rescript-compiler/commit/f603cf1779e08544e26e1c53925f3130294cc72b

cknitt commented 2 months ago

Played a bit more with this:

let f1: string => string = Obj.magic("not actually a function, but never mind")
let f2: string => string = Obj.magic((a, b) => a ++ b)
let f3: string => string = Obj.magic((a, b, c) => a ++ b ++ c)

let x1 = f1("test")
let x2 = f2("test")
let x3 = f3("test")

outputs

let f1 = "not actually a function, but never mind";

function f2(a, b) {
  return a + b;
}

function f3(a, b, c) {
  return a + b + c;
}

let x1 = f1("test");

function x2(param) {
  return f2("test", param);
}

function x3(param, param$1) {
  return f3("test", param, param$1);
}

in v12 alpha.

So somehow even though the type of each function is string => string after "casting" with Obj.magic, the original arity of the function is preserved and affects the generated code.

fhammerschmidt commented 2 months ago

I tried a slightly modified example in the playground with v10:

type error

let reason = (. error: error, ~nestedLevel as _): string => error->Obj.magic
let reason: error => string = Obj.magic(reason)

let message = (error: error) => `Failed reason: ${error->reason}

which gives me v10:

function message(error) {
  return "Failed reason: " + (function (param) {
            return reason(error, param);
          }) + "";
}

v11:

function message(error) {
  return "Failed reason: " + reason(error);
}

v12:

function message(error) {
  return "Failed reason: " + param => reason(error, param);
}
cristianoc commented 1 month ago

First, simplified example:

let first = (x, y) => x
let hack: int => int = Obj.magic(first)
let test = x => hack(x)

It's pretty questionable whether this should work, but one can see why the behaviour is like it is and what changed in v12. For sure, moving hack to another file would make it impossible to even know that there exists first underneath and the issue would go away.

cristianoc commented 1 month ago

The generated code looks fine on master. But not on playground alpha.3 https://rescript-lang.org/try?version=v12.0.0-alpha.3&code=FAGwpgLgBAZglgJwM7QLxQBQA8A0UCeAlFKgHxRaiRQAWAhgMYDWAXFHAHZrmdpQDyAIwBWAOgC2dAOZwGGeMgiEq0CGBQkKJcvWbZCQA

% ./bsc tst.res
// Generated by ReScript, PLEASE EDIT WITH CARE
'use strict';

let Obj = require("rescript/lib/js/obj.js");

function first(x, _y) {
  return x;
}

let hack = Obj.magic(first);

function test(x) {
  return hack(x);
}

exports.first = first;
exports.hack = hack;
exports.test = test;
/* hack Not a pure module */

@cknitt was this fixed?

cknitt commented 1 month ago

@cristianoc No, it was not fixed. I can reproduce it with your example if I add your example to tests/tests/src and run node ./scripts/test.js -node.

I get

function test(x) {
  return param => first(x, param);
}

then.

cristianoc commented 1 month ago

We have a couple of small issues. One is the bsc commands used are a bit opaque, and verbose mode should show them in detail. The other is rescript does not use default options while it should.

Here's the repro:

./bsc -bs-cross-module-opt tst.res

If rescript always uses that option, then it should be the default.

cristianoc commented 1 month ago

So what's going on is that it's inlining the function, so it kind of does what you tell it to do.

cristianoc commented 1 month ago

Here's the "fix":

let opaque_cast =  x => Obj.magic(x)

let first = (x, _y) => x
let hack: int => int = opaque_cast(first)
let test = x => hack(x)
cknitt commented 1 month ago

Hmm, -bs-cross-module-opt is not the default though. In the compiler repo, it is set explicitly in bsc-flags when building the runtime + tests.

But as far as I can see, it is not set in rescript-schema.

cristianoc commented 1 month ago

Hmm, -bs-cross-module-opt is not the default though. In the compiler repo, it is set explicitly in bsc-flags when building the runtime + tests.

But as far as I can see, it is not set in rescript-schema.

Then we should be consistent, and use defaults in tests, unless we are explicitly testing the non-default options.

cknitt commented 1 month ago

Then we should be consistent, and use defaults in tests, unless we are explicitly testing the non-default options.

Agreed! And this unearthes other interesting stuff: https://github.com/rescript-lang/rescript-compiler/pull/7071

zth commented 1 month ago

Is there a reason cross module opt isn't the default?

cknitt commented 1 month ago

Is there a reason cross module opt isn't the default?

It is labeled as experimental

    "-bs-cross-module-opt", set Js_config.cross_module_inline, 
    "*internal* Enable cross module inlining(experimental), default(false)";
zth commented 1 month ago

Yes but does it need to be?