Battle test with JSX PPX V4 #615

mununki · 2022-07-19T04:38:54Z

1. You forgot to handle a possible case here, for example:

// original
@react.component
let make = (~height, ~width, ~fill, ~className=?) => body

// error message
You forgot to handle a possible case here, for example: 
  {height: _, width: _, fill: Some(_), className: None, _}
| {height: _, width: _, fill: None, className: Some(_), _}

2. The type variable name '_disabled is not allowed in programs

// original
@react.component
let make = (~_disabled=?, ()) => body

3. A type parameter occurs several times
4. Values do not match between intf and impl

let make: props<option<string>, 'a, 'b, 'c> => React.element
is not included in
let make: React.componentLike<
props<string, string, int, React.element>

5. At most one component definition per module or one gets an error of type props being defined multiple times "Multiple definition of the type name props".
6. Explicit type annotation for optional arg in definition
This issue seems related to 4

let make = (~x: option<int>=?) => body
// generated
type props<'x> = {key?: string, x?: 'x}
let make = ({?x, _}: props<option<int>>) => body
// error type mismatch option<int> vs. option<option<int>>

7. Conversion for the alias

let make = (~x as y) => body

8. Backward compatibility for passing the optional expression to prop
This expression seems not working with V4, should work in different expression.

x=?{a->returnOption} ❌ 

x=?{
  let _ = ()
  a->returnOption
} ⭕

9. Raise error with loc
10. If a component is calledfoo not make the ppx tries to call make.
11. Error without location when a required prop is not supplied.

The text was updated successfully, but these errors were encountered:

mununki · 2022-07-19T04:41:24Z

WIP: #614

mununki · 2022-07-19T17:09:00Z

I've looked into each issue and finally make my company project build successfully.
I added some new issues and work left to do though, but it won't take a long time.

mununki · 2022-07-21T06:15:13Z

@cristianoc I have a question about the issue no. 8. I've investigated some and found this. I think it is a breaking change for users to have to change their code.

type x = {x?: int}
let x = Some(1)

let a = {x: ?x}
let b = {x: ?x->Option.map(x => x)} // error
//           ^^^^^^^^^^^^^^^^^^^^ This has type: option<int>
//                                Somewhere wanted: int
let c = {x: ?Option.map(x, x => x)} // ok

I guess it might be related to the operator precedence or just the wrong syntax usage by users. As V4 parses the props and value into the record, it causes a build error there. If users have their code base expressing let b way, then it causes a breaking change. Can we make a workaround to prevent a break?

cristianoc · 2022-07-21T06:28:51Z

Can you add examples with and without parens? Then we know what's going on.

Examples checked in as tests. So we can look at the generated code.

cristianoc · 2022-07-21T06:58:20Z

How can that be a breaking change if the syntax with ? did not exist before?

mununki · 2022-07-21T07:20:57Z

How can that be a breaking change if the syntax with ? did not exist before?

Ah! I meant that breaking between V3 -> V4 with V10 compiler. I'll add the example in tests right a way.

cristianoc · 2022-07-21T07:34:45Z

One more:

  @react.component
  let anotherComponent = (~b) => {
    React.string(b)
  }

  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/rescript-compiler/jscomp/gentype_tests/typescript-react-example/src/Hooks.res

  The value make can't be found

mununki · 2022-07-21T07:36:57Z

ead4831 The example added

mununki · 2022-07-21T07:39:47Z

 @react.component
  let anotherComponent = (~b) => {
    React.string(b)
  }

It is correct. As long as the parser do parse the uppercase jsx to Foo.make, the make is only allowed for the react component. I think the error message should be polished.

EDIT: Plus, V4 allows only one react component per module.

cristianoc · 2022-07-21T07:42:16Z

