Add Stdlib.todo

[pkhry]

Please refer to #13072.

Added val todo : ?msg:string -> __LOC__:string -> unit to Standard library toplevel definitions.
?msg - is optional custom message, by default it is "not implemented yet".
__LOC__ - is optional code location, where the function was called to simplify tracking of calls to todo.

Also i added a few tests, but i've put them into testsuite/tests/lib-stdlib-todo/*, was this a correct destination?

[OlivierNicole]

Thank you for this PR. I am rather in favor of having such a function, but I have doubts about the interface design. Do feel free to counter-argument.

[OlivierNicole]

Correct me if I’m wrong, but to me, the main benefit of this function is to get compile-time warnings. If that’s the case, then we should be able to test that compiler output in the testsuite, too.

[pkhry]

i feel like the main benefit is to have a shorthand to signify that we will implement a given piece of functionality later.
Also it should be possible to get a compile time Error from this:

[@@@ocaml.alert "++todo"]
let () = 
 Stdlib.todo () 

should give us:

Line 3, characters 0-11:
3 | Stdlib.todo ();;
    ^^^^^^^^^^^
Error (alert todo): Stdlib.todo
TODO: not implemented yet

I will push the test for the error.
Or you thought about introducing a new kind of warning into the compiler?

[OlivierNicole]

I don’t know what I think about labelling an argument __LOC__. In my mind this name is reserved as a lexer placeholder, and the use of the function looks verbose to em. To get the location of the error, we have exception backtraces.

[pkhry]

Fair enough, but i was going from the convention of passing explicit LOC from tezt or something akin to ppx_here.
But yeah, given the fact that user can just call Printexc.record_backtrace true this seems excessive. removed

[MisterDA]

Using __LOC__ is a hack that I think I've seen in other libraries. There's no backtrace if compiled without debug information. This could be solved elegantly with Automatically insert source location #126.
EDIT: ah, sorry, I misunderstood. There's indeed no need to override the location with this parameter, the exception will do.

[dbuenzli]

Here are a few thoughts:

