core-libraries-committee
core-libraries-committee copied to clipboard
Add the `todo` function
Dear Haskell core library committee.
Currently there are multiple ways of describing something as unimplemented
- undefined
- error "not implemented yet"
- various other, similar functions from external libraries, such as the ones in alternative preludes like
Relude
placeholders or the todo package - typed holes
problems of the above solutions
- Problems of the functions currently in
base
:
-
undefined
:- doesn't give you a warning if it remains in code
- doesn't read as "this is open, I will definitely implement it soon"
- a lot to type for "please compiler, be quiet for the second, I will come back to this"
-
error
:- also doesn't give you a warning if it remains in code
- may read as "this is open, I will definitely implement it soon" but this is a lot to type
- even more to type for "please compiler, be quiet for the second, I will come back to this", sometimes even needs paranthesis
- external dependencies:
- just for doing a quick debugging
todo
, it doesn't seem worth adding an alternativePrelude
or even add a dependency just for the sake of this
- typed holes
- they are, by default, errors, not warnings
- they are very noisy (by default they produce a lot of output)
- they are slow (by default)
That's why propose a function todo
that has the following properties:
- has a warning annotation
"todo remains in code"
in groupx-todo
so that it can be used withWError
for e.g. CI - has the same type as
undefined
- gets (re)exported by
base:Prelude
- is to be used whenever there is a place in the code where we want to temporary fill holes
- the warning message is not fixed as shown in the example implementation and may be adjusted, similar things hold for the documentation
implementation of the solution
{-# LANGUAGE ImplicitParams #-}
{-# LANGUAGE MagicHash #-}
{-# OPTIONS_GHC -Wall #-}
module Todo (todo) where
import GHC.Base (raise#, TYPE, RuntimeRep)
import GHC.Exception (errorCallWithCallStackException)
import GHC.Stack (HasCallStack)
{- | 'todo' indicates unfinished code.
It is to be used whenever you want to indicate that you are missing a part of
the implementation and want to fill that in later.
The main difference to other alternatives like typed holes and 'undefined'
or 'error' is, this does not throw an error but only emits a warning.
Similarly to 'undefined', 'error' and typed holes, this will throw an error if
it is evaluated at runtime which can only be caught in 'IO'.
This is intended to *never* stay in code but exists purely for signifying
"work in progress" code.
To make the emitted warning error instead (e.g. for the use in CI), add
the @-Werror=x-todo@ flag to your @OPTIONS_GHC@.
==== __Examples__
@
superComplexFunction :: 'Maybe' a -> 'IO' 'Int'
-- we already know how to implement this in the 'Nothing' case
superComplexFunction 'Nothing' = 'pure' 42
-- but the 'Just' case is super complicated, so we leave it as 'todo' for now
superComplexFunction ('Just' a) = 'todo'
@
-}
todo :: forall {r :: RuntimeRep} (a :: TYPE r). (HasCallStack) => a
todo = raise# (errorCallWithCallStackException "Prelude.todo: not yet implemented" ?callStack)
{-# WARNING in "x-todo" todo "todo remains in code" #-}
try it out in a separate module, as such:
module TodoExample where
import Todo
superComplexFunction :: Maybe a -> IO Int
-- we already know how to implement this in the 'Nothing' case
superComplexFunction Nothing = pure 42
-- but the 'Just' case is super complicated, so we leave it as 'todo' for now
superComplexFunction (Just a) = todo
This implementation will work from >= ghc981
impact
on hoogle I currently find 4 functions which this would break, two of which have similar semantics to this one, making it improbable that they will be found in code out there.
Another advantage of this function is that there will be no need of a *-compat
package because code that does contain this function is not supposed to live anywhere or compile with more than the compiler than the base
version this is going to be shipped with supports.
I will obviously run a proper impact assessment though.
why I think this belongs in Prelude
The main reason I think this belongs into Prelude
is because it is not possible to replace this with the same level of simplicity with any other solution
- libraries add technical issues like unused packages warnings by cabal and/ or the overhead of having to define
mixins
or add new imports; Additionally already having to add an additional dependency to use this, would, I think, be too much effort for many people to use it - external tools are external tools, they require additional installation, they don't ship with
GHC
, they are not supported first class byGHC
, with the advent of the{-# WARNING -#}
we have first class support for these kinds of things that don't add any additional overhead -
error
andundefined
don't haveWARNING
s attached to them and I don't think they should have; they have been as they are for a long time, it would impose a great cost to change their semantics; even though it is debatable whether you should, people useundefined
in their code to signify unreachable or permanently unimplemented parts of their code, if we want to add a warning, I think it should be on something that clearly tells the programmer and the reader of the code "this is not done yet"
I think this will also have the positive impact of offering a real alternative to dangerous uses of undefined
also look at
rust std's todo!
and unimplemented!
macros
rendered docs
some more screenshots
-
how it looks, loading a module using
todo
intoGHCi
: -
how it looks when trying to load a module using
todo
with{-# OPTIONS_GHC -Werror=x-todo #-}
intoGHCi
:
Amendment to this proposal (28 Mar 2024)
After what I consider a very productive and diverse discussion, I think the most prominent conclusion is that this might not make the jump into Prelude
all at once. I still want to proceed with this proposal for the following two reasons:
- accessibility and visibility as well as usefulness in general (least overhead, least tooling issues, least possibility of divergence, etc.) will be greatest if this is in
base
- and most importantly: I still want this is
Prelude
eventually; this seems to be only possible when something has been inbase
for a long time and one has to start at some point ;)
a new module, ~~Debug.Placeholder
~~ Develop.Placeholder
There will be a new module ~~Debug.Placeholder
~~ Develop.Placeholder
that will contain both the
-
todo
function as described above and implemented in ekmett'splaceholder
library (thank you for throwing this together) (except some improvements I will propose wrt type applications) - the
TODO
pattern synonym as is implemented in theplaceholder
library (except some improvements I will propose wrt type applications)
These implementations include many useful improvements to the function described above, which is really awesome.
The name of the module is justified as follows:
- I don't think that this belongs into
Control
or the like because it's only temporarily, you do something with the definitions in the process of programming, but don't really intend to keep the definitions - ~~
Debug
is the namespace that I think most fits the semantics of these functions intuitively, after all,todo
is something you insert in the code while "writing code" which is closest to "debugging", I'd say~~ @tbidne has proposed the namespaceDevelop
which I think is the most fitting until now
Note: if people think that the proposed namespace is incorrect, I would like to hear convincing arguments and am happy to adjust accordingly.
out of scope for this proposal
While there were some really nice suggestions to make the proposal "more complete", I will consider the following out of scope for this proposal while expressing the strong intention to later add them in a follow-up proposal:
- the
{u,U}nimplemented
function and patterns: the reason why I want to avoid them for now is that they will spark additional discussions on whether you really need both of these and what the semantics of them should really be, whether they require warnings, etc. - the
{unimplemented,todo}IO
variations of these: the reason why I want to avoid these is that I think it will spark discussion on whether we will need these in the case of onlytodo
existing, as it may never remain in code - adding a new module
Develop
that will export many useful functions that you need only for work-in-progress code, such as thetodo
family of functions as well as thetrace*
family of functions
I use
pattern TODO :: HasCallStack => () => a
pattern TODO <- _unused
where TODO = error "TODO"
{-# COMPLETE TODO #-}
it allows you to use TODO
as a pattern too:
case TODO of
Nothing -> print "nothing to do"
TODO -> TODO -- I'm too lazy to cover the rest of the cases for now
Also, CAPS MAKE IT STAND OUT :)
Warning is a nice touch. I think the custom warning could also be added to the functions in Debug.Trace
. I used to redefine them and deprecate so I'll won't leave them in the code either, but now we have custom warnings, so it can be done in base
. I'll :+1: the proposal for that.
On Thu, 7 Mar 2024, Mango The Fourth wrote:
All of these have problems:
- Problems of the functions currently in base:
- undefined:
- doesn't give you a warning if it remains in code
- doesn't read as "this is open, I will definitely implement it soon"
- a lot to type for "please compiler, be quiet for the second, I will come back to this"
- error:
- also doesn't give you a warning if it remains in code
- may read as "this is open, I will definitely implement it soon" but this is a lot to type
- even more to type for "please compiler, be quiet for the second, I will come back to this", sometimes even needs paranthesis
You can generate such warnings with HLint.
That's why propose a function todo that has the following properties:
What about Lentil with Todo/Fixme comments or custom comment tags?
Why raise#
and not just throw
?
Why
raise#
and not justthrow
?
Probably just copying the definition of undefined
, https://hackage.haskell.org/package/base-4.19.1.0/docs/src/GHC.Err.html#undefined
undefined :: forall (r :: RuntimeRep). forall (a :: TYPE r).
HasCallStack => a
-- This used to be
-- undefined = error "Prelude.undefined"
-- but that would add an extra call stack entry that is not actually helpful
-- nor wanted (see #19886). We’d like to use withFrozenCallStack, but that
-- is not available in this module yet, and making it so is hard. So let’s just
-- use raise# directly.
undefined = raise# (errorCallWithCallStackException "Prelude.undefined" ?callStack)
@thielema:
yes you can generate these warnings with hlint and yes there are external tools that may allow these things in some more convoluted way; however, adding warnings to all undefined
or error
does defeat the purpose in the same way as having to add an external dependency; the whole point of this is to make it as low barrier to use as possible and have as few external tools/ dependencies necessary as possible. Additionally, both words, error
and undefined
, convey different meanings than todo
does.
@treeowl:
It is as @phadej says, this is pretty much exactly how undefined
is defined, also afaict throw
would just be like composing with id
as throw = raise# . toException
and toException @SomeException = id
On Thu, 7 Mar 2024, Mango The Fourth wrote:
@thielema:
yes you can generate these warnings with hlint and yes there are external tools that may allow these things in some more convoluted way; however, adding warnings to all undefined or error does defeat the purpose in the same way as having to add an external dependency; the whole point of this is to make it as low barrier to use as possible and have as few external tools/ dependencies necessary as possible. Additionally, both words, error and undefined, convey different meanings than todo does.
You can also define "todo = error" and add a HLint warning to your "todo".
Why base
and not a tiny package?
Why
base
and not a tiny package?
@rhendric I can answer this question from a supply chain perspective: Because dependencies are not free. A dependency graph is something that will ask time, sometimes money, and overall energy to maintain. One more dependency to vet is one more maintainer to trust and, should things go wrong, support.
I think it belongs in base because it's extremely common to mark parts of your code as todo.
A lot of good proposals here for small, easily-implemented-elsewhere functions get shot down because they don't meet the Fairbairn threshold. I don't really have a problem with the CLC deciding that this one is valid but I don't yet see the reasoning. If ‘dependencies aren't free’ and ‘many people want this’ are the only considerations, base
ought to be a whole lot larger.
Additionally to the named above reasons there's also a technical reason that just came to my mind: add a library to cabal that just adds something that you need for debugging, remove the debugging part, now you have a cabal warning about unused packages; are you going to
- add a new package to cabal
- reload your language server
- wait until it's up
- import
Debug.Todo
(or do cabal mixins, same deal) - write down
todo
every time you want to indicate code as not done? I don't think that's feasible.
On Thu, 7 Mar 2024, Mango The Fourth wrote:
Additionally to the named above reasons there's also a technical reason that just came to my mind: add a library to cabal that just adds something that you need for debugging, remove the debugging part, now you have a cabal warning about unused packages; are you going to
- add a new package to cabal
- reload your language server
- wait until it's up
- import Debug.Todo (or do cabal mixins, same deal)
- write down todo every time you want to indicate code as not done? I don't think that's feasible.
You can see it as an advantage: By removing the 'todo' dependency, you know for sure, that your code is free of 'todo's.
This is your monthly public service reminder that adding stuff to base
is not free either (although it may be a cost paid by someone other than you).
I imagine adding something like this to Prelude
would clash with identifiers already called todo
. Perhaps something like this could be exposed from Debug.Trace
instead? I feel like that would greatly reduce the risk of breaking folks code. Though I agree it would be less convenient. If it proves to be very popular, perhaps it could be added to Prelude
too in time.
https://hackage-search.serokell.io/?q=%5Etodo+%3A%3A
31 packages have a top-level identifier called todo
.
Lots of packages seem to use todo
as a variable name, which wouldn't lead to errors but would lead to shadowing warnings. It's hard to get a definite number because todo
is a very common string in comments.
This is your monthly public service reminder that adding stuff to
base
is not free either (although it may be a cost paid by someone other than you).
I don't think comments like this are constructive, it comes off as passive aggressive. @MangoIV has listed a number of reasons why they think todo
deserves to be in base
/Prelude
so I think they're well aware that adding things is not free and requires justification.
On Thu, 7 Mar 2024, Morrow wrote:
This is your monthly public service reminder that adding stuff to base is not free either (although it may be a cost paid by someone other than you).
I don't think comments like this are constructive, it comes off as passive aggressive.
I understood it as humorously told counterargument to "adding a new package is not free" and wanted to give the same things to consider.
What often goes unsaid in appeals to the Fairbairn threshold is that there is a natural tension with another useful feature: discoverability. Many functions in base
(e.g. much of Data.List
) could be argued as "trivial combinations", yet I am grateful for their existence as I don't always know what I need, and I am not confident I could implement many of them without inadvertently tripping some subtlety (e.g. laziness, performance, bottom).
Providing an "obvious" function can have great value when it shepherds users into Doing the Right Thing :tm: . Bonus points if the function in question is canonical, in some sense.
Of course this has to be balanced with the downsides of increasing base
's surface area, which are well-articulated.
I was skeptical when I read the title as I was picturing the trivial todo = error "todo"
, yet the OP is well-motivated, especially given the custom warning and reference to rust (the pattern synonym is also intriguing).
From a POSIWID perspective, the purpose of base
is not to be a batteries-included collection of best practices for new users who want to write professional code. The extensive use of String
, if nothing else, should make that clear. That responsibility has been taken up by a variety of alternate prelude libraries—I'm partial to protolude
myself, and Protolude.Debug.notImplemented
indeed fills the same niche as this proposal, including the warning.
Should we make incremental improvements to base
to bring it closer to the best-practice ideal? Of course I think so, but I also think that backward-compatibility concerns will always prevent base
from doing as good a job of this as an independent package can. I think that weakens the argument that functions should be included in base
in order to funnel users towards good patterns—users who want the best patterns would be better served by using an alternate prelude that doesn't also cater to professors teaching from fifteen-year-old textbooks.
I appreciate the well-written and detailed proposal and I largely agree with the direction, but I'm very hesitant to add anything to Prelude
without lots of prior art and experience. Adding to Debug.Trace
(or maybe to a new module Debug
) would be more plausible.
This is your monthly public service reminder that adding stuff to base is not free either (although it may be a cost paid by someone other than you).
I don't think comments like this are constructive, it comes off as passive aggressive.
I understood it as humorously told counterargument to "adding a new package is not free" and wanted to give the same things to consider.
Thank you, yes, that was exactly my intention.
@Bodigrim I absolutely understand your issue with this and agree on the basic gripes with the idea of adding something “completely new” to Prelude
.
However, consider that there’s nothing new here, really, except the warning pragma, the exact same implementation has been part of Prelude
and battle tested for years now in the form of undefined
.
Additionally we really can only get the most out of this change if people actually use this. If the barrier to using it is any higher than the functions we don’t want users to use (undefined
), the users will probably instead use those.
typed holes ... they are slow (by default)
In what way are typed holes slow?
I personally oppose adding this to Prelude
. There are just too many situations where it seems to make sense as a variable name.
Additionally we really can only get the most out of this change if people actually use this.
Maybe, but we can evolve to this state gradually. If there are early adopters keen to use such function even if it is not in Prelude
(or even not in base
), it would make a strong argument that we can reap more benefits from putting it there. Otherwise it's hard to predict a future impact, which combined with a fact that adding things to Prelude
is pretty much irreverisble makes me sceptical.
Changing the set of entities exported by default in every Haskell program is not a decision to be taken lightly.
Great idea, in spirit! However I must agree with others who raise the "supply chain" concern.
How about we add compiler warnings to undefined
and error
instead?
Why
base
and not a tiny package?@rhendric I can answer this question from a supply chain perspective: Because dependencies are not free. A dependency graph is something that will ask time, sometimes money, and overall energy to maintain. One more dependency to vet is one more maintainer to trust and, should things go wrong, support.
Weak argument to me.
Haskell is about reusing code. Long running haskell projects accumulate sometimes hundreds of direct dependencies. And now they crumble by adding one more?
What are the arguments to add things to base? From what I see:
- things that other boot libraries need
- things the standard mentions
- things that are otherwise very fundamental building blocks, widely necessary and have stopped randomly evolving
The proposed function is a neat idea and I can see some use for it in education. But in the end it just feels like a synonym for undefined
. As such I join @ocramz in the question why can't have warnings for undefined? There are a couple legitimate use cases and it shouldn't be hard to do some transition period (warning off by default first).
why can't have warnings for undefined?
I have justified above (but I should probably also add it to the proposal itself, pending) why I don’t think adding warnings to undefined and error themselves is a good idea.
Adding warnings to these long used functions is probably an infinitely more breaking change than adding a new function that is used only 20 times in hackage, often for the same reason as this proposal intends.
@hasufell
Out of curiosity, how many of the declarations in base
were added to it because of the reasons you named and how many were not? Do we have any numbers on that?
Adding warnings to these long used functions is probably an infinitely more breaking change
I don't think so. The -Werror
faction should stop making everyone else pay for their decision.
Warnings can and already do change. And we have mechanisms to introduce new warnings. See -Wcompat.
I do not consider those breaking changes.
Recent discussions in GHC SC proposals such as 625 have refined the understanding of these warning categories and transition procedures.
@hasufell Out of curiosity, how many of the declarations in
base
were added to it because of the reasons you named and how many were not? Do we have any numbers on that?
I don't think we have any numbers. What purpose would that data serve?