Skip to content

Pipe expression for optional record fields expects non-optional type #5627

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nkrkv opened this issue Aug 31, 2022 · 16 comments
Closed

Pipe expression for optional record fields expects non-optional type #5627

nkrkv opened this issue Aug 31, 2022 · 16 comments
Labels
Milestone

Comments

@nkrkv
Copy link
Contributor

nkrkv commented Aug 31, 2022

Experimenting with Playground. The following compiles with an error:

type t = {
  optValue?: int,
}

let x = Some(3)
let s1 = {optValue: ?(x->Belt.Option.map(a => a+1))} // <-- Type error
let s2 = {optValue: ?(Belt.Option.map(x, a => a+1))}
Type Errors
[E] Line 6, column 22:

This has type: option<int>
  Somewhere wanted: int

Screenshot:

image

So, the pipe expression highlighted has indeed type option<int> but the compiler wants a plain int for some reason.

Interesting, the same expression without the pipe compiles just fine.

@cristianoc
Copy link
Collaborator

Do you have a repro that does not use the ? syntax?
Does it also happen on a record with an explicit option type?

@cristianoc cristianoc added this to the v10.1 milestone Sep 1, 2022
@cristianoc
Copy link
Collaborator

Possibly related? rescript-lang/syntax#203

@mununki
Copy link
Member

mununki commented Sep 1, 2022

Maybe? #5585

@cristianoc
Copy link
Collaborator

Maybe? #5585

Tried on master and this issue is still there.

@nkrkv
Copy link
Contributor Author

nkrkv commented Sep 1, 2022

Here’s some extra cases. Not sure if you meant that for repro:

type t = {
  optValue?: int,
  explicit: option<int>,
}

let x = Some(3)
let s1 = {explicit: (x->Belt.Option.map(a => a+1))} // <-- No error
let s2 = {explicit: None, optValue: @optional (x->Belt.Option.map(a => a+1))} // <-- Error

Possibly related? rescript-lang/syntax#203

I thought about it, but arguably no. I’ve put parens around the expression to exclude ? vs -> precedence issues.

@cristianoc
Copy link
Collaborator

Let's see what the parser does.

Input:

type t = {optValue?: int}

let x = Some(3)
let s1 = {optValue: ? (x->Belt.Option.map(a => a + 1)) } // <-- Type error
let s2 = {optValue: ? (Belt.Option.map(x, a => a + 1))}

Result:

type nonrec t = {
  optValue: int [@ns.optional ]}
let x = Some 3
let s1 = { optValue = ((x |. (Belt.Option.map (fun a -> a + 1)))[@ns.optional ]) }
let s2 = { optValue = ((Belt.Option.map x (fun a -> a + 1))[@ns.optional ]) }

@cristianoc
Copy link
Collaborator

That looks fine. It seems likely, the annotation is moved around during the processing of pipe in the compiler.

@cristianoc
Copy link
Collaborator

Let's do a formatting test now.

This:

let s1 = {optValue: @foo (x->Belt.Option.map(a => a + 1))}
let s2 = {optValue: @foo (Belt.Option.map(x, a => a + 1))}

Gets formatted to:

let s1 = {optValue: x->Belt.Option.map(a => a + 1)}
let s2 = {optValue: @foo Belt.Option.map(x, a => a + 1)}

so the annotation is eaten up in the first case

@cristianoc
Copy link
Collaborator

So it seems that the situation is:

  • ? gets transformed to @bs.optional
  • @bs.optional is not preserved in the AST at that position

Next up: investigate the reformatting issue.

cristianoc added a commit to rescript-lang/syntax that referenced this issue Sep 1, 2022
@cristianoc cristianoc added the bug label Sep 1, 2022
@cristianoc
Copy link
Collaborator

The formatting issue affects |> too, so formatting could be a red herring, unless the same problematic example can be constructed using |>.

@cristianoc
Copy link
Collaborator

This works fine:

let s1 = {optValue: ?((a => a+1) |> Belt.Option.map(x))}

bringing more credibility to the red herring hypothesis.

@cristianoc
Copy link
Collaborator

Maybe? #5585

Tried on master and this issue is still there.

No I didn't.
It is in fact fixed in master.
Oh well, this turned out an unrelated printing issue at least.

cristianoc added a commit to rescript-lang/syntax that referenced this issue Sep 2, 2022
@vknez
Copy link

vknez commented Mar 9, 2023

@cristianoc Sorry to bother you here, but this is still not valid:

price: ?variant.price.amount->Float.fromString
// instead I need to do
price: ?Float.fromString(variant.price.amount)

Is the first line supposed to work?

#5733

@cristianoc
Copy link
Collaborator

@cristianoc Sorry to bother you here, but this is still not valid:

price: ?variant.price.amount->Float.fromString
// instead I need to do
price: ?Float.fromString(variant.price.amount)

Is the first line supposed to work?

#5733

Can you give a self-contained example, e.g. a link to the playground.

@vknez
Copy link

vknez commented Mar 11, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants