Fix issues where pipe "->" processing eats up attributes in 1 arity function

[mununki]

This PR is the following PR to #5581 and fixes same issue in the single arity function.

[mununki]

@cristianoc I'm not sure, but I think here needs same fix as #5581 https://github.com/rescript-lang/rescript-compiler/blob/e7c408ee63b86327bde3783a68cf673d475b4c8f/jscomp/frontend/ast_exp_apply.ml#L180 Am I correct? If so, I'm gonna add commits to keep the attributes and Pexp_ident here too.

[cristianoc]

Not sure I understand this.
It does not seem only about attributes.
Apart from attributes, what else changes without this case?
Is this case perhaps handled somewhere below?

[mununki]

I got your point. I think this commit change handles in more comprehensive cases. 16a42b7

[cristianoc]

Indeed it's handled below in:

| _ -> Ast_compatible.app1 ~loc fn new_obj_arg

and can be handled as

{(Ast_compatible.app1 ~loc fn new_obj_arg) with pexp_attributes = e.pexp_attributes}

[cristianoc]

I got your point. I think this commit change handles in more comprehensive cases. 16a42b7

perfect

[mununki]

It seems same, doesn't it?

Ast_compatible.app1 ~loc ~attrs:e.pexp_attributes fn new_obj_arg

[cristianoc]

@cristianoc I'm not sure, but I think here needs same fix as #5581

https://github.com/rescript-lang/rescript-compiler/blob/e7c408ee63b86327bde3783a68cf673d475b4c8f/jscomp/frontend/ast_exp_apply.ml#L180

Am I correct? If so, I'm gonna add commits to keep the attributes and Pexp_ident here too.

It's a good question.
First of all, can you trigger this example with an example?

[cristianoc]

I wonder whether Ast_open_cxt.destruct should be removed. That is for foo -> M.(suff) and that syntax does not exist in ReScript. So at the moment that case only complicates the implementation of ->.

[cristianoc]

So don't even try to find an example: it does not exist. Unless the example is written in .ml syntax.

[mununki]

I wonder whether Ast_open_cxt.destruct should be removed. That is for foo -> M.(suff) and that syntax does not exist in ReScript. So at the moment that case only complicates the implementation of ->.

Understood. Do you want me to remove the destruct part or leave for now?

[cristianoc]

I wonder whether Ast_open_cxt.destruct should be removed. That is for foo -> M.(suff) and that syntax does not exist in ReScript. So at the moment that case only complicates the implementation of ->.

Understood. Do you want me to remove the destruct part or leave for now?

Maybe leave it as it is and create a reminder issue.

[mununki]

I wonder whether Ast_open_cxt.destruct should be removed. That is for foo -> M.(suff) and that syntax does not exist in ReScript. So at the moment that case only complicates the implementation of ->.

Understood. Do you want me to remove the destruct part or leave for now?

Maybe leave it as it is and create a reminder issue.

Got it. I've tried and think the tests need to be fixed too. Even though those test are *.ml

[mununki]

I think this PR is done now.

[cristianoc]

I had a look and while the foo -> M.(...) examples can be converted from .ml syntax to .res (creating {open M; ...}), the -> ppx does not work on the result of the conversion.
So it's not clear whether that kind of example should be supported. And if it should, not in the way it is supported now, which can possibly only work for ml in a way that cannot even be manually converted to .res.

[cristianoc]

I've done it in a separate PR, which can be looked at with calm -- don't need to do it now.

#5587

[cristianoc]

The other thing is, the comments suggest that -> works for uncurried functions.
But I don't believe it does.

[cristianoc]

This is ready to merge right?