haskell / core-libraries-committee

95 stars 15 forks source link

Add the `todo` function #260

Open MangoIV opened 4 months ago

MangoIV commented 4 months ago

Dear Haskell core library committee.

Currently there are multiple ways of describing something as unimplemented

problems of the above solutions

  1. 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
  2. external dependencies:
    • just for doing a quick debugging todo, it doesn't seem worth adding an alternative Prelude or even add a dependency just for the sake of this
  3. 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:

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

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

rendered docs without expanded Examples rendered docs with expanded Examples

some more screenshots

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:

  1. 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
  2. and most importantly: I still want this is Prelude eventually; this seems to be only possible when something has been in base 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

These implementations include many useful improvements to the function described above, which is really awesome.

The name of the module is justified as follows:

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:

phadej commented 4 months ago

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.

thielema commented 4 months ago

On Thu, 7 Mar 2024, Mango The Fourth wrote:

All of these have problems:

  1. 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?

treeowl commented 4 months ago

Why raise# and not just throw?

phadej commented 4 months ago

Why raise# and not just throw?

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)
MangoIV commented 4 months ago

@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

thielema commented 4 months ago

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".

rhendric commented 4 months ago

Why base and not a tiny package?

Kleidukos commented 4 months ago

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.

jappeace commented 4 months ago

I think it belongs in base because it's extremely common to mark parts of your code as todo.

rhendric commented 4 months ago

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.

MangoIV commented 4 months ago

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

every time you want to indicate code as not done? I don't think that's feasible.

thielema commented 4 months ago

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.

tomjaguarpaw commented 4 months ago

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).

TeofilC commented 4 months ago

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.

MorrowM commented 4 months ago

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.

thielema commented 4 months ago

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.

tbidne commented 4 months ago

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).

rhendric commented 4 months ago

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.

Bodigrim commented 4 months ago

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.

tomjaguarpaw commented 4 months ago

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.

MangoIV commented 4 months ago

@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.

tomjaguarpaw commented 4 months ago

typed holes ... they are slow (by default)

In what way are typed holes slow?

treeowl commented 4 months ago

I personally oppose adding this to Prelude. There are just too many situations where it seems to make sense as a variable name.

Bodigrim commented 4 months ago

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.

ocramz commented 4 months ago

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?

hasufell commented 4 months ago

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:

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).

MangoIV commented 4 months ago

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.

MangoIV commented 4 months ago

@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?

hasufell commented 4 months ago

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 commented 4 months ago

@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?

MangoIV commented 4 months ago

The -Werror faction should stop making everyone else pay for their decision.

So you’re proposing we’re penalising Werror users? Inconsiderate of the breakage that will cause for them? Of course there’s always a decision to use a certain language feature, why would we punish people for doing so?

What purpose would that data serve?

It would make it more clear, whether your opinions on what belongs into base are common sense and I think that would be an important information, no?

hasufell commented 4 months ago

The -Werror faction should stop making everyone else pay for their decision.

So you’re proposing we’re penalising Werror users?

No, we're not penalizing anyone. Warnings are a means of communication. They're meant to be seen.

There has never been a contract or policy stating that we keep warnings stable for the rest of GHCs lifetime.

People using -Werror should know what they're getting into.

If you use -Werror you're saying you want the compiler to abort on warnings. Good. That includes a warning about undefined. Why would they not have the new GHC version abort? Maybe it uncovers a bug. You're making assumptions they don't want their pipeline to fail. If that is the case, they probably don't want -Wdefault, but select the warnings more fine grained.

What purpose would that data serve?

It would make it more clear, whether your opinions on what belongs into base are common and I think that would be an important information, no?

I'm not sure it's relevant. CLC does not have a common document on which we reach decisions.

This is just my opinion.

MangoIV commented 4 months ago

Why would they not have the new GHC version abort?

because the word undefined doesn’t clearly say “this mustn’t stay in code”.

Also thank you for clarifying, now it’s much more obvious what you were saying with this.

This is just my opinion.

