tweag / ormolu

A formatter for Haskell source code
https://ormolu-live.tweag.io
Other
958 stars 83 forks source link

Type synonym is reformatted into a less readable form #148

Closed int-index closed 5 years ago

int-index commented 5 years ago

Before Ormolu:

type CoerceLocalSig m m' =
  forall r a.
  LocalSig m  r a ->
  LocalSig m' r a

After Ormolu:

type CoerceLocalSig m m'
  = forall r a. LocalSig m r a
  -> LocalSig m' r a

That's clearly worse because now LocalSig isn't aligned. Now, I'm not sure what the style guide Ormolu follows, but if my hand-written version is incompatible with it (because of the trailing ->), at least keep the forall on its own line:

type CoerceLocalSig m m'
  = forall r a.
     LocalSig m r a
  -> LocalSig m' r a
mrkkrp commented 5 years ago

This is something to discuss I guess. I, for one, am OK with the output. I'm not against changing the logic though, if we can decide what is a better way to format things like this.

Low priority for now.

mboes commented 5 years ago

And if this changes, it'll have to change elsewhere too: function type signatures, GADT constructors, etc.

facundominguez commented 5 years ago

forall in a separate line sounds good to me.

neongreen commented 5 years ago

Another minor argument in favor of trailing arrows is that then it also makes sense to leave :: on the same line as the function name:

foo ::
  Int ->
  Bool ->
  Something

This will help people who use grep "function ::" as a substitute for jump-to-definition (which is admittedly a zero-setup solution, unlike hasktags or HIE).

int-index commented 5 years ago

Keep in mind that GHC also has the forall a -> syntax and soon will get top-level kind signatures. So, this definition would be valid:

type P :: forall s. forall k -> k s -> Type
data P k (a :: k s) = MkP

If the kind signature for P would be longer and split into multiple lines, would one use leading punctuation?

type P
  :: forall s
   . forall k
  -> k
  -> Type

Trailing punctuation?

type P ::
  forall s.
  forall k ->
  k ->
  Type

or some sort of hybrid?

type P
  :: forall s.
     forall k ->
     k
  -> Type
mboes commented 5 years ago

There is also the matter of linear types, where arrows take three characters, so the types after leading arrows won't line up despite users expecting that.

mrkkrp commented 5 years ago

I do not like the idea of trailing arrows.

mrkkrp commented 5 years ago

@neongreen The grepping argument is very weak. You want to use Ormolu to introduce a sort of convention that would allow you to search for functions. This is a) not technically correct way to do it b) unreliable way to do it, because a convention will never be followed in all cases, even if it's imposed by a formatting tool. Not 100% of Haskell programmers will use Ormolu, anyway.

int-index commented 5 years ago

There is also the matter of linear types, where arrows take three characters, so the types after leading arrows won't line up despite users expecting that.

This concern isn't limited to linear types, there's also matchability polymorphism. We'll get ->> and ->{m} or something along these lines, which are more than two characters as well.

I'm not used to that style. I'm having lots of troubles reading signatures with trailing arrows. <..> The tool should output conventional Haskell formatting, just a bit more systematic than what people do manually.

The problem with this concept of "conventional formatting" is that there are different conventions! In contrast to your preferred conventions, I use trailing punctuation and I find leading punctuation harder to read. Especially when it comes to leading commas: no natural language uses leading commas, so they set off all the alarms in my brain. So when it comes to arguments such as these, I try to take a neutral stance and find some objective metrics, such as being git-diff friendly, making more things align naturally (assuming that adequate alignment is a universal readability boost), etc.

mrkkrp commented 5 years ago

Re matchability polymorphism: the answer is the same it's OK if things do not align. Nothing is really aligned with Ormolu, so why are you so bothered by the allows? Let them be of any length, that's perfectly fine.

Conventions are different, but there is a majority and the prevailing style.

neongreen commented 5 years ago

You want to use Ormolu to introduce a sort of convention that would allow you to search for functions.

No, I have proper jump-to-definition set up. I was talking about other people.

The grepping argument is very weak.

I have not claimed that it's a strong argument, merely that it's one more argument.

a convention will never be followed in all cases, even if it's imposed by a formatting tool

Many programmers spend the majority of their time working on one codebase (the one they have at $dayjob). Moreover, the amount of programmers who have troubles setting up jump-to-definition is depressingly high, in my experience anyway.

mboes commented 5 years ago

