Relax some of the indentation rules

[baronfel]

Submitted by Tomas Petricek on 8/3/2015 12:00:00 AM
54 votes on UserVoice prior to migration

Currently, the general rules for indentation in F# is that the code on the next line should be indented further than the thing that determines its starting point on the previous line.

See http://fssnip.net/rZ for examples.

There are a number of cases where this quite annoyingly means that you have to indent things very far (or, to avoid that, add lots of unnecessary line breaks). One example is when you have nesting in a method call. For example:

Chart.Geo(growth)
|> Chart.WithOptions(Options(colorAxis=ColorAxis(values=[| -100;0;100;200;1000 |], colors=[| "#77D53D";"#D1C855";"#E8A958";"#EA4C41";"#930700" |])))

Now, there is almost no way to make this code snippet look decent. I would want to write something like this:

Chart.Geo(growth)
|> Chart.WithOptions(Options(colorAxis=ColorAxis(values=[| -100;0;100;200;1000 |], 
    colors=[| "#77D53D";"#D1C855";"#E8A958";"#EA4C41";"#930700" |])))

But this is not allowed, because "colors" should start after the opening parenthesis of ColorAxis, so I would need 50 spaces! To make the number of spaces smaller, you can add additional newline (to get the "ColorAxis" more to the left), but this looks pretty bad:

Chart.Geo(growth)
|> Chart.WithOptions
    (Options
      (colorAxis =
        ColorAxis
          (values=[| -100;0;100;200;1000 |], 
           colors=[| "#77D53D";"#D1C855";"#E8A958";"#EA4C41";"#930700" |])))

Another example is very similar, but with list expressions. I want to write:

let pop2010 = series [ for c in wb.Countries ->
c.Name => c.Indicators.``CO2 emissions (kt)``.[2010]]

This actually works, but it gives warning. Again, it wants me to indent the second line so that it is after "for", but then I'm not saving pretty much anything by the newline. Or, I can introduce lots of additional newlines and write:

let pop2010 =
  series
    [ for c in wb.Countries -> 
        c.Name => c.Indicators.``CO2 emissions (kt)``.[2010]]

I think that in situations like these, the rules should be relaxed. In particular, we should not require new line to be intended further than the "starting thing" on the previous line. Just further than the previous line.

Original UserVoice Submission

[rmunn]

The original UserVoice request had a link to http://fssnip.net/rZ to show the actual code snippets as intended (before UserVoice stripped the indentation). I used to have them duplicated in this comment, but now that they've been edited in to the issue description, I've removed them in order to save space.

[baronfel]

Thanks Robin! If you don't mind I'll edit the original submission with your formatting update.

[smoothdeveloper]

Here is a simple use case that kind of trumps the principle of least surprise:

let foo = ([|
  1
  2
|])

let bar = [|
  1
  2
|]

the first one gives

Possible incorrect indentation: this token is offside of context started at position (550:12). Try indenting this token further or using standard formatting conventions.

[cartermp]

@smoothdeveloper Great example. That's definitely going to be something that throws people off.

[MiloszKrajewski]

Current rules are also inconsistent:

    let config = { // this one is OK
        defaultConfig with 
            bindings = [ // while this one is NOT
                HttpBinding.create Protocol.HTTP IPAddress.Any 1337us
            ]
        }

I guess the rule could be: It has to be indented relatively to previous line. It should not be important how far though. Actually, expecting indentation to be adjusted to match something exactly is not sustainable, it may not survive refactoring, like changing bindings (above) to slightlyLongerBindings.

For now, I have a feeling I add too much line-breaks to avoid tweaking indentation with extra spaces.

[rmunn]

#557 points out another place where the current indentation rules are inconsistent and confusing:

type A = { A : Unit }
let values = Seq.empty<A>

let doYield = [
    for value in values do yield {
        A = () // OK.
    }
]

let arrow = [
    for value in values -> {
        A = () // Possibly incorrect indentation.
    }
]

Why does the do yield version compile without a warning, but the -> equivalent produce a "Possibly incorrect indentation" warning? I don't know; I can't figure out what the rule is here. The first column where I can put the A so that it doesn't warn me of incorrect indentation is to line it up just under the {, like so:

let arrow = [
    for value in values -> {
                          A = () // Possibly incorrect indentation.
    }
]

let arrow = [
    for value in values -> {
                           A = () // OK.
    }
]

While we're fixing indentation inconsistencies, let's make sure this one gets fixed as well.

[cloudRoutine]

While we're at it let me point out some other bugs in the parser I've found so far
#504

open System

    module Z =
        type Alpha< ^b, ^a 
    when ^     a    :  (member Name:string)
and    ^a:        (member Zip
   : ^b when
^b : struct   )
and             ^a
:                 (static member(+)
    :    'a * 'a 
-> 'a 
) 
         > () = 
            member inline __.X = ()
        with  
        static member inline Y = ()

The error in how the ranges are parsed signature files is a bit too long to explain here, but check it out if you're interested

I know @dsyme likes to say that

the cost for implementing the indentation aware syntax here is quite high, and the need is relatively rare.

It's by design that no new indentation contexts are pushed in that region.
It's also by design that violations of existing (already-pushed) indentation contexts give the warnings

But I'm hoping that the combination of the need to fix the flaws in the implementation of the parser, to address the overlooked areas that allow bizarre behaviors that flagrantly disregard all F# conventions, and the potential benefits or making some improvements to the parser and UAST collectively make this endeavor worth undertaking.

