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

Improve stdlib support for result values #1956

Merged
merged 2 commits into from Aug 8, 2018

Conversation

@dbuenzli
Copy link
Contributor

commented Aug 1, 2018

This PR improves support stdlib support for result values. It adds a Result module to the standard library and the Format.pp_print_result function.

It is the counter part, for the result type, to the stdlib improvements for options. For those who agree to the latter, this one should be rather uncontroversial, if not boring, as it is fully modelled after it.

Note that:

  1. The PR does not and will not attribute meaning to the error case type. I'd be glad if this can be kept out of the discussion.
  2. The PR does not and will not provide any infix operator. I'd be glad if this can be kept out of the discussion.
val compare :
ok:('a -> 'a -> int) -> error:('e -> 'e -> int) -> ('a, 'e) result ->
('a, 'e) result -> int
(** [compare ~ok ~error r0 r1] totally orders [r0] and [r1 ] using [ok] and

This comment has been minimized.

Copy link
@hcarty

hcarty Aug 1, 2018

Contributor

Small typo - [r1 ] rather than [r1].

let value r ~default = match r with Ok v -> v | Error _ -> default
let get_ok = function Ok v -> v | Error _ -> invalid_arg "result is Error _"
let get_error = function Error e -> e | Ok _ -> invalid_arg "result is Ok _"
let bind r f = match r with Ok v -> f v | Error _ as e -> e

This comment has been minimized.

Copy link
@pqwy

pqwy Aug 1, 2018

If this was specifically excluded from Option, it should probably not be here either.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 1, 2018

Author Contributor

Let me rephrase that for you. If this is specified here it should probably be specified in Option aswell. Why not ? If people agree I'll add it there -- on a personal note I'm not making much use of of option as a monad, in a lot of the code I write I'm rather doing stuff until I get Some _, but if we strive for consistency you are certainly right.

let get_ok = function Ok v -> v | Error _ -> invalid_arg "result is Error _"
let get_error = function Error e -> e | Ok _ -> invalid_arg "result is Ok _"
let bind r f = match r with Ok v -> f v | Error _ as e -> e
let join = function Ok r -> r | Error _ as e -> e

This comment has been minimized.

Copy link
@pqwy

pqwy Aug 1, 2018

This is called flatten elsewhere, and can be described as such here too.

On the other hand, +1 for pervasive join.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 1, 2018

Author Contributor

If people are ok with pervasives monad terminology I'd rather switch from flatten to join there.


(** Result values.
@since 4.08 *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

This could probably be a bit more explanatory. How about

Result values.
The Result type is much like the Option type, but it allows an explicit error value to be carried instead of None.

This comment has been minimized.

Copy link
@pqwy

pqwy Aug 1, 2018

Result values.

Result is the canonical coproduct.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

As I've said elsewhere, the purpose of documentation is often to guide beginners through understanding a facility. No beginner will know what the "canonical coproduct" means.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

I think the new text is better, but perhaps a slight tweak to "Result values are used to return computation results in an explicit and declarative manner..." would be better. Then it is obvious to a beginner that this is used as a return type to wrap results.

(** The type for result values. Either a value [Ok v] or an error [Error e]. *)

val ok : 'a -> ('a, 'e) result
(** [ok v] is [Ok v]. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

I personally find this documentation string confusing.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

How about [ok v] returns the value [Ok v] for any v.

This comment has been minimized.

Copy link
@pqwy

pqwy Aug 1, 2018

It's about emphasizing equalities, and not computational behaviour, of pure functions.

Think of that "is" as the function (=).

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 1, 2018

Author Contributor

See below. (somehow repeating what @pqwy said, missed his comment)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

I understand, but documentation is meant to be explanatory, not minimal. The explanation is correct, but you want to help beginners, not just experts.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 1, 2018

Author Contributor

Sorry I'm not too obsessed with beginners, there are introductory texts and tutorials for that. I'm obsessed with the working OCaml programmer.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 2, 2018

Member

Beginners will never become working OCaml programmers if they hit too many obstacles. A word or two here and there hardly slows down the experienced programmer, but will make it easier for people who are new. Note that I've been using the language for half a year and this documentation string stopped me cold for a minute until I read the code itself. That might not be what you want, correct?

(** [ok v] is [Ok v]. *)

val error : 'e -> ('a, 'e) result
(** [error e] is [Error e]. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

I also find this documentation string confusing. Similarly, maybe:

[error v] returns the value [Error v] for any v.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

(BTW, I recognize "is" is shorter, but "returns" feels more idiomatic to me in documentation.)

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 1, 2018

Author Contributor

The fact that I'm calling a function here is pointless hence no "return" needed.

The doc string describes the resulting value and you can substitute the expression on the left of the is by what is mentioned on the right (and vice-versa) and your program will behave the same.

That's the power of functional programming (something that somehow seems to have been lost in the FP hype).

In other words I'm describing an equality here and is is the right word for this.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 2, 2018

Member

You don't need documentation, you wrote the code. However, I found this "equality" opaque when I started reading the .mli file. The code is much more comprehensible than the docstring, and that's bad.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

You don't need documentation, you wrote the code.

Don't know where you got this from I constantly need documentation for all the code I write.

This is not related to me writing the code or not, it's about distilling a mindset on how to think about your programs: in terms of values, their denotation and their equalities rather than the mechanics of their evaluation.

The doc string you propose is cumbersome and pure noise to me, it doesn't give me the essence of what the application stands for in code. With a little bit of practice I'm sure you'll eventually understand, get used to it and even grow fond of it.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 2, 2018

Member

The doc string you propose is cumbersome and pure noise to me

Replacing "is" with "returns" (which is far more idiomatic to most computer scientists I know) is cumbersome and pure noise? I can see you arguing that it is unnecessary, but cumbersome? noisy? That's a bit of a stretch, no?

The test here isn't whether you think the docs are okay. It's whether others find them confusing or not. You can't tell me "you didn't find it confusing", I did. You can argue that I'm an imbecile, and perhaps you would be true, but that's the point, not everyone using the documentation will be as smart as you, or will start with your "distilled mindset on how to think about programs". Some people will be like me, will look at it, and say "that's tautological and doesn't really explain what's going on to me". After a few moments they may get it, but that's unnecessary friction.

Unless your documentation is purely for yourself, the mere presence of third parties that don't understand it means it is a failure. And again, you can argue until you're blue in the face that you prefer the documentation one way or another, but the fact that someone else didn't find it clear is not something you can validly dismiss. Well, I suppose you could if you want to claim that the documentation is only intended for smart people and that no one else should use OCaml. If that's not your claim, you have to write documentation that other people understand, and merely because you find phrasing that they understand unpleasantly verbose because it uses two or three extra words makes no difference if they don't understand what you wrote.

This comment has been minimized.

Copy link
@yallop

yallop Aug 2, 2018

Member

I like the "f x is G x" style for pure code; it encourages a functional mindset. It's used quite a bit in the standard library already:

$ grep '(\*\*.*\] is \[' stdlib/*.mli
stdlib/bigarray.mli:  (** [size_in_bytes a] is [a]'s {!kind_size_in_bytes}. *)
…
stdlib/int32.mli:(** Successor.  [Int32.succ x] is [Int32.add x Int32.one]. *)
…
stdlib/listLabels.mli:(** [cons x xs] is [x :: xs]
…
stdlib/stdlib.mli:(** [succ x] is [x + 1]. *)
…

In these comments is tells you more than returns would; it says that the result of the function is all that there is to know about it.

However, the passive style is less helpful for imperative functions that have both a result and an effect, so I'd prefer not to see it here:

val iter : ('a -> unit) -> ('a, 'e) result -> unit
(** [iter f r] is [f v] if [r] is [Ok v] and [()] otherwise. *)

Perhaps is here could be changed to calls (e.g. [iter f r] calls [f v] iff [r] is [Ok v]).


This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

The test here isn't whether you think the docs are okay. It's whether others find them confusing or not.

Neither is whether you think they are not; note that for now you seem to be the only one complaining. Over the years I have published almost 40'000 loc of commented .mli file using exactly this style and while I do get the occasional complaint about a confusing or unclear doc strings noone ever complained about this documentation style which is fairly natural to functional programmers.

Let me be clear now: I won't make any change to these doc strings so there's no need to continue this discussion especially since I don't think it helps up moving this PR forward.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 2, 2018

Member

Neither is whether you think they are not; note that for now you seem to be the only one complaining

I give up.


val get_error : ('a, 'e) result -> 'e
(** [get_error r] is [v] if [r] is [Ok v] and @raise Invalid_argument
otherwise. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

This seems to be a typo. Shouldn't it be if [r] is [Error v]?

(** [bind r f] is [Ok (f v)] if [r] is [Ok v] and [r] if [r] is [Error _]. *)

val join : (('a, 'e) result, 'e) result -> ('a, 'e) result
(** [join rr] is [r] if [rr] is [Ok r] and [rr] if [rr] is [Error _]. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

This explanation is too confusing. From what I can tell, this is extracting a nested Ok. That should be part of the explanation. Also, rather than r and rr use some better names like inner and outer or what have you?


val fold : ('a -> 'c) -> ('e -> 'c) -> ('a, 'e) result -> 'c
(** [fold ok error r] is [ok v] if [r] is [Ok v] and [error e] if [r]
is [Error e]. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

I found the chosen function names momentarily confusing given that there's an ok and an error function defined in the module. Perhaps the docs could say ok_f and error_f or some such.

('a, 'e) result -> bool
(** [equal ~ok ~error r0 r1] tests equality of [r0] and [r1] using [ok]
and [error] to respectively compare values wrapped by [Ok _] and
[Error _]. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

I've noted earlier that ok and error are slightly cognitively disturbing choices since there are already functions in the module with those names. Maybe ok_f and error_f or ok_eq and error_eq or what have you. BTW, that goes similarly for the code itself in the .ml file.

This comment has been minimized.

Copy link
@hcarty

hcarty Aug 1, 2018

Contributor

I strongly disagree - I think ok and error are reasonable and expected names in this context.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

Not to a beginner. I'm looking at this with mind of a person who has no expectations, doesn't really know OCaml, is reading their first program, and has consulted the documentation to figure out what something does. The purpose of documentation isn't to be minimal, but rather to be as helpful as possible without being patronizing.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

(Phrased another way: anyone who already knows what it does by looking at the type signature doesn't need the documentation. The documentation is for other people who are having more difficulty. Also, even very small tweaks to documentation can often make significant differences in readability.)

('a, 'e) result -> int
(** [compare ~ok ~error r0 r1] totally orders [r0] and [r1 ] using [ok] and
[error] to respectively compare values wrapped by [Ok _ ] and [Error _].
[Ok _] values are smaller than [Error _] values. *)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

Similarly here on ok and error as names.

@pqwy

This comment has been minimized.

Copy link

commented Aug 1, 2018

Here's a few more:

recover: ('e -> 'a) -> ('a, 'e) result -> ('a, _) result
bimap: ('a -> 'b) -> ('e -> 'f) -> ('a, 'e) result -> ('b, 'f) result
@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2018

@pqwy I added recover. Regarding bimap I understand your obsession with structures but how often did you really find yourself missing that function ? I left it out for now. I'll add it if there's strong desire for it.

let map f = function Ok v -> Ok (f v) | Error _ as e -> e
let fold ok error = function Ok v -> ok v | Error e -> error e
let iter f = function Ok v -> f v | Error _ -> ()
let recover f r = function Ok _ as v -> v | Error e -> Ok (f v)

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

Shouldn't that read | Error e -> Ok (f e) ??

This comment has been minimized.

Copy link
@pmetzger

pmetzger Aug 1, 2018

Member

Also, r isn't being used correctly I think? Either you match on r or leave it off, yes? Or am I crazy?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 1, 2018

Author Contributor

Not crazy. Forgot to compile before pushing... Thanks.

@dbuenzli dbuenzli force-pushed the dbuenzli:result-support branch from c956e7c to 3aaa32e Aug 1, 2018

val iter : ('a -> unit) -> ('a, 'e) result -> unit
(** [iter f r] is [f v] if [r] is [Ok v] and [()] otherwise. *)

val recover : ('e -> 'a) -> ('a, 'e) result -> ('a, 'f) result

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Aug 2, 2018

Contributor

What's the intended use for recover? Why not return the 'a and let the caller compose with ok if needed? Or completely merge with value:

val {value, recover, ???}: error:('e -> 'a) -> ('a,'e) result -> 'a

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

What's the intended use for recover?

Getting rid of errors when you are in a sequence of binds. We could indeed return a naked 'a though which would give us the "lazy" version of Result.value. @pqwy you proposed recover what do you think ?

Regarding completely merging Result.value, I'd say Result.value is the counter part of Option.value and I think it's good if we keep it that way so that they have the same interface.

This comment has been minimized.

Copy link
@pqwy

pqwy Aug 6, 2018

I won't insist on recover; it's always easy to fashion additional combinators.

Having said that, the function ('e -> 'a) -> ('a, 'e) result -> 'a is fold id, so it doesn't bring too much to the table.

Feel free to remove it if there is no consensus.

('a, 'e) result -> int
(** [compare ~ok ~error r0 r1] totally orders [r0] and [r1] using [ok] and
[error] to respectively compare values wrapped by [Ok _ ] and [Error _].
[Ok _] values are smaller than [Error _] values. *)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Aug 2, 2018

Contributor

Let's be coherent with Option, and put Error before Ok (or change the PR about Option).

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

I prefer to follow the order of the cases in the type definitions.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

Ah sorry I misunderstood that one. I thought you were talking about argument order of the signature. My comment might still apply even though I'm less assertive about it.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

On the plus side of the current def. is that assuming [ok] and [error] are compatible with polymorphic compare, the resulting comparison will be. This might be "useful" if you mistakingly use polymorphic comparison instead of the specialized comparison you should have used.


(** {1:convert Converting} *)

val to_option : ('a, 'e) result -> 'a option

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Aug 2, 2018

Contributor

This could also be called get_ok_opt to be in line with other Stdlib functions that come with both a raising and an option-returning variant. But I'm not against to_option. In that case, what about of_option (I know there is also Option.to_result, but one might expect to have both to_option/of_option at the same place)?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 2, 2018

Author Contributor

I'm fine with people using Option.to_result for that. That way we just have to_ functions in both Option and Result. If there is strong desire I can add both Option.of_result and Result.of_option.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

In addition to or instead of bimap, what about map_error: ('a, 'e) result -> ('e -> 'f) -> ('a, 'f) result? Could be useful to do e.g. map_error Printexc.to_string.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@alainfrisch added Result.{iter,map}_error.

@alainfrisch alainfrisch added the stdlib label Aug 2, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

Are the CI errors related to this PR ?

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

This I would also like to know as I'll soon be making the final touches, so that this can be hopefully merged.

It seems this and this test are consistently failing test but I'm not sure why this PR would affect them.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

Maybe related to #1892 ? cc @trefis

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

Those tests are printing some ident stamps, which tends to be quite brittle. I am going to open a PR on this issue soon.

(** [map_error f r] is [Error (f e)] if [r] is [Error e] and [r] if
[r] is [Ok _]. *)

val fold : ('a -> 'c) -> ('e -> 'c) -> ('a, 'e) result -> 'c

This comment has been minimized.

Copy link
@keleshev

keleshev Aug 6, 2018

Contributor

If equal and compare take labelled ~ok and ~error parameters, why not fold?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 6, 2018

Author Contributor

fold is likely to be partially applied and when you partially apply the labels leak in the result which is often annoying. For compare and equal it is much less likely that you'll partially apply them.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 6, 2018

Author Contributor

(For an example of this see @pqwy's examples for fold (bacon sic) on the Option module, had the argument been labelled those couldn't be written without eta expanding).

This comment has been minimized.

Copy link
@mshinwell

mshinwell Aug 6, 2018

Contributor

Eta expanding is often a good thing though, in my experience. I've found it means less work when refactoring. Clever partial application tricks should be avoided as much as possible, IMO.

That said, if you're going to partially apply only one of [ok] or [error], isn't it going to be clearer at the place where the other of those is finally applied to have a label? I think it probably is, if that place is some distance away.

Labelling them also means that the argument of type "result" can be written first. This keeps the thing being folded over close to the word "fold" in the code, rather than potentially many lines lower, resulting in easier-to-read code.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Aug 6, 2018

Author Contributor

Ok I'd like to bring all this to an end so I labelled both {Result,Option}.fold

@dbuenzli dbuenzli force-pushed the dbuenzli:result-support branch from 4bd66bf to 2a51719 Aug 6, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

So I rebased the PR and:

  1. Failing to find a good unifying name for Option and given @pqwy's comment dropped Result.recover.
  2. Kept the doc string of Result.iter despites @yallop's comment since the function itself is pure and the doc string an accurate description of the equality.
  3. Kept the order in compare despite @alainfrisch's comment to remain compatible with polymorphic comparison.
  4. Added a test suite.
@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

I also respect @dbuenzli's time and effort, and can't expect for him to adopt this mindset. So I suggest that once this PR is finalized, I will submit a separate PR to make the docs of Result and Option more outsider-friendly.

Look it's not as if I didn't take time to write these things. Besides your comment about my ability to be able to adopt mindsets is borderline offending.

I have nothing against you adding more paragraphs to explain what options and results are, and/or make an introduction on why the doc strings are written that way so that newcomers understand better the functional mindset, but I'd be glad if you could avoid changing the style of the doc string as it exists now.

Vocabulary shapes the way you think about things and the way they are written now is to encourage readers, especially newcomers, to have a functional programming mindset.

@keleshev

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@dbuenzli I am sorry for making it sound like that. To clarify, I did not expect you to adopt a mindset, but I do not challenge, in any way, your ability to adopt a mindset. Would that be fair?

I will be very happy to have you as the reviewer for future documentation PRs in this area.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2018

@nojb, @shindere it seems there's still something glitchy about the test suite. The errors here which I can reproduce locally after a git clean -fdx are:

File "test.ml", line 10, characters 10-19:
Error: Unbound module Result

But as far as I can tell there's nothing I'm doing differenly here from the Option PR, in which there is no such problem.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2018

@dbuenzli You are missing module Result = Result in stdlib.mli.

@dbuenzli dbuenzli force-pushed the dbuenzli:result-support branch from e757037 to 490bbed Aug 6, 2018

@nojb nojb closed this Aug 7, 2018

@nojb nojb reopened this Aug 7, 2018

@dbuenzli dbuenzli force-pushed the dbuenzli:result-support branch 2 times, most recently from 957f50c to 1e78ec5 Aug 7, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

CI green! The PR seems to have reached a stable point, so I am planning to merge this tomorrow unless someone speaks up, or beats me to it.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@dbuenzli Could you update Changes ? (note there is a missing : after the GPR number)

@dbuenzli dbuenzli force-pushed the dbuenzli:result-support branch from 1e78ec5 to 51802ed Aug 8, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2018

Added the :. Since many people contributed to the discussion and design I choose to keep the credits part to what it is now.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

I am @gasche and I approve this credit message.

(reference)

@nojb nojb merged commit 86ea7ba into ocaml:trunk Aug 8, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

Thanks all for the nice PR!

@dra27

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

This GPR creates a small issue which we've found, amusingly, building topkg! This new Result module shadows the compatibility layer defined in https://github.com/janestreet/result which unfortunately defines type result, not type t.

At the moment we've hacked it by adding type result to this module, but I'm not sure what the long term fix should be. The other option is to add type t to the compatibility layer and constrain all existing packages.

It looks like the choice is between inconveniencing a lot of package authors with the release of 4.08 or having both Result.t and Result.result in the standard library - is there another option?

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

This new Result module shadows the compatibility layer defined in https://github.com/janestreet/result which unfortunately defines type result, not type t.

Why is that the case ? Aren't libraries supposed to shadow the stdlib if they define a module of the same name ?

(I thought that was the point of using the terrible namespacing through module aliases hack: adding new modules to stdlib should not disturb client code)

@dra27

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

I believe that's correct, yes, but it's not working:

        OCaml version 4.08.0+dev0-2018-04-09

Findlib has been successfully loaded. Additional directives:
...
# #require "result";;
/home/dra/.opam/ocaml-trunk/lib/result: added to search path
/home/dra/.opam/ocaml-trunk/lib/result/result.cma: loaded
# #show_module Result;;
module Result = Result
module Result :
  sig
    type ('a, 'e) t = ('a, 'e) result = Ok of 'a | Error of 'e
    val ok : 'a -> ('a, 'e) result
    val error : 'e -> ('a, 'e) result
...
  end
# #quit;;
[dra@ocamldefang t]$ touch result.cmi
[dra@ocamldefang t]$ ocaml
        OCaml version 4.08.0+dev0-2018-04-09

Findlib has been successfully loaded. Additional directives:
...

# #show_module Result;;
Error: Corrupted compiled interface result.cmi
# #require "result";;
/home/dra/.opam/ocaml-trunk/lib/result: added to search path
/home/dra/.opam/ocaml-trunk/lib/result/result.cma: loaded
# #show_module Result;;
module Result :
  sig type ('a, 'b) result = ('a, 'b) result = Ok of 'a | Error of 'b end

There seems to be something funny going on with .cmi searching and the implicit open Stdlib.

@diml

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

I see. That's basically an issue with the toplevel but that shouldn't happen during normal compilation. What happens is that the removal of stdlib modules from the environment is only done during the computation of the initial environment, but not when incrementally augmenting the search path.

FTR, my plan was to rewrite the implementation of include paths so that they simply populate the environment with pointers. This is a much more straightforward way of representing the environment and would naturally avoid these problems.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

@diml - I had thought it was only the toplevel which was affected, but in a test I ran it seemed that ocamlc -c was suffering too. Except I now find my test was faulty!

topkg is a moderately special case, but as a dependency it affects a lot of packages. Are you likely to be able to work on this before the 4.08 freeze?

@diml

This comment has been minimized.

Copy link
Member

commented Aug 22, 2018

Yes, that's fine

@dra27

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

I've opened MPR#7841 to keep track of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.