Open pkhry opened 4 weeks ago
Here are a few thoughts:
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.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
) 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
. Fun
? Here are a few thoughts:
- 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?
- 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 typetodo
(orFun.todo
) to stub out stuff. (i.e. the type isval todo : 'a
) Not sure about making it a primitive.- I'm not sure the
msg
bit is that useful (and you couldn't have it withtodo : 'a
). With the alert/or stacktrace you get to the location where a comment can equally serve as themsg
.- 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.
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): https://github.com/ocaml/ocaml/blob/eabbb4002a0dabdbcf708f52a1db1d7a11618119/utils/warnings.ml#L868
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.)
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).
- 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 typetodo
(orFun.todo
) to stub out stuff. (i.e. the type isval 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.
- 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
.
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
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 asfailwith "..."
Rather like assert false
.
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 withFun
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 asfailwith "..."
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.
Alert.todo
orError.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 todo
s 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 *)
The semantics of this seem unclear to me: if I understand you correctly,
todo
would behave asfailwith "..."
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.
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.
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.
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?
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)
I believe that is pretty much what current implementation does, except for the dedicated Todo
exception, which I agree is a good idea.
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 totodo
.Also i added a few tests, but i've put them into
testsuite/tests/lib-stdlib-todo/*
, was this a correct destination?