Skip to content
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

Allow easier access to properties common to all cases in Discriminated Union #564

Closed
5 of 6 tasks
tastyeggs opened this issue Apr 23, 2017 · 13 comments
Closed
5 of 6 tasks

Comments

@tastyeggs
Copy link

tastyeggs commented Apr 23, 2017

I propose we add property accessors for all properties that are common among all cases in a discriminated union at the type level. Both the Label and the Type of the property can be used to determine the set of common properties.

For example:

type ShoppingCart = 
| EmptyCart of AppliedDiscountCode: string
| CartWithItems of AppliedDiscountCode: string * Items: string list
| CompletedCart of AppliedDiscountCode: string * Items: string list * CalculatedTotal: decimal

AppliedDiscountCode is common among all cases, and if I had a variable v of type ShoppingCart, I must be able to do v.AppliedDiscount.

The existing way of approaching this problem in F# is:

  1. If I'm just accessing this property once or twice, I can write up a match statement
  2. Or I can define a member (which is usually what I end up doing, but this is so much boilerplate, especially if I have a lot of cases)

Pros and Cons

The advantages of making this adjustment to F# are:

  • An easier way to represent properties that are common to the DU type (as opposed to just a case), similar to the Base Class / Derived Class representations in OO languages
  • Single-case DUs become much easier to use

The disadvantages of making this adjustment to F# are:

  • It may be a bit presumptuous to assume that these properties in the DU mean semantically the same thing. However, if we limit this to properties that have an explicit case (as opposed to the Item1, Item, .. default cases assigned to by the compiler, this may not be a bad assumption)
  • It may conflict with properties that are already defined -- if that's the case, simply don't auto-generate these properties if there's a conflict.

Extra information

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

Affadavit (must be submitted)

Please tick this by placing a cross in the box:

  • This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • 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:

  • This is not a breaking change to the F# language design
  • I would be willing to help implement and/or test this
  • I or my company would be willing to help crowdfund F# Software Foundation members to work on this
@tastyeggs tastyeggs changed the title Allow easier access to properties common to all cases Allow easier access to properties common to all cases in Discriminated Union Apr 23, 2017
@matthid
Copy link

matthid commented Apr 29, 2017

I'm against this because is breaks code on surprising ways when adding another case :)
You can already bind partially on named fields

type ShoppingCart =
    | EmptyCart of AppliedDiscountCode: string
    | CartWithItems of AppliedDiscountCode: string * Items: string list
    | CompletedCart of AppliedDiscountCode: string * Items: string list * CalculatedTotal: decimal
    member x.DiscountCode = x |> function
        | EmptyCart (AppliedDiscountCode=discount) | CartWithItems(AppliedDiscountCode=discount)
        | CompletedCart(AppliedDiscountCode=discount) -> discount

which makes this a bit better. The only way I see this feature is when it's only possible to access it from within the DU definition (to limit the number of surprising code locations where stuff breaks). But that would mean special syntax or special rules within the DU (which again is bad)...

Also if stuff is part of every case you might want to consider to re-design it to

type ShoppingCartList =
    | EmptyCart
    | CartWithItems of Items: string list
    | CompletedCart of Items: string list * CalculatedTotal: decimal

type ShoppingCart =
    { AppliedDiscountCode: string; CartList: ShoppingCartList }

if you properly hide stuff behind modules this works surprisingly well, and often improves the code as well.

@dsyme
Copy link
Collaborator

dsyme commented Apr 29, 2017

An alternative is to allow union types to also have a common set of fields:

type ShoppingCart =
    val DiscountCode: string
    | EmptyCart
    | CartWithItems of Items: string list
    | CompletedCart of Items: string list * CalculatedTotal: decimal

There's no real underlying technical reason we can't allow something this, though it's a bit odd. Is it a record type or a union type? I'd be interested in Scott Wlaschin's take on this for modelling.

The construction syntax is a challenge, probably would need to use a named arg e.g. EmptyCart(DiscountCode=code).

@dsyme
Copy link
Collaborator

dsyme commented Feb 1, 2018

@forki I note that some Elmish.Fable view models would definitely benefit from this, e.g.