One of the most unfortunate aspects of the UAST we get back from the compiler is that it omits the positions of many operators, punctuation, and keywords. If that information were included it would make source formatting and transformation much easier. it would make it easier to establish contexts for autocompletion, expression positions for setting breakpoints, expression locations for refactoring, etc.

Maybe this could also be the time to enhance fslexxyacc's ability to integrate with the compiler/compiler service and tooling, to allow F# devs to write parsers with the ease ocaml devs currently enjoy with ocamllex & ocanmlyacc

[nikonthethird]

There is one other place where I find the indentation rules infuriating: Having a record inside a record.

In the code below, I would like to write nested records like the value doesNotWork, which just looks like it should be correct. But it prints the warning.

Instead, you have to write something like worksButLooksWrong, which I don't think looks nice.

I found a curious workaround, though. By just placing a call to id in front of the nested record definition, suddenly the compiler is happy, as shown in the works value. I have no idea why.

type A = { Value : Unit }
type B = { Content : A; More : Unit }

let doesNotWork = {
    Content = {
        Value = () // Possibly incorrect indentation.
    }
    More = ()
}

let worksButLooksWrong = {
    Content =
        {
            Value = () // Works but just looks wrong.
        }
    More = ()
}

let works = {
    Content = id {
        Value = () // OK.
    }
    More = ()
}

[MiloszKrajewski]

+infinity for 'id' trick. I would like to add that that syntactically significant whitespace is controversial enough already (although, I actually like it). Making it very strict may alienate people, though. I guess "semantic indentation" is good, "presentational indentation" is not sustainable (renaming variables, moving blocks of code around, etc.) and annoying as it may push code far to the right.

See: "Kevlin Henney - Seven Ineffective Coding Habits of Many Programmers" @ ~14:30 (https://vimeo.com/97329157)

[dsyme]

One of the most unfortunate aspects of the UAST we get back from the compiler is that it omits the positions of many operators, punctuation, and keywords. If that information were included it would make source formatting and transformation much easier.

You can certainly submit PRs to Microsoft/visualfsharp or fsharp/FSharp.Compiler.Service to add more or less any location information you like. Just add it to the relevant node, and add the information in pars.fsy. Some of that (location of commas etc.) has already been done but it's ok to add more.

[cloudRoutine]

@dsyme oh wow! I always assumed it'd be too much of a breaking change so I never even bothered asking if I could do that

[0x53A]

Just another annoying data point:

from my FAKE script:

    (cd @@ "TimPrecast.sln")
    |> MSBuildHelper.build (fun p -> { p with MaxCpuCount = Some (Some Environment.ProcessorCount)
                                              DistributedLoggers = Some [
                                                                    { MSBuildDistributedLoggerConfig.AssemblyPath = ((getExtractedToolsDir()) @@ @"Microsoft.Build.Logging.StructuredLogger\lib\net46\StructuredLogger.dll")
                                                                      MSBuildDistributedLoggerConfig.ClassName = Some "StructuredLogger"
                                                                      MSBuildDistributedLoggerConfig.Parameters = Some [ "LogFile", (logDir @@ "TimPrecast.buildlog") ]
                                                                    }, None
                                                                    ^ <-- I need to indent it to "Some" + 1
                                              ]
                                              Properties =
                                              [
                                                "Configuration", buildConfiguration
                                                "Platform", hardcodedPlatform
                                              ]})

here I need to indent the record MSBuildDistributedLoggerConfig up to the Some.

I would have expected the [ to start a new scope so that I would not have to indent it so far.

---

Reduced example:

// works
let myList = [
    1
]

// does not work
let myList = Some [
    1
]

Looking through the existing examples, it has probably the same root cause as the example by smoothdeveloper, where ( and Some start new scopes.

[xperiandri]

What about computation expressions with opening curly brace at a new line?

[Happypig375]

I just hit this today:

use paint = new SkiaSharp.SKPaint(Shader = SkiaSharp.SKShader.CreateLinearGradient(
    SkiaSharp.SKPoint(0.f, 0.f), SkiaSharp.SKPoint(0.f, h),
    [|0xff39a700u; 0xff67b93cu; 0xff65d52au|] |> Array.map SkiaSharp.SKColor.op_Implicit
  , [|0.f; 0.473f; 1.f|], SkiaSharp.SKShaderTileMode.Clamp
))

Warnings at second line (first SkiaSharp and first opening bracket) and third line (first [|). That's 3 warnings at the same time.

[cartermp]

@Happypig375 Indentation with static members is covered by dotnet/fsharp#6808

[cartermp]

Or rather, it should be.

[dsyme]

I haven't marked this as approved because it's a bit of a free-for-all

The one I really don't like is this:

let pop2010 = series [ for c in wb.Countries ->
c.Name => c.Indicators.``CO2 emissions (kt)``.[2010]]

or this

let pop2010 = series [ 
for c in wb.Countries ->
   c.Name => c.Indicators.``CO2 emissions (kt)``.[2010]]

We have to have some limitations on how much undentation is allowed. This seems too much.

[Happypig375]

@dsyme That comment is one that I didn't implement because it does look jarring to me.

[dsyme]

@dsyme That comment is one that I didn't implement because it does look jarring to me.

Cool thanks

[dsyme]

Completed (in preview) dotnet/fsharp#11772

[redbar0n]

@dsyme I guess this issue can be closed, then.