Anonymous record execution order

[kevmal]

Fields are ordered as outlined in the RFC. However,

{|
    C = printfn "0"
    B = printfn "1"
    A = printfn "2"
|}

outputs

2
1
0

Maybe the RFC just needs to be updated but without thinking about it I would still just assume top-down execution. Doesn't seem too crazy to have something like

{|
    Id = binaryReader.ReadInt32()
    DateTime = binaryReader.ReadInt64()
    ....
|}

[cartermp]

It's not 100% clear in the RFC (you really need to read through it to get here): https://github.com/fsharp/fslang-design/blob/master/FSharp-4.6/FS-1030-anonymous-records.md#alternative-do-not-sort-fields-by-name

I think the core issue is that the sorting affects reflection-based tools like FSI

[enricosada]

there is more info about it (field names are sorted, so initialization is sorted) in #6422

but in this case, maybe make sense to initialize field values in a binding or the il stack and later assign in order to anon method constructor?

[TIHan]

@dsyme what are your thoughts?

[enricosada]

As a note, that's not how normal types works

let t1 = { A = printfn "1"; B = printfn "2"; C = printfn "3"; }
// sorted field list, output: 1 2 3

let t2 = { B = printfn "1"; C = printfn "2"; A = printfn "3"; }
// same type, different order for fields, output: 1 2 3

let t3 = {| F = printfn "1"; E = printfn "2"; D = printfn "3"; |}
// output: 3 2 1

[matthid]

yes re-ordering side effects is definitely unexpected. Question is if the ship has sailed and we need to live with that quirk now?

[smoothdeveloper]

It would feel more natural if the values were computed before in the order they appear before initializing the record constructor, the same way as seen in plain records.

It doesn't feel we need to change any decision about field ordering to do this but this is indeed a breaking change.

Since anonymous records are very recent addition, I'd go with removing the gotcha and make them behave the same as records in terms of rhs evaluation.

[cartermp]

There's a relatively short window where we could make a breaking change to do the right thing here (you could argue that this is a bug). But eventually, enough people will depend on the current execution order, so we'd have to do this ASAP.

[rojepp]

Please fix then!

[cartermp]

Need @dsyme input on if we consider this a bug or not.

[dsyme]

Yes, this is a bug, top-down ordering should be used. Mea culpa. We should just make the change.

[abelbraaksma]

I would argue that relying on execution order of fields in an initialization expression suggests bad coding practice. Ideally, a functional language compiler should be able to reorganize executing if that benefits performance or code size.

Of course, we're dealing with a functional language compiler that allows side effects, so in the end execution order matters, should be documented and be predictable. I sometimes wished we had a mechanism to detect side effect-free code and optimize that ;).

[cartermp]

FYI @dsyme we have until next Wednesday to feasibly get something in for the VS 16.1 release, otherwise this would likely get pushed out to some future date (and thus be a wider breaking change)

[dsyme]

There goes my Easter weekend :)

[nightroman]

Will (or when?) this fix be included to FSharp.Compiler.Service?

NB I noticed similar issues playing with Deedle and anonymous records. This code creates a frame with columns LastWriteTime, Name instead of expected Name, LastWriteTime. I wonder if it's the same issue.

open Deedle
open FarNet

let data =
    far.Panel.ShownFiles
    |> Seq.map (fun x -> {| Name = x.Name; LastWriteTime = x.LastWriteTime |})
    |> Frame.ofRecords

data.Print true

[matthid]

@nightroman I don't think you scenario is part of the fix, nor is it considered an issue. afaik fields are ordered alphabetically in the type definition...

[cartermp]

Correct, fields are ordered alphabetically.

[nightroman]

@matthid @cartermp Thank you, it's good to know that this difference with regular records is the feature.

[cartermp]

Re-opening as this is not resolved in VS

[cartermp]

Re-opening as this is not resolved in VS

[cartermp]

Confirmed as resolved in 16.2.