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 `bool` #2010

Merged
merged 2 commits into from Oct 23, 2018

Conversation

@dbuenzli
Copy link
Contributor

commented Aug 30, 2018

This PR improves stdlib support for bool values. It adds a Bool module to the standard library and deprecates the bool_of_string, bool_of_string_opt and string_of_bool functions in favor of
Bool.of_string.

A few comments:

  1. The Bool.of_string function is bool_of_string_opt. No raising variants are provided. Adding a raising variant would entail adding a function that raises Failure, however no such function ever existed in the stdlib: bool_of_string raises, against the now established convention, Invalid_argument. The trend being to move away from raising on non-exceptional errors I think it's fine that way. If people really prefer that function to be called Bool.of_string_opt I can do this (I wonder though why the convention was not chosen to be Bool.opt[ion]_of_string at the time).
  2. The Bool.negate function is something that seems to be absent of most stdlib extension libraries. Had OCaml a function composition operator, this would hardly be needed but given the lack of it, it's quite useful in practice for negating predicates to be given to filter functions. Lack of such a trivial mecanism in the stdlib actually triggered me to put up this PR.
@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2018

The trend being to move away from raising on non-exceptional errors I think it's fine that way.
If people really prefer that function to be called Bool.of_string_opt I can do this

I tend to agree, but we recently added Float, which keeps the raising variant (and thus use the Float.of_string_opt name for the option returning one). This is the usual tension between uniformity and modernization, but it's a bit unfortunate that even very recent additions already clash with new ones. Probably a lack of clear guidelines (and I might be the one to blame).

What do other people think?

On a related note, the introduction of the Float module did not explicitly deprecate corresponding functions in Stdlib. Is it a good idea to produce deprecation warnings immediately after introducing the new module?

Had OCaml a function composition operator, this would hardly be needed but given the lack of it

So why not add the composition operator instead (allowing (pred %> not) instead of (Bool.negate pred))?

@alainfrisch alainfrisch added the stdlib label Aug 30, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2018

I tend to agree, but we recently added Float, which keeps the raising variant (and thus use the Float.of_string_opt name for the option returning one).

I'd just like to mention that I did not find occurences of the raising variant in ocaml's code base (well there was one but it was reimplementing string_of_bool_opt).

On a related note, the introduction of the Float module did not explicitly deprecate corresponding functions in Stdlib. Is it a good idea to produce deprecation warnings immediately after introducing the new module?

As recently discussed I think it's fine if we provide users of (reasonably) older version with a decent mecanism to get rid of the warning. In this case stdlib-shims will provide the Bool module as soon as it gets distributed by ocaml itself so I think it's fine to have the warnings.

So why not add the composition operator instead (allowing (pred %> not) instead of (Bool.negate pred))?

Much more contentious... for one thing I would let mathematical practice design the argument order of the operator; but I'll let someone else pick up that fight.

In this particular case I personally find the usage of Bool.negate more readable and obvious than the syntax you propose.

stdlib/bool.mli Outdated Show resolved Hide resolved

@dbuenzli dbuenzli force-pushed the dbuenzli:bool-support branch from 75c6680 to ee801db Aug 30, 2018

stdlib/bool.mli Outdated Show resolved Hide resolved
stdlib/bool.mli Outdated Show resolved Hide resolved
stdlib/bool.mli Show resolved Hide resolved
stdlib/bool.mli Outdated Show resolved Hide resolved

@dbuenzli dbuenzli force-pushed the dbuenzli:bool-support branch from ee801db to ce5cf51 Aug 30, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

I would like to move on this PR.

It seems no one has a strong objection about the module's functionality and existence except for the non regularity mentioned in point 1. Here are the options to consider:

  1. Keep it as it is now and accept that non regularity in the stdlib.
  2. Rename the Bool.of_string of this PR into Bool.of_string_opt (sic).
  3. Add a function Bool.of_string that raises Failure even though such a function never existed in the stdlib (as such the deprecation warning of bool_of_string would go towards Bool.of_string_opt not Bool.of_string to avoid disasters).

While I favour 1. full Stdlib regularity would entail 2+3.

In this message @gasche expressed concerns about the current rash of Pervasives deprecations. If the deprecations of this PR are blocking merge I'm willing to remove them from the PR (personally as mentioned in subsequent messages there I precisely think it's a good time to deprecate these, if I get a clear sign that this is desirable I can put up a PR with further deprecations for the values which have a mecanism that allows to silence the warning in previous version of OCaml, e.g. the float functions).

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

