Include Stdlib.Result on OCaml >= 4.08.0

[aantron]

...also make sure Result.t is defined on all versions.

Fixes #8.

cc @TypesLogicsCats @hcarty @hongchangwu

[hcarty]

I won't be able to test for a while, but reads correct to me. Thank you!

[aantron]

Want to prod about this, as result is a base package in many projects' dependency graphs. Apart from the people in the issue, I've seen the problem affect several people in Reason over time.

[vog]

I just ran across the same issue, almost wanted to blame caqti until I realized that this package is the offender.

[aantron]

I'll be happy to do the release if given access to the repo.

[ghost]

Sorry for the delay, I missed the notification. The changes look good to me. @aantron I gave you commit access, feel free to merge and do a new release!

[aantron]

Great, thanks! I will do the merge and release shortly.

[olafhering]

This caused a regression in https://github.com/ocaml-ci/ocaml-version.git:

[   41s] Command [4] exited with code 2:
[   41s] $ (cd _build/default && /usr/bin/ocamlc.opt -w -40 -g -bin-annot -I .ocaml_version.objs/byte -I /usr/lib64/ocaml/result -intf-suffix .ml -no-alias-deps -o .ocaml_version.objs/byte/ocaml_version.cmo -c -impl ocaml_version.ml)
[   41s] File "ocaml_version.ml", line 62, characters 10-15:
[   41s] 62 |   compare major a.major ++ fun () ->
[   41s]                ^^^^^
[   41s] Error: This expression has type int but an expression was expected of type
[   41s]          ('a, 'b) Stdlib.result
[   41s] Command [5] exited with code 2:

[aantron]

Thanks. I was counting on the opam revdeps test to find things like this. I guess this means we can't expose t, and we probably can't include Result directly, but need to do it value-by-value. However, even that can shadow values in existing code, if it opens Result.

[olafhering]

odoc fails:

[   84s] Command [237] exited with code 2:
[   84s] $ (cd _build/default && /usr/bin/ocamlc.opt -w -40 -w -18 -g -bin-annot -I src/odoc/.odoc_odoc.objs/byte -I /usr/lib64/ocaml/astring -I /usr/lib64/ocaml/compiler-libs -I /usr/lib64/ocaml/fpath -I /usr/lib64/ocaml/re -I /usr/lib64/ocaml/result -I /usr/lib64/ocaml/seq -I /usr/lib64/ocaml/tyxml -I /usr/lib64/ocaml/tyxml/functor -I /usr/lib64/ocaml/uutf -I src/compat/.odoc_compat.objs/byte -I src/compat/.odoc_compat.objs/native -I src/html/.odoc_html.objs/byte -I src/html/.odoc_html.objs/native -I src/loader/.odoc_loader.objs/byte -I src/loader/.odoc_loader.objs/native -I src/model/.odoc_model.objs/byte -I src/model/.odoc_model.objs/native -I src/parser/.odoc_parser.objs/byte -I src/parser/.odoc_parser.objs/native -I src/xref/.odoc_xref.objs/byte -I src/xref/.odoc_xref.objs/native -no-alias-deps -open Odoc_odoc -o src/odoc/.odoc_odoc.objs/byte/odoc_odoc__Or_error.cmi -c -intf src/odoc/or_error.mli)
[   84s] File "src/odoc/or_error.mli", line 2, characters 0-76:
[   84s] 2 | type ('a, 'e) result = ('a, 'e) Result.result =
[   84s] 3 |   | Ok of 'a
[   84s] 4 |   | Error of 'e
[   84s] Error: This variant or record definition does not match that of type
[   84s]          ('a, 'e) Result.result
[   84s]        Their kinds differ.

zed fails:

[   42s] Command [48] exited with code 2:
[   42s] $ (cd _build/default && /usr/bin/ocamlopt.opt -w -40 -safe-string -g -I src/.zed.objs/byte -I src/.zed.objs/native -I /usr/lib64/ocaml/bytes -I /usr/lib64/ocaml/camomile -I /usr/lib64/ocaml/camomile/default_config -I /usr/lib64/ocaml/camomile/dyn -I /usr/lib64/ocaml/camomile/lib_default -I /usr/lib64/ocaml/camomile/library -I /usr/lib64/ocaml/charInfo_width -I /usr/lib64/ocaml/react -I /usr/lib64/ocaml/result -intf-suffix .ml -no-alias-deps -o src/.zed.objs/native/zed_string.cmx -c -impl src/zed_string.ml)
[   42s] File "src/zed_string.ml", line 1:
[   42s] Error: The implementation src/zed_string.ml
[   42s]        does not match the interface src/.zed.objs/byte/zed_string.cmi:
[   42s]        Values do not match:
[   42s]          val compare_index :
[   42s]            t ->
[   42s]            ('a, 'b) result ->
[   42s]            ('a, 'b) result ->
[   42s]            ok:('a -> 'a -> int) -> error:('b -> 'b -> int) -> int
[   42s]        is not included in
[   42s]          val compare_index : t -> index -> index -> int
[   42s]        File "src/zed_string.mli", line 184, characters 0-46:
[   42s]          Expected declaration
[   42s]        File "src/zed_string.ml", line 454, characters 6-19:
[   42s]          Actual declaration

