stdlib: add an experimental alert to the Effect and Domain modules #11526

Octachron · 2022-08-31T13:33:57Z

As discussed in #11074, this PR proposes to add an experimental alert to the Effect and Domain module to make it extra-explicit that the interface of those modules might change in later versions.

The alert is disabled by default.

For now, I am using ocaml5_experimental to emphasize the point that those modules are experimental feature in OCaml 5 and make it easier to selectively enable (or disable) this alert.

favonia · 2022-09-10T12:46:02Z

Thanks! I hope that either the alerts can happen by default (due to end users using Effect or Domain, not other internal uses of them), or the Changes, the manual, or the documentation of the standard library can mention this so that users like me know there's an alert to enable.

dbuenzli · 2022-09-12T12:35:43Z

I'm not sure I understand the rationale behind having something special for OCaml 5.0.

Why not have a general @unstable alert/warning enabled by default that we can also reuse in our libraries ?

Octachron · 2022-09-13T10:53:18Z

Alerts are now part of the documentation generated by both ocamldoc and odoc.

Concerning the name, I hesitated with having a more generic experimental or unstable alert. However, if the alert is adopted in various libraries, it would be harder to selectively detect use of the Effect or Domain module. We could have both the specific and generic alerts, but that seems over-complicated. Maybe the generic name is the simpler choice.

Similarly, I am not sure if we want the alert to be enabled by default.
Do we want to impose the ceremony of writing an [@@@alert "-unstable"] attribute before starting to use the Effect and Domain modules?

nojb · 2022-09-13T12:19:20Z

Concerning the name, I hesitated with having a more generic experimental or unstable alert. However, if the alert is adopted in various libraries, it would be harder to selectively detect use of the Effect or Domain module. We could have both the specific and generic alerts, but that seems over-complicated. Maybe the generic name is the simpler choice.

In my experience this is a real issue: even with the deprecated alert it may be that you want to toggle it for some libraries and not others, which, today, is impossible. Given this, I think it is better to stay with the more specific alert instead of a generic one.

One could think of improvements to the alert UI (eg hierarchical alert names unstable.xxx or attaching alerts to libraries or ...) that could be made to improve this situation, but that's for later.

Similarly, I am not sure if we want the alert to be enabled by default.

Disabled by default seems OK. We want people to use the new modules, but we provide a way to enforce not using them if the user wants to.

dbuenzli · 2022-09-13T12:24:07Z

However, if the alert is adopted in various libraries, it would be harder to selectively detect use of the Effect or Domain module.

Is that a goal ? Personally I'm fine with devising a more general mechanism that allows the eco-system to release "design probes" and let user know that they are. I don't feel Effect or Domain need to be targeted specially.

Similarly, I am not sure if we want the alert to be enabled by default.

I think you are right. It boils down to this (if I understand correctly the overriding behaviour between file annotations and cli arguments¹) :

If it's enabled by default, we immediately warn users about use of unstable features.
If it's not enabled by default, we allow users to detect their use of unstable features (provided there is no file level overriding)

Option 2. seems a better strategy since people will react to 1. by using file level overrides and then definitively prevent detection (unless there was some kind of super-overriding command line flag which is something that should maybe be considered).

Which is slightly odd if you consider what is usually done in cli interface where configuration files are overriden by cli arguments. ↩

dbuenzli · 2022-09-13T12:43:59Z

One could think of improvements to the alert UI (eg hierarchical alert names unstable.xxx or attaching alerts to libraries or ...) that could be made to improve this situation, but that's for later.

Oh yes ! If only we had a notion of library in the compiler :-)

If you are going to introduce such a convention later maybe it would be a good idea to introduce use it now as far as this particular alert is concerned. I.e. call it @unstable.ocaml rather than @ocaml5_experimental.

gasche · 2022-09-13T12:56:51Z

I like the idea of an API-generic unstable or experimental alert. Can we take a bit of time to discuss whether that could work? @nojb points out that it would be harder to selectively look at just "alerts related to non-stable OCaml5 stdlib APIs"; ok, but is this really an issuee? (What are the user workflows we have in mind?)

nojb · 2022-09-13T13:22:35Z

I like the idea of an API-generic unstable or experimental alert. Can we take a bit of time to discuss whether that could work? @nojb points out that it would be harder to selectively look at just "alerts related to non-stable OCaml5 stdlib APIs"; ok, but is this really an issuee? (What are the user workflows we have in mind?)

Suppose you introduce this generic alert and library developers start using it. Now suppose that you decide you want to use one specific library (marked with the experimental alert). That means you need to disable the alert wholesale, which is not great.

The same happens with the "deprecated" alert; for example in #10737 we went with an ad-hoc alert instead of reusing the deprecated alert, partly because of this.

dbuenzli · 2022-09-13T13:37:22Z

