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

Style regression with backticks #907

Closed
georgefst opened this issue Jul 11, 2022 · 12 comments · Fixed by #955
Closed

Style regression with backticks #907

georgefst opened this issue Jul 11, 2022 · 12 comments · Fixed by #955
Assignees
Labels
style Nitpicking and things related to purely visual aspect for formatting.

Comments

@georgefst
Copy link

georgefst commented Jul 11, 2022

Mixing backtick-ed functions with ordinary infix operators often looks worse than it used to. This is presumably a victim of the fixity overhaul.

Ormolu 0.4:

def1 =
  (,,)
    <$> gvar (primitiveGVar "natToHex")
      `app` ne
    <*> con cNothing
      `aPP` tcon tChar

def2 =
  tds1
    && lc1 == mempty
    && gc1 `M.isSubmapOf` gc2
    && sh1 == sh2

Ormolu 0.5:

def1 =
  (,,)
    <$> gvar (primitiveGVar "natToHex")
    `app` ne
    <*> con cNothing
    `aPP` tcon tChar

def2 =
  tds1
    && lc1
    == mempty
    && gc1
    `M.isSubmapOf` gc2
    && sh1
    == sh2
@mrkkrp mrkkrp added the style Nitpicking and things related to purely visual aspect for formatting. label Aug 2, 2022
@jonathanlking
Copy link

Using the def1 example provided, if you run it with ormolu --no-cabal -f 'infixl 9 `isSubmapOf`', then you get the same output as Ormolu 0.4.

Quoting this Stack Overflow answer, quoting the Haskell 98 report:

An operator is either an operator symbol, such as + or $$, or is an ordinary identifier enclosed in grave accents (backquotes), such as op. For example, instead of writing the prefix application op x y, one can write the infix application x op y. If no fixity declaration is given for op then it defaults to highest precedence and left associativity (see Section 4.4.2).

Perhaps for an ordinary identifier enclosed in grave accents with no fixity declaration, ormolu should infer infixl 9?

@mrkkrp mrkkrp self-assigned this Jan 3, 2023
@tbagrel1
Copy link
Member

tbagrel1 commented Jan 4, 2023

Perhaps for an ordinary identifier enclosed in grave accents with no fixity declaration, ormolu should infer infixl 9?

We initially decided to stick to "unknown fixity" for any operator that is not in the knowledge base. So that the formatting would look as neutral as possible (which means, everything on its own line with no differentiated indentation) when we don't have enough info.

According to the haskell 98 report,

If the digit is omitted, level 9 is assumed. Any operator lacking a fixity declaration is assumed to be infixl 9

So we would probably need to do the change for both text and symbolic operators.

But that would force every programmer who introduces a new operator with a fixity different than infixl 9 to edit the fixity override config file or they would get potentially wrong and opiniated formatting from ormolu.

I don't know what is the good call to make here, but I see that @mrkkrp self-assigned this issue, so he will probably have better insight on this than me :)

@mrkkrp
Copy link
Member

mrkkrp commented Jan 4, 2023

@tbagrel1 Have a fix now in #955. AFAIK this issue is just about defaulting to infixl 9 for all backticked operators that lack explicit fixity information.

@tbagrel1
Copy link
Member

tbagrel1 commented Jan 4, 2023

@mrkkrp Sure, but the haskell report states that backticked operators are no different from "symbolic" operators when it comes to default fixity. So, if we decide against the "neutral when no explicit fixity declaration is present" idea I had originally (which I'm completely OK with), maybe we should do it uniformly?

@mrkkrp
Copy link
Member

mrkkrp commented Jan 4, 2023

I see now. I somehow assumed (given the quote above by @jonathanlking) that only backticked operators are inferred to be infixl 9 by default. With every operator defaulting to that the situation certainly becomes trickier for us.

@mrkkrp
Copy link
Member

mrkkrp commented Jan 4, 2023

Perhaps it could make sense to default to infixl 9 if no information is available. Depends on how it would look. Arguably infixl 9 could look better than our current default. @tbagrel1 do you remember what the algorithm currently does for operators with undefined fixity? How does it treat them (especially when they are mixed with operators with defined fixity)?

@georgefst
Copy link
Author

georgefst commented Jan 4, 2023

I'd actually forgotten that backticked operators could have fixity declarations. It's possible that the title should be:

Style regression with operators with no fixity declarations

@tbagrel1
Copy link
Member

tbagrel1 commented Jan 4, 2023

do you remember what the algorithm currently does for operators with undefined fixity? How does it treat them (especially when they are mixed with operators with defined fixity)?

If their fixity is unknown, we use FixityInfo {minPrec=0, maxPrec=9, direction=Nothing}. And then, the algorithm only builds subtrees when an operator has lower max than the neighbour min precedence OR higher min than the neighbour max precedence. So, when unknown operators are mixed with known operators, no subtree can be built, so the result is the same as using operators having all the same precedence level (like a chain of + and -). That's what I qualified as

as neutral as possible (which means, everything on its own line with no differentiated indentation)

in my comment above.


Style regression with operators with no fixity declarations

I don't remember, are we parsing the fixity declarations from the source file being formatted at the moment @mrkkrp?

@mrkkrp
Copy link
Member

mrkkrp commented Jan 4, 2023

No, we do not.

@tbagrel1
Copy link
Member

tbagrel1 commented Jan 4, 2023

So at the moment, we can't know whether or not user-defined operators have a fixity decl

@mrkkrp
Copy link
Member

mrkkrp commented Jan 4, 2023

Yes, that's true. I guess the question here is: is the current formatting better or worse then assuming infixl 9 as the default? Both options are similarly faulty because they make the reassociation algorithm default to something that may be incorrect (unless explicit user overrides are provided).

@tbagrel1
Copy link
Member

tbagrel1 commented Jan 4, 2023

I have no opinion on this.

A better solution, in theory at least, might be to deduce fixity from the input:

from this:

x1 U x2
  A x3 U x4
  B x5 U x6
  C x6

where A, B, C are known operators, and U is unknown, we could deduce that precedence(U) >= precedence(A, B, C).
I'm not sure how robust we could make this system in practice though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style Nitpicking and things related to purely visual aspect for formatting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants