Skip to content
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

Fix position of expressions regarding of comments in infix-op expressions #970

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
35 changes: 35 additions & 0 deletions src/Ast.ml
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,8 @@ and Requires_sub_terms : sig
val parenze_pat : pattern In_ctx.xt -> bool

val parenze_exp : expression In_ctx.xt -> bool

val parenze_nested_exp : expression In_ctx.xt -> bool
end = struct
open In_ctx

Expand Down Expand Up @@ -2393,7 +2395,40 @@ end = struct
| Ptyp_tuple l -> exposed_right_typ (List.last_exn l)
| Ptyp_object _ -> true
| _ -> false

let parenze_nested_exp {ctx; ast= exp} =
let infix_prec ast =
match ast with
| Exp {pexp_desc= Pexp_apply (e, _); _} when is_infix e ->
prec_ast ast
| Exp
( { pexp_desc=
Pexp_construct
( {txt= Lident "::"; loc= _}
, Some
{ pexp_desc= Pexp_tuple [_; _]
; pexp_loc= _
; pexp_attributes= _
; _ } )
; pexp_loc= _
; pexp_attributes= _
; _ } as exp )
when not (is_sugared_list exp) ->
prec_ast ast
| _ -> None
in
(* Make the precedence explicit for infix operators *)
match (infix_prec ctx, infix_prec (Exp exp)) with
| Some (InfixOp0 | ColonEqual), _ | _, Some (InfixOp0 | ColonEqual) ->
(* special case for refs update and all InfixOp0 to reduce parens
noise *)
false
| None, _ | _, None -> false
| Some p1, Some p2 -> Poly.(p1 <> p2)
end

include In_ctx
include Requires_sub_terms

let is_monadic_expr e =
match prec_ast (Exp e) with Some InfixOp0 -> true | _ -> false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This causes a break just after |> operators

-      |> List.sort
-           ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)
+      |>
+      List.sort ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)

Copy link
Collaborator Author

@gpetiot gpetiot Aug 26, 2019

Choose a reason for hiding this comment

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

Yes, that is the new behavior for all these operators, that's why I was filtering a bit (size > 2, checking the last character), so should |> be treated like the rest or not?
I don't really have an opinion on that, and @craigfe has a diverging opinion, so if the both of you could agree, that would be great :D

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I also think this break looks odd. My toy example was using infix operators at the end of each line, a la:

(* Open the repo *)
initialise >>=
(* Perform a subsequent action *)
subsequent_action >|=

but when I'm writing this sort of code personally, I keep the infix operators at the start:

x
>>= foo
>>= bar

(or the same with |>). I don't really mind which of the two OCamlformat chooses to pick (this is probably a reasonably important decision), but I do care about its interaction with comments. i.e., this looks wrong to me:

initialise
>>= (* Perform a subsequent action *)
    subsequent_action 
>|= (* Keep going... *)
    another_action

And if OCamlformat chooses to enforce one or the other, there is the question of what to do with input flies that are doing it the other way (comments before/after the infix operator).

Copy link
Contributor

Choose a reason for hiding this comment

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

(this may be an instance where it's worth having an option to select between prepending or appending infix operators)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that would be good to have that but it requires a bigger rewrite of the code, the sequences of infix operations are currently sugared into a weird structure that mix different operators (as long as they have the same precedence, e.g. >>= and >|=), so it's not as simple as it was for sequences of items separated by ; only.

6 changes: 6 additions & 0 deletions src/Ast.mli
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ val index_op_set_sugar :
-> ((string * Char.t * Char.t) Location.loc * expression list * expression)
option

val is_monadic_expr : expression -> bool

val is_monadic_binding_id : string -> bool

val is_monadic_binding : expression -> bool
Expand Down Expand Up @@ -206,6 +208,10 @@ val parenze_exp : expression xt -> bool
(** [parenze_exp xexp] holds when expression-in-context [xexp] should be
parenthesized. *)

val parenze_nested_exp : expression xt -> bool
(** [parenze_nested_exp xexp] holds when nested expression-in-context [xexp]
should be parenthesized. *)

val parenze_mty : module_type xt -> bool
(** [parenze_mty xmty] holds when module_type-in-context [xmty] should be
parenthesized. *)
Expand Down
23 changes: 12 additions & 11 deletions src/Cmts.ml
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,8 @@ let relocate (t : t) ~src ~before ~after =
Location.fmt before Location.fmt after ;
let merge_and_sort x y =
List.rev_append x y
|> List.sort
~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)
|>
List.sort ~compare:(Comparable.lift Location.compare_start ~f:Cmt.loc)
in
update_multi t.cmts_before src before ~f:merge_and_sort ;
update_multi t.cmts_after src after ~f:merge_and_sort ;
Expand Down Expand Up @@ -626,15 +626,16 @@ let has_after t loc =
let remaining_comments t =
let get t before_after =
Hashtbl.to_alist t
|> List.concat_map ~f:(fun (ast_loc, cmts) ->
List.map cmts ~f:(fun (cmt : Cmt.t) ->
( cmt
, before_after
, let open Sexp in
List
[ List [Atom "ast_loc"; Location.sexp_of_t ast_loc]
; List [Atom "cmt_loc"; Location.sexp_of_t cmt.loc]
; List [Atom "cmt_txt"; Atom cmt.txt] ] )))
|>
List.concat_map ~f:(fun (ast_loc, cmts) ->
List.map cmts ~f:(fun (cmt : Cmt.t) ->
( cmt
, before_after
, let open Sexp in
List
[ List [Atom "ast_loc"; Location.sexp_of_t ast_loc]
; List [Atom "cmt_loc"; Location.sexp_of_t cmt.loc]
; List [Atom "cmt_txt"; Atom cmt.txt] ] )))
in
List.concat
[ get t.cmts_before "before"
Expand Down
Loading