Suppose you introduce this generic alert and library developers start using it. Now suppose that you decide you want to use one specific library (marked with the experimental alert). That means you need to disable the alert wholesale, which is not great.

That's not entirely true, you can always selectively disable the warning at the file level on a section of code using that library. Of course it's rather inconvenient it the library usage is not localized – but then you can segregate its use via a module.

If there's really a plan to introduce a more general scheme, I'm fine with keeping the alert of this PR for the "ocaml library" only. But I also think it's better to think about making it compatible with what could come in the future rather than this ad-hoc alert for ocaml 5 features.

nojb · 2022-09-13T13:43:32Z

That's not entirely true, you can always selectively disable the warning at the file level on a section of code using that library. Of course it's rather inconvenient it the library usage is not localized – but then you can segregate its use via a module.

True.

If there's really a plan to introduce a more general scheme, I'm fine with keeping the alert of this PR for the "ocaml library" only. But I also think it's better to think about making it compatible with what could come in the future rather than this ad-hoc alert for ocaml 5 features.

I don't have any concrete suggestion for improvement at the moment, so am OK either way; I just wanted to point out that using generic alerts have a bit of a downside today, but as you point out there are also workarounds as well...

dbuenzli · 2022-09-13T14:13:10Z

I just wanted to point out that using generic alerts have a bit of a downside today,

Note that if the route "of attaching alerts to libraries" is followed in the future then introducing the generic alert seems the best course of action to me.

The current inconveniences can be (admittedly a bit painfuly) worked around and if in the future alerts are implicitely attached to a notion of source (e.g. a library name or a compilation unit). Then it will just be a matter of providing cli and attribute syntax upstream to selectively enable or disable alerts from specific source. The eco-system will need no change in its use of the alert.

Octachron · 2022-09-26T13:24:12Z

I have the impression that the summary of the discussion is that we should go go for a generic unstable alert as first step, and then spend some time later devising a system for enabling or disabling alerts module-by-module or library-by-library.

dbuenzli

Since the default is not enabled I didn't get why you added all the inlined alert disables.

dbuenzli · 2022-09-26T17:03:59Z

stdlib/domain.mli

