Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the todo function #260

Closed
MangoIV opened this issue Mar 7, 2024 · 154 comments
Closed

Add the todo function #260

MangoIV opened this issue Mar 7, 2024 · 154 comments
Labels
declined Declined by CLC vote

Comments

@MangoIV
Copy link

MangoIV commented Mar 7, 2024

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

  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
  1. 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
  1. 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 group x-todo so that it can be used with WError 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 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
  • 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; even though it is debatable whether you should, people use undefined 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

rendered docs without expanded Examples
rendered docs with expanded Examples

some more screenshots

  • how it looks, loading a module using todo into GHCi:
    screenshot of terminal showing how it looks to load a module using the todo function

  • how it looks when trying to load a module using todo with {-# OPTIONS_GHC -Werror=x-todo #-} into GHCi:
    image

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:

  • 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 namespace Develop 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 only todo 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 the todo family of functions as well as the trace* family of functions
@phadej
Copy link

phadej commented Mar 7, 2024

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 👍 the proposal for that.

@thielema
Copy link

thielema commented Mar 7, 2024 via email

@treeowl
Copy link

treeowl commented Mar 7, 2024

Why raise# and not just throw?

@phadej
Copy link

phadej commented Mar 7, 2024

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
Copy link
Author

MangoIV commented Mar 7, 2024

@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
Copy link

thielema commented Mar 7, 2024 via email

@rhendric
Copy link

rhendric commented Mar 7, 2024

Why base and not a tiny package?

@Kleidukos
Copy link
Member

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
Copy link

jappeace commented Mar 7, 2024

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

@rhendric
Copy link

rhendric commented Mar 7, 2024

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
Copy link
Author

MangoIV commented Mar 7, 2024

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.

@thielema
Copy link

thielema commented Mar 7, 2024 via email

@tomjaguarpaw
Copy link
Member

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
Copy link

TeofilC commented Mar 7, 2024

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
Copy link

MorrowM commented Mar 7, 2024

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
Copy link

thielema commented Mar 7, 2024 via email

@tbidne
Copy link

tbidne commented Mar 7, 2024

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 ™️ . 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
Copy link

rhendric commented Mar 7, 2024

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
Copy link
Collaborator

Bodigrim commented Mar 7, 2024

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
Copy link
Member

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
Copy link
Author

MangoIV commented Mar 7, 2024

@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
Copy link
Member

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

In what way are typed holes slow?

@treeowl
Copy link

treeowl commented Mar 7, 2024

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

@Bodigrim
Copy link
Collaborator

Bodigrim commented Mar 8, 2024

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
Copy link

ocramz commented Mar 8, 2024

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
Copy link
Member

hasufell commented Mar 8, 2024

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

@MangoIV
Copy link
Author

MangoIV commented Mar 8, 2024

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
Copy link
Author

MangoIV commented Mar 8, 2024

@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
Copy link
Member

hasufell commented Mar 8, 2024

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
Copy link
Member

hasufell commented Mar 8, 2024

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

@Bodigrim Bodigrim removed the awaits-MR No GHC MR (https://gitlab.haskell.org/ghc/ghc/-/merge_requests) has been raised yet label Aug 5, 2024
@Bodigrim
Copy link
Collaborator

Bodigrim commented Aug 5, 2024

Dear CLC members, let's vote on the proposal to add a new module Develop.Placeholder with two entities todo and pattern TODO + a dedicated exception type TodoException. Here todo is essentially an enhanced version of undefined to use in your code during development and pattern TODO is even more powerful because it can be used to silence incomplete pattern matching warnings. Both entities have {-# WARNING in "x-todo" #-} to prevent you from leaving them in your code unnoticed. The MR is available at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12934/diffs.

@tomjaguarpaw @hasufell @mixphix @angerman @parsonsmatt @velveteer


+1 from me, I think it strikes right balance.

@Bodigrim Bodigrim added the vote-in-progress The CLC vote is in progress label Aug 5, 2024
@mixphix
Copy link
Collaborator

mixphix commented Aug 6, 2024

-1, typed holes provide more information, only slightly slow down the language server, and are shorter to type.

@hasufell
Copy link
Member

hasufell commented Aug 6, 2024

I think this is a well put together proposal. And the main reason I find it interesting is pattern TODO, which I don't think can be achieved with current methods. If it wasn't for the pattern, I'd give this a -1 without further thought... after all, you can already write _todo.

And yet, I'm unsure how this fits in the existing realm of typed holes (should it maybe be an implicit typed hole with -fdefer-typed-holes enabled for the given expression? Why can't we have typed pattern holes? Is it just because _ is reserved?). As already pointed out: typed holes carry more information. But todo seems marginally more intuitive/discoverable. Maybe that's an issue of documentation/teaching/etc. Will adding another way improve status quo or make it more messy? I'm not confident to answer.

I also realized both methods suffer from frequent "Ambiguous type variable" errors. Try to use length todo or length _xs. So the ergonomics are still problematic and I'm not sure this is going to be that much fun for beginners anyway.

https://gitlab.haskell.org/ghc/ghc/-/issues/25115#note_577782 suggests that this type of functionality needs ad-hoc logic in GHC too. So I'm wondering whether this should be turned into a language extension instead or just become a part of existing typed holes.

Remember: once we added it to base, it can't really be removed easily.

As such I suggest to give this more iterations, maybe add the current implementation to ghc-experimental and let it sit for a while.


-1 (for now)

@MangoIV
Copy link
Author

MangoIV commented Aug 6, 2024

thank you for your elaboration @hasufell. @BinderDavid has already made a nice suggestion wrt this idea:

So I'm wondering whether this should be turned into a language extension instead or just become a part of existing typed holes.

This is a link to the corresponding comment

As an alternative it might be possible to special case a named typed hole _todo in GHC which would then by default not have these downsides but the semantics you want? Upsides would be that there isn't an additional definition in base and it is almost impossible to break any code since published code usually doesn't contain typed holes.

wrt this:

https://gitlab.haskell.org/ghc/ghc/-/issues/25115#note_577782 suggests that this type of functionality needs ad-hoc logic in GHC too. So I'm wondering whether this should be turned into a language extension instead or just become a part of existing typed holes.

whether adding custom logic is necessary is not too clear to me, see this: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12934#note_578239

@tomjaguarpaw
Copy link
Member

-1


I would like to credit @MangoIV for putting together an excellent proposal with a very clear motivation. My personal bar for adding new things base is very high, and this proposal nearly clears the bar. In particular I find the claim that importing a separate feature for this functionality defeats the point: it needs to be easily accessible.

However, the proposal falls down in one major respect: the claim that typed holes are not good enough. @merijn summarizes my thoughts about this precisely:

that's why there is a flag to turn typed holes into warnings. The entire point of that is that you can set that flag while developing. And then, when you want to go to production you remove that flag and any untyped holes you missed/forgot to implement turn into errors.

Overall, enabling 1 single flag during development and using typed holes, seems a lot simpler than this proposal. Which seems a lot of work AND a modification for base to get a less robust solution to a problem that's already solved?

I do not currently see why it's better to use

import Develop.Placeholder (todo)
...
    TODO -> todo

than

{-# OPTIONS_GHC -fdefer-typed-holes -Wwarn=typed-holes #-}
...
    _ -> _

The claim is that "typed holes are slow". I asked in what way they are slow and received no response. Therefore I have no evidence on which to base a judgement that this proposal is better than typed holes. (FWIW, if typed holes really are slow then that seems like something that should be fixed in GHC rather than by adding a library workaround.)

@hasufell
Copy link
Member

hasufell commented Aug 8, 2024

I do not currently see why it's better to use

Those things aren't really the same.

TODO the pattern is not expressible with typed holes afais.

Compare with

case foo of
    Constr1 -> True
    Constr2 -> False
    TODO -> f == x
    _ -> False

Here the _ is a regular catch-all, but you're not yet sure which cases it should match (or which constructors the type shall have)!

@tomjaguarpaw
Copy link
Member

I agree they are not the same. I did not say they were the same! FWIW you can do

{-# OPTIONS_GHC -fdefer-typed-holes -Wwarn=typed-holes -Wwarn=overlapping-patterns #-}

case foo of
    Constr1 -> True
    Constr2 -> False
    _TODO -> f == x
    _ -> False

@L0neGamer
Copy link

How do typed holes work in the following sort of case vs TODO?

case foo of
  Constr1 -> True
  Constr2 -> False
  TODO -> f == x

There isn't a redundant pattern match so that wouldn't be warned against.

I would contend that TODO is more ergonomic than typed holes in development because with typed holes you have to remember to turn on and off the related warnings and such, meaning forgetting to do so results in bad code being left in, meanwhile a todo will continue warning unless you're explicitly turning it off, in which case you accept the consequences there.

@tbidne
Copy link

tbidne commented Aug 8, 2024

Maybe I'm just unreasonably lazy, but imo shuffling warnings is pretty unergonomic. If adding/removing warnings to modules/.cabal files is part of a dev tool's workflow, I'm just not going to bother. I'd rather stick to undefined at that point.

@sullyj3
Copy link

sullyj3 commented Aug 8, 2024

It really does seem like it would be ideal for this to be in Prelude, to eliminate trivial inconveniences impeding its use.

It makes sense that that shouldn't be done yet for backwards compat reasons, but getting it into base could be a step towards that one day.

So it seems worth also thinking about about how

{-# OPTIONS_GHC -fdefer-typed-holes -Wwarn=typed-holes #-}
...
    _ -> _

might one day compare against

    TODO -> todo

(ie, no import required)

@endgame
Copy link

endgame commented Aug 8, 2024

The claim is that "typed holes are slow". I asked in what way they are slow and received no response. Therefore I have no evidence on which to base a judgement that this proposal is better than typed holes. (FWIW, if typed holes really are slow then that seems like something that should be fixed in GHC rather than by adding a library workaround.)

IME it's not the holes themselves, but GHC's attempts to find valid hole fits that what cause the slowdown. On codebases which use advanced features, I almost always set -fno-show-valid-hole-fits. This seems difficult to fundamentally fix.

@MangoIV
Copy link
Author

MangoIV commented Aug 8, 2024

GHC's attempts to find valid hole fits

Exactly this. GHC finding valid hole fits or valid refinement holes is really slow, depending on the code base. IME they’re slow in almost all cases except the trivial ones.

@tomjaguarpaw
Copy link
Member

imo shuffling warnings is pretty unergonomic

I agree!

GHC's attempts to find valid hole fits that what cause the slowdown. On codebases which use advanced features, I almost always set -fno-show-valid-hole-fits

I see, so there is a mitigation.

These are all fair points against the typed holes alternative, but they should have been made before the vote was called. I asked five months ago.

@MangoIV
Copy link
Author

MangoIV commented Aug 8, 2024

I asked 5 months ago

Please excuse me, @tomjaguarpaw, this is obviously on me, I should have answered this on time, I might have overlooked or forgotten to come back to this question.

@merijn
Copy link

merijn commented Aug 8, 2024

@tbidne I don't think that's unreasonably lazy, but I would like to point out that you can use cabal.project.local to add (local) overrides such as GHC flags that get appended to cabal's flags.

So, the way I've been using it for years now is to append -fdefer-typed-holes and -fwarn-typed-holes (or whatever the new flag name is) in my cabal.project.local so that locally for development it "Just Works", with zero risk of those flags making it into, e.g., my CI pipeline (as cabal.project.local isn't and shouldn't be tracked in version control).

@tomjaguarpaw
Copy link
Member

tomjaguarpaw commented Aug 8, 2024

It's OK @MangoIV, you don't have anything to apologise for. It's your proposal and you're welcome to conduct it as you wish. There was plenty of discussion of typed holes. My question about slowness was only one component of that. When the vote was called I reread the entire discussion and I was not convinced by the arguments that the proposal was sufficiently better than typed holes to warrant inclusion in base (I have a very high bar).

Still, I commend you for putting together an excellent proposal and I would be pleased to see someone put it into a stand-alone package. As I said, I take the point that having it in a separate package somewhat defeats the utility, but nonetheless I think it would be worth trialling "in the wild". At the very least I would encourage maintainers of "alternative preludes" to consider adopting it if it fits with their design ethos. There is clearly a lot of scope for improving the ergonomics in this area.

@googleson78
Copy link
Contributor

Maybe I'm just unreasonably lazy, but imo shuffling warnings is pretty unergonomic. If adding/removing warnings to modules/.cabal files is part of a dev tool's workflow, I'm just not going to bother. I'd rather stick to undefined at that point.

+1 to this.

Additionally, adding the -fdefer-typed-holes flag has one (minor?major?) drawback: I can now no longer use typed holes as an error alongside with typed holes as a warning. To me todo serves the purpose of saying "I really do not care about this right now, please just let me get along with my work", whereas typed holes serve the purpose of saying "Hey compiler, what do I need to put here?". It's fine to say you can use typed holes for the "I don't care, leave me alone" case if you turn on the warning, but then you immediately lose the (original?) utility of typed holes as a dialogue with the compiler. My file will now compile, but I lose the ability to reason about whether it compiles because I've pushed some work back (ala todo) or whether it compiles because I forgot to finish my current work (ala _) (without looking through all the warnings manually).

Of course, this is really minor and workflow dependent, but I feel that it essentially encapsulates what I personally perceive the intent and semantics behind these two features to be, as well as pointing out one small limitation with -fdefer-typed-holes, so I feel that it's worth mentioning.

@angerman
Copy link

angerman commented Aug 9, 2024

After re-reading this a few times, I admit, I like the motivation a lot, but I'm just not convinced about including this in base. I'm not sure if the current base-split will help with finding alternative/less-permanent options.

Hence, I'm -1 on this one in its current form. This is another one score for re-installable base, sigh.

@parsonsmatt
Copy link

I'm +1.

This is a nicer todo design than I've ever come up with, and I've invented it in three different work projects independently.

I can see a benefit to it landing in base. If it doesn't land in base, then I think it'd make a wonderful package to incorporate into alternative prelude packages elsewhere on Hackage.

@MangoIV
Copy link
Author

MangoIV commented Aug 10, 2024

As far as I understood that's it then, thank you all for your consideration, discussion and finally the vote.

I hope this can be revamped when making something like base 5 that may introduce a large amount of breakage but really rethink what a modern standard library should look like. Of course, base doesn't become one just by introducing little quality of live improvements like todo but it's quite a good testimony of how cumbersome any substantial improvements, many of which are even somewhat universally agreed on, will be, just for the reason of them creating a lot of churn in the ecosystem. Mind that this is no criticism but instead a pledge for a more glorious future after the base split.

I don't think I will want to introduce something like the special cased _todo. It has several disadvantages compared to this, very simple approach

  • it has low discoverability
  • it would require significant tooling integration (thing e.g. HLS)
  • it would require a non-trivial implementation in GHC
  • it would require more special casing in the compiler, which I generally think is not a good idea
  • it would explode even more, if we were to introduce a special cased pattern that does what TODO would have done
  • it would require at least one GHC proposal

Generally, afaiu, it just increases complexity and shifts responsibility from the core libraries to the compiler which is even less agile.

Again, thank you so much for the discussion and feedback and most importantly, your time.

@MangoIV MangoIV closed this as completed Aug 10, 2024
@endgame
Copy link

endgame commented Aug 11, 2024

o7 @MangoIV

Can I put a big +1 on "make a standalone package"? I think it would be cool to see how it goes in alternate preludes, so there's usage data when the issue is next raised?

@MangoIV
Copy link
Author

MangoIV commented Aug 11, 2024

There is already a standalone package on ekmett/placeholder. It just needs to be published on hackage.

@Bodigrim
Copy link
Collaborator

Additionally, adding the -fdefer-typed-holes flag has one (minor?major?) drawback: I can now no longer use typed holes as an error alongside with typed holes as a warning.

This. That's why I never considered typed holes as an alternative for todo, I'm an ardent adept of hole-driven development, so suppressing the warning is not an option for me. I feel that holes are much more widerly used for HDD than for -fdefer-typed-holes.

@Bodigrim Bodigrim added declined Declined by CLC vote and removed vote-in-progress The CLC vote is in progress labels Aug 14, 2024
@Bodigrim
Copy link
Collaborator

Bodigrim commented Aug 14, 2024

@MangoIV thanks for the proposal, I greatly appreciate all work and effort you put into it.

...it's quite a good testimony of how cumbersome any substantial improvements, many of which are even somewhat universally agreed on, will be, just for the reason of them creating a lot of churn in the ecosystem. Mind that this is no criticism but instead a pledge for a more glorious future after the base split.

I'm not quite sure what you refer by "the base split" and how it would help to evolve base faster. If anything, splitting ghc-internal is to make base more stable by excluding the volatile GHC component.


While the fact that CLC votes against vox populi might be perceived by some as worrying, I strongly believe that it's not. Let me remind that everyone wishing to affect CLC decisions is most welcome to nominate themselves at the next CLC elections (which is January 2025) and help us together make the world a better place to write Haskell.

@tomjaguarpaw
Copy link
Member

There is already a standalone package on ekmett/placeholder. It just needs to be published on hackage.

It has now been published: https://hackage.haskell.org/package/placeholder

Thanks to @MangoIV and @ekmett for making this idea into a package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined Declined by CLC vote
Projects
None yet
Development

No branches or pull requests