sorry I think this was transported the wrong way. I’m not going to doubt the validity of your opinion, I was just curious on whether this was considered common sense.

hasufell commented 4 months ago

because the word undefined doesn’t clearly say “this mustn’t stay in code”.

There's tons of warnings of that nature:

hasufell commented 4 months ago

Btw, given that we warn on head/tail now it seems a bit contrived to not warn on undefined, which (unlike head/tail) will crash your program for sure when evaluated.

Profpatsch commented 4 months ago

I’ve been using nearly the same definition, albeit with a little nicer messages, for years in our production code:

{-# WARNING todo "'todo' (undefined code) remains in code" #-}
todo :: forall (r :: RuntimeRep). forall (a :: TYPE r). (HasCallStack) => a
todo = raise# (errorCallWithCallStackException "This code was not yet implemented: TODO" ?callStack)

Well, not in production, because it creates a warning at compile time :)

I originally copied the idea from the protolude library if I remember correctly.

I am an full favor of including it in Prelude.

Profpatsch commented 4 months ago

FWIW if you include the word TODO in the runtime error message, pragmatically it’s easy to filter for in log analyzers and not specific to Haskell error messages like Prelude.todo would be.

miguel-negrao commented 4 months ago

I’ve been using nearly the same definition, albeit with a little nicer messages, for years in our production code:

