Lwt_sequence deprecated but still used by Lwt_main

[bikallem]

Lwt 4.0.0 deprecates Lwt_sequence.t. However, some functions in Lwt_main still seem to use it. As a result of which there is no coherent migration path from not using Lwt_sequence. I am attempting to upgrade lambda_term to lwt 4.0.0 and came across this issue - https://github.com/diml/lambda-term/issues/60.

The functions using Lwt_sequence is Lwt_main are as follows:

1. val enter_iter_hooks : (unit ‑> unit) Lwt_sequence.t
2. val leave_iter_hooks : (unit ‑> unit) Lwt_sequence.t
3. val exit_hooks : (unit ‑> unit Lwt.t) Lwt_sequence.t

I tried to copy/paste Lwt_sequence code to lambda-term but it seems lwt is still using Lwt_sequence internally which introduces another issue with module names.

Is there a more robust and suggested path for migrating code to lwt 4.0.0 modulo disabling the deprecation warning.

[rgrinberg]

I think this issue is really a problem of lambda-term. It was probably a mistake to let the user be aware that callbacks are stored in Lwt_sequence.t at all.

To fix this issue, I think we need lambda-term to have an abstract type for hooks. Now unfortunately, that will have to be a breaking change, but there's probably no reasonable way to avoid a breaking change here. The way to support old users in lambda-term at all costs would be to import Lwt_sequence.t as you did and include the type equality Lambda_term.Lwt_sequence.t = Lwt_sequence.t with conditional compilation.

[aantron]

Lwt uses Lwt_sequence like this:

https://github.com/ocsigen/lwt/blob/9340e6c661ddb06014a230d4299338563c30e0ad/src/core/lwt_condition.ml#L29-L36

If that's not an option (is Lwt_sequence exposed in the lambda-term API?), we can remove the Lwt_sequence warning in a 4.0.1 release.

[aantron]

@rgrinberg's type alias suggestion could work even for an API, actually.

[bikallem]

@rgrinberg Just tried the following to ensure type equality of the imported Lwt_sequence and original lwt Lwt_sequence.t. I am not sure if type equality works with abstract type. I tried the following but got a compilation error.

[@@@ocaml.warning "-3"]
type 'a t = 'a Lwt_sequence.t = {
  mutable prev : 'a t;
  mutable next : 'a t;
}

type 'a node = 'a Lwt_sequence.node = {
  mutable node_prev : 'a t;
  mutable node_next : 'a t;
  mutable node_data : 'a;
  mutable node_active : bool;
}
[@@@ocaml.warning "+3"]

This gives an error below,

Error: This variant or record definition does not match that of type
>          'a Lwt_sequence.t
>        Their kinds differ.

Am i missing something? We need type equality between the two because we use Lwt_main.enter_iter_hooks which as noted earlier return Lwt_sequence.

cc @diml

[bikallem]

@aantron It seems there isn't a straight-forward path to getting around the deprecation of Lwt_sequence. Even if the module itself is deprecated, lwt still exposes public apis that make use of Lwt_sequence and which are not deprecated and don't have a corresponding non-deprecated api that we can migrate to. This is a show stopper for utop which depends on lambda term and which isn't compatible with lwt 4.0.0. As such one currently has to choose between utop and lwt 4.0.0.

If that's not an option (is Lwt_sequence exposed in the lambda-term API?), we can remove the Lwt_sequence warning in a 4.0.1 release.

Perhaps your suggestion is the most viable option for now.

[aantron]

I took a superficial glance at lambda-term's usage of Lwt_sequence by doing a GitHub code search for Lwt_sequence in the lambda-term repo.

This could be wrong, but I suggest that you may be able to readily work around all the usages.

1. lTerm_widget_base_impl.ml, lTerm_buttons_impl.ml, lTerm_scroll_impl.ml all use the Lwt_sequence-based machinery from lTerm_widget_callbacks.mli, which doesn't look like it's related to Lwt, in other words it is using Lwt_sequence because it is convenient that Lwt has a linked list implementation, which is the kind of usage we want to discourage.
2. Only lTerm.ml uses Lwt_sequence to communicate with Lwt (Lwt_main), and this usage looks to be purely internal and isolated to a small amount of code in that module. So, you can readily suppress the deprecation warning for it. This usage of Lwt_sequence doesn't look related to the one in the previous point.

I don't know what is the best for Lambda Term to do about (1), but you should be able to work around the deprecation warnings using a module alias, signature inclusions, etc., or you can simply live with the warnings.

Is LTerm_widget_callback directly used by the users of Lambda Term, or is it mainly an internal module to Lambda Term?

[aantron]

In the meantime, we should probably provide a non-Lwt_sequence-based API for those hooks in Lwt_main, in a future release of Lwt.

[bikallem]

In the meantime, we should probably provide a non-Lwt_sequence-based API for those hooks in Lwt_main, in a future release of Lwt.

Indeed. After that lwt users should have a coherent way to migrating their codebase. Thanks.

[aantron]

This was also noted in https://github.com/ocsigen/lwt/issues/361#issuecomment-461354553 (after this issue was opened), and non-Lwt_sequence-based APIs were added in #660, which was released as part of Lwt 4.2.0. The new API is:

https://github.com/ocsigen/lwt/blob/e78f955574a136fea7659369560372c74f24f1e6/src/unix/lwt_main.mli#L49-L91