```rescript
 @react.component
  let anotherComponent = (~b) => {
    React.string(b)
  }
It is correct. As long as the parser do parse the uppercase jsx to Foo.make, the make is only allowed for the react component. I think the error message should be polished.
EDIT: Plus, V4 allows only one react component per module.

The component does not need to be called make. This would be a very big breaking change.

mununki · 2022-07-21T07:47:05Z

@react.component
let make = (~b) => React.string(b)

It would be okay. Not sure I understand your point.

The component does not need to be called make.

Can you elaborate little more?

cristianoc · 2022-07-21T08:24:43Z

As in V3, the component can have any name. It can be called "default" to use file-level conventions for access from JS.
It could be called any name.

A direct <Foo ....> call, which obviously only works on components called make, is not the only way to consume a component.

mununki · 2022-07-21T08:42:09Z

I finally got your point, I verified the same error. It seems an unreasonable error even without actually calling the component from outside. I'll look at it.

mununki · 2022-07-21T08:49:30Z

I found the bug, I'll fix it soon!

mununki · 2022-07-21T08:56:20Z

As in V3, the component can have any name. It can be called "default" to use file-level conventions for access from JS. It could be called any name.

A direct <Foo ....> call, which obviously only works on components called make, is not the only way to consume a component.

This should be fixed 42032a2

cristianoc · 2022-07-22T05:20:35Z

@mattdamon108 component definitions seems to support uncurried function definitions of make and treat them like curried ones. (In V3, this is an error). Is this intentional?

Asking as it would be interesting to figure out whether uncurried components could be supported in future. (Where the component application calls make as an uncurried function).

mununki · 2022-07-22T06:37:59Z

That's not intentional, it is not in consideration actually.
It is very interesting. As far as I remember that the uncurried function will be the default, what would it affect the react component definition in the future? I expect that V3 will have a direct impact on the change, not V4. (V3 keep using labeled args as the original code, but V4 transforms labeled args to record as single arg)

cristianoc · 2022-07-22T07:12:20Z

I think that's the answer. Since V4 only ever takes 1 argument, it does not really matter if curried or uncurried is used. One can switch between the two easily.

cristianoc · 2022-07-22T07:17:08Z

One more issue.
An error without location is reported when a required prop is not supplied.

Example:

module C = {
  @react.component
  let make = ( ~x, ~y) => React.string(x++y)
}

let c = <C x="abc" />

Compilation error:

FAILED: src/V4.cmj

  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/rescript-compiler/jscomp/gentype_tests/typescript-react-example/src/V4.res

  Some record fields are undefined: y

FAILED: cannot make progress due to previous errors.

mununki · 2022-07-22T07:20:07Z

One more issue. An error without location is reported when a required prop is not supplied.

Example:

module C = {
  @react.component
  let make = ( ~x, ~y) => React.string(x++y)
}

let c = <C x="abc" />

Compilation error:

FAILED: src/V4.cmj

  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/rescript-compiler/jscomp/gentype_tests/typescript-react-example/src/V4.res

  Some record fields are undefined: y

FAILED: cannot make progress due to previous errors.

I'll look into it. Anyway I'm working on the raising errors with loc. It seems related issues.

cristianoc · 2022-07-22T07:20:54Z

It would be nice to have an explicit mode where after formatting, the result of running the ppx is left as normal code.
This could be very useful for debugging, and to make sure that the ppx transformation is not magic but is opt-in: the user can decide whether or not to use it. And might decide to use it only once, but check in the result directly.

cristianoc · 2022-07-22T07:24:58Z

One more issue. An error without location is reported when a required prop is not supplied.
Example:
module C = {
  @react.component
  let make = ( ~x, ~y) => React.string(x++y)
}

let c = <C x="abc" />
Compilation error:
FAILED: src/V4.cmj

  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/rescript-compiler/jscomp/gentype_tests/typescript-react-example/src/V4.res

  Some record fields are undefined: y

FAILED: cannot make progress due to previous errors.
I'll look into it. Anyway I'm working on the raising errors with loc. It seems related issues.

I can confirm it is a location issue: in the following:

// let c1 = C.make({x: "abc"})
// let c2 = <C x="abc" />

only c2 gives an error without location.

mununki · 2022-07-22T18:45:38Z

11 shoud be Fixed 89561df

FAILED: src/AnyName.cmj

  We've found a bug for you!
  /Users/woonki/Github/projects/react-ppx-v4/src/AnyName.res:7:11-22

  5 │ 
  6 │ // let c1 = C.make({x: "abc"})
  7 │ let c2 = <C x="abc" />
  8 │ // let c3 = React.jsx(C.make, {x: "abc"})
  9 │ 

  Some record fields are undefined: y

FAILED: cannot make progress due to previous errors.
>>>> Finish compiling(exit: 1)

cristianoc · 2022-07-22T19:05:49Z

What's up with 8: is there an actual example that has a problem?

mununki · 2022-07-22T19:14:10Z

It's mysterious 😄 You confirm that it is compiled just fine.#617 (comment) The same example is not compiled in my local.

cristianoc · 2022-07-22T19:16:44Z

Let me try to add it to a PR then.

mununki · 2022-07-22T19:17:03Z

Ah! I noticed it is different between V4 "classic" and "automatic"

cristianoc · 2022-07-22T19:37:05Z

@@jsxConfig({version: 4})

let optionMap = (x, f) =>
  switch x {
  | None => None
  | Some(v) => Some(f(v))
  }

module A = {
  @react.component
  let make = (~name=?) => <div ?name />
}

module Direct = {
  @react.component
  let make = (~name=?) => {
    <A
      name=?{
      optionMap(name, x => x)
      }
    />
  }
}

module Pipe = {
  @react.component
  let make = (~name=?) => {
    <A
      name=?{
        name->optionMap(x => x)
      }
    />
  }
}

cristianoc · 2022-07-22T19:40:56Z

Looks like it's the implementation of -> in the compiler creating trouble.
And it might also create trouble whenever it's used inside function application.

cristianoc · 2022-07-22T19:58:30Z

Here's a complete self-contained example without any ppx:

let optionMap = (x, f) =>
  switch x {
  | None => None
  | Some(v) => Some(f(v))
  }

type props<'name> = {key?: string, name?: string}

let name = None

let ok = {name: ?optionMap(name, x => x)}
let bad = {name: ?name->optionMap(x => x)}

cristianoc · 2022-07-22T19:59:11Z

Compiler bug on ->.

cristianoc · 2022-07-22T20:10:46Z

To confirm: this

let ok = {name:  @foo (optionMap(name, x => x))}
let bad = {name: @foo (name->optionMap(x => x))}

formats to

let ok = {name: @foo optionMap(name, x => x)}
let bad = {name: name->optionMap(x => x)}

which means the annotation @foo is eaten up by formatting.

cristianoc · 2022-07-22T20:12:06Z

And to simplify further, this:

let ok =  @foo (optionMap(name, x => x))
let bad = @foo (name->optionMap(x => x))

formats to this:

let ok = @foo optionMap(name, x => x)
let bad = name->optionMap(x => x)

cristianoc · 2022-07-22T20:20:43Z

Added examples here: #621

mununki · 2022-07-23T00:43:56Z

Wow! yes, I've mentioned the operator precedence is suspicious though #615 (comment) I think you're right and your approach will solve this issue!

cristianoc · 2022-07-23T01:19:39Z

Even though formatting is an issue, that needs to be fixed, I don't think it can be a cause of the bug you have observed (there's no printing involved, just parsing).
So I suspect there's at least one more issue after parsing, happening inside the compiler when -> is processed (as parsing seems OK).

cristianoc · 2022-07-23T01:33:08Z

The error one gets is the same as one would get by turning <A name=?{optionMap(name, x => x)} /> into <A name={optionMap(name, x => x)} /> i.e. removing ? i.e. removing @ns.optional from the AST.

mununki · 2022-07-23T01:53:51Z

The error one gets is the same as one would get by turning <A name=?{optionMap(name, x => x)} /> into <A name={optionMap(name, x => x)} /> i.e. removing ? i.e. removing @ns.optional from the AST.

Not sure I understand what you meant. Do you mean that there is still an error in the example without @ns.optional?
I thought it is fixed by PR #621 as the printer keeps the attribute @foo in printing.

cristianoc · 2022-07-23T02:25:03Z

As I said above there's no printing when you compile your original example: just parsing followed by type checking.
So I suspect a second bug is causing that.

cristianoc · 2022-07-23T02:51:15Z

Here's the bug: rescript-lang/rescript#5581

cristianoc · 2022-07-23T02:59:21Z

Merged.

mununki · 2022-07-23T03:00:16Z

I’ll test it

mununki · 2022-07-23T03:01:01Z

As I said above there's no printing when you compile your original example: just parsing followed by type checking.
So I suspect a second bug is causing that.

Now I get it, thanks!

mununki · 2022-07-23T04:34:46Z

I can confirm it fixed! 👍

cristianoc · 2022-07-23T04:49:33Z

Congrats on completing the first version of PPX V4.
This is one of the most elaborate features ever introduced in ReScript.

mununki · 2022-07-23T04:57:04Z

@cristianoc I reopen issue no. 8. I found it has the same error with 1 arity fn.

mununki · 2022-07-23T05:06:53Z

I'm not sure but suspicious here. https://github.com/rescript-lang/rescript-compiler/blob/e7c408ee63b86327bde3783a68cf673d475b4c8f/jscomp/frontend/ast_exp_apply.ml#L180

EDIT: Oops, maybe not

mununki · 2022-07-23T05:24:59Z

Ah! I think Pexp_ident case needs to be added. If I'm right, can I make a PR to fix it?

cristianoc · 2022-07-23T05:26:27Z

Ah! I think Pexp_ident case needs to be added. If I'm right, can I make a PR to fix it?

Would you like to try to fix it directly?

mununki · 2022-07-23T05:27:21Z

Ah! I think Pexp_ident case needs to be added. If I'm right, can I make a PR to fix it?

Would you like to try to fix it directly?

Yes, I'll give it a try!

cristianoc · 2022-07-23T05:27:24Z

Start by adding an example, just like the previous PR.

mununki · 2022-07-23T05:29:56Z

If you're not in hurry, can I start it when I get back?

mununki · 2022-07-23T10:53:35Z

Issue no. 8 is fixed by rescript-lang/rescript#5585

mununki · 2022-07-23T10:55:30Z

Congrats on completing the first version of PPX V4. This is one of the most elaborate features ever introduced in ReScript.

Thank you for everything. It can't be done without your help and lead. I learned a lot. I'll keep contributing to the compiler, tools, and community as much as I can.

cristianoc added this to the v10.1 "The New Features" milestone Jul 19, 2022

mununki mentioned this issue Jul 21, 2022

Fix jsx V4 issue with type constraints and components names other than "make". #617

Merged

3 tasks

mununki mentioned this issue Jul 22, 2022

Improve error with loc #620

Merged

mununki closed this as completed Jul 23, 2022

mununki reopened this Jul 23, 2022

mununki closed this as completed Jul 23, 2022

Battle test with JSX PPX V4 #615

Battle test with JSX PPX V4 #615

Comments

mununki commented Jul 19, 2022 • edited Loading

mununki commented Jul 19, 2022

mununki commented Jul 19, 2022

mununki commented Jul 21, 2022

cristianoc commented Jul 21, 2022

cristianoc commented Jul 21, 2022

mununki commented Jul 21, 2022 • edited Loading

cristianoc commented Jul 21, 2022

mununki commented Jul 21, 2022

mununki commented Jul 21, 2022 • edited Loading

cristianoc commented Jul 21, 2022

mununki commented Jul 21, 2022

cristianoc commented Jul 21, 2022

mununki commented Jul 21, 2022

mununki commented Jul 21, 2022

mununki commented Jul 21, 2022

cristianoc commented Jul 22, 2022 • edited Loading

mununki commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

mununki commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022 • edited Loading

mununki commented Jul 22, 2022

cristianoc commented Jul 22, 2022

mununki commented Jul 22, 2022 • edited Loading

cristianoc commented Jul 22, 2022

mununki commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

cristianoc commented Jul 22, 2022

mununki commented Jul 23, 2022

cristianoc commented Jul 23, 2022 • edited Loading

cristianoc commented Jul 23, 2022

mununki commented Jul 23, 2022 • edited Loading

cristianoc commented Jul 23, 2022 • edited Loading

cristianoc commented Jul 23, 2022

cristianoc commented Jul 23, 2022

mununki commented Jul 23, 2022

mununki commented Jul 23, 2022

mununki commented Jul 23, 2022

cristianoc commented Jul 23, 2022

mununki commented Jul 23, 2022

mununki commented Jul 23, 2022 • edited Loading

mununki commented Jul 23, 2022 • edited Loading

cristianoc commented Jul 23, 2022

mununki commented Jul 23, 2022

cristianoc commented Jul 23, 2022

mununki commented Jul 23, 2022

mununki commented Jul 23, 2022

mununki commented Jul 23, 2022

mununki commented Jul 19, 2022 •

edited

Loading

mununki commented Jul 21, 2022 •

edited

Loading

mununki commented Jul 21, 2022 •

edited

Loading

cristianoc commented Jul 22, 2022 •

edited

Loading

cristianoc commented Jul 22, 2022 •

edited

Loading

mununki commented Jul 22, 2022 •

edited

Loading

cristianoc commented Jul 23, 2022 •

edited

Loading

mununki commented Jul 23, 2022 •

edited

Loading

cristianoc commented Jul 23, 2022 •

edited

Loading

mununki commented Jul 23, 2022 •

edited

Loading

mununki commented Jul 23, 2022 •

edited

Loading