(To be clear, my personal opinion/worries on deprecations is not blocking anything. In general I'm trying to not get involved in stdlib PRs, and leave it to the people who volunteered to take care of them. I was just chiming in for a comment.)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

I think my preference would be to go with (1). Considering that Float is a recent addition and the community moves rather slowly anyway, what do you guys think about changing its API to match this more modern style for the next release? I'd be surprised if a lot of code on OPAM depended on Float already.

Concerning deprecation: it has never been really discussed the deprecation markers are intended as a hint for users to switch to a more modern alternative, or as a hint that the legacy feature will be removed (soon/eventually). Most features marked as deprecated in the stdlib have remained there for years or sometimes decades. The thing is that the gain of removing legacy features is rather small for maintainers and it usually does not seem worth potentially breaking existing code bases. But considering that, the best pragmatic choice for authors of libraries supposed to be compatible with multiple versions of OCaml could very well be to ignore deprecation warnings. This seems certainly easier than adapting the code + adding more dependencies (on compatibility layers).

So I feel we should discuss the actual meaning of the deprecation warnings, and whether we will actively remove deprecated features at some point. This could mean that the deprecation markers should not be introduced as soon as a modern alternative is available, but rather when we actually schedule the removal.

Perhaps my PR on generalized alerts could allow providing multiple different hints.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

Personally I think (apologies for having commented above in the "wrong" PR for what follows) that, if we are not really sure of the meaning of deprecation warning and their effect on users, maybe it is not wise to deprecate int_of_string which is used by essentially any OCaml codebase under the sun (and teaching material, etc.).

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

I think my preference would be to go with (1). Considering that Float is a recent addition and the community moves rather slowly anyway, what do you guys think about changing its API to match this more modern style for the next release? I'd be surprised if a lot of code on OPAM depended on Float already.

I have no problem with this. Besides stdlib-shims has received no release yet we can directly have the "correct" Float module there. I'm willing to do the work once my PR queue has been cleared.

Concerning deprecation: it has never been really discussed the deprecation markers are intended as a hint for users to switch to a more modern alternative, or as a hint that the legacy feature will be removed (soon/eventually).

As a basic first rule I see no point in deprecating things if you never intend to remove them, otherwise you just create a weight accruing system. Now indeed there may be things for which the gain of eventually removing them is too little in contrast to the perspective of breaking existing code bases (int_of_string is certainly a candidate); but as far as the notion of deprecation is concerned I don't think that makes a difference, if you don't want people to use that function you should deprecate it and make as if it will eventually be removed.

At a certain point I think one has to accept the idea that software rots and needs maintenance; otherwise your system just rots with your users. I prefer a system that evolves and keeps its pointedness while helping me to keep up with changes in a meaningful manner (by batching deprecations rather than introducing them one by one on each new version, by providing me sound and clear upgrade paths, etc.) than a system that rots because of its users.

@gasche Note that at the moment there's no PR that deprecates int_of_string since it's raising feature is used by the ocaml code base itself and would require a bit of careful work to deprecate on the code base (work which I would be willing to do if there's a clear sign such a PR would be merged). Regarding teaching material yes, maybe there's material that uses int_of_string but don't we all agree at that point that this function raising was a bad idea ? Don't you find it wonderful to get auto-updating teaching material that warns you about bad design and practice ;-)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

by batching deprecations rather than introducing them one by one on each new version, by providing me sound and clear upgrade paths, etc.

I agree that batching deprecation is a good idea. This would be an argument against adding deprecation attributes as soon as new features are introduced, and in favor of waiting to have a reasonable wide coverage of a "modernized" stdlib.

Considering the level of activity around the stdlib these days, I'm leaning towards not adding any deprecation (and possibly removing some which haven't been released yet) for the next version.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

Why does batching deprecations help with workload? To my mind, that just makes for more difficulty in scheduling work, as you might end up with a big chunk of work that has to be done at once as a result of a large number of deprecations.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

Why does batching deprecations help with workload?

Handling deprecations usually involves going over your whole code base, possibly making refactoring and issuing new releases as a result. That's not something I want to do every time there's a new OCaml release.

To my mind, that just makes for more difficulty in scheduling work, as you might end up with a big chunk of work that has to be done at once as a result of a large number of deprecations.

Nothing prevents your from treating the deprecations in smaller batches.

Considering the level of activity around the stdlib these days, I'm leaning towards not adding any deprecation (and possibly removing some which haven't been released yet) for the next version.

@alainfrisch Tell me when you have made you decision so that I can update my PRs to remove the deprecations. This would also mean reverting #1605 I guess. If you want to go the other direction and rather batch the latter with the deprecation of other Pervasives.* values for the next release I'm willing to help.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

The community has a tendency to stay behind and stick to rather old versions of OCaml (and then spend a lot of energy on compatibility story, omp, stdlib-shims, maintaining correct OPAM package descriptions for old version, etc). It would be globally better if people were adopting new versions quickly, but forcing users to spend time fixing their code base to take deprecation warnings into account does not help. It's fine to break compatibility once in a while, and people should be ready to adapt their code accordingly, but not systematically, for each release. Giving users incentives to disable deprecation warnings is not good either.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2018

