fsharp / fslang-design

RFCs and docs related to the F# language design process, see https://github.com/fsharp/fslang-suggestions to submit ideas
520 stars 143 forks source link

[RFC FS-1004 Discussion] Result type #49

Closed enricosada closed 7 years ago

enricosada commented 8 years ago

This issue is used to track discussions of F# RFC FS-1004 - "Result type". Please discuss in thread below (if necessary)

dsyme commented 8 years ago

I'm not actually in favour of the abbreviation I mentioned - I just wanted to gauge reaction.

Despite the last few comments, the third choice of adding a new type alone (and nothing else) to FSharp.Core is a valid option, for these reasons:

There are other consistent design points, e.g. adding the Result type along with builders for option, asyncOption, result, asyncResult and modules of functions for these too. However equally that could be considered a separate request/RFC ("add option, asyncOption, result and asyncResult builders to FSharp.Core").

@vasily-kirichenko The naming would be OK/Error not Ok/Err. I suppose that means I'm Ok with Ok as an abbreviation, but Err is a problem :)

vasily-kirichenko commented 8 years ago

yes, maybe and asyncMaybe builders should be added, too, but it's another story.

why OK/Error? write a full screen of code that makes heavy usage of such names and you will see if it's a good idea.

vasily-kirichenko commented 8 years ago

OCaml

module Result : sig
   type ('a,'b) t = | Ok of 'a
                    | Error of 'b
end

Rust

enum Result<T, E> {
   Ok(T),
   Err(E)
}

Swift

enum Result<T, E> {
  case Success(T)
  case Failure(E)
}

Scala, Haskell: Either = Left | Right

radekm commented 8 years ago

@vasily-kirichenko Additionally Scala has Try (defined in Try.scala):

sealed abstract class Try[+T] { /* ... */ }
final case class Failure[+T](val exception: Throwable) extends Try[T] { /* ... */ }
final case class Success[+T](value: T) extends Try[T] { /* ... */ }
dsyme commented 8 years ago

@vasily-kirichenko

why OK/Error?

The F# design guidelines are pretty clear about abbreviations. We're not going to break the design guidelines in the core library just to save two characters.

vasily-kirichenko commented 8 years ago

Ok, I think the whole idea was wrong. Choice'2 is just fine.

dsyme commented 8 years ago

