fslang-suggestions icon indicating copy to clipboard operation
fslang-suggestions copied to clipboard

Allow cloning of generic records

Open mvkara opened this issue 3 years ago • 7 comments

Allow cloning of generic records

I propose we allow the record cloning syntax to allow us to clone records with different parameterized types.

The existing way of approaching this problem in F# is cloning the whole record from scratch assigning all fields.

type MessageEnvelope<'t> = { 
    CommonMetadata: Metadata
    // more fields ...
    Data: 't 
}

let clone (original: MessageEnvelope<'f>) (newData: 't> : MessageEnvelope<'t> = {
    CommonMetadata = original.Metadata
    Timestamp = original.Timestamp
    // more field assignments ...
    Data = newData
}

Data types like this are quite common with things like deserialized messages from protocols, etc where the message often carries metadata such as headers and timestamps. When transforming the data I would prefer to keep the majority of the message without having to maintain 'clone' functions.

Pros and Cons

The advantages of making this adjustment to F# are ...

  • Less boilerplate when cloning things with common fields especially when the object has deep hierarchies. This comes up quite a bit when transforming payloads but having the same metadata for the given message carried through (for example timestamps, versioning, ID information,

The disadvantages of making this adjustment to F# are ...

  • None I can think of.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: (put links to related suggestions here)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [X] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [X] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [X] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • [X] This is not a breaking change to the F# language design
  • [X] I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

mvkara avatar Aug 15 '21 02:08 mvkara

Does { original with Data = newData } somehow not work here?

Happypig375 avatar Aug 15 '21 04:08 Happypig375

@Happypig375 No it doesn't work because the original and returned records have different types (MessageEnvelope<'f> vs MessageEnvelope<'t>). I've encountered this a couple times.

I agree that it would be nice to allow this, as long as all the fields whose type is or contains the generic parameter are changed by the with, obviously.

I'm guessing the main question this poses is about type inference. Right now, { x with ... } is inferred to have the exact same type as x. Should it remain like this, with an exception to allow changing parameters when the return type is explicitly specified to be the same with different parameters? Maybe if the changed field value has an explicitly specified different type too? Or should the inference be loosened for all uses of with on a parameterized type? This latter option sounds like a breaking change.

Tarmil avatar Aug 15 '21 09:08 Tarmil

@Tarmil My impression was that the "With" syntax simply compiles into a similar form as having to create the record and assigning the fields yourself. In this case type inference simply works on the expanded form of the "with" syntax. For example:

type ExampleRecord<'t> = { OriginalTimestamp: int; Data: 't }

let cloneManual (original: ExampleRecord<'f>) (newData: 't) : ExampleRecord<'t> = {
    OriginalTimestamp = original.OriginalTimestamp
    Data = newData
}

let generateData() = 
    let sourceData = { OriginalTimestamp = 0; Data = "Test" }
    let clonedManual = cloneManual sourceData 1
    let clonedAutomatic = { sourceData with Data = "1" } // This is what I would prefer to assign to an Int.
    (clonedManual, clonedAutomatic)

The generateData() function decompiles to the below. Interestingly the "cloneManual" is inlined in even without the inline keyword. I'm not sure why the type inference couldn't work on the expanded result?

public static Tuple<ExampleRecord<int>, ExampleRecord<string>> generateData()
{
    return new Tuple<ExampleRecord<int>, ExampleRecord<string>>(new ExampleRecord<int>(0, 1), new ExampleRecord<string>(0, "1"));
}

mvkara avatar Aug 15 '21 09:08 mvkara

"Breaking change" doesn't mean that it wouldn't work; just that the same code will give a different result. If we write clone without type annotations:

let clone original newData = { original with Data = newData }

Current F# will infer val clone : MessageEnvelope<'a> -> 'a -> MessageEnvelope<'a>

A loosened implementation would infer val clone : MessageEnvelope<'a> -> 'b -> MessageEnvelope<'b>

Tarmil avatar Aug 15 '21 15:08 Tarmil

This is a breaking change for a number of reasons - though one I personally wouldn't mind making if we could find a way to make it.

e.g.

let f x =
    let c = clone x { Data = "" }
    c.Data.Length

Here c.Data.Length will only resolve if Length is known to be a string.

dsyme avatar Aug 15 '21 22:08 dsyme

Oh this would help quite a lot with similar things I am doing. I have boilerplate like this just for the sake for casting.

module GenericCommandHandler =
  let private downcastCmdEnv (cmd: CommandEnvelope<ICommand, IMetadata>): CommandEnvelope<#ICommand, Metadata> =
    { CommandEnvelope.Payload = downcast cmd.Payload
      AggregateId = cmd.AggregateId
      CommandId = cmd.CommandId
      CausationId = cmd.CausationId
      CorrelationId = cmd.CorrelationId
      ProcessId = None
      CopyMetadata = cmd.CopyMetadata
      Metadata = cmd.Metadata |> Option.map (fun md -> md :?> Metadata)
      ExpectedVersion = cmd.ExpectedVersion }
  
  let upcastEvtEnv (evt: EventEnvelope<#IEvent, Metadata>): EventEnvelope =
    { EventEnvelope.Payload = evt.Payload :> IEvent
      AggregateId = evt.AggregateId
      EventId = evt.EventId
      CorrelationId = evt.CorrelationId
      CausationId = evt.CausationId
      ProcessId = None
      EventVersion = evt.EventVersion
      EventPosition = evt.EventPosition
      Metadata = evt.Metadata |> Option.map (fun md -> md :> IMetadata)
      CreatedUtc = evt.CreatedUtc }
    
  let inline private downcastEvtEnv (evt: EventEnvelope): EventEnvelope<#IEvent, Metadata> =
    { EventEnvelope.Payload = evt.Payload :?> _
      AggregateId = evt.AggregateId
      EventId = evt.EventId
      CorrelationId = evt.CorrelationId
      CausationId = evt.CausationId
      ProcessId = None
      EventVersion = evt.EventVersion
      EventPosition = evt.EventPosition
      Metadata = evt.Metadata |> Option.map (fun md -> md :?> Metadata)
      CreatedUtc = evt.CreatedUtc }

Swoorup avatar Oct 03 '21 04:10 Swoorup

I'm not really sure the best way of making progress on this, given that the most natural implementation would be a breaking change. Requiring explicit type annotations seems a bit unfortunate but I guess is the only way forward

The problematic line is here: https://github.com/dotnet/fsharp/blob/c4ff15f6f4e73cec65d2b3a782bac53bc5dbf4c2/src/Compiler/Checking/CheckExpressions.fs#L7385. This uses the overallTy as the precise known type for the origExpr. At this point we don't even know the record type associated with the fields. This means in cases like

let f (x: R<'a>) : R<'b> = { x with ... }

we've already fed in R<'b> as the expected type for x which will force a unification (with a warning that 'a = 'b - not an error). It's just difficult to see where we could stop this process and "know" that we have enough concrete type information for x and the record type to avoid going down the unification path - unless we special case to when x is a value or similar.

dsyme avatar Jun 16 '22 17:06 dsyme