Checking the backward compatibility of V10

[mununki]

While I'm writing JSX PPX V4, I tried to verify that V4 will not break a large existing application. So, I ran it on some projects and encountered some errors which seem not related to V4, I guess.

1. Attributes not allowed here
   I'm not sure what causes this error. I've looked for the error msg in the compiler, but no gain.

2. Multiple definition of the type
   It was fine in V9, but I found this is not allowed anymore in V10. It could break the existing projects, I guess.

   module NonEmpty = {
     include Belt.Array
   
     type t<'a> = array<'a> // error!
   }

3. Not a valid global name
   I think this kind of issue goes to each submodule, but it may cause breaking. Maybe it could be documented as some sort of migration plan for open source maintainers.

   external formatISO: Js.Date.t => string = "date-fns/formatISO";

[cristianoc]

Can you try without turning on V4? Just use the latest compiler.
And add any breaking changes to this list: #5413

For 1: perhaps /** doc comments */ ? Do you have any in the middle of the code?

[mununki]

Can you try without turning on V4? Just use the latest compiler.

Can you explain what the latest compiler version or commit hash is? No compilation problem with v9.1.4. If you let me know what commit hash of V10 wants to be tested, I'll go for it.

For 1: perhaps /** doc comments */ ? Do you have any in the middle of the code?

I already noticed this, there are no /** doc comments */ in my company projects.

[cristianoc]

Can you try without turning on V4? Just use the latest compiler.

Can you explain what the latest compiler version or commit hash is? No compilation problem with v9.1.4. If you let me know what commit hash of V10 wants to be tested, I'll go for it.

Sorry I just meant without turning on any specific V4 options. But it sounds like you've already done that.

For 1: perhaps /** doc comments */ ? Do you have any in the middle of the code?

I already noticed this, there are no /** doc comments */ in my company projects.

Got a repro that can be extracted from the real code?

[mununki]

Got a repro that can be extracted from the real code?

Let me talk to the security supervisor first if I can add you as a collaborator to my company repo. I hope that I can give the company code base as corpus for V10 test. Is it helpful for you to repro?

[cristianoc]

Got a repro that can be extracted from the real code?

Let me talk to the security supervisor first if I can add you as a collaborator to my company repo. I hope that I can give the company code base as corpus for V10 test. Is it helpful for you to repro?

Actually that does not seem necessary. It should really be about copying a couple of lines of code and remove any sensitive information.
Is that difficult?

[mununki]

Alright. I have 17 files with error 1. This is one of them. I've confirmed by manager that it is possible to invite you as collaborator to my company forked repo. Feel free to let me know if you need.
sample_v10.res.zip

[cristianoc]

If you remove the part that does `%relay. ...`, does the error go away?

[mununki]

Oh, you're correct. Error 1 is gone after removing the module bindings with %relay... Doesn't V10 allow the attribute in module binding?

[cristianoc]

I think it does. The question is what code the relay ppx generates.

[cristianoc]

@zth any thoughts?

[cristianoc]

In fact don't even know: is the error message coming from the relay ppx or from the compiler (on the generated code).

[zth]

I'd have to look a bit deeper to be able to say anything meaningful. Let me see if I can find time for it during the weekend, will get back to you.

[mununki]

@zth My company decided to open the code base in public for V10. Maybe it could be a test bed https://github.com/green-labs/sinsunhi-frontend-mirror

[cristianoc]

@zth My company decided to open the code base in public for V10. Maybe it could be a test bed https://github.com/green-labs/sinsunhi-frontend-mirror

This seems to build fine with current compiler master.

[cristianoc]

@zth My company decided to open the code base in public for V10. Maybe it could be a test bed https://github.com/green-labs/sinsunhi-frontend-mirror

This seems to build fine with current compiler master.

Actually not, took a little time to convince yarn to install a local version of the compiler.
Can see the issues.

[cristianoc]

OK so for 2, now Belt.Array already defines a type t so it should not be redefined anymore, as that's not allowed.
So not technically a breaking change, just a change in the library that may make some projects not compile anymore.

[cristianoc]

As for 3, I'm not sure that externals without decorators were ever supported.

[cristianoc]

This leaves us with 1 to investigate.

[cristianoc]

@mattdamon108 Is there a version of the repo that reproduces 1? As currently compilation stops because of the other errors so I did not get past building dependencies.

[mununki]

@cristianoc Did you pass the compilation successfully with the current master V10 of compiler? What kind of error stop you building dependencies?

[mununki]

If the error of building dependency is from Garter, it should be Error 2. You can comment it temporarily then building will be proceeded.

[zth]

Haven't had time to look yet unfortunately, but if it's just rescript-relay in general that fails, then trying to build https://github.com/zth/rescript-relay/tree/master/example with the new compiler should also fail, and might be a bit easier to get setup, if there were dependency issues in the other repo.

[cristianoc]

Here:

% grep "Attributes not allowed here" node_modules/rescript-relay/ppx
Binary file node_modules/rescript-relay/ppx matches

[cristianoc]

It's the ppx that does not like some attributes. Which presumably are introduced by the parser.

[cristianoc]

OK so the ppx uses some off-the-shelf utils that do this:
https://github.com/ocaml-ppx/ppxlib/blob/2e0aee3b7c6a49496dd804ad97323c4d75744311/test/extensions_and_deriving/test.ml#L54

[mununki]

OK so for 2, now Belt.Array already defines a type t so it should not be redefined anymore, as that's not allowed.
So not technically a breaking change, just a change in the library that may make some projects not compile anymore.

I'm not sure, but I can't find the type constructor Belt.Array.t<'a> in Belt.Array module. I'm wondering if it is the same case to #5498

[cristianoc]

https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/others/belt_Array.mli

[mununki]

https://github.com/rescript-lang/rescript-compiler/blob/master/jscomp/others/belt_Array.mli

Ah.. Sorry I found it too 0cdb221 I mistakenly tried to find it inside the project which is V9. I realized again that the release had been made quite long ago.

[cristianoc]

acknowledged current status in release notes

[zth]

The rescript-relay issues have now been solved in 1.0.0-beta.24.