1. I don't think the alert should be enabled by default. I often need todo functionality but I don't want it to remind me on every build what remains todo as it will make it harder to sort through more important messages. I'd rather invoke a build profile that enables the alert when I want to pay attention to this.
2. Except for the fact that the alert allows to detect locations at compile time, in the current state this doesn't bring much over failwith "TODO" (which is a bit painful to write when you stub things). Couldn't this be a compiler primitive
   so that we can simply type todo (or Fun.todo) to stub out stuff. (i.e. the type is val todo : 'a)
3. I'm not sure the msg bit is that useful (and you couldn't have it with todo : 'a). With the alert/or stacktrace you get to the location where a comment can equally serve as the msg.
4. Adding to the initial scope is always a bit tricky. Should perhaps this rather be added to Fun ?

[pkhry]

Here are a few thoughts:

1. I don't think the alert should be enabled by default. I often need todo functionality but I don't want it to remind me on every build what remains todo as it will make it harder to sort through more important messages. I'd rather invoke a build profile that enables the alert when I want to pay attention to this.

how about we exclude this alert by default and enable it only when we pass -alert +todo to the compiler?

2. Except for the fact that the alert allows to detect locations at compile time, in the current state this doesn't bring much over failwith "TODO" (which is a bit painful to write when you stub things). Couldn't this be a compiler primitive
   so that we can simply type todo (or Fun.todo) to stub out stuff. (i.e. the type is val todo : 'a)
   Not sure about making it a primitive.
3. I'm not sure the msg bit is that useful (and you couldn't have it with todo : 'a). With the alert/or stacktrace you get to the location where a comment can equally serve as the msg.
4. Adding to the initial scope is always a bit tricky. Should perhaps this rather be added to Fun ?

All in all it does seem like a its a better idea to add this as primitive to the compiler with a corresponding warning that can be enabled.
However I think support for custom messages is quite valuable when there multiple todos in a codebase.

[yawaramin]

how about we exclude this alert by default and enable it only when we pass -alert +todo to the compiler?

Sure, but then we should add a note in the docstring so that users would be aware that they need to explicitly enable the alert.

EDIT: it looks like the normal practice is already to disable by default any new alerts added to the standard library, which makes sense as I assume otherwise it could break builds (in the general case):

ocaml/utils/warnings.ml

Line 868 in eabbb40

let default_disabled_alerts = [ "unstable"; "unsynchronized_access" ]

[gasche]

I'm not fond of the __LOC__ argument. I think that we could remove it and rely on exception backtraces to get the location, or make progress on #126 instead of adding yet another approach. (Or we could decide to have a todo keyword in the language to have the compiler generate a location, like we do for assert false. But that sounds harder to sell.)

[OlivierNicole]

I like the current design. I agree with @dbuenzli and @yawaramin that the alert should be disabled by default (I believe @yawaramin pointed to how to do that).

2. Except for the fact that the alert allows to detect locations at compile time, in the current state this doesn't bring much over failwith "TODO" (which is a bit painful to write when you stub things). Couldn't this be a compiler primitive so that we can simply type todo (or Fun.todo) to stub out stuff. (i.e. the type is val todo : 'a)

The semantics of this seem unclear to me: if I understand you correctly, todo would behave as failwith "..." when evaluated. However since the expression todo is an identifier and not a function application, this breaks the property that due to call-by-value evaluation, identifiers can be assumed to be already evaluated. The two following codes would no longer be equivalent:

if String.equal Config.architecture "amd64" then
  let offset = todo in
  List.iter (fun x -> x := !x + offset) l

let offset = todo in
if String.equal Config.architecture "amd64" then
  List.iter (fun x -> x := !x + offset) l

It seems challenging to maintain correctness of the usual optimizations like hoisting operations out of loops, CSE… in this setting. I don’t think we want to go down this rabbit hole for the small gain in convenience.

4. Adding to the initial scope is always a bit tricky. Should perhaps this rather be added to Fun ?

Maybe I should know this, but why is adding to the initial scope frowned upon? I’m not against putting it elsewhere, but I don’t see a link between todo and the function manipulation module Fun.

[zapashcanon]

I would prefer the following:

let todo _ = assert false

val todo : _ -> _

So one can write:

let f = todo "explain whatever needs to be done"

And:

if ... then ... else todo @@ some code that I know is wrong but is a good draft

[dbuenzli]

Maybe I should know this, but why is adding to the initial scope frowned upon?

Because you are withdrawing names to use from the user. The user may want to use todo in her code, but then she can no longer use the stdlib todo, except by prefixing it with Stdlib.todo.

And then when people read her code it's not immediately clear if todo is the Stdlib or her user defined todo.

Finally it can also leads to strange error messages, or worse, lack thereof, when you move code around without, by mistake, taking all bound names with you.

Having todo in a proper module alleviates a lot of these problems.

but I don’t see a link between todo and the function manipulation module Fun.

If we agree adding to the initial scope is not a good idea. It has to go somewhere, feel free to propose a better place. We could say here that except for toplevel expressions, the raise is always part of a function that ends up todo, so I'm fine with Fun here.

The semantics of this seem unclear to me: if I understand you correctly, todo would behave as failwith "..."

Rather like assert false.

[OlivierNicole]

So one can write:

let f = todo "explain whatever needs to be done"

And:

if ... then ... else todo @@ some code that I know is wrong but is a good draft

Mmh… I’m afraid that it could confuse newcomers by letting them think that the argument is magically left unevaluated (which isn’t true).

but I don’t see a link between todo and the function manipulation module Fun.

If we agree adding to the initial scope is not a good idea. It has to go somewhere, feel free to propose a better place. We could say here that except for toplevel expressions, the raise is always part of a function that ends up todo, so I'm fine with Fun here.

Alert.todo or Error.todo?
I won’t fight against Fun.todo, I just find it not very clear and thus aesthetically disappointing.

The semantics of this seem unclear to me: if I understand you correctly, todo would behave as failwith "..."

Rather like assert false.

Understood. But if todo is to be a primitive keyword like assert, it doesn’t fit within the module system. Either we can export it from a module like Fun, but then its behaviour must be coherent with that of an identifier, and we fall in the issues that I raised above; or it is a primitive keyword and it lives in the global scope, which I think we don’t want.

[dbuenzli]

Alert.todo or Error.todo?

Again, I'd rather not withdraw (less to some extent now that the stdlib is namespaced) these generic module names from use by users. Especially if it's only to add this single identifier.

If we take the constraint to not add a new module for it and not have it in the initial scope the only other candidate I see would be the "throw-it-all" Sys. But then I prefer to read:

let f x = Fun.todo ()

than

let f x = Sys.todo ()

then its behaviour must be coherent with that of an identifier, and we fall in the issues that I raised above;

I had thought that having it as an external : Fun.todo = "%todo" wouldn't, but you certainly know any better.

Note the reason why I wanted a keyword-like todo is that very often my todos do not even get to see a real run of the program. I use them e.g. to stub branches in pattern matches to trick merlin into thinking some of the work is done so that it can help me with the work I'm doing now:

match e with 
| cond0 -> failwith "TODO"
| cond1 -> … (* writing this first, help me merlin *)

[nojb]

The semantics of this seem unclear to me: if I understand you correctly, todo would behave as failwith "..."

Rather like assert false.

Don't have a horse in this race, but: if the gist of the proposal is to allow triggering an alert from arbitrary points in the code, why not do that instead? Then one could write assert false [@alert todo "bla bla"] to trigger the todo alert at that point. There could even be a shorthand [@todo "bla bla"] to make it a bit more concise.

[nojb]

In another direction we could have [%todo "bla bla"] expand to assert false and trigger the alert. This would be almost like introducing a new keyword.

[dbuenzli]

why not do that instead?

Because if your todos get in an execution and are hit the reported error does not clearly distinguish between a todo or something that should have not happened.

[gasche]

If we want users to actually use the feature, it should be convenient to type, so assert false [@todo "..."] is not reasonable I believe. [%todo "..."] is okay. I don't personally have a problem with the idea of handling it specially in the compiler by introducing an error message that contains the location. ut again, why not just use plain exceptions with an exception backtrace?

[gasche]

More precisely, I think we could just do:

(* in Fun or Sys or wherever we want *)
exception TODO of string
let todo msg = raise (TODO msg)

[OlivierNicole]

I believe that is pretty much what current implementation does, except for the dedicated Todo exception, which I agree is a good idea.