@vasily-kirichenko The logical consequence of that would be to recommend the use of the Choice2` type for functional error handling across component boundaries. However, I don't see us actually doing that. And even if we did recommend it I don't think people would actually use it.

Could you explain more why you think adding a Result type for use for functional error handling across component boundaries is fundamentally wrong? I assume it doesn't come down to the name of the Error case?

Thanks Don

vasily-kirichenko commented 8 years ago

OK, that was an emotional answer, sorry. However, if it's going to be OK and Error, I will define aliases for them, but it's OK.

Speaking about F# design guidelines, could you comment why List/Seq/Array's iter function is not named iterate?

dsyme commented 8 years ago

Speaking about F# design guidelines, could you comment why List / Seq / Array 's iter function is not named iterate ?

It's an outlier. We inherited it from OCaml, but for the majority of OCaml abbreviations we took the step to unabbreviated, e.g. hd --> head, tl --> tail - good changes IMHO and these changes helped us add more functions in F# 4.0 like last, tryLast and tryHead. But somehow we just couldn't bring ourselves to do iter - ultimately we told ourselves that iter is like a verb in the language and that made us feel better. But it's definitely an outlier.

p.s. the "we" in the above are the various contributors to F# 1.0/2.0 mentioned here, though ultimately I had to make the final choices

chkn commented 8 years ago

I think @smoothdeveloper already mentioned it awhile back, but there may be a 4th option: Extend the functionality of type aliases. What I'm imagining would have 2 parts:

type Result<'T1,'T2> = Choice<'T1,'T2>
   | Ok              = Choice1Of2 
   | Error           = Choice2Of2

Or consider a more semantic tuple:

type Color = r:float * g:float * b:float

At the end of the day, the semantics of type aliases wouldn't change, so @vasily-kirichenko could use a library that exposes a Result alias with Error, but instead define his own alias with Err and the types would be compatible because the underlying type is still just Choice

Now I realize this is a language change, which is probably the most complex option, but it has much wider utility than just the Result case, and it could save us in the future from cluttering FSharp.Core with all sorts of types to accommodate everyone's pet programming paradigm.

ploeh commented 8 years ago

I support the idea of adding a type for functional error handling, but I find the proposed names less than ideal.

The name Result indicates that the value is a return value. What if someone wants to use it as a function argument?

I realise that this is still technically possible, but naming is important, and I feel that Result implies asymmetry in use, which is unfortunate.

Likewise, suggestions like Success, Failure, OK, etcetera imply that this type can only be used for error handling, which is, again, I think, unfortunate.

Why not follow established practice in other languages (as well as theory) and call it by its 'proper' name?

type Either<'a, 'b> = Left of 'a | Right of 'b

This would make it easier for Functional Programmers coming from other languages. There's also already a lot of existing litterature about the Either monad.

Like the arguments against Choice, you can argue that it isn't entirely clear that Right means success, but I think that for any programmer who have never heard about the Either monad before, a crash course is required anyway.

smoothdeveloper commented 8 years ago

The mnemotechnical retention cost for Right = correct is low for me, as a non-native english speaker and left handed person; it is far lower than Choice1Of2 = correct

eiriktsarpalis commented 8 years ago

@ploeh the fact that the Choice type is baked into the F# active pattern mechanism allows for easy creation of appropriate aliases without significant abstraction penalties. For example,

type Either<'a, 'b> = Choice<'a, 'b>

let inline Left a : Either<'a, 'b> = Choice1Of2 a
let inline Right b : Either<'a, 'b> = Choice2Of2 b
let inline (|Left|Right|) (e : Either<'a, 'b>) = e

match [Left 1 ; Right "2"] with
| Left x :: _ -> Some x
| _ -> None

Obviously this technique can be used to further constrain the 'a and 'b type parameters.

enricosada commented 8 years ago

I really like f# because it give me a pragmatic approach, when i read i Result = Ok | Error it's really easy to know what mean. Choice1Of2 it's the same as Result, Either too, different naming. But these doesnt click as fast as Result in my mind. It's sintactic sugar? sure, and that's fine for me, if works better.

I know Either exists in some language, and there is a the monad and tutorials, etc. There are tutotorial aliasing Success = Choice1Of2 and Failure = Choice2Of2 too (i do Success/Failure because it make easier to read the code), and some language use Result instead of Either.

bikallem commented 8 years ago

+1 for adding Result(OK/Error) to FSharp.Core.

vasily-kirichenko commented 8 years ago

One more argument for Ok/Err: it's what Elm language use http://package.elm-lang.org/packages/elm-lang/core/1.0.0/Result

chkn commented 8 years ago

The fact that people are still debating Error vs Err, Result vs Either, is all the more reason we should consider my proposal. Otherwise, we will simply have a similar debate all over again for the next thing. Armed with the aliases, people could use whatever names for things they prefer.

smoothdeveloper commented 8 years ago

I think in this situation, on haskell mailing list, the situation would get solved like this:

We were able to pull out a nifty F# logo this way so figuring out two alias seems like a lesser deal.

Can we attempt something similar?

dsyme commented 8 years ago

I've submitted a PR here with a resolution of most design issues in favour of a new Result type with cases Ok and Error.

The one remaining issue which concerns me is whether we should delay adding this type until support is added to allow union types to be structs. I need to think about this further: although options and choices are reference types in F#, that decision was made when struct support was not great for tailcalls and generics in .NET. Things have changed in 2016, and there are proposals to add StructOption in any case.

@chkn The naming Err/Error isn't really an open issue - as mentioned above we will choose Error since it is consistent with the F# naming guidelines. (We would only choose Err if we also amended those guidelines in such a way to make that the right choice, which isn't being proposed, and would itself lead to increased inconsistency - why not Ch1Of2 instead of Choice1Of2 for example?). I'm sympathetic to looking at naming from other languages but ultimately we need to follow the existing F# guidelines on naming.

@smoothdeveloper The process for resolving design issues is ultimately through myself amending the RFC to record a design decision. I take into account feedback in doing this.

dtchepak commented 8 years ago

Sorry if this is too off-topic, but I would like some clarification on the "functions defined in packages" approach, rather than defining them with the type itself in Core.

I can understand the burden on maintenance/support etc for certain functions and methods, but for unambiguously defined functions like ('a->'b) -> Result<'a, 'c> -> Result<'b, 'c> I don't understand the reservation. There is only one valid behaviour for this so I don't think there is a maintenance burden here.

archaeron commented 8 years ago

I think it's a mistake to not bundle at least the basic functions like

Maybe even:

otherwise all code bases that use Result will have to implement them

radekm commented 8 years ago

otherwise all code bases that use Result will have to implement them

They can use some package which will implement it for them.

I think it's a mistake to not bundle at least the basic functions like

Implementing only basic functions will in most cases lead to some external package with more functions anyway.

smoothdeveloper commented 8 years ago

If no function in FSharp.Core is going to use Result, why not move everything altogether in a FSharp.Result or FSharp.Core.Result package? no overhead on the core F# maintainers.

vasily-kirichenko commented 8 years ago

What overhead? We are talking about 4-6-10 tiny, straightforward functions. I don't understand Don's position.

ploeh commented 8 years ago

Where in this thread does it say that no functions can be supplied with this type?

dtchepak commented 8 years ago

@ploeh:

I first saw this mentioned in this PR, then looked up this thread and found a few comments explaining the rationale:

"I think it's better to have only the Result type in FSharp.Core, not a default implementation of combinators, computation exceptions, helper functions etc. ... Any external library (Ext.Core, FSharp.Core.Attempt, you create it) can add these." -- enricosada

"BTW I agree with @enricosada's arguments about "less is more" in FSharp.Core, continuing to embrace the nuget package ecosystem." -- dysme

dsyme commented 8 years ago

I'm in listening mode on this. Please supply a definitive proposal for the 4-6-10 functions as a PR to the RFC which we can discuss. And does that include a computation expression builder or not (which would already be ~8 methods)? One concern here is mission-creep/slippery-slope, that the actual number of functions will soon be 20-30.

Without a definitive stable proposal, the only stable point in the design space is zero functions.

Besides coding and testing, overheads for FSharp.Core include writing English-language documentation and code samples for MSDN, and localizing these documentation and samples into Japanese and a couple of other languages. My understanding is that the source for the MSDN docs will be open-sourced, including localization, which will give us an opportunity to address this more efficiently, and track progress openly.

Cheers Don

dtchepak commented 8 years ago

Added PR with some possible functions for discussion: https://github.com/dtchepak/FSharpLangDesign/commit/eada90e4c8c954668ab595cd8416fb75b8cd7fc3

Chicker commented 8 years ago

I strongly agree with opinion that the Result type without the implementation of basic functions for working with it is useless. Obviously there are at least two reasons:

A few examples:

Example 1

Only type definition:

let getProductById productId : Result<Product, ApiError> =
  let product = Map.tryFind id productList
  match product with
  | Some(p) -> Ok p
  | None -> Error <| ProductNotFoundError id

With base combinators

let getProductById productId : Result<Product, ApiError> =
  let product = Map.tryFind id productList

  Result.ifNone (ProductNotFoundError id) product

Example 2

Only type definition:

let getProductCost productId : Result<float, ApiError> =
  let product = getProductById id
  match product with
  | Ok p -> Ok p.Cost
  | Error e -> Error e

With base combinators

let getProductCost productId : Result<float, ApiError> =
  let product = getProductById id

  Result.map (fun p -> Ok p.Cost) product

Example 3

Only type definition:

let calcDiscountTotal1 prod1Id prod2Id discount : Result<float, ApiError> =
  let product1 = getProductById prod1Id
  let product2 = getProductById prod2Id

  match (product1, product2) with
  | (Ok p1, Ok p2) ->
      let (c1, c2) = minByCost p1 p2
      Ok <| c1 - (discount / 100.0 * c1) + c2
  | (Error e1, _) -> Error e1
  | (_, Error e2) -> Error e2

With base combinators and computation expression

let calcDiscountTotal1 prod1Id prod2Id discount : Result<float, ApiError> =
  result {
    let! product1 = getProductById prod1Id
    let! product2 = getProductById prod2Id 
    let (c1, c2) = minByCost product1 product2
    return (c1 - (discount / 100.0 * c1) + c2)
  }

Minimal implementation of Result type with some combinators and computation expression.

module Result =
  type Result<'TResult, 'TError> =
    | Ok of 'TResult
    | Error of 'TError

  let ifNone (error : 'TError) = function
    | Some (o) -> Ok o
    | None -> Error error

  let map f = function
    | Ok v -> f v
    | Error e -> Error e

  let bind m f =
    match m with
    | Ok v -> f v
    | Error e -> Error e

  let retn v = Ok v

  let retnFrom v = v

  let tryResult (body, err) =
    try
      retn <| body()
    with e -> Error err

  type ResultBuilder () =
    member __.Zero () = Error
    member __.Bind (m , f) = bind m f
    member __.Return (v) = retn v
    member __.ReturnFrom (v) = retnFrom v
    member __.Delay (f) = fun () -> f ()
    member __.Run (f) = f()
    member __.TryWith (body, handler) =
      try
        __.Run body
      with e -> handler e

    member __.TryFinally (body, handler) =
      try
        __.Run body
      finally
        handler ()

  let result = ResultBuilder()
Rickasaurus commented 8 years ago

I'm really hoping a computation expression is included with this. We're sorely missing it for option and so I end up pasting it into all of my projects.

vasily-kirichenko commented 8 years ago

@Rickasaurus I agree, but asyncMaybe will very likely to be missed, too.

dsyme commented 8 years ago

@Rickasaurus @vasily-kirichenko I'm not sure of exact timelines in Microsoft\visualfsharp, but I expect the window of opportunity for F# 4.1 is closing down rapidly. I'd expect essentially all of the existing green PRs to be incorporated, including the basic definition of the Result type - which is certainly useful for interop scenarios (we would use it in the FCS API, for example). Without a full PR and RFC for a set of functions + builder, with full tests and completed discussion, I think that's all we see in 4.1, though we can revisit the rest later.

enricosada commented 8 years ago

I think a FSharp.Results nuget packages (who depends on F# 4.1 with the new type) can be created, with the helper functions and a computation expr (i need that :heart_eyes: , i always add an attempt computation expr in every proj). It's not ootb with only FSharp.Core, but it's easier and faster to evolve. Because the type is shared, interop is easier, and can be replaced. If impl is ok, can be incorporated later in fsharp.core too.

I really like the idea of

Types (in FSharp.Core with core functions)
Functions (FSharp assemblies who add functions using shared types)
My App

A meta package FSharp.Library who reference both FSharp.Core and the additional useful packages can be created too. Or not. Nuget package are a good asset, if .net is moving to have them as first class (same level as assemblies), life is easier.

wallymathieu commented 8 years ago

In preparation for the next pull request I've created the following repository. Most of the code is loosely based on Chessie and the discussion here. In the RFC there is mention about attempt instead of tryResult for instance. Most of the builder looks trivial, but there might still be a need to review the signature for the Result module

The signatures map, mapError and bind can be found in the RFC

The full code can be found here: https://github.com/wallymathieu/FSharp.Results/

wallymathieu commented 8 years ago

Where do you find correct documentation about how to implement Builders?

vasily-kirichenko commented 8 years ago

@wallymathieu http://fsharpforfunandprofit.com/series/computation-expressions.html

wallymathieu commented 8 years ago

Thanks!

On 11 Jul 2016, at 13:44, Vasily Kirichenko notifications@github.com wrote:

@wallymathieu http://fsharpforfunandprofit.com/series/computation-expressions.html

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

wallymathieu commented 8 years ago

Has anyone thought about "attempt" mentioned in the rfc?

wallymathieu commented 8 years ago

for instance :

let result = attempt<int> { 
   return maybeThrowsAnException()
}

where the type of the result would be: Result<int, exn>

as can be seen in the test implementation Ping @enricosada @forki @haf

enricosada commented 8 years ago

https://github.com/Microsoft/visualfsharp/blob/master/tests/fsharp/PlatformHelpers.fs There is the attempt i use. Cleaned up version https://github.com/enricosada/dotnet-mergenupkg/blob/master/src/dotnet-mergenupkg/railway.fs

Example usage in fsharp suite tests, like https://github.com/Microsoft/visualfsharp/blob/master/tests/fsharp/typecheck/tests_typecheck.fs

Or files in https://github.com/enricosada/dotnet-mergenupkg/tree/master/src/dotnet-mergenupkg

enricosada commented 8 years ago

@wallymathieu why generic template? attempt { is not enough?

wallymathieu commented 8 years ago

@enricosada it was because my attempt at attempt wasn't good enough (I didn't introduce an additional type) ;)

wallymathieu commented 8 years ago

@enricosada did you have in mind something like:

    let res = Attempt.run attempt { 
        return somethingThatMaybeThrowsExceptions()
    }

?

vasily-kirichenko commented 8 years ago

@wallymathieu what that run supposed to do?..

wallymathieu commented 8 years ago

In the example code that @enricosada linked to, an attempt block returns an Attempt.

attempt : (unit -> 'a) -> Result<'a,exn> from proposal for additional functions.

In order to get the value, you need to run the Attempt (catching any exception and returning Result<'a,exn>).

haf commented 8 years ago

I would love to at least have these functions on the type; enough to work with. Also, I think that attempt should be better named tryWith or similar.

wallymathieu commented 8 years ago

I can't find for instance any "apply" in the current fsharp.core library. Perhaps there are types that should have it? For instance option? I know that it's mentioned in fsharp for fun and profit.

dsyme commented 8 years ago

@haf Looking at those I think bindError and toOption are reasonable for the Result module. ofOption doesn't make much sense because we don't know whet to fill in as the error data (and filling in unit seems unusual).

asik commented 8 years ago

@dsyme The one remaining issue which concerns me is whether we should delay adding this type until support is added to allow union types to be structs. I need to think about this further: although options and choices are reference types in F#, that decision was made when struct support was not great for tailcalls and generics in .NET. Things have changed in 2016, and there are proposals to add StructOption in any case.

I agree. There is no urgency to add a Success/Failure type; several alternatives already exist (Choice, third-party implementations, writing it by hand). It's more important to do it right for FSharp.Core, and it should definitely be a value type if possible; this is important for low-allocation scenarios (i.e. real-time systems) and performance in general.

enricosada commented 7 years ago

Pls wait and make result a struct. We should try to make it possible yo use it also where allocations count for performance. No need to rush, there is already Choice as replacement