fsharp / fslang-design

RFCs and docs related to the F# language design process, see https://github.com/fsharp/fslang-suggestions to submit ideas
519 stars 144 forks source link

[style-guide] Stroustrup bracket style #706

Closed josh-degraw closed 1 year ago

josh-degraw commented 2 years ago

In the Fantomas repo, I opened an issue a while ago proposing to allow Fantomas to format F# code using Stroustrup style, which essentially just places the open brace on the same line as the binding, and the closing brace on its own line.

Although originally decided against, the issue was reopened and some discussion and development has happened there and in the Fantomas discord. Along with the technical and ergonomic arguments in its favor, because this style is much more common in other programming languages than "classic" F# code style, part of the reason this was reopened in Fantomas was because there has been some agreement that it could help make F# code feel more natural to newcomers, and that impact would have more effect if it was represented in the F# official style guide.

Since it looks like there is the possibility that this could potentially be considered an officially "blessed" F# coding style, @nojaf has suggested that it would be better to discuss specifics and have a discussion here, rather than just in the Fantomas space, since Fantomas aims to format F# code according to the F# style guides rather than just arbitrary preferences.

One of the main technical arguments in favor of Stroustrup style is that it allows for easily shifting lines around in an object. For example, to rearrange any member of this type requires both vertical and horizontal context, because the brackets are on the same line as two of the members:

type PostalAddress =
    { Address: string
      City: string
      Zip: string }

Whereas with Stroustrup style, if you needed to add, move, or remove a member in this type definition, you can insert a line anywhere in the object without problem. In my editor, that means I can even quickly move lines via Alt+Up or Alt+Down, which I couldn't do with the "classic" style:

type PostalAddress = {
    Address: string
    City: string
    Zip: string
}

There are a few situations where this style is already used in F#, specifically Elmish code, but until now it hasn't been widely used in any official F# style guide.

There are pros and cons to this style, as there are with any formatting style, and I don't intend to list them all here myself right now, rather I wanted to formally open the discussion here to both get more community input and come up with an actual style guide for when and where this style would/could/should be used, so we can hopefully have this style in the official style guide in some capacity.

TheAngryByrd commented 2 years ago

I'm a big fan of style. Particularly Elmish benefits greatly but other places do as well, such as Expecto. The shuffling of horizontal as @josh-degraw becomes irritating when needing to do things like:

Currently the Formatting Guidelines seems inconsistent when it prefers StroustrupAllman, when it prefers something like Pico/Lisp.


While some of these are more "Allman" there's a lot of ergonomic similarities between to two so I'm going to include them.

Places it seems to prefer Stroustrup

Formatting application expressions

Example:

// ✔️ OK
someTupledFunction (
    478815516,
    "A very long string making all of this multi-line",
    1515,
    false
)

// OK, but formatting tools will reformat to the above
someTupledFunction
    (478815516,
     "A very long string making all of this multi-line",
     1515,
     false)

Formatting union case expressions

// ✔️ OK
let tree1 =
    BinaryNode(
        BinaryNode (BinaryValue 1, BinaryValue 2),
        BinaryNode (BinaryValue 3, BinaryValue 4)
    )

Formatting quoted expressions

// ✔️ OK
<@
    let f x = x + 10
    f 20
@>

// ❌ Not OK
<@ let f x = x + 10
   f 20
@>

Formatting computation expression operations

let myNumber =
    math {
        addOne
        addOne
        addOne
        subtractOne
        divideBy 2
        multiplyBy 10
    }

Places it seems to not prefer them

Formatting lambda expressions

Example:

// ✔️ OK
Target.create "Build" (fun ctx ->
    // code
    // here
    ())

Formatting record expressions

// ✔️ OK
let newState =
    { state with
          Foo = Some { F1 = 0; F2 = "" } }

// ❌ Not OK, code formatters will reformat to the above by default
let newState =
    {
        state with
            Foo =
                Some {
                    F1 = 0
                    F2 = ""
                }
    }

Formatting object expressions

// ✔️ OK
let comparer =
    { new IComparer<string> with
          member x.Compare(s1, s2) =
              let rev (s: String) = new String (Array.rev (s.ToCharArray()))
              let reversed = rev s1
              reversed.CompareTo (rev s2) }

Places it seems conflicted