Like @int-index says, there are a lot of conventions out there at this point. If leading -> was a near universal style, this would be a different matter, but it's not. Leading commas are also arguably dominant (but not universal), but only because of an accident of history: https://mail.haskell.org/pipermail/ghc-devs/2014-September/006365.html. Trailing commas are now permitted in import and export lists, and will soon be in lists, records and elsewhere. Consider also https://mail.haskell.org/pipermail/ghc-devs/2014-September/006409.html.

yumiova commented 5 years ago

For the record, plenty of "natural languages" have leading constructs. Specifically, most structured English text tends to structure larger lists through bulleted or numbered lists, rather than ending each element by a comma and a new line. There does seem to be some precedent for structured lists preferring leading symbols.

mrkkrp commented 5 years ago

I think constructions where punctuation introduces next item are a lot easier to read, like the mentioned above bullet and numbered lists.

mboes commented 5 years ago

Bullet points / numbered lists are orthogonal to punctuation. In English bullet list items still need punctuation at the end. Having written thousands of lines of Python in the last 2 years, I don't buy the readability argument in favour of leading commas, which, if it were true in practice, would likely have caused at least some other languages to follow suite.

mboes commented 5 years ago

Also FTR, eschewing alignment when it's content sensitive doesn't mean we should forget about it when it's not. In the latter case, aligning doesn't cost any diff amplification.

mrkkrp commented 5 years ago

The truth here is that it's not at all that important how to print this or that as long as it follows the core principles of Ormolu. But if you chose something that is terribly original, people will just fork the tool and get their own way anyway. I do not see export lists like this:

module Foo
  ( foo,
    bar,
  )
where

If Haskellers wanted this, they would use it already.

mrkkrp commented 5 years ago

I mean, come on, we should try to unify the way people format things and to appeal to majority instead of trying to be clever here. For me it means that the output should look conventional. I don't know the numbers, but the code that I read has reading commas and leading arrows.

mboes commented 5 years ago

people will just fork the tool and get their own way anyway

Merits of your arguments aside - note that there's nothing wrong with the above.

mrkkrp commented 5 years ago

I guess I'm just very deeply against fragmentation.

mrkkrp commented 5 years ago

If I worked on a formatter for Python, I'd never try to make it use leading commas or something like this. I'd produce something that people expect. That's the point.

int-index commented 5 years ago

@mrkkrp Just to be sure, are you arguing in favor of the "hybrid" style?

type P
  :: forall s.
     forall k ->
     k
  -> Type

or would you rather have . of the forall on the next line as well?

mboes commented 5 years ago

Which again, quite apart from anything else, shouldn't matter. You say

we should try to unify the way people format things

But no one will achieve that. No one cares to have consistent style between say bytestring and Agda. The scope of consistency isn't "all Haskell that was ever written". It's "all Haskell in a single project".

neongreen commented 5 years ago

Huh. I'd love to have consistent style between ~one client's codebase and another client's codebase~ bytestring and Agda.

mrkkrp commented 5 years ago

I also think about how it is going to affect the ecosystem and new libraries that will be written. Wouldn't it be good for readability if all code you're going to read was in the same style? I think it's a good thing, right? If so, that could be a potential goal, even if unachievable, but nevertheless a goal to strive for.

If people like the tool, they are going to format all the stuff they produce with it, including libraries, and maybe already existing libraries too. Not everyone will be doing that, but imagine if Ormolu is the only formatter that works well and is acceptable, it may very well strongly affect the dominant style of the community.

int-index commented 5 years ago

Every choice comes with mental overhead and ground for disagreement. I like @mboes idea of the scope of consistency being a project rather than all Haskell code, which would entail having a per-project configuration, but there's a cost to that. It means that now every team has choices to make, and team members will fight over these choices, possibly as long as the project lives (with arguments along the lines of "it's not too late to change this!")

It's a very real social cost which goes beyond readability.

That's why I think that Ormolu has to nail the One True Style for all Haskell code.

So, there are specific criteria that are met here: enable people to work on smaller screens (or with bigger fonts), and save people time on rebases.

I'd like other choices to be made by optimizing for other important criteria, which should probably be more substantial than existing habits or individual preferences. People can change their habits, I'll use leading punctuation if there's a compelling argument in its favor.

Here's an argument in favor of trailing punctuation. Adding a linear function parameter results in a single-line diff instead of a two-line diff.

Trailing arrows, one line added:

f ::
  Int ->
  String