type Model =
    val User : UserData option
    | HomePageModel
    | LoginModel of Login.Model
    | WishListModel of WishList.Model

instead of the two types:

type PageModel =
    | HomePageModel
    | LoginModel of Login.Model
    | WishListModel of WishList.Model

type Model =
  { User : UserData option
    PageModel : PageModel }

It's nice that one type has been saved - it makes the model more obvious, and this feels like a common pattern for UI view models that compose a set of possible views and add some global information relevant to all the views.

However it's not obvious what copy-and-update would be for such a type when updating only the union portion, e.g. what is the equivalent of this?

        { model with
            PageModel = WishListModel m }

e.g. something new like

    { model with | WishListModel m }

@cartermp
Copy link
Member

cartermp commented Feb 1, 2018

I'm not sold on this being a good idea. I agree that it can sometimes be annoying or inconvenient to have to define a DU and a Record separately, but that separation also forces you to keep things a bit cleaner.

In @dsyme's example this is convenient for a small Fable app, but a PageModel is a lot more generic than the example given, and could see use without the User portion in a larger app. I don't think that's enough to justify the kludging together of DU and Record into a Ducard/Recunion/etc.

@Rickasaurus
Copy link

I really like the idea of base fields for union types. This would save a lot of extra objects and/or duplication we have going on right now. Struggling to find any real downsides because you can just not use the feature if you're not a fan. It almost points to something a bit richer, allowing multiple levels with certain things shared at each level.

@forki
Copy link
Member

forki commented Feb 2, 2018 via email

@NullVoxPopuli
Copy link

NullVoxPopuli commented Mar 28, 2018

I would very much like to be able to access common properties within a union type.

See this example (Extracting parsed json provider payloads to a common type):

namespace CryptoApi.Data

open Rationals
open System


type public Order = {
    Id: Guid;

    Market: Market;

    State: string;
    Side: string;
    Type: string;
    Price: Rational;
    Size: Rational;
    Filled: Rational;

    Timestamp: int64;
}

////////////////////////////////////////////////
namespace CryptoApi.Exchanges.Cobinhood.Data.Providers

open FSharp.Data

module Trading =
    type GetOrderResponse = JsonProvider<"""{
        "success": true,
        "result": {
            "order": {
                "id": "37f550a202aa6a3fe120f420637c894c",
                "trading_pair": "BTC-USDT",
                "state": "open",
                "side": "bid",
                "type": "limit",
                "price": "5000.01",
                "size": "1.0100",
                "filled": "0.59",
                "timestamp": 1504459805123
            }
        }
    }""">

    type GetAllOrdersResponse = JsonProvider<"""{
        "success": true,
        "result": {
            "orders": [
                {
                    "id": "37f550a202aa6a3fe120f420637c894c",
                    "trading_pair": "BTC-USDT",
                    "state": "open",
                    "side": "bid",
                    "type": "limit",
                    "price": "5000.01",
                    "size": "1.0100",
                    "filled": "0.59",
                    "timestamp": 1504459805123,
                    "eq_price": "5000.01"
                }
            ]
        }
    }""">


////////////////////////////////////////////////
namespace CryptoApi.Exchanges.Cobinhood.Data.Transformers

open CryptoApi.Data

open CryptoApi.Exchanges.Cobinhood.Data.Providers.Trading