Formatting list and array expressions

// ✔️ OK
let pascalsTriangle =
    [| [| 1 |]
       [| 1; 1 |]
       [| 1; 2; 1 |]
       [| 1; 3; 3; 1 |]
       [| 1; 4; 6; 4; 1 |]
       [| 1; 5; 10; 10; 5; 1 |]
       [| 1; 6; 15; 20; 15; 6; 1 |]
       [| 1; 7; 21; 35; 35; 21; 7; 1 |]
       [| 1; 8; 28; 56; 70; 56; 28; 8; 1 |] |]

//vs

let daysOfWeek includeWeekend =
    [
        "Monday"
        "Tuesday"
        "Wednesday"
        "Thursday"
        "Friday"
        if includeWeekend then
            "Saturday"
            "Sunday"
    ]

Formatting record declarations

// ✔️ OK
type PostalAddress =
    { Address: string
      City: string
      Zip: string }
    member x.ZipAndCity = $"{x.Zip} {x.City}"

// ❌ Not OK, code formatters will reformat to the above by default
type PostalAddress = {
    Address: string
    City: string
    Zip: string
  }
  with
    member x.ZipAndCity = $"{x.Zip} {x.City}"
  end

however when there are doc-comments, Allman style is preferred

// ✔️ OK
type PostalAddress =
    {
        /// The address
        Address: string

        /// The city
        City: string

        /// The zip code
        Zip: string
    }

    /// Format the zip code and the city
    member x.ZipAndCity = $"{x.Zip} {x.City}"
pkese commented 2 years ago

My personal preferred syntax for records is:

type PostalAddress = {
    Address: string
    City: string
    Zip: string
  } with
    member x.ZipAndCity = $"{x.Zip} {x.City}"

(end is not necessary)

Anyone else?

TheJayMann commented 2 years ago

Looking at a personal project I've been working on recently, it appears that I prefer to have with on its own line.

type Config = {
  Pin: string
  MpvPath: string
  NpvrBaseAddress: Uri
  TempDirPath: string
  PlaybackSpeed: float
}
with 
  static member Create(config: IConfiguration) = {
    Pin = config.Item("Pin")
    MpvPath = config.Item("MpvPath")
    NpvrBaseAddress = 
      config.Item("NpvrBaseAddress") 
      |> Option.tryParseUri UriKind.Absolute
      |> Option.defaultValue (Uri("http://localhost:8866"))
    TempDirPath = config.Item("TempDirPath")
    PlaybackSpeed = 
      config.Item("PlaybackSpeed") 
      |> Option.tryParse
      |> Option.defaultValue 1.0
  }
nojaf commented 2 years ago

Hello @josh-degraw! Many thanks for bringing this up in fslang-design! This is a topic worth talking about as we saw a lot of interest over at the Fantomas issue.

I'd strongly prefer to have a more high-level conversation about this first. What is the expected outcome of this conversation? Having a section of Stroustrup in the style guide without addressing the whole guide seems like a mistake. If you have a guide that advocates three ways of writing the same code:

type PostalAddress =
    { Address: string
      City: string
      Zip: string }

type PostalAddress =
    { 
        Address: string
        City: string
        Zip: string 
    }

type PostalAddress = { 
    Address: string
    City: string
    Zip: string 
}

That just seems like a step back in getting the F# community to format code all in the same way. It also sends a wrong message to newcomers to the F# language.


As @TheAngryByrd rightfully pointed out, there are places in the style guide where Stroustrup is used. These to me appear to be cases of a "self-contained" Stroustrup:

An example of function invocation:

someTupledFunction (
    478815516,
    "A very long string making all of this multi-line",
    1515,
    false
)

This will result in a single SynExpr.App if you like. This style is used regardless of where in the code this expression is placed.

Whereas an example of a record instance can be perceived as "non-self-contained".

let x = {
    X = 1
    Y = 2
}

The record instance (SynExpr.Record) here in this Stroustrup style is doing so because it is the right-hand side of a let binding.

Another example

let x =
   let y = "some string"
   { X = 1
     Y = 2 }
   |> printfn "%A"

In this case, the same SynExpr.Record is used in combination with an infix operator (SynExpr.App). (Nested in a SynExpr.LetOrUse if you will.) You cannot (to my knowledge) pull off a Stroustrup in that scenario.

