Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
faa9510
add test of pipe "->" eats up attrs in 1 arity fn
mununki Jul 23, 2022
d5b55b5
fix pipe "->" eats up attrs in 1 arity fn
mununki Jul 23, 2022
16a42b7
fix pipe "-> eats up attrs in comprehensive cases
mununki Jul 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions jscomp/frontend/ast_exp_apply.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,12 @@ let app_exp_mapper (e : exp) (self : Bs_ast_mapper.mapper) (fn : exp)
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| Pexp_ident _ ->
Copy link
Collaborator

@cristianoc cristianoc Jul 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

perfect

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems same, doesn't it?

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

{
pexp_desc = Pexp_apply (fn, [ (Nolabel, new_obj_arg) ]);
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| _ -> (
match Ast_open_cxt.destruct fn [] with
| ( { pexp_desc = Pexp_tuple xs; pexp_attributes = tuple_attrs },
Expand Down
18 changes: 18 additions & 0 deletions jscomp/test/res_debug.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@ var bad = {
name: bad_name
};

function identity(x) {
return x;
}

var name1 = "ReScript";

var ok1 = {
name: name1
};

var bad1 = {
name: name1
};

var v2 = newrecord;

var v1 = {
Expand All @@ -73,4 +87,8 @@ exports.optionMap = optionMap;
exports.name = name;
exports.ok = ok;
exports.bad = bad;
exports.identity = identity;
exports.name1 = name1;
exports.ok1 = ok1;
exports.bad1 = bad1;
/* Not a pure module */
9 changes: 8 additions & 1 deletion jscomp/test/res_debug.res
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,11 @@ type props<'name> = {key?: string, name?: string}
let name = None

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

let identity = x => x

let name1 = Some("ReScript")

let ok1 = {name: ?identity(name1)}
let bad1 = {name: ?name1->identity}
6 changes: 6 additions & 0 deletions lib/4.06.1/unstable/js_compiler.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269546,6 +269546,12 @@ let app_exp_mapper (e : exp) (self : Bs_ast_mapper.mapper) (fn : exp)
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| Pexp_ident _ ->
{
pexp_desc = Pexp_apply (fn, [ (Nolabel, new_obj_arg) ]);
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| _ -> (
match Ast_open_cxt.destruct fn [] with
| ( { pexp_desc = Pexp_tuple xs; pexp_attributes = tuple_attrs },
Expand Down
6 changes: 6 additions & 0 deletions lib/4.06.1/unstable/js_playground_compiler.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271009,6 +271009,12 @@ let app_exp_mapper (e : exp) (self : Bs_ast_mapper.mapper) (fn : exp)
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| Pexp_ident _ ->
{
pexp_desc = Pexp_apply (fn, [ (Nolabel, new_obj_arg) ]);
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| _ -> (
match Ast_open_cxt.destruct fn [] with
| ( { pexp_desc = Pexp_tuple xs; pexp_attributes = tuple_attrs },
Expand Down
6 changes: 6 additions & 0 deletions lib/4.06.1/whole_compiler.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281299,6 +281299,12 @@ let app_exp_mapper (e : exp) (self : Bs_ast_mapper.mapper) (fn : exp)
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| Pexp_ident _ ->
{
pexp_desc = Pexp_apply (fn, [ (Nolabel, new_obj_arg) ]);
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes;
}
| _ -> (
match Ast_open_cxt.destruct fn [] with
| ( { pexp_desc = Pexp_tuple xs; pexp_attributes = tuple_attrs },
Expand Down