module Orders =
    open Rationals

    type TypeOfOrderResponse = 
        | OrdersOrder of GetAllOrdersResponse.Order
        | OrderOrder of GetOrderResponse.Order

    let ExtractOrderFromPayload (knownMarkets: Market[]) (payload: TypeOfOrderResponse): Order = 
        let symbolFromOrder = match payload with
                              | OrdersOrder o -> o.TradingPair
                              | OrderOrder o -> o.TradingPair

        let marketEqualsSymbol = fun km -> km.symbol.Equals(symbolFromOrder)
        let market = knownMarkets |> Array.find marketEqualsSymbol

        let order = {
            Market = market;

            Id = match payload with
                     | OrdersOrder o -> o.Id
                     | OrderOrder o -> o.Id;

            State = match payload with
                     | OrdersOrder o -> o.State
                     | OrderOrder o -> o.State;
            Side = match payload with
                     | OrdersOrder o -> o.Side
                     | OrderOrder o -> o.Side;
            Type = match payload with
                     | OrdersOrder o -> o.Type
                     | OrderOrder o -> o.Type;
            Price = match payload with
                     | OrdersOrder o -> Rational.Approximate(o.Price)
                     | OrderOrder o -> Rational.Approximate(o.Price);
            Size = match payload with
                     | OrdersOrder o -> Rational.Approximate(o.Size)
                     | OrderOrder o -> Rational.Approximate(o.Size);
            Filled = match payload with
                     | OrdersOrder o -> Rational.Approximate(o.Filled)
                     | OrderOrder o -> Rational.Approximate(o.Filled);
            Timestamp = match payload with
                     | OrdersOrder o -> o.Timestamp
                     | OrderOrder o -> o.Timestamp;
        }

        order

    let ExtractOrder (knownMarkets: Market[]) (payload: GetOrderResponse.Root): Order =
        (OrderOrder payload.Result.Order)
        |> ExtractOrderFromPayload knownMarkets

    let ExtractOrders (knownMarkets: Market[]) (payload: GetAllOrdersResponse.Root): Order[] =
        payload.Result.Orders
        |> Array.map (fun o -> ExtractOrderFromPayload knownMarkets (OrdersOrder o))

If there is a more concise technique for writing ExtractOrderFromPayload I'm all ears.
But it seems to be that being able to get common properties would be the easiest/cleanest solution.

@smoothdeveloper
Copy link
Contributor

I encounter similar patterns in my code, but it is rare that it is simple field access among the DU cases, it generally involves additional logic, so I'll likely not use this frequently, at least in my current approach.

I'm not against it, but also concur that it might be a design smell, why not have a record containing the shared field along with an instance of DU?

type ShoppingCartStatus = 
| EmptyCart
| CartWithItems of Items: string list
| CompletedCart of Items: string list * CalculatedTotal: decimal

type ShoppingCart = { AppliedDiscountCode: string; Status: ShoppingCartStatus }

Going with the idea, it might be relevant to have ability to expose an option value for fields that aren't defined for all cases (it would be exposed if a field is present in more than a single case).

type ShoppingCart = 
| EmptyCart of AppliedDiscountCode: string
| CartWithItems of AppliedDiscountCode: string * Items: string list
| CompletedCart of AppliedDiscountCode: string * Items: string list * CalculatedTotal: decimal

This would have an Items: string list option due to the field being present in two cases.

@NullVoxPopuli
Copy link

I think my case may be more specific to JsonProviders where multiple providers can contain similar payloads

@jbeeko jbeeko mentioned this issue Apr 5, 2018
5 tasks
@chkn
Copy link

chkn commented Jan 26, 2020

I'd like to poke this thread, since I've just run into this case myself.

My thoughts on previous comments:

I'm against this because is breaks code on surprising ways when adding another case :)

I don't see what is surprising. Pattern matching code already breaks when adding a new case (and rightly so).

An alternative is to allow union types to also have a common set of fields

Personally, I think the originally proposed syntax is preferable to @dsyme's proposal, because it doesn't interfere with positional construction or deconstruction, and is a less invasive change to the language.

Ultimately, I'd like to advocate for this change, since I think it helps streamline and remove boilerplate in some cases, with relatively no drawback.

@Swoorup
Copy link

Swoorup commented Feb 28, 2020

I like this version instead.
#115

If you add a new case with fields no longer common, its should be removed as a member?

@Swoorup
Copy link

Swoorup commented Feb 3, 2022

#115

How would the creation of such type look like?
I have been needing this in a quite of bit of places, and I ponder if I am better of not using DUs but classes and interfaces sometimes.

Although Scala 3 has enum which is close to DU in F#, users still use sealed traits which models this kind of data.

@dsyme
Copy link
Collaborator

dsyme commented Jan 10, 2023

I'm closing this, partly because #115 was also rejected. Neither of these suggestions look right to me, e.g. see @cartermp 's comment above

@dsyme dsyme closed this as completed Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants