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

Generate F# 9-style Is* property for single case discriminated union #1394

Open
6 tasks done
tw0po1nt opened this issue Nov 27, 2024 · 18 comments
Open
6 tasks done

Generate F# 9-style Is* property for single case discriminated union #1394

tw0po1nt opened this issue Nov 27, 2024 · 18 comments

Comments

@tw0po1nt
Copy link

I propose we generate an Is* boolean property for single case unions, as F# 9 does today for multi-case unions.

module Types

open System

type AssetType = Image // Intention is to add to this definition in the future as asset types are needed

type Asset =
  { Type : AssetType 
    SourceURL : Uri }

// I want to write a function like this, such that even if new cases are added in the future, this function gives me only the ones of type `Image`
let onlyImages assets =
  assets
  |> List.filter _.Type.IsImage // as is, this line errors with: 'The type 'AssetType' does not define the field, constructor, or member 'IsImage'. Maybe you want one of the following: Image'

The existing way of approaching this problem in F# is to add a throwaway associated value and pattern match, like so:

type AssetType = Image of int // could be any type, in theory

let onlyImages assets =
  assets
  |> List.filter (fun ({ Type = (Image _) }) -> true)

Pros and Cons

The advantages of making this adjustment to F# are that the property is generated for all cases of a DU, not just when there are multiple. While in theory the Is property is redundant for that case since it is guaranteed to be true, this enhancement improves the behavior for "planning for future cases" by allowing to use the same syntax, instead of having to do a hacky workaround just for the single case scenario

The disadvantages of making this adjustment to F# are that perhaps this is a small thing and maybe the juice isn't worth the squeeze just for this extra consistency?

Extra information

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

Affidavit (please submit!)

Please tick these items 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
  • This is a language change and not purely a tooling change (e.g. compiler bug, editor support, warning/error messages, new warning, non-breaking optimisation) belonging to the compiler and tooling repository
  • 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
  • I have searched both open and closed suggestions on this site and believe this is not a duplicate

Please tick all that apply:

  • This is not a breaking change to the F# language design
  • 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 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

@brianrourkeboll
Copy link

Not sure how I feel about this.

One odd thing is that you can't polyfill it yourself if you want:

type U =
    | A
    member this.IsA = match this with A -> true
> type U =
-     | A
-     member this.IsA = match this with A -> true;;

      member this.IsA = match this with A -> true;;
  ----------------^^^

stdin(3,17): error FS0023: The member 'IsA' cannot be defined because the name 'IsA' clashes with the default augmentation of the union case 'A' in this type or module

I wonder if that is by design.

@brianrourkeboll
Copy link

It looks like the original implementation emitted this, and it was later changed not to: fsharp/fslang-design#517 (comment)

The RFC itself does not mention it: https://github.com/fsharp/fslang-design/blob/3a46bc9342f4c2c38d09617b4d95909697e7b9d6/RFCs/FS-1079-union-properties-visible.md

@tw0po1nt
Copy link
Author

Interesting. Doesn't seem like there was a ton of discussion involved - just "this feels weird" and then it gets removed. I think we should revisit that. Having it missing for single case unions feels like a "gotcha".

But that is also good news in that the exact place to implement this is straightforward.

@brianrourkeboll
Copy link

just "this feels weird" and then it gets removed.

To be fair, @T-Gro did mention the main reason why:

it increased the surface area of IL-exposed members since those were not generated before either (testers for single-case unions).

and, in dotnet/fsharp#16571,

[the RFC was originally about] exposing those members to F# as well, not changing codegen.

I.e., the F# compiler already did generate Is* properties for multi-case unions before F# 9; they just weren't visible from F#. Hence the RFC title: "Make .Is* discriminated union properties visible from F#."

But the compiler did not generate an Is* property for single-case unions before F# 9, so making it do so is somewhat a separate question from exposing the existing generated properties to F#.

@tw0po1nt
Copy link
Author

Yes, that makes sense.

Then this could probably be added as a con to implementing this.

increased the surface area of IL-exposed members since those were not generated before

I do think the consistency it would bring would be worth the effort.

@T-Gro
Copy link
Contributor

T-Gro commented Nov 28, 2024

Not generating the warning in the described case is doable, that was definitely an oversight and mismatch in codegen vs. warning.
i.e. changing the warning to allow defining your own .Is* member for single-case DU. As a workaround, a any other name can be used of course.

I must admit I do not fully see the motivation for practical F# programs.
When extending a single-case DU to >1 cases, warnings/errors are produced for code working with it, such as function arguments or various forms of pattern matching. Therefore making it clear which pieces of business logic must be changed for the newly added cases.

 let workWithImage Image = 42
 let workWithImages imgList =
     match imgList with
     | Image :: _rest = 42
     | [] -> 2112

@charlesroddie
Copy link

Regularity is good so this should be added.
Use-cases niche, e.g.

// This code is OK and remains OK when more shape cases are added.
type Shape = | Rectangle of Dimensions
let countRectangles(shapes: Shape list) =
    shapes |> List.countWhere(s -> s.IsRectangle)

But you don't need big use cases to justify making the langauge more regular. Even if there were no use case it should be there!

@T-Gro
Copy link
Contributor

T-Gro commented Nov 28, 2024

If you would have hand-written logic for .IsRectangle, you could amend it if/once future extension arrives.
Let's say the next ones to be added will be Circle and Square, following most OO teaching materials.

I am the kind of person who would rather get a warning and adjust logic to new requirements case by case. Rather then having no new warnings, but wrong business logic caused by pre-existing usage of .IsRectangle . (because once new requirements were added, counting rectangles might had to include squares as well).

But you are right that this equally applies to any number of cases, not only size=1. Usage for size=1 just makes it apparent that the intention is to prepare for unknown future/possible requirements, which I guess is the part which feels wrong about this.

@Martin521
Copy link

Usage for size=1 just makes it apparent that the intention is to prepare for unknown future/possible requirements, which I guess is the part which feels wrong about this.

Yes. YAGNI.

@charlesroddie
Copy link

charlesroddie commented Nov 28, 2024

But you are right that this equally applies to any number of cases, not only size=1. Usage for size=1 just makes it apparent that the intention is to prepare for unknown future/possible requirements, which I guess is the part which feels wrong about this.

Yes it does make that apparent, as it should as this is a definition of a DU aka discriminated union. So it feels wrong that the discrimination scaffolding isn't there, including IsX.

Yes. YAGNI.

And yes to YAGNI - sometimes discriminated unions shouldn't be used but instead the underlying data type or a simpler non-discriminated non-union wrapper used, for situations in which it's not expected that cases will be added. But this doesn't mean that it's never useful to start with the possibility of discrimination and so there shouldn't be a restriction on DUs that they should have at least 2 cases. E.g. I might know I am going to write more Shapes, and I might want my serialization/deserialization of Shapes to conform to a DU structure so that it doesn't change when adding more cases.

Given that we are not going to ban DUs with one case, ensuring that they are treated as DUs and not something special is important for consistency.

@tw0po1nt
Copy link
Author

Even if there were no use case it should be there!

Disagree here, but I think you put it well:

Given that we are not going to ban DUs with one case, ensuring that they are treated as DUs and not something special is important for consistency

Some additional context might be helpful - I am a relative newcomer to F#, I've only just recently started writing meaningful amounts of F# code. When I saw the Is* feature as added in F# 9, I immediately had an intuition as to how it should behave based on how the feature was advertised. Namely:

Previously, you had to write something like:

let canSendEmailTo person =
    match person.contact with
    | Email _ -> true
    | _ -> false

Now, you can instead write:

let canSendEmailTo person =
    person.contact.IsEmail

Nothing in this presentation makes me think "instead you can write this, but only in the case of a DU with at least 2 cases"

It's a small inconsistency, sure. It's also one that, if fixed, arguably might not necessarily benefit a vast number of use cases.

But it's an inconsistency nonetheless. And in my opinion, small inconsistencies are the ones that are the most off-putting for me as a user of a language.

@T-Gro
Copy link
Contributor

T-Gro commented Jan 20, 2025

I think we should allow to polyfill that function yourself, and rework the existing warning to only warn for #cases>1.

I am still not convinced that the compiler should automatically generate it for every single DU (of size 1). Single case DUs are a very common data modelling tool and this just increases the surface area (both F#-visible as well as in the generated IL) for a use case that does not have strong support, and encourages a programming style that is not always recommended.

@tw0po1nt
Copy link
Author

I think we should allow to polyfill that function yourself, and rework the existing warning to only warn for #cases>1.

Agreed. At a minimum we should do this.

encourages a programming style that is not always recommended.

F# supports object programming, but I would hardly say it encourages that style.

It could also be argued that having Is* properties exposed at all encourages circumventing exhaustive pattern matching. And yet F# 9 exposed them because they're a convenient thing to have.

Single case DUs are a very common data modelling tool

Indeed. I would say that DUs in general are. Why treat the single-case as something special? That's ultimately what this boils down to for me: they're treated as special -- which constitutes an inconsistency in the language -- but are not particularly well-documented as such.

@brianrourkeboll
Copy link

I think we should allow to polyfill that function yourself, and rework the existing warning to only warn for #cases>1.

Agreed.

Why treat the single-case as something special?

Single-case unions are special, though. There are no other cases that they could be, and this fact is known at compile time. It is always synchronically redundant to ask which case an instance of a single-case union is.

I don't find the "what if more cases are added to the union in the future" argument compelling:

  1. If a second case is never added, an Is* property is never useful and only serves to mislead and increase code size.

  2. If a second case is added later, the compiler does generate Is* properties (because it has become meaningful to ask which case an instance of the type is).

  3. If you are concerned about your code correctly handling potential future cases, don't use a nullary union case as a tag. Instead, wrap the data inside of the union case so that you can only act on an Image at the moment when you are guaranteed to in fact have an Image, i.e., when you have pattern-matched on it. (See the next point.) That will also mean that, when a new case is added, the compiler will automatically remind you both that you must handle it and where you must handle it, and you can decide how to handle it then, when you have more information.

  4. I understand that the original example is probably intentionally made trivial, but I wonder whether it may be too trivial to be instructive.
    Given the example code, there is nothing you can do with the results of this expression other than (1) get the length of the resulting list (as in Generate F# 9-style Is* property for single case discriminated union #1394 (comment)) or (2) access Asset.SourceURL on each item in the list:

    assets |> List.filter _.Type.IsImage

    Part of that is perhaps because the example uses a union that is not only single-case but also nullary, i.e., it has no fields.

    Once a union has one or more fields, you must pattern-match on the case or cases to do anything useful (other than counting) anyway. For example, if instead of

    type AssetType = Image
    
    type Asset =
      { Type : AssetType
        SourceURL : Uri }

    you wrote

    type Asset = Image of sourceUrl : Uri

    and

    assets |> List.filter _.IsImage

    you could not do anything at all with the resulting list of Assets other than inspect its length—or iterate over it again and pattern-match on each Image to extract the sourceUrl, e.g.,

    assets |> List.map (fun (Image sourceUrl) -> sourceUrl)

    —which you could have just done in the first place.

The upshot to me feels like:

  1. Having the Is* available on single-case unions is never useful at the time of writing it.
  2. If another case is added later, having been able to use Is* for the original single-case union will only have become useful if (1) that case was nullary or effectively nullary (being used as a tag), (2) you are counting something.

That seems like a pretty limited amount of utility, and I think there are nearly always better ways to do things anyway.

But you don't need big use cases to justify making the langauge more regular. Even if there were no use case it should be there!

I disagree with this. I don't think we should increase code size without the utility to justify it.

@charlesroddie
Copy link

charlesroddie commented Jan 21, 2025

@brianrourkeboll Having the Is* available on single-case unions is never useful at the time of writing it.

We agree that the Is feature in general is not useful all that often, but is useful for counting and can also be used for cases with no data.

You say that Is is not useful for single case discriminated unions. I don't think this argument is consistent. Take my shapes example for concreteness. Without Is you would not want to use countRectangles(shapes: Shape list) = shapes.Length, since this will become incorrect when more cases are added. Instead you would use shapes |> List.filter (fun s -> s.IsRectangle) or shapes |> List.filter (fun s -> match s with | Shape.Rectangle _ -> true | _ -> false) and we have already agreed that the former can be preferable, since we agreed that there is a use case for Is namely in counting.

I don't think we should increase code size

You mention this consideration was the reason originally not to include these methods. This is not a cost in any context where this is important.

Dotnet trimmers are very good for any type-safe contexts, including F#. The only case where you would deploy something without trimming is if you don't care about compiled code size.

If you don't use Is, it will be trimmed away.

I disagree with this.

An example of my point about consistency even without use cases is the code let x = 2 + 0. I don't think anyone should write this, and there is no good use case for writing code with explicit addition of zero. However it should be allowed for consistency. I won't stress the point though because the Is feature does have a use case as discussed above.

@charlesroddie
Copy link

charlesroddie commented Jan 21, 2025

@T-Gro Single case DUs are a very common data modelling tool... encourages a programming style that is not always recommended

We have to maintain this as absolute:

A discriminated union is discriminated

To argue that a we should have single-case discriminated unions and at the same time argue that they have no need to discriminate and that all discrimination logic can be removed is nonsensical.

Can you use a discriminated union if you just have one case and have no intention to discriminate any point in the future? Yes it's possible. You just don't use the discrimination features in any useful way.

I would strongly recommend not doing so since it is better to use a non-discriminated type. A single-field record is better and so is a struct containing a single value. Both are similar to using a single-case DU without the discrimination (matching, Is) and discrimination is not required.

And even if this were to become common, it should in no way stop discriminated unions from being discriminated!

"I used a type called discriminated something; please can you remove discrimination costs from this because it's not useful to me" is an argument that should therefore have zero weight.

A similar argument has been made for measures, "I want to use F# units of measure for type safety and instead of a record or class; please can you remove the constraint that measures apply to types that can be multiplied?" Similarly breaking the intent and meaning of the original feature.

@T-Gro
Copy link
Contributor

T-Gro commented Jan 22, 2025

To argue that a we should have single-case discriminated unions and at the same time argue that they have no need to discriminate and that all discrimination logic can be removed is nonsensical.

The recommended way remains pattern matching. It works for unions of any size, and if number of cases grows from 1 to 2 or more, all places pattern matching it will show a warning.

The rare and opinionated cases that require '.IsX' (always returning true) in their single-case-DU scenarios should be allowed to opt-in into that, I agree. The warning should be lifted and definition of such custom member allowed.

@charlesroddie
Copy link

I agree that in general generating IsX vs allowing user-defined IsX would both have both OK, but this is a consistency issue. n=1 should behave the same way as n>1. For backwards compatibility I believe it's impossible to change n>1 to opt-in, so n=1 should be changed to generated.

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

5 participants