ormolu icon indicating copy to clipboard operation
ormolu copied to clipboard

do notation in left operand looks ambiguous

Open roberth opened this issue 4 years ago • 18 comments

Ormolu formats to this code, which isn't very clear:

y = do
  _ <- Nothing
  undefined
  <|> do
    Just 2

If you're not intimately familiar with do syntax, you may wonder whether y is Nothing or Just 2.

This layout is easier to understand:

y = do
    _ <- Nothing
    undefined
  <|> do
    Just 2

roberth avatar Mar 27 '20 10:03 roberth

I think we should try to shift the infix operator one indentation step to the left in this particular case. As the last resort we could also shift half-step, but we need a good reason to do that.

mrkkrp avatar Oct 07 '21 08:10 mrkkrp

The desired output for the example provided by the OP:

y = do
  _ <- Nothing
  undefined
 <|> do
    Just 2

mrkkrp avatar Oct 11 '21 09:10 mrkkrp

If the LHS do block is singleline, should we also half-indent (I don't think so, but I just want to be sure)?

y = do undefined
 <|> do
    Just 2

tbagrel1 avatar Oct 11 '21 14:10 tbagrel1

I don't know. It looks a little messy, but so does the current formatting. What's the alternative?

roberth avatar Oct 11 '21 14:10 roberth

@roberth, were you responding to @mrkkrp comment or mine? I don't really think we have an alternative for the multiline LHS do block that would be somewhat consistent with the rest of ormolu formatting rules.

For the singleline do case, we could either keep the current behaviour (a full indent step before <|>), or make it consistent with the multiline case (a half indent step before <|>).

tbagrel1 avatar Oct 13 '21 06:10 tbagrel1

I was responding to @tbagrel1 about the single line do.

I don't have a strong opinion about the single line case. Does a half indent lead to a smaller line-based diff when going from single line to multi-line do?

roberth avatar Oct 13 '21 08:10 roberth

@roberth, yes, I suppose the diff is smaller when going from single to multi-line do if we stay consistent for both single-line and multi-line do blocks.

I currenlty have a hard time figuring out what would be the desired output of a chain of {do block} op {do block} op {do block}...:

Option 1:

x =
  do
    something
   <> do
      something
   <> do
      something
   <> do
      something

Option 2:

x =
  do
    something
   <> do
      something
     <> do
        something
       <> do
          something

My current implementation gives this:

x =
  do
    something
   <> do
      something
    <> do
      something
    <> do
      something

which is not acceptable

tbagrel1 avatar Oct 20 '21 06:10 tbagrel1

Shouldn't the first <> in your current implementation be non-indented because it is technically in a hanging context?

x =
  do
    something
   <> do    -- unnecessary indent
      something
    <> do   -- correct given context but still indented too much because of first <>'s rhs
      something
    <> do   -- correct given context but still indented too much because of first <>'s rhs
      something

roberth avatar Oct 20 '21 08:10 roberth

On another note, I'm surprised to learn that operators are not sensitive to indentation. I figured you needed a test case for this, but apparently you don't. It's all the same.

Slightly off-topic

-- > points to the tail
infixr 1 %>

-- < points to the tail
infixl 1 <%

(%>) :: String -> String -> String
a %> b = "[" <> a <> ", " <> b <> "]"

(<%) :: String -> String -> String
a <% b = "(" <> a <> ", " <> b <> ")"

rr =
  "a"
    %>
      "b"
      %>
        "c"

rl =
 
        "a"
      %>
        "b"
  %>
    "c"


lr =
  "a"
    <%
      "b"
      <%
        "c"

ll =
        "a"
      <%
        "b"
  <%
    "c"

main = do
  print rr
  print rl
  print lr
  print ll

{-
"[a, [b, c]]"
"[a, [b, c]]"
"((a, b), c)"
"((a, b), c)"
-}

roberth avatar Oct 20 '21 08:10 roberth

@tbagrel1 For that one, let's go with the option 1. I think the lesser evil here is to have all operators equally "half-indented". Regarding your question about ::

x = {do xxx} : {do xxx} : {do xxx} ...

I don't see any problem with placing : at the end of the line like usual.

mrkkrp avatar Oct 20 '21 13:10 mrkkrp

@roberth The implementation I displayed above was clearly wrong. It was the result of a naive patch, which doesn't work well for chains of do blocks.

While experimenting with this issue, I also found that ormolu is currently unable to format this:

b =
  1 :
    do
      2
     : do
        3
       : do
          [4]

@mrkkrp, if I understand correctly, we should do something like this? :

x =
  1 :
  do
    2 :
  do
    3 :
  do
    [4]

In this context, is the : applied only to the Int, or applied to the whole do block?

tbagrel1 avatar Oct 20 '21 13:10 tbagrel1

Good question. It's hard to tell :-D Seriously though, It looks like (:) will be applied to the whole do block. And yes, that's what I had in mind. Simply because it is easier to spot (:) than deduce that all operands are do blocks (what to do with a mixed sequence?).

mrkkrp avatar Oct 20 '21 13:10 mrkkrp

At the beginning, I tried something like this, which would work even with mixed sequences

x =
  1 :
  do
    2
   :
  do
    3
   :
  do
    [4]

But I got parsing errors when the do block is in hanging position, and it would be hard to implement cleanly

tbagrel1 avatar Oct 20 '21 14:10 tbagrel1

I can't find a way to format do : do : do : do chains the way you want @mrkkrp, because of this case (data/examples/declaration/value/function/list-notation-3.hs):

foo =
  reportSDoc "tc.cc" 30 $ sep $ do
    (prettyTCM q <+> " before compilation") :
      do
        map (prettyTCM . map unArg . clPats) cls

where the indent after the : is mandatory (which contradicts the no-indent rule for : chains)

Do you know what could be the criterion/selector to know if we need to indent?

tbagrel1 avatar Oct 21 '21 07:10 tbagrel1

Can we have this rule: if at least one operand in an operator chain is a do block, then do this:

x =
  1
 : do
  blah
  blah
 : do
  foo
  bar

Frankly, I think this is not a common kind of expression, so we can do whatever is easier to implement in a consistent way.

mrkkrp avatar Oct 21 '21 14:10 mrkkrp

This issue might be linked to https://github.com/tweag/ormolu/issues/785 (at least, in the code, we will need to update the same p_exprOpTree function)

tbagrel1 avatar Oct 25 '21 07:10 tbagrel1

As far as I know, we currently don't have any way to know "where we are" in the operator chain when we need to output something, we are only aware of the expression sub-tree. We could maybe special-case when the sub-tree is degenerated (i.e. a call chain with a left-associative (resp. right associative) operator of the same priority at every point), but I don't see how to formulate a general rule for this.

tbagrel1 avatar Oct 25 '21 09:10 tbagrel1

I tried yesterday to improve the handling of operator chains, but I discovered that we don't know the correct call tree (because fixities are not known yet at the parsing phase). So I can't really do something fancy here.

I see several potential solutions:

  • not changing anything
  • indenting do bodies with 4 indents
  • (my favorite, but it would change a lot of existing code): always put the operator in multiline operator chains at the beginning of the line, without an indent (with no special case for :). The only special case would be for $-like operators, which would stay at the end of the line, followed by newline + indent
  • always put the operator in multiline operator chains at the beginning of the line, with a half-indent

I also don't really understand why : have a special case right now. Is this explained in a previous issue?

EDIT: I will work on a prototype, and submit it when it's ready

tbagrel1 avatar Oct 27 '21 06:10 tbagrel1