rescript-lang / syntax

ReScript's syntax as a standalone repo.
MIT License
256 stars 38 forks source link

Type error on empty record with fragments in automatic mode #703

Closed cristianoc closed 1 year ago

cristianoc commented 1 year ago

This:

module Automatic = {
  @react.component
  let make = () => {
    <> </>
  }
}

produces:

module Automatic = {
  type props = {}

  @react.component
  let make = (_: props) => {
    React.jsx(React.jsxFragment, {children: {}})
  }
  let make = {
    let \"OptionalKeyType$Automatic" = props => make(props)

    \"OptionalKeyType$Automatic"
  }
}

which gives a type errow (without location information):

  We've found a bug for you!
  /Users/cristianocalcagno/GitHub/rescript-vscode/analysis/tests/src/Completion.res

  Empty record literal {} should be type annotated or used in a record context.
mununki commented 1 year ago

I'll look into it

cristianoc commented 1 year ago

Probably one need to add a type annotation just like in other cases : {} : props<_>

mununki commented 1 year ago

babel output

I think it should be:

React.jsx(React.jsxFragment, {}: props<_>)
mununki commented 1 year ago

Or, in rescript-react

// React.res
type fragmentProps<'children> = {children?: 'children} // ? optional

then

React.jsx(React.jsxFragment, {})

It works fine

cristianoc commented 1 year ago

Or, in rescript-react

// React.res
type fragmentProps<'children> = {children?: 'children} // ? optional

then

React.jsx(React.jsxFragment, {})

It works fine

This looks cleaner.

mununki commented 1 year ago

I'll make a PR soon.

cristianoc commented 1 year ago

The one thing to check is, that when children is actually present, the compiler understands it's a record type inside, and does not add any of that runtime magic for optionals.

mununki commented 1 year ago

The one thing to check is, that when children is actually present, the compiler understands it's a record type inside, and does not add any of that runtime magic for optionals.

If I understand correctly, isn't it same to the uppercase and lowercase component too? Z.props would have children props as optional and Jsx.domProps has an optional children field already.

<Z /> // jsx(Z.make, {})
<Z> ... </Z> // jsx(Z.make, {children: ...})

<div /> // jsx("div", {})
<div> ... </div> // jsx("div", {children: ... })

I think the current type fragmentProps is wrong. The children field should be optional as we discussed, because the fragment could have children or not.