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

Deprecate Pervasives #1605

Merged
merged 13 commits into from Aug 27, 2018

Conversation

Projects
None yet
@diml
Copy link
Member

commented Feb 12, 2018

Following some comments on #1010, this PR deprecates the Stdlib.Pervasives module in favour of just Stdlib.

In particular this makes the Stdlib module cleaner since we don't have the Pervasives sub-module that is immediately included.

@c-cube

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Isn't that just going to kill retrocompatibility with any program that refers to Pervasives explicitely?

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2018

It's only deprecated, not removed, so it should be fine.

@c-cube

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Forcing a deprecation warning still seems annoying to me. How about toplevel Pervasives being a module alias to Stdlib? People are not going to suddenly name their modules Pervasives, are they?

@bluddy

This comment has been minimized.

Copy link

commented Feb 12, 2018

Isn't a warning exactly what we want? Pervasives is a terrible name, and this is a chance to slowly get rid of it. A warning will make authors want to rename their module access without causing compilation errors.

@diml

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2018

How about toplevel Pervasives being a module alias to Stdlib?

This is independent from adding a warning or not. However, if Pervasives is an alias to Stdlib, then that mean that one might write Pervasives.List which might make things even more confusing.

I guess we need a compatibility package stdlib in opam so that we can use Stdlib with older compilers. I'll prepare that.

@c-cube

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

Ok, having a stdlib package gives a clear migration path and justifies the deprecation warning for me.

