Fix @this attribute dropping parens #7025
Conversation
| let attrs_string attrs = | ||
| List.map (fun (attr, _) -> print_endline attr.Asttypes.txt) attrs | ||
|
|
||
| let ident_string ident = | ||
| match ident with | ||
| | Longident.Lident v -> v | ||
| | Longident.Ldot (_, v) -> v | ||
| | Longident.Lapply _ -> "Lapply" | ||
|
|
||
| let print_ct ct = | ||
| match ct with | ||
| | {ptyp_desc = Ptyp_constr (ident, _); ptyp_attributes = attrs} -> | ||
| let _ = | ||
| print_endline | ||
| ("Ptyp_constr: " ^ ident_string ident.txt ^ " attrs: " | ||
| ^ string_of_int (List.length attrs)) | ||
| in | ||
| let _ = attrs_string attrs in | ||
| () | ||
| | {ptyp_desc = Ptyp_arrow _; ptyp_attributes = attrs} -> | ||
| print_endline | ||
| ("Ptyp_arrow: " ^ " attrs: " ^ string_of_int (List.length attrs)) | ||
| | {ptyp_desc = Ptyp_variant _} -> print_endline "Ptyp_variant" | ||
| | _ -> print_endline "Something else" |
There was a problem hiding this comment.
Just some debugging code, I'll remove it if we think the solution to this is fine.
| let print_arrow ?(arity = max_int) typ_expr = | ||
| let attrs_before, args, return_type = | ||
| ParsetreeViewer.arrow_type ~arity typ_expr | ||
| ParsetreeViewer.arrow_type ~arity ~attrs:parent_attrs typ_expr |
There was a problem hiding this comment.
Here's where the issue lies, the @this attribute is attached to the Ptyp_constr not Ptyp_arrow. Essentially what I'm doing here is just picking up the attributes from the Ptyp_constr and pass it as the initial attrs for the ParsetreeViewer.arrow_type to work with.
| @@ -1841,6 +1845,8 @@ and print_typ_expr ~(state : State.t) (typ_expr : Parsetree.core_type) cmt_tbl = | |||
| in | |||
| let should_print_its_own_attributes = | |||
| match typ_expr.ptyp_desc with | |||
| | Ptyp_constr _ when Ast_uncurried.core_type_is_uncurried_fun typ_expr -> | |||
There was a problem hiding this comment.
Let the attribute printing handled by print_typ_expr, otherwise it's gonna cause duplicated attributes.
There was a problem hiding this comment.
We need to add a task to remove dead code once uncurried is the only possible function type
|
If they are okay, I'll clean this PR and add more tests. The linked diff is the only broken existing test. |
|
Awesome! Output diff looks ok to me. Let's ask @IwanKaramazow and @cristianoc to have a look at the changes, too. |
|
@cristianoc @zth could you have a look at this, too?
Is this approach fine? Or is there something that should be changed in the parser? |
|
My hunch would be that the parser should parse this correctly instead. But I'm not very familiar with the parser. @cristianoc what do you think? |
|
What's special about |
It should just be like any other annotation. This PR just changed how the attribute is printed (i.e. with parens). |
OK sorry same Q: does |
Which answer itself as soon as one looks at the changed files. So please ignore the question. |
shulhi
force-pushed
the
fix-arrow-attr-parens
branch
2 times, most recently
from
fb7b67e to
9254df9
Compare
Check for existence of attr at Ptyp_constr (function$) as the @this attribute is attached to the Ptyp_constr, not the Ptyp_arrow.
shulhi
force-pushed
the
fix-arrow-attr-parens
branch
from
9254df9 to
0b48963
Compare
Fix #6638
@cknitt Can you look at the diff output for the test and see whether they are acceptable in terms of formatting changes? They look okay to me. https://github.com/rescript-lang/rescript-compiler/pull/7025/files#diff-2333e082552a974a4c2fc20db5dbd0415ae96a46b675d967a6d9804f7497ad08
I've checked all the attributes we have here (https://rescript-lang.org/syntax-lookup) and there are only a few that is at field level (e.g.
@as,@this, etc).I had worked on this during the retreat but Max preferred not to hard code attribute value for the check, so this PR try to generalize the check without checking for specific attribute. (Prev PR: #6773).