f' ::
  Int ->
  Bool ->.
  String

Leading arrows, one line added and one changed:

f
  :: Int
  -> String

f'
  :: Int
  -> Bool
  ->. String

I think this is evidence that the arrow belongs on the same line as the parameter. The arrow "flavor" (linear, matchable/unmathable, etc) is more about the parameter than the following line.

Maybe you have a reason to prefer leading punctuation instead. But this has to be a convincing reason rather than "it's a historical accident, it's just how we're doing stuff around here". Then I'll accept this reason and abandon my old habits. Because habits can change, but objective reasoning stays there regardless of style preferences of individuals.

Without such a reason, I'm more likely to fork Ormolu for my project, not less likely as you would prefer. (Actually, the chances of that are close to zero, because I'd rather spend my time on a structure editor to solve the problem of formatting on a deeper level, but this aside...)

int-index commented 5 years ago

On the other hand, it's true that some people don't care about reasons for this or that, they just want a style that feels familiar. Obviously I can't tell you what sort of audience to cater to 🙂

int-index commented 5 years ago

I do not see export lists like this:

module Foo
  ( foo,
    bar,
  )
where

If Haskellers wanted this, they would use it already.

@mrkkrp A similar style is quite common in GHC sources, for example:

module BasicTypes(
        Version, bumpVersion, initialVersion,

        LeftOrRight(..),
        pickLR,

        ConTag, ConTagZ, fIRST_TAG,

        Arity, RepArity, JoinArity,

        Alignment, mkAlignment, alignmentOf, alignmentBytes,

        PromotionFlag(..), isPromoted,
        FunctionOrData(..),

        WarningTxt(..), pprWarningTxtForMsg, StringLiteral(..),

        Fixity(..), FixityDirection(..),
        defaultFixity, maxPrecedence, minPrecedence,
        negateFixity, funTyFixity,
        compareFixity,
        LexicalFixity(..),

        RecFlag(..), isRec, isNonRec, boolToRecFlag,
        Origin(..), isGenerated,

        RuleName, pprRuleName,

        TopLevelFlag(..), isTopLevel, isNotTopLevel,

        OverlapFlag(..), OverlapMode(..), setOverlapModeMaybe,
        hasOverlappingFlag, hasOverlappableFlag, hasIncoherentFlag,
        Boxity(..), isBoxed,

        PprPrec(..), topPrec, sigPrec, opPrec, funPrec, appPrec, maybeParen,

        TupleSort(..), tupleSortBoxity, boxityTupleSort,
        tupleParens,

        sumParens, pprAlternative,

        -- ** The OneShotInfo type
        OneShotInfo(..),
        noOneShotInfo, hasNoOneShotInfo, isOneShotInfo,
        bestOneShot, worstOneShot,

        OccInfo(..), noOccInfo, seqOccInfo, zapFragileOcc, isOneOcc,
        isDeadOcc, isStrongLoopBreaker, isWeakLoopBreaker, isManyOccs,
        strongLoopBreaker, weakLoopBreaker,

        InsideLam, insideLam, notInsideLam,
        OneBranch, oneBranch, notOneBranch,
        InterestingCxt,
        TailCallInfo(..), tailCallInfo, zapOccTailCallInfo,
        isAlwaysTailCalled,

        EP(..),

        DefMethSpec(..),
        SwapFlag(..), flipSwap, unSwap, isSwapped,

        CompilerPhase(..), PhaseNum,

        Activation(..), isActive, isActiveIn, competesWith,
        isNeverActive, isAlwaysActive, isEarlyActive,
        activeAfterInitial, activeDuringFinal,

        RuleMatchInfo(..), isConLike, isFunLike,
        InlineSpec(..), noUserInlineSpec,
        InlinePragma(..), defaultInlinePragma, alwaysInlinePragma,
        neverInlinePragma, dfunInlinePragma,
        isDefaultInlinePragma,
        isInlinePragma, isInlinablePragma, isAnyInlinePragma,
        inlinePragmaSpec, inlinePragmaSat,
        inlinePragmaActivation, inlinePragmaRuleMatchInfo,
        setInlinePragmaActivation, setInlinePragmaRuleMatchInfo,
        pprInline, pprInlineDebug,

        SuccessFlag(..), succeeded, failed, successIf,

        IntegralLit(..), FractionalLit(..),
        negateIntegralLit, negateFractionalLit,
        mkIntegralLit, mkFractionalLit,
        integralFractionalLit,

        SourceText(..), pprWithSourceText,

        IntWithInf, infinity, treatZeroAsInf, mkIntWithInf, intGtLimit,

        SpliceExplicitFlag(..)
   ) where