In case it is useful to others I got the code above to work with the following imports and language extensions:

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE ImplicitParams #-}
{-# LANGUAGE MagicHash #-}
{-# LANGUAGE PolyKinds #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE Trustworthy #-}
{-# LANGUAGE NoImplicitPrelude #-}

module Todo (todo) where

import GHC.Exception (errorCallWithCallStackException)
import GHC.Prim (TYPE, raise#)
import GHC.Stack.Types (HasCallStack)
import GHC.Types (RuntimeRep)

{-# WARNING todo "'todo' (undefined code) remains in code" #-}
todo :: forall (r :: RuntimeRep). forall (a :: TYPE r). (HasCallStack) => a
todo = raise# (errorCallWithCallStackException "This code was not yet implemented: TODO" ?callStack)

This is really useful, thanks !! I'm in favor of adding this somewhere that it is easy to use.

googleson78 commented 4 months ago

I think the pattern proposed by @phadej would also be super helpful

tomjaguarpaw commented 4 months ago

It's unclear to me why TODO -> TODO is better than _ -> todo. Can someone explain?

MangoIV commented 4 months ago

Matching on TODO instead of _ is probably in the same way better as error "this is unimplemented" is better than undefined

I don't expect much improvement over _ -> todo though, you are right...

googleson78 commented 4 months ago

Ah, I guess from a technical perspective, it's not super useful - I was imagining some world where we had https://github.com/ghc-proposals/ghc-proposals/pull/454 accepted and implemented, where it would indeed be very much technically useful. The one point is that it's somewhat easier to grep for than _.

I personally feel it's more useful from a "human" perspective, in that it clearly indicates intent, compared to _, which might just be what's intended. This is of course also easily solvable by instead using _TODO.

My main point for adding it instead would be that if we're making a change to add todo, we might as well add the pattern (instead), although I haven't thought too in depth whether it would be easier or harder to use.

Bodigrim commented 3 months ago

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?

The only two additions to Prelude during my CLC term (which is from the autumn of 2021) are re-exporting liftA2 and foldl', no new functions at all.


One non-obvious effect of todo :: a in Prelude is that GHC will happily suggest it to fill each and every _ hole. This needs special treatment in the compiler, similar to undefined.

sergv commented 3 months ago

Regarding adding warnings to undefined, it will trigger completely legitimate uses of undefined for cases like Storable typeclass: its sizeOf method is typically supplied with undefined and doesn't evaluate it, throwing warnings in such situations will just create noise https://hackage.haskell.org/package/base-4.19.1.0/docs/Foreign-Storable.html#v:sizeOf. It's an artifacts before Proxy was widespread yet it's still in use.

hasufell commented 3 months ago

it will trigger completely legitimate uses [...] throwing warnings in such situations will just create noise

The same is true for head and tail, is it not?

mixphix commented 3 months ago

I strongly disapprove of adding this function to base.


@MangoIV

You mention in your original post that the todo package already exists. The benefits of having a package outside of base to host this minuscule "function", if it can be so called, are as @thielema says above:

By removing the todo dependency, you know for sure, that your code is free of todos.

This is impossible should todo be exported from base, eliminating any such guarantee.

just for doing a quick debugging todo, it doesn't seem worth adding an alternative Prelude or even add a dependency just for the sake of this

"Just for doing a quick debugging todo, it doesn't seem worth adding this function to the standard Prelude just for the sake of this"...

Now, let's go over the reasons you mention for adding this function to base.

has a warning annotation "todo remains in code" in group x-todo so that it can be used with WError for e.g. CI

Typed holes are errors by default and are "shorter to type" (yet it seems to me you don't actually have a problem with typing a lot). If the purpose is to throw errors when using -Werror, why not just use typed holes? You state that

they are, by default, errors, not warnings

so I don't see what the problem is here; you then complain that

they are very noisy (by default they produce a lot of output)

So what? There's lots of information GHC can tell you about with a typed hole that you'd be obscuring with a simple message. If you're actually, completely, 100% serious that this is a real problem you have that's causing you pain, raise an issue to shorten the error messages at the Haskell Error Index. I'm sure they'll welcome and appreciate your efforts.

So you’re proposing we’re penalising Werror users? Inconsiderate of the breakage that will cause for them? Of course there’s always a decision to use a certain language feature, why would we punish people for doing so?

I... am baffled. Moving on:

has the same type as undefined

This sounds like an opportunity to add a warning to undefined; in fact you mention that

the exact same implementation has been part of Prelude and battle tested for years now in the form of undefined

but you complain that

the word undefined doesn’t clearly say “this mustn’t stay in code”.

If it shouldn't stay in the code, then why bother putting it there in the first place? Ah, you say that it

is to be used whenever there is a place in the code where we want to temporary fill holes

so I assume you want to compile code that you have not yet written -- code that, as of the time of compilation, is undefined?

Alternatively, leave the hole there and bask in the free type information and refinements the glorious Glasgow Haskell Compiler provides you. Or, use error -- oh, how dreadfully long of a five-letter word! Seems simple to me -- unless I'm missing something extremely obvious.

Let's move on to why you think this would be a good idea to add to Prelude.

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

You are making the grand assumption that people would use it even if it was in base already.

external tools are external tools, they require additional installation, they don't ship with GHC, they are not supported first class by GHC, with the advent of the {-# WARNING -#} we have first class support for these kinds of things that don't add any additional overhead

None of the alternatives you mention previously (error, undefined, typed holes) require any additional overhead. While @rhendric and @thielema suggest HLint and other alternatives, base already provides tools that seem to cover your use cases effectively.

error and undefined don't have WARNINGs 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

I don't think adding warnings to error is a good idea, but there is community precedent of adding a warning to undefined (see Relude, Protolude). I can assure you that the cost of adding a warning to undefined would be smaller than adding a redundancy (and I quote, "there's nothing new here, really"), judging by the number of others in this thread who are suggesting it as well.

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.

I don't know what kind of setup you're using, but reloading my own language server takes exactly one keybind. Modifying .cabal files is just a normal part of my day as a Haskell developer; I fail to understand how this is a cataclysmic event to be avoided at all costs.

Given that the todo package has 2,500 total downloads, I'd say that the number of times people want to fail to write code while writing their code is rather small.


@Kleidukos

Dependencies are not free, but a dependency consisting of only three "functions" is certainly very close. As the maintainer of a package manager I would have thought you would be eager to modularize.

One more dependency to vet is one more maintainer to trust and, should things go wrong, support.

And yet, the world still turns...


@tbidne

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).

With a tool as powerful as Hoogle I find appeals to discoverability laughable. Perhaps in this case the type is of no help: but neither is the implementation, so what's the use?


@Profpatsch

What a lovely example of this expression in the wild. Have you considered offloading the maintenance to an existing implementation?


I repeat myself:

"Pedagogical value" is not a good reason to add contrived functions to base. It is, however, a great reason to add documentation to base. The library is first and foremost a set of utilities for writing bigger programs, and is not necessarily the best home for a function with ambiguous names and purposes to reside. link

base is not a convenience package. It is a package providing the absolute fundamentals for writing a program. It's in the name. It is not ideal that every library and application has to depend on base directly. link

ekmett commented 3 months ago

It's unclear to me why TODO -> TODO is better than _ -> todo. Can someone explain?

To me, _ -> ... something indicates that its actually okay that I'm omitting these cases. I don't find it unless I remember to put a TODO on the right of the -> and it kind of indicates to me that the pattern part is done, just that case needs to be handled, whereas TODO -> ... seems to indicate I still have thinking to do about the pattern. I wonder if it wouldn't be better to make the pattern error out if considered at all by using a view pattern. e.g. something like:

pattern TODO :: forall (r :: RuntimeRep) (a :: TYPE r). HasCallStack => () => a
pattern TODO <- (raise# (errorCallWithCallStackException "This code was not yet implemented: TODO" ?callStack)
 -> _unused) where
  TODO =  raise# (errorCallWithCallStackException "This code was not yet implemented: TODO" ?callStack)
{-# COMPLETE TODO #-}
{-# WARNING TODO "'TODO' (undefined code) remains in code" #-}
silky commented 3 months ago

I agree with @mixphix

It's a nice function; it's handy to have in your toolbox in big systems, but I'd not be overjoyed to find it in base; just as I haven't been overoyed to find it in in Elm (of course the situation there is worse, but it reminds me of that.)

Personally, I just grep for 'undefined' and it works fine for me.

I agree with the theory that base should aim to be a bit minimal, and while I think there's certainly value and interest in having this function, feels like more of an opt-in; i.e. in order to get good support for it you'd need appropriate steps in CI/etc anyway, so I think it's fine that it just becomes a blog post/best practice in that way.

MangoIV commented 3 months ago

@mixphix

I can assure you that the cost of adding a warning to undefined would be smaller than adding a redundancy

I think pretty strong arguments have been named why the opposite is the case, so why do you think the redundancy would be an issue?

With a tool as powerful as Hoogle I find appeals to discoverability laughable.

this is just not realistic. I learned about maybeToList after programming Haskell for over one year, of course there’s a discoverability issue and be it to discover best practices in general. Of course that can be fixed in other ways but that’s a bit out of scope to discuss here.

Typed holes are errors by default and are "shorter to type" (yet it seems to me you don't actually have a problem with typing a lot). If the purpose is to throw errors when using -Werror, why not just use typed holes? You state that

because you don’t use werror in your editor but only later, in CI. What do you think about the fact that typed holes are by default very slow?

I don't know what kind of setup you're using, but reloading my own language server takes exactly one keybind

it’s not incremental though, it takes a lot of time, even on small projects. (And yes, I consider multiple seconds a long time in that case )

Given that the todo package has 2,500 total downloads, I'd say that the number of times people want to fail to write code while writing their code is rather small.

this is not a proof that not many people want it, it can also be a testimonial of it not being discoverable enough. I’d guess it is the latter, not the former, but I wont claim that because I don’t have evidence.

You are making the grand assumption that people would use it even if it was in base already.

Yes I do and I can’t see why this is an absurd thought, I purposefully linked the rusts standard library which has this function (even in two variations!)

hasufell commented 3 months ago

it’s not incremental though, it takes a lot of time, even on small projects. (And yes, I consider multiple seconds a long time in that case )

I'm sorry, but this is starting to get bizarre.

Are you suggesting we add stuff to base, because after editing your cabal file, HLS doesn't start fast enough??

There are arguments to add todo to base. But this is not one of them.