@dbuenzli Well, if you want to have the deprecation warning-as-error enabled (for example), then you will need to fix all of the deprecations at once. If you're not going to do that, then you would seem to be willing to disable the warning in some selective manner---and you could do that just as easily in the situation where the deprecations were not batched, if you don't want to fix them with each release.

@alainfrisch Forcing users to deal with large amounts of deprecations at once doesn't seem helpful either. I don't see how that approach scales well; and I suspect it probably increases the chance of users disabling warnings (if a huge chunk of work is needed at once).

With this kind of somewhat debatable thing I tend to think that we should be adopting the approach which is the least time consuming for upstream developers, since we have rather limited time, and reduces the chance that deprecations are going to be forgotten. To my mind that is pretty clearly adding the deprecations when a replacement function is introduced.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

Side note: it might be nice to build a refactoring tool to implement some of the transformations needed when code gets deprecated. Given how good the OCaml AST libraries are in the front end, I'm somewhat surprised such tools don't already exist.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 11, 2018

(BTW, one of the things I can praise about the Go community is that they have such tools, and thus are very aggressive about improving the language ergonomics because they can automatically make correct wholesale changes to large codebases. They thus change APIs when it makes sense without much fear.)

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

With this kind of somewhat debatable thing I tend to think that we should be adopting the approach which is the least time consuming for upstream developers, since we have rather limited time, and reduces the chance that deprecations are going to be forgotten. To my mind that is pretty clearly adding the deprecations when a replacement function is introduced.

@mshinwell I can certainly agree with that and some of your points. My original poorly worded comment was rather that deprecations should be guided and principled. Deprecating things randomly here and there on every release will make end-users mad.

So my plea would rather be that when you deprecate something you should make it fit in a larger, communicable, design goal. For example this could be "in this release we are getting rid of Pervasives and cleanup what you get in your opened module". Or on a smaller scale, "in this release we are getting rid of Pervasives functions type_of_othertype, use the functionality in the corresponding Type module". Then one should make sure that the release meets the design goal.

This may entail more work for people trying to deprecate things but it also increases the chances that work is done consistently and that it is well understood and perceived by end-users.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Deprecating things randomly here and there on every release will make end-users mad. So my plea would rather be that when you deprecate something you should make it fit in a larger, communicable, design goal.

I fully agree with that. It wouldn't make sense to me, for instance, to deprecate some int-related functions but not similar float-related ones. That's why I'm suggesting that we wait for this wave of stdlib modernizations to calm down before we add deprecation warnings. Or even waiting for one more release; not pushing users to switch too quickly to the most recent additions give us a chance to apply occasionally small tweaks (such as the one discussed for Float) without risking too much breakage.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Given how good the OCaml AST libraries are in the front end, I'm somewhat surprised such tools don't already exist.

The problem is not upgrading the AST, but the source code. ocamlformat might give us a chance to do that, once it is robust enough (including for comments).

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Ok, let's not get this stuck because of deprecation attributes. I suggest to avoid adding more deprecation attributes for now, and discuss their addition independently, perhaps as part of a bigger discussion about deprecation (including actual removal of deprecated features).

stdlib/arg.ml Outdated Show resolved Hide resolved
(* *)
(* OCaml *)
(* *)
(* The OCaml programmers *)

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Sep 12, 2018

Contributor

I don't know if we care about providing real author names here. @xavierleroy @damiendoligez : do we care?

What about the copyright owner (is it ok to put INRIA even if no INRIA contributor has been involved in writing the code)?

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 12, 2018

Author Contributor

Note that I already made this attribution in the Result and Option modules. Personally I see little point in having real names on collaboratively edited files and I'm fine with git blame for my ego-trips.

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 12, 2018

Author Contributor

(is it ok to put INRIA even if no INRIA contributor has been involved in writing the code)?

This should be covered by point 2. of the individual cla that I signed.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Sep 12, 2018

Contributor

I don't care personally either, but there might be legal consequences to not mentioning original authors (IANAL). Hence my request for inputs from the higher authorities.

@dbuenzli dbuenzli force-pushed the dbuenzli:bool-support branch from ce5cf51 to d897b73 Sep 12, 2018

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

I agree that negate seems weird -- can we find another name? What about invert_sense?

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

For what it's worth, I much prefer negate over invert_sense. The name is fine, except that there is no way to guess from it that it is not the same as not, but its version lifted to predicates: you have to know the type as well to tell what it does. I think that complement would also be a reasonable choice: you can see a predicate ('a -> bool) as describing a subset of 'a, and then negate performs subset complementation.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

"Negate" doesn't make sense though. You negate integers, not booleans.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