Note that there are also newlines for grouping. Could Ormolu perhaps preserve these?

mboes commented 5 years ago

Don't get me wrong, I don't wish to argue for configuration. I advocated for removing the little configuration Ormolu had in https://github.com/tweag/ormolu/issues/69#issuecomment-502819544. My argument is that global consistency is unachievable, while project-level consistency is, so that's really the focus of Ormolu at this point. Because Ormolu isn't configurable, arguments over how to customize it when starting a project shouldn't arise. We can furthermore encourage consistent style across projects through lack of configuration, but we can't prevent forking, nor predict what will cause others to fork. Even if we wanted to make forks less likely, who's to say that adherence to historical accidents is particularly effective. @int-index demonstrated above that proponents of previously minority styles might care more about style choice than those in the majority, in which case we'd see more forks, not less.

The linear types example by @int-index shows that the language is evolving, and that the pros of historically dominant styles might now be outweighed by cons that did not previously exist. Just like Johan Tibell admitted he'd have written his style guide differently if Haskell had allowed trailing commas, it's unclear that sticking to Johan's style for types is necessarily the best choice in a world of explicit foralls, linear types, explicit quantification on kinds, matchability polymorphism etc.

mrkkrp commented 5 years ago

OK, I'm going to interpret this issue as a request to change how we format foralls in all sort of type signatures. The example from the first message in this thread should look like this after formatting (multi-line):

type CoerceLocalSig m m'
  = forall r a.
     LocalSig m r a
  -> LocalSig m' r a
guaraqe commented 5 years ago

Just a point about trailing arrows. This is my preferred style, but it is not compatible with Haddock - the following does not work:

f ::
  a -> -- ^ lala
  b

So a workaround would have to be found. If I remember well, the following works:

f ::
  a {- ^ lala -} ->
  b

But it is more verbose.

basile-henry commented 5 years ago

@guaraqe What about doing this instead:

f ::
  -- | lala
  a ->
  b
mrkkrp commented 5 years ago

Here are some more examples from capability showing that at least a line break after forall is necessary:

wrapError :: forall outertag innertag t outer inner m a.
  ( forall x. Coercible (t m x) (m x)
  , forall m'. HasCatch outertag outer m'
    => HasCatch innertag inner (t m')
  , HasCatch outertag outer m )
  => (forall m'. HasCatch innertag inner m' => m' a) -> m a

-- becomes:

wrapError
  :: forall outertag innertag t outer inner m a. ( forall x. Coercible (t m x) (m x),
                                                   forall m'. HasCatch outertag outer m'
                                                   => HasCatch innertag inner (t m'),
                                                   HasCatch outertag outer m
                                                   )
  => (forall m'. HasCatch innertag inner m' => m' a)
  -> m a

and

magnify :: forall outertag innertag t outer inner m a.
  ( forall x. Coercible (t m x) (m x)
  , forall m'. HasReader outertag outer m'
    => HasReader innertag inner (t m')
  , HasReader outertag outer m )
  => (forall m'. HasReader innertag inner m' => m' a) -> m a

-- becomes:

magnify
  :: forall outertag innertag t outer inner m a. ( forall x. Coercible (t m x) (m x),
                                                   forall m'. HasReader outertag outer m'
                                                   => HasReader innertag inner (t m'),
                                                   HasReader outertag outer m
                                                   )
  => (forall m'. HasReader innertag inner m' => m' a)
  -> m a
mrkkrp commented 5 years ago

I implemented the behavior @int-index proposed in #382, however I do not like it because:

It's hard to come up with a satisfying formatting decision for this one. I think the implicit/explicit constraints are the following:

So I'd rather propose perhaps either this:

type CoerceLocalSig m m'
  = forall r a.
  LocalSig m  r a
  -> LocalSig m' r a

or

type CoerceLocalSig m m'
  = forall r a.
    LocalSig m  r a
  -> LocalSig m' r a

IMO, the first version is better because it also doesn't require any hacks to implement with our printing framework.

basile-henry commented 5 years ago

I much prefer the arrow at the beginning of the line. 👍 I just thought if @guaraqe is going to use the arrows at the end, they should know about the other type of haddock comment. 😄