Skip to content

Mark return expressions in parsed and typed ASTs #7898

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Mark return expressions in parsed and typed ASTs #7898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion analysis/src/CompletionFrontEnd.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@ let rec exprToContextPathInner ~(inJsxContext : bool) (e : Parsetree.expression)
{funct = d; args = (Nolabel, lhs) :: args; partial; transformed_jsx};
pexp_loc;
pexp_attributes;
pexp_is_return = false;
}
| Pexp_apply
({
Expand All @@ -294,11 +295,18 @@ let rec exprToContextPathInner ~(inJsxContext : bool) (e : Parsetree.expression)
Pexp_apply
{
app with
funct = {pexp_desc = Pexp_ident id; pexp_loc; pexp_attributes};
funct =
{
pexp_desc = Pexp_ident id;
pexp_loc;
pexp_attributes;
pexp_is_return = false;
};
args = [(Nolabel, lhs)];
};
pexp_loc;
pexp_attributes;
pexp_is_return = false;
}
| Pexp_apply {funct = e1; args} -> (
match exprToContextPath ~inJsxContext e1 with
Expand Down
8 changes: 8 additions & 0 deletions compiler/frontend/ast_compatible.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ let apply_simple ?(loc = default_loc) ?(attrs = []) (fn : expression)
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc =
Pexp_apply
{
Expand All @@ -49,6 +50,7 @@ let app1 ?(loc = default_loc) ?(attrs = []) fn arg1 : expression =
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc =
Pexp_apply
{
Expand All @@ -63,6 +65,7 @@ let app2 ?(loc = default_loc) ?(attrs = []) fn arg1 arg2 : expression =
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc =
Pexp_apply
{
Expand All @@ -77,6 +80,7 @@ let app3 ?(loc = default_loc) ?(attrs = []) fn arg1 arg2 arg3 : expression =
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc =
Pexp_apply
{
Expand All @@ -91,6 +95,7 @@ let fun_ ?(loc = default_loc) ?(attrs = []) ?(async = false) ~arity pat exp =
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc =
Pexp_fun
{
Expand All @@ -108,13 +113,15 @@ let const_exp_string ?(loc = default_loc) ?(attrs = []) ?delimiter (s : string)
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc = Pexp_constant (Pconst_string (s, delimiter));
}

let const_exp_int ?(loc = default_loc) ?(attrs = []) (s : int) : expression =
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc = Pexp_constant (Pconst_integer (string_of_int s, None));
}

Expand All @@ -123,6 +130,7 @@ let apply_labels ?(loc = default_loc) ?(attrs = []) fn
{
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
pexp_desc =
Pexp_apply
{
Expand Down
3 changes: 3 additions & 0 deletions compiler/frontend/ast_exp_apply.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ let app_exp_mapper (e : exp) (self : Bs_ast_mapper.mapper) : exp =
{funct = fn1; args = (Nolabel, a) :: args; partial; transformed_jsx};
pexp_loc = e.pexp_loc;
pexp_attributes = e.pexp_attributes @ f.pexp_attributes;
pexp_is_return = false;
}
Comment on lines 96 to 100
Copy link

Choose a reason for hiding this comment

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

[P1] Preserve return markers when rewriting application expressions

The new pexp_is_return flag is set by Res_return_marker before the built‑in PPX rewrites applications, but Ast_exp_apply.app_exp_mapper now recreates the expression with pexp_is_return = false unconditionally. Any call expression that passes through this mapper (which is invoked for every Pexp_apply in bs_builtin_ppx) will therefore lose its return annotation even when it was in tail position, so the typed AST never exposes exp_is_return for the common case of let f x = g x. The mapper should carry over the original expression’s flag (and similar for nested cases) rather than resetting it.

Useful? React with 👍 / 👎.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@codex see if you can repro the issue in a test

Copy link

Choose a reason for hiding this comment

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

Summary

Testing

  • ⚠️ dune runtest tests/ounit_tests (fails because dune is not available in the container)

View task →

| Pexp_tuple xs ->
bound a (fun bounded_obj_arg ->
Expand All @@ -122,11 +123,13 @@ let app_exp_mapper (e : exp) (self : Bs_ast_mapper.mapper) : exp =
transformed_jsx;
};
pexp_attributes = [];
pexp_is_return = false;
pexp_loc = fn.pexp_loc;
}
| _ ->
Ast_compatible.app1 ~loc:fn.pexp_loc fn bounded_obj_arg));
pexp_attributes = f.pexp_attributes;
pexp_is_return = false;
pexp_loc = f.pexp_loc;
})
| _ -> Ast_compatible.app1 ~loc ~attrs:e.pexp_attributes f a)
Expand Down
3 changes: 3 additions & 0 deletions compiler/frontend/ast_external_mk.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ let local_external_apply loc ?(pval_attributes = []) ~(pval_prim : string list)
Pexp_ident
{txt = Ldot (Lident local_module_name, local_fun_name); loc};
pexp_attributes = [];
pexp_is_return = false;
pexp_loc = loc;
}
: Parsetree.expression)
Expand Down Expand Up @@ -90,6 +91,7 @@ let local_external_obj loc ?(pval_attributes = []) ~pval_prim ~pval_type
Pexp_ident
{txt = Ldot (Lident local_module_name, local_fun_name); loc};
pexp_attributes = [];
pexp_is_return = false;
pexp_loc = loc;
}
: Parsetree.expression)
Expand Down Expand Up @@ -126,5 +128,6 @@ let local_extern_cont_to_obj loc ?(pval_attributes = []) ~pval_prim ~pval_type
Pexp_ident
{txt = Ldot (Lident local_module_name, local_fun_name); loc};
pexp_attributes = [];
pexp_is_return = false;
pexp_loc = loc;
} )
1 change: 1 addition & 0 deletions compiler/frontend/ast_open_cxt.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ let restore_exp (xs : Parsetree.expression) (qualifiers : t) :
({
pexp_desc = Pexp_open (flag, lid, x);
pexp_attributes = attrs;
pexp_is_return = false;
pexp_loc = loc;
}
: Parsetree.expression))
7 changes: 6 additions & 1 deletion compiler/ml/ast_helper.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,12 @@ end

module Exp = struct
let mk ?(loc = !default_loc) ?(attrs = []) d =
{pexp_desc = d; pexp_loc = loc; pexp_attributes = attrs}
{
pexp_desc = d;
pexp_loc = loc;
pexp_attributes = attrs;
pexp_is_return = false;
}
let attr d a = {d with pexp_attributes = d.pexp_attributes @ [a]}

let ident ?loc ?attrs a = mk ?loc ?attrs (Pexp_ident a)
Expand Down
1 change: 1 addition & 0 deletions compiler/ml/parsetree.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ and expression = {
pexp_loc: Location.t;
(* Hack: made pexp_attributes mutable for use in analysis exe. Please do not use elsewhere! *)
mutable pexp_attributes: attributes; (* ... [@id1] [@id2] *)
mutable pexp_is_return: bool;
}

and expression_desc =
Expand Down
Loading
Loading