(it'd also be a foot in the door for detaching the stdlib from the compiler so it can evolve a bit… less… slowly, if I allow myself to dream)

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2018

@bluddy

This comment has been minimized.

Copy link

commented Feb 12, 2018

A released open-source library should never have warnings as errors IMO, since it's impossible to predict what warnings will be added to the compiler in the future, and then your package will stop working. Warnings as errors is great for a closed, internal business environment, where exhaustive error checking is the highest priority.

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

This PR is getting a bit old. Is anyone willing to review it? If not, I'll just close it.

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

Thanks. I'm going to rebase it then

@diml diml force-pushed the diml:deprecate-pervasives branch from 6a16f8c to a7c4c13 Aug 21, 2018

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

It's now rebased on trunk

@@ -403,15 +403,13 @@ stdlib_man/Pervasives.3o: $(OCAMLDOC) $(STDLIB_MLIS)
$(MAKE) unprefix_stdlib_for_ocamldoc
$(MKDIR) stdlib_man
$(OCAMLDOC_RUN) -man -d stdlib_man -nostdlib -I stdlib_non_prefixed \
-t "OCaml library" -man-mini $(STDLIB_MLIS) \
-initially-opened-module Pervasives
-t "OCaml library" -man-mini $(STDLIB_MLIS)

stdlib_html/Pervasives.html: $(OCAMLDOC) $(STDLIB_MLIS)

This comment has been minimized.

Copy link
@nojb

nojb Aug 21, 2018

Contributor

I am completely ignorant of how the "ocamldoc hack" works so can't venture a reason, but this is what the HTML doc for Pervasives looks in my machine (obtained by make stdlib_html/Pervasives.html):
screen shot 2018-08-21 at 21 22 50

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 21, 2018

Contributor

I will have a look but I believe that it is easy enough to fix this issue by adding a basic support for include module type of struct include ocamldoc end in ocamldoc .

This comment has been minimized.

Copy link
@diml

diml Aug 22, 2018

Author Member

thanks

This comment has been minimized.

Copy link
@nojb

nojb Aug 23, 2018

Contributor

Could you rebase now that #2000 has been merged?

This comment has been minimized.

Copy link
@diml

diml Aug 23, 2018

Author Member

done

(* *)
(**************************************************************************)

[@@@deprecated "Use Stdlib instead"]

This comment has been minimized.

Copy link
@nojb

nojb Aug 21, 2018

Contributor

How do I trigger this warning?

I tried referencing values from pervasives (e.g. let _ = Pervasives.exp 1.0) with -w A, but did not get any warning.

This comment has been minimized.

Copy link
@diml

diml Aug 21, 2018

Author Member

Indeed, it seems that this attribute is silently ignored by the compiler. Do you know where we are supposed to put it?

This comment has been minimized.

Copy link
@nojb

nojb Aug 21, 2018

Contributor

The following works, but requires a nested module:

module M = struct let x = 12 end [@@deprecated "foo"]

This comment has been minimized.

Copy link
@nojb

nojb Aug 22, 2018

Contributor

If one uses the long name (eg Stdlib__pervasives.exp), then the warning is correctly triggered.

This comment has been minimized.

Copy link
@diml

diml Aug 22, 2018

Author Member

I see, I'll try to put the attribute on the alias in stdlib.mli then

This comment has been minimized.

Copy link
@diml

diml Aug 22, 2018

Author Member

Adding it to the alias in stdlib.mli worked. In particular it caught all the cases in the compiler itself

@@ -1276,6 +1266,8 @@ module Obj = Obj
module Oo = Oo
module Option = Option
module Parsing = Parsing
module Pervasives = Pervasives
[@@deprecated "Use Stdlib instead"]

This comment has been minimized.

Copy link
@nojb

nojb Aug 22, 2018

Contributor

Add final dot after instead (also in Makefile.unprefix).


include Pervasives
(* Because [Pervasives] is deprecated *)
[@@@warning "-3"]

This comment has been minimized.

Copy link
@nojb

nojb Aug 22, 2018

Contributor

Is this necessary, given that the deprecated attribute is now put on the module alias in stdlib.mli ?

This comment has been minimized.

Copy link
@diml

diml Aug 22, 2018

Author Member

I believe it still is. In the lib-stdlib/pervasives_deprecated.ml test, the compiler reports a warning for module X = Pervasives

This comment has been minimized.

Copy link
@diml

diml Aug 22, 2018

Author Member

I forgot that this is a copy of the stdlib... So this is indeed useless, I removed it

@diml diml force-pushed the diml:deprecate-pervasives branch from 1ff501b to 6fe4de6 Aug 23, 2018

Pervasives.(+) 1 1;;
^^^^^^^^^^^^^^
Error (warning 3): deprecated: module Stdlib.Pervasives
Use Stdlib instead

This comment has been minimized.

Copy link
@nojb

nojb Aug 23, 2018

Contributor

The test needs to be updated due to the final dot after instead.

This comment has been minimized.

Copy link
@diml

diml Aug 23, 2018

Author Member

yep, just saw that

@nojb

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

This looks good to go once the test is updated.

@nojb

nojb approved these changes Aug 23, 2018

Copy link
Contributor

left a comment

Good to merge once CI passes. @diml Could you take care of the actual merge (I have rather spotty network access at the moment) ? Also, am not sure if there is a policy in place, but you may want to squash or tidy up the commit history before merging.

@@ -1464,15 +1464,15 @@ val getsockopt_error : file_descr -> error option
val open_connection : sockaddr -> in_channel * out_channel
(** Connect to a server at the given address.
Return a pair of buffered channels connected to the server.
Remember to call {!Pervasives.flush} on the output channel at the right

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 23, 2018

Contributor

There is still a Pervasives.flush in open_process_args_out (and on the labels side).

This comment has been minimized.

Copy link
@diml

diml Aug 27, 2018

Author Member

Fixed

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

@nojb sure. In this case I think we can squash everything.

@diml diml merged commit 9124ab8 into ocaml:trunk Aug 27, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

Is there clear migration info for users hitting a deprecation warning? A place where what to do in this case is explained clearly?

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

IIUC, the deprecation warning says "Use Stdlib instead". Do you see extra information which would be useful to provide?

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

I understand from the PR discussion that there exists a third-party stdlib package to allow people to write code which also supports older OCaml versions. If that indeed is the case, it sounds like an important thing to say. If this is not the case, I suppose that users will just silence the warning if they need to write code for distributions.

gasche added a commit that referenced this pull request Aug 27, 2018

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

This is not specific to this module though, the same case exists for any module/function that is renamed. It feels a bit heavy to mention the third party package every time.

@gasche

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

I'm not saying that the third-party package should necessarily be mentioned in the compiler deprecation message (although why not?), but it should be easy to find for users migrating to the new OCaml version. Currently googling "ocaml stdlib" does not give any relevant result that I can find. Does the third-party project have a webpage, is the URL mentioned somewhere (for example in this PR discussion?), could a pointer be included in the Changes entry?

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

There are currently two projects that provide such a feature:

stdcompat is more comprehensive, however it relies on Obj.magic. I guess we could add a pointer to these two projects in the changelog entry.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

I'm a bit dubious about the timing for this. For my old packages I'm currently generally targeting 4.03 as a minimal requirement, it will be a few years before 4.07 can be reasonably asked for which is the minimal version that provides the Stdlib module.

Do we really want to see the Pervasives warnings in post 4.07 builds for all these years ? I don't think the projects @diml mentions provide an appropriate mecanism to get rid of the warning.

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

Actually, why can't we provide a Stdlib module in stdlib-shims? It would just be:

include Pervasives
module String = String
module List = List
...
@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

@diml if you can make that to work why not. We should just make sure not to repeat the error we made with deprecating String.{lowercase,uppercase,capitalize} too early which only resulted in seeing annoying warnings in builds for quite some time.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2018

On a more general note I think that a good and user-friendly deprecation mecanism should allow both indicating the version in which an element gets deprecated and the actual minimal version a client of the library needs to support, so that no false positives are reported.

@avsm

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

I'm not saying that the third-party package should necessarily be mentioned in the compiler deprecation message (although why not?), but it should be easy to find for users migrating to the new OCaml version.

ocaml/stdlib-shims is under the ocaml/ namespace in GitHub, so it seems fine to mention this as a pointer in the deprecation warning. It is very difficult to find otherwise.

@diml

This comment has been minimized.

Copy link
Member Author

commented Aug 28, 2018

PR to add a Stdlib module to stdlib-shims: ocaml/stdlib-shims#5

On a more general note I think that a good and user-friendly deprecation mecanism should allow both indicating the version in which an element gets deprecated and the actual minimal version a client of the library needs to support, so that no false positives are reported.

Agreed

@nobrowser

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

The Library chapter of the Manual still talks about Pervasives in its opening paragraph, only to switch to Stdlib later on in the page (and mentioning that Pervasives is an alias). Quite confusing, and probably not what was intended?

@pmetzger

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

@nobrowser I suggest submitting a distinct bug report, or a pull request with a fix.

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