That is one of my major concerns with this. When should it work? What is the heuristic? What is the fallback when it cannot work?


I really would like to address these higher-level questions first instead of this becoming a thread of "I like detail X" or "I like detail Y". Liking anything doesn't really add anything to this discussion to me.

TheJayMann commented 2 years ago

Not sure how you could not pull it off in that example. I reworked it and tested it in the AST on Fantomas website, and, with exception of range numbers, ended up with the same exact AST.

let x =
  let y = "some string"
  { 
    X = 1
    Y = 2
  }
  |> printfn "%A"

I find myself using that style often when I'm needing to pipe the result of a Computation Expression into a function pipeline. While you were probably trying to cover a more general case, in this specific case with a new record syntax, I would either make a function constructor, or just use function application.

let x =
  let y = "some string"
  printfn "%A" { 
    X = 1
    Y = 2
  }
nojaf commented 2 years ago

While you were probably trying to cover a more general case

Yes, are dancing around the problem there. My point indeed was that there are scenario's in general where you cannot apply Stroustrup.

  printfn "%A" { 
    X = 1
    Y = 2
  }

This is almost indistinguishable from a computation expression that takes a parameter. I'm surprised the parser even allows this.

TheJayMann commented 2 years ago

Well, if the issue is the visual ambiguity between function application of a record constructor and a computation expression, you could make use of (and possibly enforce, though I don't know what issues that may cause) the backwards pipe operator.

printfn "%A" <| {
  X = 1
  Y = 2
}
josh-degraw commented 2 years ago

there are scenario's in general where you cannot apply Stroustrup.

  printfn "%A" { 
    X = 1
    Y = 2
  }

This is almost indistinguishable from a computation expression that takes a parameter. I'm surprised the parser even allows this.

Are you suggesting that we shouldn't be able to use Stroustrup in that example? That's exactly how I would suggest to format that example.

I'm not sure I see the major problem with this; they're all functions in the end right? As in, especially in the case of a parameterized computation expression, that's essentially just a function that happens to accept the computation expression as the second parameter.

nojaf commented 2 years ago

Are you suggesting that we shouldn't be able to use Stroustrup in that example?

Exactly, because it is visually too close to computation expression.

My original point I tried to make with:

   { X = 1
     Y = 2 }
   >=> printfn "%A"

(Note that I'm using a different operator >=> to not get sidetracked again) I'm not convinced that you can or should use Stroustrup in any given situation.

I think it would be best to limit it to what covers 80% of it and has absolutely no ambiguity. Another example I don't think it works well:

let items = [ {
    X = 1
    Y = 2
} {
    X = 3
    Y = 4
} {
   X = 5
   Y = 6
} ]

As some middle ground I would format it like:

let items = [ 
    {
        X = 1
        Y = 2
    }
    {
        X = 3
        Y = 4
    } 
    {
       X = 5
       Y = 6
    } 
]

Falling back Stroupstrup to Allman when edge cases arise.

TheJayMann commented 2 years ago

So, am I understanding correctly that when you say it is not possible to do Stroupstrup because there is no "parent line" where the opening brace can be added to the end, and any attempt would end up with Allman style? In my opinion (which may be wrong), in the even there is no "parent line," Allman and Stroupstrup are identical. Thus, this idea of falling back to Allman style in certain situations where strict Stroupstrup is impossible or makes the reading and editing more difficult makes sense to me.

Also, in these given examples, where the record names are short and there are few of them, I'd tend to single line the constructors, reserving the block approach for long names and/or many records, as the advantage for using block styles are when the names are long, in order to prevent long lines, and when there are more than a couple properties, so that deleting, adding, and reordering is simpler.

josh-degraw commented 2 years ago

Another example I don't think it works well:

let items = [ {
    X = 1
    Y = 2
} {
    X = 3
    Y = 4
} {
   X = 5
   Y = 6
} ]

As some middle ground I would format it like:

let items = [ 
    {
        X = 1
        Y = 2
    }
    {
        X = 3
        Y = 4
    } 
    {
       X = 5
       Y = 6
    } 
]

Falling back Stroupstrup to Allman when edge cases arise.

Yikes, yeah that first example physically hurts 🤣 For what it's worth, when I originally opened the fantomas issue, the main use case I had in mind was that I disliked the "classic" style, and I wanted to use Stroustrup for type declarations, I hadn't really thought about a heuristic on when else to use it so I didn't really mention anything else. Most other times, Allman style works perfectly fine, and I think it's a very fair compromise to use in the cases like this where Stroustrup doesn't work as well.

nojaf commented 2 years ago

For what it's worth, when I originally opened the fantomas issue, the main use case I had in mind was that I disliked the "classic" style

Yes, and I totally understand you here. In the past, a principle got approved where the guide vaguely said: "avoid vanity alignment". This principle stands but it was quickly used as a stick to club the Fantomas maintainers to death. People opened issues where the guide really didn't go into detail, all under the pretence of a vague rule.

That is why I really would like to have a clear scope of where the guide recommends you can use Stroustrup. Everything not mentioned should (for safety reasons) be considered to fall back to Allman. I think we can come up with a good list to start with that covers about 80% of the cases. And everything not in there would then later on need to discuss case per case.

josh-degraw commented 2 years ago

I ran into a situation for Stroustrup related to https://github.com/fsprojects/fantomas/pull/2513 and https://github.com/fsprojects/fantomas/pull/2515 that I think is worth discussing here:

Given this input in the 'classic' style:

let compareThings (first: Thing) (second: Thing) =
    first = { second with
                Foo = first.Foo
                Bar = first.Bar }

Currently, we seem to format Stroustrup with expressions the same as for Allman style:

let compareThings (first: Thing) (second: Thing) =
    first = { second with
                Foo = first.Foo
                Bar = first.Bar
            }

To me, this feels like a more "correct" Stroustrup style for the expression:

let compareThings (first: Thing) (second: Thing) =
    first = {
        second with
            Foo = first.Foo
            Bar = first.Bar
    }

This would also be the only format of the 3 (as far as I can tell) that could fully and directly utilize a single additional tab stop for each level.

josh-degraw commented 2 years ago

Another situation worth discussing is types with accessibility modifiers and additional members, e.g. https://github.com/fsprojects/fantomas/issues/2511.

Given the following input

type NonEmptyList<'T> =
    private
        { List: 'T list }

    member this.Head = this.List.Head
    member this.Tail = this.List.Tail
    member this.Length = this.List.Length

Currently, fantomas appears to want to fall back to Allman style:

type NonEmptyList<'T> =
    private
        {
            List: 'T list
        }
    member this.Head = this.List.Head
    member this.Tail = this.List.Tail
    member this.Length = this.List.Length

My personal preference would be something like this:

type NonEmptyList<'T> = private {
    List: 'T list
} 
with
    member this.Head = this.List.Head
    member this.Tail = this.List.Tail
    member this.Length = this.List.Length

but I know you hate having to pull out the with keyword (which I do think merits its own separate discussion about the objective pros and cons from an end-user perspective), so there is another option in this case. This is also valid, and actually utilizes some Stroustrup alignment without needing the with keyword.

type NonEmptyList<'T> =
    private {
        List: 'T list
    }
    member this.Head = this.List.Head
    member this.Tail = this.List.Tail
    member this.Length = this.List.Length

IMHO any option is probably fine as long as it's valid code, but it's worth discussing here for the sake of having a formal specification.

josh-degraw commented 2 years ago

Apropo of my last comment, I think a discussion about pros and cons of allowing the with keyword for types with Stroustrup style is warranted. I understand it probably adds a decent amount of complexity in the code to allow for it from the fantomas maintainer perspective, but I thought I'd bring up one objective benefit from an end-user perspective.

If we have a type formatted via Stroustrup:

type Person = {
    FirstName: string
    LastName: string
}

And later we want to add an additional member e.g. a FullName property, if we utilize the with keyword properly, we can keep the original type definition unmodified, and keep the diff clean, which makes it immediately clear that the only functional change was adding a member:

type Person = {
    FirstName: string
    LastName: string
}
+with
+    member this.FullName = 
+        this.FirstName + " " + this.LastName

But if we enforce falling back to Allman when additional members are present, we end up with a much larger diff because every line of the type needed to be adjusted:

-type Person = {
-  FirstName: string
-  LastName: string
-}
+type Person =
+    {
+      FirstName: string
+      LastName: string
+    }
+    member this.FullName =
+        this.FirstName + " " + this.LastName

So in short, allowing the with keyword for additional type members with Stroustrup style does have at least one major, objective benefit, which is that it allows for keeping the type definition consistent regardless of additions and keeps diffs much more clear.

TheAngryByrd commented 2 years ago

Not exactly style but it might be worth figuring out if the with keywork is really needed. Meaning can we adjust the parser to work like Allman style but with Stroustrup.

josh-degraw commented 2 years ago

Not exactly style but it might be worth figuring out if the with keywork is really needed. Meaning can we adjust the parser to work like Allman style but with Stroustrup.

I agree, it would be pretty nice if that was possible, if this was acceptable by the parser:

type Person = {
    FirstName: string
    LastName: string
}
member this.FullName = 
    this.FirstName + " " + this.LastName

Although I don't know nearly enough about the compiler to know how big of a change that would be.

nojaf commented 2 years ago
    first = {
        second with
            Foo = first.Foo
            Bar = first.Bar
    }

This just really gives me computation expression vibes. I take the same issue with non-infix applications:

fn arg1 {
   X = x
   Y = y
}

This is visually just too close to me for

seq {
    ...
    ...
}

Regarding the with keyword, what if it was placed right after the closing brace?

type Person = {
    FirstName: string
    LastName: string
} with
    member this.FullName = 
        this.FirstName + " " + this.LastName

That wouldn't be overly hard to implement and might be a good trade-off. Updating the lexer rules would require a new relaxation to not have to type the with. That, euhm, would be a lot of fun.

josh-degraw commented 2 years ago

Regarding the with keyword, what if it was placed right after the closing brace?

type Person = {
    FirstName: string
    LastName: string
} with
    member this.FullName = 
        this.FirstName + " " + this.LastName

That wouldn't be overly hard to implement and might be a good trade-off.

Personally I would have no issues with that. The keys for me are just not needing to completely reformat the type in order to extend it and keeping the diff small, and this checks both boxes for me, so especially if it's easier to implement I'm all for it👍🏻

    first = {
        second with
            Foo = first.Foo
            Bar = first.Bar
    }

This just really gives me computation expression vibes. I take the same issue with non-infix applications:

fn arg1 {
   X = x
   Y = y
}

This is visually just too close to me for

seq {
    ...
    ...
}

You've mentioned this concern a few times, and I'd be interested to see if the community has strong opinions about it one way or another.

On one hand I totally understand, but on the other hand my gut still likes it for the visual consistency of the brackets haha. I suspect that might come down to a personal preference thing tbh, because the general case of function parameter application I'm inclined to prefer this way, but I do understand the desire to keep computation expressions a "special circumstance."

I will say though that I do think that in this specific situation it should be clear enough that it's not a computation expression since there's no actual "expression" name.

Updating the lexer rules would require a new relaxation to not have to type the with. That, euhm, would be a lot of fun.

That's about what I expected haha. Out of sheer curiosity, do you have a ballpark estimate of how difficult of a task you think that would be?

nojaf commented 2 years ago

You've mentioned this concern a few times, and I'd be interested to see if the community has strong opinions about it one way or another.

Yes, please let us know.

That's about what I expected haha. Out of sheer curiosity, do you have a ballpark estimate of how difficult of a task you think that would be?

Really hard to say, somewhere between two hours and two weeks I guess. The lexer can be quite challenging. A small tweak there could have quite the butterfly effect.

dsyme commented 2 years ago

Just to say that, from a style-guide perspective, I'm ok with there being an end-to-end Stroustrup option, though I realise it's a big job and hits some technical limitations.

@nojaf I can't just "accept" because this is such a big thing? What process would you like to follow? I'm thinking some criteria are

  1. @josh-degraw and others prepare a draft update to the style guide with detailed rules/examples case by case
  2. This is assessed by you and me for whether it's plausible to implement
  3. Merge into style-guide, with note that Fantomas doesn't yet support it
  4. @josh-degraw and others help implement in Fantomas
nojaf commented 2 years ago

Hello @dsyme, I think this is a sensible approach. Each section in the current style guide should have an additional paragraph on how Stroustrup could fit in there. With clear warnings where:

Everything not specifically covered in the guide would not be considered a gap in Fantomas. Similar to how we perceive things today with the style guide.

cmeeren commented 2 years ago

I haven't followed the Stroustrup discussion closely, so I'm not sure if this is timely, but I just want to point out that Stroustrup style compiles for quotations. For example, here is an Unquote assertion:

test <@
    output
    |> Array.pairwise
    |> Array.forall (fun (grp1, grp2) -> predicate (Array.last grp1) (Array.head grp2) = false)
@>

Whether this style is desirable is another matter. Allman also works, but of course adds a level of indentation:

test 
    <@
        output
        |> Array.pairwise
        |> Array.forall (fun (grp1, grp2) -> predicate (Array.last grp1) (Array.head grp2) = false)
    @>
natalie-o-perret commented 1 year ago

Ahem, chiming in with my controversial set of opinions... but I've found myself using a pattern in particular in places like function definitions whenever the content is wrapped in a CE, be it task, async, asyncSeq and whatnot:

let doStuffAsync myParameter = async {
    let! someOtherThing = giveMeSomeOtherThingAsync()
    return myParameter + someOtherThing 
}

What I'd like to know is whether this thing above 👆 would be regarded as "compliant" with the Stroustrup bracket style, or is it something different?

Cause you see, I don't really mind stuff like in the example given by @TheAngryByrd:

let myNumber =
    math {
        addOne
        addOne
        addOne
        subtractOne
        divideBy 2
        multiplyBy 10
    }

but it feels "weird" in many instances. One might expect to not have to tab the whole content just because the function body is wrapped in a CE.

Thing like below 👇 keeps things terse and neat. Imho, this improves readability without it being at the expense of adding a new line and tabbing the whole body:

let myNumber = math {
    addOne
    addOne
    addOne
    subtractOne
    divideBy 2
    multiplyBy 10
}

I guess this is probably a very unpopular opinion, but thought this might be a wee related.

TheAngryByrd commented 1 year ago

To me, I get the same benefits in either style that @natalie-o-perret describes so I don't particularly have a preference. That being said , the more inline version does not allow piping within the same expression :

let foo () = async {
  return 42
}
|> Async.RunSynchronously

Would throw an error while

let foo () = 
  async {
    return 42
  }
  |> Async.RunSynchronously

Is allowed.

josh-degraw commented 1 year ago

Yeah, I've kind of gone back and forth on this, initially I thought I preferred the CE name on the same line as the =, but I've come around to preferring it on its own line. The hard thing is that a lot of this just boils down to personal preference at the end of the day, aside from any objective benefits like the piping benefit that @TheAngryByrd mentioned. Depending on the interest in that style it might be worth having that be it's own individual setting, especially since I feel like this CE setting is really orthogonal to the Stroustrup feature.

josh-degraw commented 1 year ago

While we're bikeshedding the specifics here, I've also been wondering a lot if Stroustrup is the best name to use here. When I originally submitted the fantomas issue that started all of this, it was the most appropriate name I could think of, but the more I've thought about it the more unsure I am that it's the right name to use for this style in the context of F# code specifically.

Technically, it would never truly be Stroustrup style (which like most other named bracket styles is typically used with exclusively bracket-scoped languages) since F# is a white-space sensitive language and there are situations where it's either not applicable or where we have to do something else (e.g. nested records inside an array expression, or distinguishing computation expressions from other bracket usage types). Also the name is kind of hard to spell and I'm also not sure if I've ever pronounced it correctly 🙃.

I think it's probably fine to keep it this way if the name recognition is worth more than the other issues (although the name itself may be confusing to some as well since it's named after a person and not describing it), or if it's just too much of a headache to rename things at this point. But it's something that's been kind of nagging at me and I thought would be worth discussing.

Other potential names I've considered: - Asymmetric - Diagonal - Split

Open to other suggestions as well, and as I said if the consensus is to keep Stroustrup as is I won't complain, I just thought I should bring it up to see if anyone agrees and come to a consensus either way.

cc @nojaf

Edit: After some other discussions, consensus is to just keep using Stroustrup.

josh-degraw commented 1 year ago

With the merge of https://github.com/dotnet/docs/pull/33171, I feel like this issue can be considered resolved. If additional Stroustrup specific style-guide changes need to be proposed (e.g. for Elmish styles), I think we're at a point where they should be their own proposals. If there are no objections, I'll go ahead and close this issue.