+[@@@alert unstable
+    "The Domain module is experimental in OCaml 5.0.\
+     Its interface might be refined in future versions and \
+     break forward compatibility."


Since the alert is now unstable (which I think is better, it seems that's what other languages use aswell). I would suggest to drop the experimental terminology. I also wouldn't mention the version number. Besides I personally always get confused about forward and backward compatibility because the point where the person speaks from and where you read from makes it a bit unclear.

I would propose one of these formulations (repeating unstable depends a bit on how alerts render in the docs and in the warnings which I'm unclear of right now):

The [Domain] interface may change in the future. The [Domain] interface is unstable, it may change in the future. The [Domain] interface may change in incompatible ways in the future. The [Domain] interface is unstable, it may change in incompatible ways in the future.

It is generally useful to err on the stricter side in the compiler code base itself. For instance, we want to make it harder to introduce surreptitiously effects and effect handlers in the typechecker.

Good point on the alert message, I was blind-sided by the mechanical side of the update. The alert is currently rendered as

Alert unstable. ...

in the documentation and

Alert unstable: module Stdlib.Domain
...

in error messages.

Thus

Alert unstable. The [Domain] interface may change in incompatible ways in the future.

seems like a good option to me.

kit-ty-kate · 2022-10-11T12:36:56Z

utils/warnings.ml


 let () = ignore @@ parse_options false defaults_w
 let () = ignore @@ parse_options true defaults_warn_error
+let () =
+  List.iter (set_alert ~error:false ~enable:false) default_disabled_alerts


I don't understand why this is being disabled by default. In practice this means this new alert is rendered useless as people who use said API will simply not enable it and everyone else would have to enable it manually which they are extremely unlikely to do.

It feels like it defeats the purpose of the feature.

The alert is still documented and available for people that want to control their use of unstable features.

I think that the root question is what is the audience of the alert?

Is it beginners? Do we want to signal to them that they should not use the new features and pester them until they discover the way to silence the alert?

Or is it more advanced users? Do we want to signal to them that the feature is still really unstable and that there is a tool available to restrict the use of those features in their project if needed?

I think that given the way the toolchain works at the moment having ~~enabled~~ it disabled by default is a better strategy, see what I wrote in this comment.

EduardoRFS · 2022-10-19T17:13:18Z

For the domain module could we also get a warning regarding #11589?

It is a really tricky problem and it will bite more people as 5.0 is released.

Octachron · 2022-11-07T07:50:11Z

After more discussions, there was a consensus among the maintainers that a disabled by a default unstable alert was acceptable whereas an active alert would be counterproductive: domains and effects are here to be used.

Looking back at the PR, it seems better to drop the noisy testsuite changes.

favonia · 2022-11-07T09:09:42Z

I wonder if the word "unsafe" is the best choice here. It seems the word is for operations that could break type safety or memory safety, for example the Obj module and the unsafe* operations in Bytes and String. I believe API unstability is different from being unsafe. (EDIT: It was a typo! 😅)

Octachron · 2022-11-07T09:12:08Z

I meant unstable not unsafe, sorry. This is strictly documenting the more unstable than usual nature of the Effect and Domain modules.

stdlib: add an unstable alert to the Effect and Domain modules (cherry picked from commit d979990)

Octachron · 2022-11-24T16:41:14Z

Cherry-picked on 5.0 as dc2c2bd .

Octachron added the no-change-entry-needed label Aug 31, 2022

Octachron changed the title ~~stdlib: add an experimental alert to the Effect and Domain mdoule~~ stdlib: add an experimental alert to the Effect and Domain modules Aug 31, 2022

Octachron force-pushed the experimental_alert branch 2 times, most recently from 6068545 to e4a0a09 Compare September 26, 2022 16:24

dbuenzli reviewed Sep 26, 2022

View reviewed changes

kit-ty-kate approved these changes Sep 26, 2022

View reviewed changes

kit-ty-kate reviewed Oct 11, 2022

View reviewed changes

Octachron added 2 commits November 17, 2022 16:53

Disable unstable alert by default.

4ee8280

Stdlib: mark effect and domain modules as unstable.

98eb6b5

Octachron force-pushed the experimental_alert branch from bb3f95f to 98eb6b5 Compare November 17, 2022 16:29

Octachron merged commit d979990 into ocaml:trunk Nov 18, 2022

nojb mentioned this pull request Nov 21, 2022

Disable unstable alert in some stdlib modules #11741

Closed

Octachron added a commit that referenced this pull request Nov 24, 2022

Merge pull request #11526 from Octachron/experimental_alert

dc2c2bd

stdlib: add an unstable alert to the Effect and Domain modules (cherry picked from commit d979990)

Octachron mentioned this pull request Nov 25, 2022

Packages for OCaml 5.0.0 beta2 ocaml/opam-repository#22566

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdlib: add an experimental alert to the Effect and Domain modules #11526

stdlib: add an experimental alert to the Effect and Domain modules #11526

Octachron commented Aug 31, 2022

favonia commented Sep 10, 2022 •

edited

dbuenzli commented Sep 12, 2022

Octachron commented Sep 13, 2022

nojb commented Sep 13, 2022

dbuenzli commented Sep 13, 2022

dbuenzli commented Sep 13, 2022

gasche commented Sep 13, 2022

nojb commented Sep 13, 2022 •

edited

dbuenzli commented Sep 13, 2022

nojb commented Sep 13, 2022

dbuenzli commented Sep 13, 2022

Octachron commented Sep 26, 2022

dbuenzli left a comment

dbuenzli Sep 26, 2022

Octachron Sep 26, 2022 •

edited

kit-ty-kate Oct 11, 2022

Octachron Oct 12, 2022

dbuenzli Oct 12, 2022 •

edited

EduardoRFS commented Oct 19, 2022

Octachron commented Nov 7, 2022 •

edited

favonia commented Nov 7, 2022 •

edited

Octachron commented Nov 7, 2022

Octachron commented Nov 24, 2022

stdlib: add an experimental alert to the Effect and Domain modules #11526

stdlib: add an experimental alert to the Effect and Domain modules #11526

Conversation

Octachron commented Aug 31, 2022

favonia commented Sep 10, 2022 • edited

dbuenzli commented Sep 12, 2022

Octachron commented Sep 13, 2022

nojb commented Sep 13, 2022

dbuenzli commented Sep 13, 2022

Footnotes

dbuenzli commented Sep 13, 2022

gasche commented Sep 13, 2022

nojb commented Sep 13, 2022 • edited

dbuenzli commented Sep 13, 2022

nojb commented Sep 13, 2022

dbuenzli commented Sep 13, 2022

Octachron commented Sep 26, 2022

dbuenzli left a comment

Choose a reason for hiding this comment

dbuenzli Sep 26, 2022

Choose a reason for hiding this comment

Octachron Sep 26, 2022 • edited

Choose a reason for hiding this comment

kit-ty-kate Oct 11, 2022

Choose a reason for hiding this comment

Octachron Oct 12, 2022

Choose a reason for hiding this comment

dbuenzli Oct 12, 2022 • edited

Choose a reason for hiding this comment

EduardoRFS commented Oct 19, 2022

Octachron commented Nov 7, 2022 • edited

favonia commented Nov 7, 2022 • edited

Octachron commented Nov 7, 2022

Octachron commented Nov 24, 2022

favonia commented Sep 10, 2022 •

edited

nojb commented Sep 13, 2022 •

edited

Octachron Sep 26, 2022 •

edited

dbuenzli Oct 12, 2022 •

edited

Octachron commented Nov 7, 2022 •

edited

favonia commented Nov 7, 2022 •

edited