Also, "invert sense" corresponds closely to normal usage, as far as I know. For example, you "invert the sense of a test". That's exactly what that function is doing.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

"Negate" is also widely used to talk about an operation on boolean that is generally called "boolean negation". I don't think there is any ambiguity or doubt about what it means, and the fact that it is not the same as integer negation. On the other hand, "sense" is rarely used to talk about a boolean or boolean predicates. "Invert" makes intuitive sense, so I guess invert would also be okay -- I would prefer negate personally. (And no, people wouldn't confuse invert on predicates with the 1/n operation on number fields.)

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

Just to avoid misunderstandings: my concerns was not about specific names. I believe that composition operator(s) would be a very nice addition (in line with the addition of |>), and when we have them and they become as popular as |>, composing a predicate with not will be more compact and as readable than calling Bool.negate or whatever we call it.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

invert_test might be better than invert_sense.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

@mshinwell I find your comment a bit surprising. Unless I'm completely off my mind "negating a formula" is not alien to logic terminology (a web search should be able to convince you). Invert sense/test is a bit meh to me.

@gasche

The name is fine, except that there is no way to guess from it that it is not the same as not, but its version lifted to predicates: you have to know the type as well to tell what it does.

I find that naming proximity to be a good property and don't find it problematic. The behaviour and intent of both not and negate is to negate a boolean, when I'm writing it down that's the essence I want to communicate to readers. With that perspective I think Bool.negate does a much better job at this than Bool.invert_{sense,test}.

@alainfrisch As I said I would totally be in favour and love for a composition operator to be added to the stdlib. If you can handle the bikesheding be my guest ;-) Meanwhile I still think there's no harm to have the Bool.negate proposed in this PR.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

What about not in a submodule? As soon as you provide this
function, I expect people to request other combinators in order
to be able to write things such as:

let f x = ... and g x = ... in
let h = Bool.Predicate.(f && (not g))

(Not that Predicate would be a good name for the submodule.)

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

to be able to write things such as:

let f x = ... and g x = ... in
let h = Bool.Predicate.(f && (not g))

I would rather not do that, the && operators on predicates would violate my expectations about the short-circuit evaluation of && operators.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

I find that naming proximity to be a good property and don't find it problematic. The behaviour and intent of both not and negate is to negate a boolean, when I'm writing it down that's the essence I want to communicate to readers.

The problem is that they're too similar. I look at it and I don't have any mental distinction in my head since "negate" and "not" are so close in meaning.

The question isn't whether you find them clearly distinct. The question is whether other people find them clearly distinct, and you cannot tell if other people are having a subjective experience, you simply have to take their word for it. "You don't find this confusing!" is an unreasonable statement. If someone claims they find something confusing, you have to accept that. You can argue that you don't care that they find it confusing, but not that they are not actually confused.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

BTW, I find the "invert_*" naming notion awkward, "negate_test" is a bit less awkward, both are verbose. But, if we accept that the name isn't great, we can try to find a better one. The English language has no lack of words for describing things.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

The problem is that they're too similar. I look at it and I don't have any mental distinction in my head since "negate" and "not" are so close in meaning.

If you reread what I wrote I precisely made the point that I didn't find it a problem that they were very similar...

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

I think that the discussion is getting a bit too bike-sheddy for everyone. Could we maybe step out for a moment, for example give discussing the name of negate a one-day grace period?

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

Bool.of_string has been removed.

@dbuenzli dbuenzli force-pushed the dbuenzli:bool-support branch from 4d449f3 to 9cbf07a Oct 9, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2018

This seems to be a timeout failure can someone restart that job.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Commit 9cbf07a removed a potential material objection to this PR. I'd like to keep Bool.negate (under that name) for use by people who do not like function composition operators (and especially if those end up not being in the initial environment).

If there's consensus would it be possible to proceed to a merge ?

@nojb

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Per the previous discussion, there was no strong objection to keeping negate, so I think this can be merged as soon as the conflicts are resolved and the CI passes.

@dbuenzli dbuenzli force-pushed the dbuenzli:bool-support branch from 9cbf07a to ff5a02c Oct 23, 2018

@nojb

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

To be clear, there was plenty of disagreement/suggestions about negate, but all purely subjective arguments about naming. There were at least two core developers who indicated that they were not against keeping negate. Given all that, I am willing to give @dbuenzli the prerogative on this matter.

@dbuenzli

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

There were at least two core developers who indicated that they were not against keeping negate

I think I counted three.

@nojb nojb merged commit fcb68de into ocaml:trunk Oct 23, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nojb

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

Thanks for your contribution!

@dbuenzli dbuenzli deleted the dbuenzli:bool-support branch Oct 23, 2018

@dbuenzli dbuenzli referenced this pull request Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.