-
Notifications
You must be signed in to change notification settings - Fork 790
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
Anonymous record execution order #6487
Comments
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 |
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? |
@dsyme what are your thoughts? |
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 |
yes re-ordering side effects is definitely unexpected. Question is if the ship has sailed and we need to live with that quirk now? |
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. |
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. |
Please fix then! |
Need @dsyme input on if we consider this a bug or not. |
Yes, this is a bug, top-down ordering should be used. Mea culpa. We should just make the change. |
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 ;). |
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) |
There goes my Easter weekend :) |
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 open Deedle
open FarNet
let data =
far.Panel.ShownFiles
|> Seq.map (fun x -> {| Name = x.Name; LastWriteTime = x.LastWriteTime |})
|> Frame.ofRecords
data.Print true |
@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... |
Correct, fields are ordered alphabetically. |
Re-opening as this is not resolved in VS |
Re-opening as this is not resolved in VS |
Confirmed as resolved in 16.2. |
Fields are ordered as outlined in the RFC. However,
outputs
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
The text was updated successfully, but these errors were encountered: