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 Getters and Setters for Record Field Validation #516

Closed
5 of 6 tasks
cloudRoutine opened this issue Nov 13, 2016 · 4 comments
Closed
5 of 6 tasks

Allow Getters and Setters for Record Field Validation #516

cloudRoutine opened this issue Nov 13, 2016 · 4 comments

Comments

@cloudRoutine
Copy link

cloudRoutine commented Nov 13, 2016

Adding support for get and set for records would improve F#'s ability to make invalid
states unrepresentable at the term level.

let bounded x =
    if x < 0 then 0 
    elif x > 10 then 10 
    else x

type Data = 
    { Index : int
      Text  : string
    } 
    member self.Index 
        with get () = bounded self.Index
        and  set v  = self.Index <- bounded v
    member self.Text 
        with set v = self.Text <- if String.IsNullOrWhiteSpace v then String.Empty else v 

The get would be called each record field access, i.e.

let getText (arg:Data) = arg.Text

The set would be called during record construction and copy with, i.e.

let a = { Index = 5; Text = "aa" }
let b = { a with Text = "loop" }

Using the exact same syntax as normal property assignment might become confusing, since the
field of the record isn't mutable and it's probably unwise to create confusion on this point, especially
since records can have mutable fields. Having access to the self identifier for the type also allows the potential for many shenanigans involving other fields and methods.

An approach that addresses these issues is -

type Data = {
    Index : int
        with get (x:int) : int = bounded x
        and  set (v:int) : int = bounded v 
    Text  : string
        with set (v:string) : string = if String.IsNullOrWhiteSpace v then String.Empty else v
} 

The get and set functions would always be of the type 'fieldType -> 'fieldType where the arg is always the value of the field, which would prevent the getters and setters from being based on/tied to other fields of the record since there is no self identifier
for the type and doesn't create an illusion of mutability

In the case of a record with a mutable field set would be called on record construction, copy with, and <-

When a record has getters and setters it might be more apt to have the field come up as a property instead of a field.

Pros and Cons

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

  • Record field values can always be within an acceptable range of values
  • Removes need for boilerplate helper functions and methods to achieve the same result

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

  • Construct & copy operations with records can potentially explode
  • Could have a negative impact on pattern matching?
  • Potential for slight confusion over the difference between getters and setters for a class vs getters and setters for a record field

Extra informtion

Estimated cost L

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
@financial-engineer
Copy link

financial-engineer commented Nov 14, 2016

I support cloudRoutine's suggestion with my two hands! It is long past time to make invalid
states unrepresentable at the F# code level. I think this suggestion has to be extended to primitive types ( suggestion #517).

@dsyme
Copy link
Collaborator

dsyme commented Nov 14, 2016

Note the guidance today is to just do this:

type Data2(index : int, text  : string) =
    let mutable index = index
    let mutable text = text
    member __.Index 
        with get () = bounded index
        and  set v  = index <- bounded v
    member __.Text 
        with set v = text <- if String.IsNullOrWhiteSpace v then String.Empty else v 
    static member Create(index,text) = 
        // add some checks here
        Data2(index, text)

or if you still want a record then use this

type Data = 
    private { mutable _Index : int
              mutable _Text  : string } 
    member self.Index 
        with get () = bounded self._Index
        and  set v  = self._Index <- bounded v
    member self.Text 
        with set v = self._Text <- if String.IsNullOrWhiteSpace v then String.Empty else v 
    static member Create(index,text) = 
         // add some checks here
        { _Index=index; _Text=text }

The topic of making record types more like arbitrary object types, and object types as fully capable as record types (particularly with { a = b... } syntax including with syntax, is a recurring one in the suggestion database. The general direction is the latter more than the former.

@cloudRoutine
Copy link
Author

cloudRoutine commented Nov 16, 2016

@dsyme I don't think Data2 should be the guidance for an alternative with validation. The things that are great about records are what we get for free - structural equality, structural comparison, immutability, and pattern matching/decomposition. Data2 sacrifices all of these benefits for validation.

let a = Data2(10,"ouch")
let b = Data2(10,"ouch")
a = b // false
a > b // compiler error

The guidance for an alternative using classes should be more along the lines of -

type Data3 (index:int, text:string) =
    // the Create method in `Data2` is superfluous, a check in the constructor is sufficient
    let text = if String.IsNullOrWhiteSpace text then String.Empty else text 

    member val Index = bounded index with get
    member val Text = text with get 

    override self.Equals other = 
        match other with
        | :? Data3 as other ->
            self.Index = other.Index
            && self.Text = other.Text
        | _ -> false 

    override self.GetHashCode () =
        hash self.Index + hash self.Text

    // Before someone makes this comment, I understand that needing to define a custom comparison is 
    // perfectly reasonable for some cases, my point is it's great to have it by default when it's not 
    // necessary to do so
    interface IComparable with 
        member self.CompareTo other =
            match other with 
            | :? Data3 as other ->
                let x = compare self.Index other.Index
                if x = 0 then compare self.Text other.Text else x  
            | _ -> failwith "can't compare to a different type"

The preceding code is everything we need for this to work -

let x = Data3(10,"ouch")
let y = Data3(10,"ouch")
let z = Data3(10,"ouches")
x = y // true
z > y // true

Which gets us to the point where we at least have equality, comparison, and immutability but that's all.

The record example with Data also fails to address the objectives, a better example is -

module DM = 
    type Data4 = private { 
        // immutability is the objective, these should not be mutable
        index : int
        text  : string 
    } with 
        member self.Index with get () = self.index
        member self.Text  with get () = self.text

        static member Create idx txt = { 
            index = bounded idx
            text  = if String.IsNullOrWhiteSpace txt then String.Empty else txt
        }   

A member like Create is all fine and dandy in a trivial example like this one, but when it comes time to copy a record with 5+ fields, it becomes a hassle. Even more so when larger records of this kind are nested within one another. (Please no one bring up lenses, there are other issues to discuss them on)

open DM

let d1 = Data4.Create 10 "ouch"
let d2 = Data4.Create 10 "ouch"
let d3 = Data4.Create 10 "ouches"
d1 = d2 // true
d3 > d2 // true (although this record type should probably have [<CustomComparison>] and implement `IComparable`)

Which brings us to the same point as the prior example, but what have we lost in the process?

    let fn1_D4 (d:Data4) =
        match d with 
        | {index=0} -> true // Error - The union cases or fields of the type 'Data' are not accessible from this code location
        | _ -> false

    let fn2_D4 (d:Data4) =  {d with text = "sorry" }  // Error - The union cases or fields of the type 'Data' are not accessible from this code location

Boilerplate can help us claw our way back to a resonable semblance of an update mechanism, maybe Type Parameterized Type Providers could eventually generate these in the future, for now it's -

type Data3 with 
    member self.WithText  text  = Data3 (self.Index, text)
    member self.WithIndex index = Data3 (index, self.Text)

But for both examples pattern matching and decomposition are off the table, which are kind of the best part about records.

Structs are what get us the furthest for the least amount of effort -

[<Struct; StructuralComparison; StructuralEquality>]
type Data5 =
    val Index : int
    val Text  : string 
    new (index:int, text:string) =
        let index = bounded index 
        let text = if String.IsNullOrWhiteSpace text then String.Empty else text
        { Index = index; Text = text }

// Now we can pattern match! 
let fn1_D5 (d:Data5) = 
    match d with 
    | {Index=5} -> true 
    | {Index=10;Text=""} -> true
    | _ -> false

// And decomposition too!
let fn2_D5 (d:Data5) = d |> fun {Index=idx} -> printfn "%i" idx 

let fn3_D5 (d:Data5) = { d with Text=sorry } 
// ^ Error - The expression form { expr with ... } may only be used with record types. 
// To build object types use { new Type(...) with ... }

So the annoyance with updating remains, but like I said before maybe TPs will help this aspect in the future.

I figured this suggestion wasn't going to fly, but we should at least be honest about what the real trade-offs are with the alternatives.

@dsyme
Copy link
Collaborator

dsyme commented Apr 12, 2023

I'm going through old major issues with the "either close it or mark it as approved" approach.

This excellent comment pretty much sums things up:

I figured this suggestion wasn't going to fly, but we should at least be honest about what the real trade-offs are with the alternatives.

#164 is relevant, but the overheads are still very high.

@dsyme dsyme closed this as completed Apr 12, 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

3 participants