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

added result type #147

Closed
wants to merge 2 commits into from
Closed

added result type #147

wants to merge 2 commits into from

Conversation

@yminsky
Copy link

yminsky commented Mar 2, 2015

This PR adds a new result type, with the following signature:

type ('a,'b) result = Ok of 'a | Error of 'b

to Pervasives. This is very similar to a type that shows up in many
existing libraries, and should hopefully allow us to converge on one
definition that can be shared. Here are some other known use-cases in
prominent released libraries.

The first three of these use the polymorphic variant type,

[`Ok of 'a | `Error of 'b]

Batteries uses an ordinary variant with this layout:

type ('a,'b) t = Ok of 'a | Bad of 'b

Core_kernel uses an ordinary variant with this layout:

type ('a,'b) t = Ok of 'a | Error of 'b

After some discussion (notably including Daniel Bunzli), it seems like
the ordinary variant approach is better from a perspective of the less
error prone typing discipline of ordinary variants. In particular,
OCaml correctly detects that the following code is broken:

type ('a,'b) result =
| Ok of 'a
| Error of 'b

module Z : sig
  val f : ('a list,'b) result -> int
end = struct
  let f = function
    | Ok [] -> 0
    | Ok (_::_) -> 1
    | (OK | Error _) -> 2
end

But lets the same code done with polymorphic variants compile without
issues.

type ('a,'b) presult =
 [ `Ok of 'a
 | `Error of 'b ]

module Z : sig
  val f : ('a list,'b) presult -> int
end = struct
  let f = function
    | `Ok [] -> 0
    | `Ok (_::_) -> 1
    | (`OK | `Error _) -> 2
end

The hope is that by putting one of these in the stdlib, we can have
better interoperability between libraries on what seems to be a quite
basic type, without having to use the more complex and error prone
polymorphic variant solution. Bunzli among others have expressed some
concern with needing to pull in a separate package for such a basic
type, which is what motivates moving it to the stdlib.

@rgrinberg
Copy link
Member

rgrinberg commented Mar 2, 2015

As an OCaml user, I'd like to voice my support for this. This incompatibility between the standard libs is very painful and this PR would hopefully nudge all of them to be compatible with each other.

@Drup
Copy link
Contributor

Drup commented Mar 2, 2015

If we add this in pervasive, I'm pretty sure people will want a compatibility layer for old OCaml versions, so we will need an external library anyway and this will use the same opam mechanism than byte. I don't want to wait 2017 to use Result in debian.

If everyone agrees on the interface (and I'm quite convinced we can agree on something now that everyone realized the current situation is a huge PITA), I don't see why it should be in the stdlib.

And anyway, agreeing on a small external library is exactly as difficult as agreeing on something being integrated in Pervasive.

I vote for a mini package, in the repository https://github.com/ocaml and nothing in the stdlib. I have pretty much the same opinion about #80.

@avsm
Copy link
Member

avsm commented Mar 2, 2015

I think one issue with such tiny libraries is who 'owns' them. I'd be happy for small modules that are proposed for the standard library to go under the ocaml/ organisation on GitHub to make it clear that they are the result of some consensus. Same applies to Uchar in #80.

I do think it's a good idea to include them in stdlib, but a transition package is necessary for this to happen anyway for compatibility reasons.

@yminsky
Copy link
Author

yminsky commented Mar 2, 2015

In my mind, result is such a close parallel to option, that it strikes me as a truly natural fit with stdlib, and I think it makes sense there. I agree that we'll need a compatibility library for older compiler versions, but there is some unifying effect of putting things in the stdlib, and I think that would be a valuable contribution on its own.

@rgrinberg
Copy link
Member

rgrinberg commented Mar 2, 2015

I don't want to wait 2017 to use Result in debian.

But even with an external library you'd need your users to have opam installed. In which case, what is preventing from using a recent version of OCaml? The only possible scenario where having an external lib is favorable is if you want to use opam and the system compiler which seems very niche and pointless to me.

I think the real advantage of having this in the external lib is that it allows the team be a little less conservative and starting adding useful combinators.

@Drup
Copy link
Contributor

Drup commented Mar 2, 2015

It's easier to bundle a lib than a future version of Pervasive.

@c-cube
Copy link
Contributor

c-cube commented Mar 2, 2015

I kind of agree with @Drup about the need for compatibility with older versions (a type only available for >= 4.03 will not be used in practice before a very long time). However, it seems like people in the OCaml community like to disagree on interfaces, siwe of dependencies, etc. bound to libraries. I think it would be interesting to have "special" nano-libraries that only declare a type (be it Result.t, Sexp.t, UChar.t) and let regular libraries compete for providing a nice API to the type.

In particular, here, that's no use trying to impose a common Result library with combinators and so on: Core will keep using its own API, batteries its own one as well, etc. A library containing only the type is very consensual, doesn't need a version number, and could have a special prefix in opam (like type-result, type-sexp, ... similar to conf-gmp or conf-ssl).

@agarwal
Copy link
Member

agarwal commented Mar 2, 2015

This patch adds only a type, which I feel makes it appropriate for the stdlib. If you make a separate lib, presumably you will also be providing a bunch functions related to the type. At that point you again start having divergence of opinions.

he real advantage of having this in the external lib is that it allows the team be a little less conservative and starting adding useful combinators

Which is exactly the problem. We won't reach consensus on what these combinators should be.

Well, I see @c-cube is really proposing a library with only a single type definition. Seems a bit extreme.

@c-cube
Copy link
Contributor

c-cube commented Mar 2, 2015

@agarwal well, a full module in the stdlib won't just work (nor get merged anyway); a full library outside will make people disagree on who owns it and what should go inside; so a one-type module in the stdlib and a lib version outside (for older versions) would do the trick, I think.

@Drup
Copy link
Contributor

Drup commented Mar 2, 2015

Which is exactly the problem. We won't reach consensus on what these combinators should be.

We already did: everyone includes return, fail, bind, map, join, pp, equal. Just take the intersection of the various libraries, and you already have something non empty and useful enough. People are free to use the bigger libraries for fancy combinators.

@c-cube
Copy link
Contributor

c-cube commented Mar 2, 2015

Maybe in that case we could also have an Option module in the stdlib, with basically the same set of combinators. When one just needs Option.{map, flat_map}, it's annoying to have to use a dependency just for that.

@rgrinberg
Copy link
Member

rgrinberg commented Mar 2, 2015

Can't take map because Core uses by default but I agree with your argument regardless.

Also, I'm not against base-result it's just that this type is so fundamental and uncontroversial that it really belongs to pervasives. In any case, when 2017 comes I'm sure we'd all be saying "darn why isn't result in pervasives" anyway :D

@Drup
Copy link
Contributor

Drup commented Mar 2, 2015

it's just that this type is so fundamental and uncontroversial that it really belongs to pervasives.

This is also the case of the option module, and the reason it's not in the stdlib is not that no one proposed it.

@yminsky
Copy link
Author

yminsky commented Mar 2, 2015

What I like about this is that it's a small step in the direction of agreeing on the type, and avoids more controversial questions of library design. We can all design our own Monad interface and decide how we like to order our arguments, but it's nice that we all agree on the option type. Similarly, it would be great if we could all agree on the result type as well. I strongly prefer the one-line patch (well, two if you count the mli) to something more ambitious.

@Drup
Copy link
Contributor

Drup commented Mar 2, 2015

By the way, with result in pervasive, the compatibility layer is especially annoying to do.

@yminsky
Copy link
Author

yminsky commented Mar 2, 2015

It's a fair point. Indeed, I don't have a great idea about how to get both compatibility and having the type be implicitly available everywhere, in the same way option is. We can give up on that, of course, but it seems awkward to have such different handling for option and for result.

So, the options are:

  • Have a minimal Result module in the stdlib, and a compat module that provides the same. Constructors will be brought into the namespace explicitly, either by opening Result, or by opening another module like Core_kernel.Std which effectively acts as an alternate pervasives.
  • Provide result in Pervasives only. The compat module will provide the same type, but will require an explicit open to access it.
  • Provide result in Pervasives, as well as a Result module in stdlib that contains only that one type. The compatibility library would provide just the Result module. If you want to write code that works across versions, you must open Result.

I tend to think (3) is the best of these solutions. From the perspective of libraries like Core that provide their own pervasives-like module, these proposals are all equivalent. It's really about the experience of someone using OCaml without any of these extensions.

@agarwal
Copy link
Member

agarwal commented Mar 2, 2015

everyone includes return, fail, bind, map, join, pp, equal

There isn't agreement even on these. Some libraries label the arguments and some don't.

@damiendoligez
Copy link
Member

damiendoligez commented Mar 2, 2015

@yminsky Your options 2 and 3 are equivalent if OPAM provides a compatibility module: in 4.02 it would provide the result type and in 4.03 it would be empty (for option 2) or absent (for option 3).

@yminsky
Copy link
Author

yminsky commented Mar 2, 2015

Let's see if I understand what you're saying. With approach 2, a
compatibility module would be required for code that wants to be
compatible that runs either before or after the switch in OCaml. With
approach 3, you'd only need the compatibility module if you were
running before the switch in OCaml.

In either case, compatible code would have to be careful to only refer
to the compatibility module. This would be easy for frameworks like
batteries and core with their own pervasives replacement.

This makes me think that solution 2, which is what is in the patch, is
preferable, since it leaves OCaml itself in a more consistent final
state --- in particular, with result looking just like option.

@Drup
Copy link
Contributor

Drup commented Mar 3, 2015

Hum, what about that: we put the type in pervasive as @yminsky proposed and we put a small opam package with only a Result module with:

this for ≤ 4.02:

type ('a,'b) t = Ok of 'a | Error of 'b

and this for 4.03:

type ('a,'b) t = ('a,'b) result = Ok of 'a | Error of 'b

This is a sort of mix of (1) and (2). It doesn't force you to open if you prefer qualified usage to get the compatibility layer (much like the Bytes compatibility layer) and we can put a minimal amount of harmless combinators in it.

@yminsky
Copy link
Author

yminsky commented Mar 3, 2015

That's precisely what I was thinking, and I agree it's better, for the reasons you said.

I'd argue against putting any combinators in it for now. I think we should keep the patch simple, and keep the support in the stdlib parallel between option and result. I think the eventual fate of the compatibility layer is to just be deleted, so keeping other stuff in it is a mistake.

@Drup
Copy link
Contributor

Drup commented Mar 3, 2015

well, the Result module would not be in the stdlib at all, so the patch stay exactly the one you did.

@yminsky
Copy link
Author

yminsky commented Mar 3, 2015

I would argue that the compat module should be a bare minimum synchronization point. There are already several natural places for people to develop opinionated APIs to Result --- Core, Batteries, Bunzli's libraries, Containers. I don't think this compatibility shim should add a new one to the mix.

@alainfrisch
Copy link
Contributor

alainfrisch commented Mar 3, 2015

It would be interesting to quantify how many people are actually stuck with old versions of OCaml but would still insist to use latest versions of libraries such as the ones mentioned in this discussion. Does anybody have any rough idea of the kind of reasons for people to be in this situation? I can imagine very conservative projects (esp. in industrial settings, or for stable end-user applications) that don't want to upgrade OCaml too often, but are they the kind of projects jumping to adopt new libraries or versions of libraries?

If libraries put themselves the constraints of supporting older versions of OCaml, how do they do that? By maintaining a single code base (which means they can't benefit from any improvement to the language or stdlib), by using conditional compilation or maintaining several release branches (in which cases they could easily accomodate a special migration package putting the type in a specific module)?

@avsm
Copy link
Member

avsm commented Mar 3, 2015

It would be interesting to quantify how many people are actually stuck with old versions of OCaml but would still insist to use latest versions of libraries such as the ones mentioned in this discussion.

The primary reason is upstreaming binary packages into distributions. They hold back widescale adoption of a new compiler version until the slowest (CentOS or Debian) catch up with the new release. Moving those distributions to a multi-version capable model where several compilers could be simultaneously installed is one solution, and OPAM is another. Neither is as perfect as just typing in apt-get install myapp and having it work.

@alainfrisch
Copy link
Contributor

alainfrisch commented Mar 3, 2015

The primary reason is upstreaming binary packages into distributions.

I would have thought that users that rely on binary packages from their distribution would not be the same ones that need to use the most recent versions of libraries. Do you have an idea on how we could quantify the actual need for providing latest versions of libraries through "slow" binary distribution stuck with old versions of OCaml?

If the community wants to commit to long-term maintenance of old versions of OCaml, shouldn't this be done by having multiple released versions of libraries (e.g. "Core for OCaml 3.12")? Only important bug fixes or new features would need to be backported (this would also guarantee more stable APIs for people who don't need/want to benefit from bleeding edge features).

@yminsky
Copy link
Author

yminsky commented Mar 3, 2015

Here's a minimal compatibility package:

https://github.com/janestreet/result

Responding to Alain: with respect to this PR, I think the compatibility package is warranted: this is a rather fundamental type, and it's easy to see libraries that want some modicum of portability across OCaml versions using it. It's also useful for forward compatibility, i.e., Core can start using this now, and so when OCaml moves to a new version, people using Core with a newer version of OCaml can use the shared type in pervasives.

@dbuenzli
Copy link
Contributor

dbuenzli commented Mar 3, 2015

Le mardi, 3 mars 2015 à 13:30, Yaron Minsky a écrit :

Here's a minimal compatibility package:
https://github.com/janestreet/result

Would it be possible to define the type in Pervasives_result rather than Result for < 4.03 ? I think the Pervasives_result conveys better the provided compatibility and this is anyway meant to be opened.

Otherwise I can change my package name but it's always painful to rename packages once the infrastructure has been setup (I usually try not to squat too common toplevel names by adding a silly naming scheme, but somehow in that case I forgot to do it).

Best,

Daniel

@dbuenzli
Copy link
Contributor

dbuenzli commented May 15, 2015

Regarding the issue at hand. If Error is indeed too problematic we can still define the type using variants (the only drawback would be potentially harder to understand type errors for users).

@yminsky
Copy link
Author

yminsky commented May 15, 2015

dbuenzli: I don't understand what you're proposing. The type is already defined as a variant. Or do you mean polymorphic variants here?

@toots
Copy link
Contributor

toots commented May 15, 2015

Polymorphic variant would indeed be a good alternative I believe..

@dbuenzli
Copy link
Contributor

dbuenzli commented May 15, 2015

Ugh yes, sorry polymorphic variant.

@bluddy
Copy link

bluddy commented May 15, 2015

What this makes me realize is the value of haskell's Either type. Left and Right are unlikely to pop up in your own code except for a few limited domains, which makes Either a pretty good generic choice. Perhaps we want to go in a similar direction, rather than moving to polymorphic variants and losing some type safety. I mean, a big part of this is forcing programmers to generate and handle both possible results, right?

The other thing this makes me realize is that we need an easy way to hide parts of modules in ocaml.

@dbuenzli
Copy link
Contributor

dbuenzli commented May 15, 2015

Le vendredi, 15 mai 2015 à 20:29, Yotam Barnoy a écrit :

Left and Right are unlikely to pop up in your own code except for a few limited domains, which makes Either a pretty good generic choice.

That genericity leads to less readable code if your type is supposed to represent a possibly failing value computation which is what the result type is for.

rather than moving to polymorphic variants and losing some type safety
You don't loose any type safety by using polymorphic variants.

@Drup
Copy link
Contributor

Drup commented May 15, 2015

You don't loose any type safety by using polymorphic variants.

It's not strictly speaking type safety, but it catches less thing since it tends to catch them at use instead of definition. In particular, The anti-typo factor is less important.

result is a rather small type (and there is always the correct type alias), so it's less of an issue, but believe me, it's notable in tyxml ....

@let-def
Copy link
Contributor

let-def commented May 15, 2015

So you do loose, but you don't lose.

@bluddy
Copy link

bluddy commented May 15, 2015

On Fri, May 15, 2015 at 3:00 PM, Daniel Bünzli notifications@github.com
wrote:

Le vendredi, 15 mai 2015 à 20:29, Yotam Barnoy a écrit :

Left and Right are unlikely to pop up in your own code except for a few
limited domains, which makes Either a pretty good generic choice.

That genericity leads to less readable code if your type is supposed to
represent a possibly failing value computation which is what the result
type is for.

It's not less readable once the convention is absorbed. It's less explicit,
but you can say the same about Some and None being used to represent a
simple error state, though they do so just fine since the programmer is
aware of the convention.

@trefis
Copy link
Contributor

trefis commented May 15, 2015

2015-05-15 15:13 GMT-04:00 Yotam Barnoy notifications@github.com:

On Fri, May 15, 2015 at 3:00 PM, Daniel Bünzli notifications@github.com
wrote:

Le vendredi, 15 mai 2015 à 20:29, Yotam Barnoy a écrit :

Left and Right are unlikely to pop up in your own code except for a few
limited domains, which makes Either a pretty good generic choice.

That genericity leads to less readable code if your type is supposed to
represent a possibly failing value computation which is what the result
type is for.

It's not less readable once the convention is absorbed. It's less explicit,
but you can say the same about Some and None being used to represent a
simple error state, though they do so just fine since the programmer is
aware of the convention.

Oh yes, None | Some of 'a is clearly as confusing as Blue of 'b | Red of 'a.
Thank god someone finally had the guts to say it!

@dbuenzli
Copy link
Contributor

dbuenzli commented May 15, 2015

Le vendredi, 15 mai 2015 à 21:13, Yotam Barnoy a écrit :

It's not less readable once the convention is absorbed. It's less explicit,

What kind of reasoning is that. So now you need to maintain in your head a map Left -> Ok, Right -> Error. I call that both less explicit and less readable since a hintless convention has to be absorbed. And then you cannot even be sure that some other library writer will have another opinion about which way is right.

you can say the same about Some and None being used to represent a simple error state
Absolutely not, it's both explicit and readable without a mind map. Either there is Some value or there is None.

@yminsky
Copy link
Author

yminsky commented May 15, 2015

I think it's pretty clear that the type discipline around polymorphic variants is more complicated and less good at catching errors than that around ordinary variants. To me, that seems a sufficient argument for using ordinary variants for such a core concept as Ok | Error. The parallelism with option also argues for using the same mechanism in the type system.

@bluddy
Copy link

bluddy commented May 15, 2015

What kind of reasoning is that. So now you need to maintain in your head a
map Left -> Ok, Right -> Error. I call that both less explicit and less
readable since a hintless convention has to be absorbed. And then you
cannot even be sure that some other library writer will have another
opinion about which way is right.

This is why it's an agreed-upon convention. In haskell, people use Either
and there's no problem with it whatsoever. You get used to the convention
and move on. And I say that though I'm not a big fan of haskell.

you can say the same about Some and None being used to represent a
simple error state
Absolutely not, it's both explicit and readable without a mind map. Either
there is Some value or there is None.

It's obviously a much easier map. All I'm pointing out is that it's not
explicit about there being an error. We're still mapping concepts here --
it's just a more obvious mapping.

@dbuenzli
Copy link
Contributor

dbuenzli commented May 16, 2015

Le vendredi, 15 mai 2015 à 21:59, Yaron Minsky a écrit :

To me, that seems a sufficient argument for using ordinary variants for such a core concept as Ok | Error. The parallelism with option also argues for using the same mechanism in the type system.

FWIW I would also be in favour of keeping an ordinary variant for this. Other than that, this issue should be a reminder that using warnings to resolve PL design issues or disputes is a bad idea, it ends up all of us having different expectation and live with a different programming language.

D

@talex5
Copy link

talex5 commented May 16, 2015

It would be very nice if I could put true: target(4.02) in my _tags file and get the default set of warnings and pervasives for that version. Then new default warnings could be added and pervasive values added (or removed) without existing code generating warnings or errors.

@xavierleroy
Copy link
Contributor

xavierleroy commented May 16, 2015

Adding two cents to an already long discussion:

  • The warning is clearly wrong in the cases mentioned and needs to be improved.
  • As Damien mentioned, not all warnings are appropriate to all programming styles. I, for one, think that shadowing (of constructors or other definitions) is more often useful than wrong, and would never activate the warning under consideration here.
  • I also argue in favor of a regular variant datatype instead of an extensible variant here. Besides the sometimes surprising typings inferred for extensible variants, there is also an efficiency concern: extensible variants take up more memory space and take longer to pattern-match. In many uses this is negligible. However, if one programs in monadic style, values of type "error" are constructed and pattern-matched very often, so the efficiency of regular variants is to be preferred.
@gasche
Copy link
Member

gasche commented May 16, 2015

It would be very nice if I could put true: target(4.02) in my _tags file and get the default set of warnings and pervasives for that version.

Patches welcome, that should be easy to implement. My worry would be user expecting this flag to also enable/disable features of the language (some people have asked for this in the past, and I think it's unreasonable in terms of maintenance burden); maybe just a tag such as warnings_of_verson(4.02), or even just supporting warn(4.02) would be better.

@gasche
Copy link
Member

gasche commented May 16, 2015

I created a PR to keep track of this feature request: PR#6873.

@gasche
Copy link
Member

gasche commented May 17, 2015

The disambiguation bug was just fixed by Jacques Garrigue. @toots , could you recompile and list the remaining Warnings 41?

@toots
Copy link
Contributor

toots commented May 18, 2015

Thanks for the fix @gasche! I'm only seeing the warning in a couple of cases now. Here's one:

OCAMLOPT -c tools/http.ml
File "tools/http.ml", line 17, characters 7-12:
Warning 41: Error belongs to several types: exn result
The first one was selected. Please disambiguate if this is wrong.

Code:

type error = Socket | Response | UrlDecoding
exception Error of error

let string_of_error e =
  match e with
    | Socket -> "Http: error while communicating to socket"
    | Response -> "Http: invalid answer to request"
    | UrlDecoding -> "Http: URL decoding failed"

(** Error translator *)
let error_translator e =
   match e with
     | Error e -> Some (string_of_error e)
     | _ -> None

Warning occurs in error_translator

@gasche
Copy link
Member

gasche commented May 18, 2015

Would writing let error_translator (e : exn) = or match (e:exn) with be an acceptable workaround for you?

(If that's not too much work, listing the location of the other cases may help in documenting the kind of code patterns that users would have to change, under the assumption that the type-declaration remains as is for the 4.03 release.)

@toots
Copy link
Contributor

toots commented May 18, 2015

Yeah, I can do that. The other case is the following:

exception Error of (term*string)

let invalid t =
  match t.term with
    | Int _ | Bool _ | Float _ | String _ -> false
    | _ -> true

let generic_error t : exn =
  if invalid t then
    match t.term with
      | Var _ -> Error (t,"variables are forbidden in encoding formats")
      | _ -> Error (t,"complex expressions are forbidden in encoding formats")
  else
    Error (t,"unknown parameter name or invalid parameter value")
@damiendoligez
Copy link
Member

damiendoligez commented May 18, 2015

let generic_error t : exn =

To be clear, you only get the warning if the : exn is not there, right?

@toots
Copy link
Contributor

toots commented May 18, 2015

Yup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.