[hcarty]

Would it be sufficient to bump the major version to 2 so that affected projects could have appropriate constraints added? Then projects which fail to compile with the 2.x release can add a constraint, like odoc and zed.

This is a tough one to address since the changes are, in many cases, going to be multiple dependency steps removed from the broken projects.

[aantron]

(Just want to note: I'm already and still thinking about it, but I don't have a good solution, especially compared to simply doing nothing and not having the 1.5 release)

[aantron]

I think we should proceed with this as is (though it may be worth retagging 1.5 to add an explicit type representation).

The argument:

• Usage of result should eventually vanish, as projects stop supporting 4.02. However, a project that does open Result and removes its dependency result will suffer the same errors that the current release of result is causing. So, by dealing with this now, we are preparing projects for the future.
• This change is not breaking from the point of view of result, even though it does break users. If it were considered breaking, any addition of any field to an OCaml module should likewise be considered breaking, because there may be code that opens the module, and the new field will shadow some identifier. That doesn't seem reasonable to me. result's Result is just especially unfortunate in this regard, because its formerly single field Result.result encouraged people to open the module.

I propose to keep the release as 1.5. I'll probably retag it to add the type representation. Then, I'll constrain all packages that are broken, and send each one a PR with a fix. For packages I can't compile or test due to obscure system dependencies or the like, I'll open an issue. This will also be a good time for each project to consider whether they should drop 4.02 support and the dependency on result. But, again, if they decide so, they will need the same patch.

(In other words, it was the addition of Stdlib.Result that "broke" them, result was just masking this).

@hcarty, @diml, @olafhering, @kit-ty-kate, what do you think?

[aantron]

It may be worth adding a nested module that projects can open, that does contain only type result, to mimic Stdlib.result, and the original behavior of this package result. However, I don't think that's necessary. We can just rename these usages to Result.t.

EDIT: on balance, I think there are many other reasons why this is a bad idea, including that it's not forward-compatible with a transition away from package result.

[aantron]

(although that nested module would make the PRs I would send much simpler :))

[olafhering]

@olafhering, what do you think?

I just reported the failure, have nothing substantial to add to the issue.

[ghost]

Usage of result should eventually vanish, as projects stop supporting 4.02.

That's the important bit. 4.02 is quite old now. When we asked around about 4.02 support for Dune, only reason developers cared about 4.02 but now bucklescript has moved to 4.06.

I think we should just encourage people to stop using the result package altogether.

I haven't looked in details at the other proposed solutions, but I feel that they'll make things more complicated, which is rarely a good thing.

However, a project that does open Result and removes its dependency result will suffer the same errors that the current release of result is causing.

That seems fine to me. Removing the open Result should be easy.

[aantron]

The BuckleScript world still needs OCaml 4.02 support, because not all projects have upgraded, and, for example, PPXs are still compiling on 4.02 to support BuckleScript 5.x. However, that should become much less of a need with OMP 1.6.0, which allows writing 4.02 ASTs no matter what OCaml version the PPX was compiled with. I'm about to explain how to use it in a Reason forum post.

[ghost]

I see. BTW, I feel like the mistake we made was to name this module Result. We should have named it Result_compat right from the start, then we wouldn't have these problems.

[avsm]

It seems totally reasonable to cause projects that need 4.02 support a little more maintenance. I've patched ocaml-version along these lines ocurrent/ocaml-version#5

[aantron]

@avsm These projects need maintenance in any case. If they choose to keep 4.02 support, and thus depend on result, they need to be patched for result 1.5 on 4.08. And if they choose to drop 4.02 support, and drop result, they still need the same patch, because that will unshadow Stdlib.Result on 4.08, introducing the same identifiers.

But I agree with the underlying sentiment :) Just wanted to clarify.

[hcarty]

I'm happy with releasing 1.5 as-is. Thank you again for your work on this @aantron

[avsm]

Understood @aantron, thanks for the clarification! I'm also happy for 1.5 to be released like this -- perhaps a short note on discuss.ocaml.org will help uncover any other 4.02.3 holdouts.