haskell / core-libraries-committee

95 stars 16 forks source link

Add functions traceWith, traceShowWith to Debug.Trace #36

Closed ChickenProp closed 2 years ago

ChickenProp commented 2 years ago

(Historical note: originally this used On instead of With, but consensus seems to prefer With.)

These could be implemented simply as

traceWith :: (a -> String) -> a -> a
traceWith f a = trace (f a) a

traceShowWith :: Show b => (a -> b) -> a -> a
traceShowWith f a = traceShow (f a) a

I'd frequently find these functions useful when debugging, to give me just a small part of the large data structure being produced. But since I don't keep Debug.Trace imported long-term, I'm not going to have these defined as helper functions anywhere, so I basically just end up writing the definition inline - where I want traceShowWith f a I'd write (\x -> traceShow (f x) x) a which is awkward and obscures the actual code I'm trying to debug.

(I think flip traceShow <*> f is the same, but I wouldn't be confident I've remembered it correctly - and when I'm debugging, the last thing I want is to be wondering whether my debugging functions are doing what they should.)

This is admittedly only a mild quality-of-life improvement. But I think it's low cost. There's potential breakage because it adds exports, but I'd strongly expect there to be little-or-none in practice, and it's easy to fix where it does show up.

ETA: @noughtmare points out we'd also want to do this for traceEvent.

JakobBruenker commented 2 years ago

There's potential breakage because it adds exports

For what it's worth, https://hackage-search.serokell.io/ shows that neither name is defined anywhere in hackage.

andreasabel commented 2 years ago

With traceOn I associate code that traces if a specific condition is fulfilled. I'd like traceWith better. Btw. something like (1)

traceWith :: (a -> String) -> a -> a
traceWith f a = trace (f a) a

would trivialize traceShow to traceWith show.

https://hackage-search.serokell.io/?q=traceWith lists exactly (1) as part of DebugTraceHelpers-0.12. https://hackage-search.serokell.io/?q=traceShowWith has traceShowWith with the suggested implementation. ( Code here: https://hackage-search.serokell.io/viewfile/clash-lib-1.4.6/src/Clash/Debug.hs )

So, crowd intelligence supports my choice of names.

ChickenProp commented 2 years ago

If I was looking for these functions, I'd expect them to be named On by analogy with sortOn, groupOn and nubOn; but then, there's no traceBy and they can't be implemented using the on function, so maybe it's a weak analogy. With would certainly be the next choice - Data.List.NoneEmpty uses sortWith and group[All]With[1] (plus GHC.Exts defines sortWith and a different groupWith), but I've previously grumped about that state of affairs and I'd feel like using With here is getting more confusing, not less.

That said, I'm not inclined to fight about the names, if people prefer With then so be it.

andreasabel commented 2 years ago

Language is what people speak. (Esperanto is the exception here, but it did not really catch on, did it?, despite best efforts.)

So, in picking a name for a function the first thing I ask myself is what name people have used before... (I.e., going on a field study...)

treeowl commented 2 years ago

@andreasabel, Esperanto has done quite well as conlangs go. I have a friend who speaks it with her children, as well as Esperanto-speaking friends.

andreasabel commented 2 years ago

@andreasabel, Esperanto has done quite well as conlangs go. I have a friend who speaks it with her children, as well as Esperanto-speaking friends.

Haha, should have abstained from that comment...

ChickenProp commented 2 years ago

Language is what people speak.

I will note that I am also a person. I haven't published anything using On, but I still "speak the language" where this is called On.

Like, it seems reasonable to say "it looks like more people would expect With than On here", those search results are definitely evidence for that. But I don't think we're at the point of "it's clear that a clear majority of people will expect these to be With". Not that you said we were, but it seemed worth making that distinction explicitly.

(And to repeat, I'm not really invested in this. I think the question of which to use and why is interesting but not very important; I kind of enjoy talking about it, but if there's a risk that talking about it will derail things I'd rather stop.)

ChickenProp commented 2 years ago

CLC, can you vote on this please?

tomjaguarpaw commented 2 years ago

I like the idea of doing something along these lines. Taking the name traceWith by way of example, I think we have

trace s = traceWith (const s)
traceId = traceWith id
traceShow s = traceWith (const (show s))
traceShowId = traceWith show

-- Additionally
traceShowWith f = traceWith (show . f)

To me those read more nicely than the On versions:

trace s = traceOn (const s)
traceId = traceOn id
traceShow s = traceOn (const (show s))
traceShowId = traceOn show

-- Additionally
traceShowOn f = traceOn (show . f)

I can imagine replacing all my uses of the existing Debug.Trace above with simple applications of traceWith! That makes me think it is worthwhile adding.

The last equation makes me think we shouldn't add traceShowOn/With because the delta from traceOn/With (show . f) is very small and I don't see a strong reason to privilege Show here.

Bodigrim commented 2 years ago

@ChickenProp sorry for dropping the ball. I've just posted this link on Reddit, hoping for a wider community discussion of the proposal.

noughtmare commented 2 years ago

If this goes through, I would also expect a traceEventOn/traceEventWith.

vlatkoB commented 2 years ago

Maybe traceAs would be clear that we want to show it As something else, traceAs :: (a -> String) -> a -> a? And traceOn would be traceOn :: (a -> Bool) -> a -> a, a conditional trace?

chshersh commented 2 years ago

relude has the traceShowWith function in the same spirit as proposed here.

If you're interested in more context behind introducing this function, you can check the original discussion:

tomjaguarpaw commented 2 years ago

Thanks @chshersh. Did you consider a function of type (a -> String) -> a -> a? To me that seems like the more important one to add. Repeating what I said above, traceShowWith f = traceWith (show . f) but to get traceWith in terms of traceShowWith you have to do something like

newtype ShowStringDirect = ShowStringDirect String

instance Show ShowStringDirect where
    show (ShowStringDirect s) = s

traceWith f = traceWithShow (ShowStringDirect . f)

Unless I've missed something, the relative simplicity of one direction and the relative complexity of the other suggests we should just add a function of type (a -> String) -> a -> a.

chshersh commented 2 years ago

@tomjaguarpaw The person who implemented the PR only wanted the Show version. I don't mind having traceWith in relude with the proposed type signature ๐Ÿ‘๐Ÿป So anyone who cares enough can open a PR ๐Ÿ™‚

tomjaguarpaw commented 2 years ago

OK, thanks.

ChickenProp commented 2 years ago

The last equation makes me think we shouldn't add traceShowOn/With because the delta from traceOn/With (show . f) is very small and I don't see a strong reason to privilege Show here.

This is another thing I'm not inclined to fight about, but I'd expect the vast majority of my uses to use show. Having the extra function feels worth it to me, but I don't really know how to evaluate such questions in general. E.g. I might almost never use it if I typicaly had a prettyprinter imported.

ChickenProp commented 2 years ago

If this goes through, I would also expect a traceEventOn/traceEventWith.

Agree, thanks. I've added this to the top so it doesn't get lost.

tomjaguarpaw commented 2 years ago

I'd expect the vast majority of my uses to use show

Sure, probably mine too! I'm just saying that typing traceWith (show . f) seems so little different from typing traceShowWith f that it doesn't seem worth adding the latter (and frankly I'd have said the same about traceShow if I'd been asked at the time!).

ChickenProp commented 2 years ago

If you're interested in more context behind introducing this function, you can check the original discussion:

I'm kinda surprised the name was chosen to be consistent with sortWith and not sortOn - it looks like even from the first version, relude offered both of those names, but sortOn was reexported from Data.List and sortWith from GHC.Exts. So I'd have expected On to be the version most people turn to by default. I wonder if I'm wrong about that, and that's part of why With feels better to a lot of people?

(But it certainly looks like more people are inclined to go for With, so fair enough - I've edited the original to reflect that.)

chshersh commented 2 years ago

@ChickenProp sortOn and sortWith have different performance characteristics and their implementations are actually different.

tomjaguarpaw commented 2 years ago

Yes, it seems that sortWith is needlessly inefficient. Perhaps they have a reason to not want to memoize the sort key, but if they do they don't say so in the docs!

(sortOn, for comparison)

chshersh commented 2 years ago

@tomjaguarpaw sortWith is not needlessly inefficient. sortOn caches the result of a function and sorts a list of pairs. But if you want to sort e.g. list of records by a single field, you don't need to memorize the result of a field in a separate element of a tuple. In that case, sortWith will be actually faster and it will allocate less memory.

Improving the docs is always great ๐Ÿ™‚ We wrote a big section about all sorting functions in Bind The Gap previously where we described all the differences and explained all the use cases ๐Ÿ˜‰

thielema commented 2 years ago

On Sun, 20 Feb 2022, tomjaguarpaw wrote:

Yes, it seems that sortWith is needlessly inefficient. Perhaps they have a reason to not want to memoize the sort key, but if they do they don't say so in the docs!

(sortOn, for comparison)

OT: There is a memoizing sort on keys in: https://hackage.haskell.org/package/utility-ht-0.0.16/docs/Data-List-Key.html#v:sort

tomjaguarpaw commented 2 years ago

The reason that I prefer to avoid On for this is that there seems to be a pattern of calling something funcOn when there is funcBy and funcOn f = funcBy (relevantComparisononf), e.g.

nubOn f = nubBy ((==) `on` f)
groupOn f = groupBy ((==) `on` f)
sortOn f = sortBy (compare `on` f) (modulo efficiency, also = sortBy (comparing f))

I think we should try very hard to get this name right. We're going to be stuck with it for the entire future of Haskell.

[This is an elaboration of what @ChickenProp observed above]

tomjaguarpaw commented 2 years ago

sortWith is not needlessly inefficient

Aha, I see, thanks. Anyone who submits a documentation PR to explain that will have my gratitude.

georgefst commented 2 years ago

The last equation makes me think we shouldn't add traceShowOn/With because the delta from traceOn/With (show . f) is very small and I don't see a strong reason to privilege Show here.

Speaking from my own experience of a few years of having both of these functions defined in the preludes of some of my projects, traceShowOn is the one I reach for >95% of the time. In practice, when you want to quickly reach for an a -> String function for debugging, show is almost always what you want if it's available.

In an environment with autocompletion (e.g. HLS), entering traceShowWith f can be significantly quicker than traceWith (show . f).

ChickenProp commented 2 years ago

The reason that I prefer to avoid On for this is that there seems to be a pattern of calling something funcOn when there is funcBy and funcOn f = funcBy (relevantComparison `on` f)

Yeah, and I like this pattern. I guess where we differ is it feels to me like traceOn here would fit with it, like an extension rather than a violation? Like, "we haven't yet seen what this pattern does in this situation, it might do this thing or it might do this other thing". Where the way it fits is "funcOn is func, but taking an additional function to use as a projection".

But it's a bit of a stretch, especially since it's "replace completely unrelated value with projection function" rather than simply adding a projection. (It fits better as traceIdOn and traceShowIdOn. Especially traceShowIdOn, where it's projecting to a datatype that satisfies a required constraint. But those are awful names.)

So I think I'd still lean for On, but writing this has made me care even less.

I think we should try very hard to get this name right. We're going to be stuck with it for the entire future of Haskell.

I agree it's worth some effort. Part of why I'm emphasizing so hard that I'm fine either way, is that I think I personally am at risk of bikeshedding. I'm less likely to do that if I've publicly announced that, for all that I can talk for hours about the color of the bikeshed, I don't think the color of the bikeshed is very important. I might still talk about it, but I'm less likely to hold things up.

But I also have a heuristic of "hard decisions don't matter" - obviously not always true, but sometimes, when it's not clear which of two things is better, that's because they're about equally good. My sense is that's the case here. (There are also lots of bad names, I'd look very askance at someone suggesting traceButCallThisFunctionFirst, but I mostly don't expect those to be considered.)

TikhonJelvis commented 2 years ago

Since we already have trace and traceShow, I would expect both traceWith and traceShowWith as a user. traceShowWith is pretty close to traceWith (show . f) and traceShow is pretty close to trace . show, so there's a reasonable case that neither is worth definingโ€”but I don't think there's a reasonable case to define one but not the other.

In practice, I've found traceShow to be pretty useful. I often need to add/remove traces to already busy expressions, and not needing to fiddle around with extra parentheses makes traceShow surprisingly more convenient than trace + show separately. I expect that traceShowWith would be similar.

Bodigrim commented 2 years ago

@ChickenProp could you please prepare a specific proposal for CLC to vote? Would you like to pursue adding traceEventWith?

ChickenProp commented 2 years ago

Yes to With, but I guess I'm not 100% on what you're looking for that's different from what I've currently written. Just an unambiguous "this is the final complete proposal" with all relevant detail?

(If so, to check: is that better to edit into the top post or to put in thread? Better to have traceWith and traceShowWith as separate proposals, or as one monolithic one?)

tomjaguarpaw commented 2 years ago

@ChickenProp I think @Bodigrim might mean "please submit a PR on GitLab". That seems to have been required for other proposals to receive a vote.

ChickenProp commented 2 years ago

Ah, thanks, that makes sense. I confess I was hoping someone else would take it upon themselves to do the busywork :p But I certainly can do so, and probably in the next few days.

(Also, whoops, I misread "Would you like to pursue adding traceEventWith?" as "do you want to go ahead with the With versions of the names?" To the actual question: yes, I think traceEventWith would be good too.)

ChickenProp commented 2 years ago

Created a merge request at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7729.

Bodigrim commented 2 years ago

Shall we add traceEventShowWith? Or is it too much?

TikhonJelvis commented 2 years ago

What problems could including that function cause?

My rule of thumb is that when we have a pattern, we should keep the pattern complete unless we have a specific reason not to. Consistency helps me because I don't have to remember which functions are defined and which ones aren't, I can just learn the pattern and rely on it when I'm iterating on code (or, in this case, when I'm systematically debugging).

ChickenProp commented 2 years ago

I would have added it if there was traceEventShow, and I'd be happy to add them both. But I dunno if they'd be valuable. I don't use the event log much myself so I don't know its usage patterns.

mpickering commented 2 years ago

Any updates from the CLC?

Bodigrim commented 2 years ago

We might add both traceEventShow and traceEventShowWith. Dunno, I never used traceEvent, so do not really know much about its usability. @mpickering you might actually be a person with a right experience. Do you find these helpers useful?

Bodigrim commented 2 years ago

@noughtmare maybe you can chime in on behalf of traceEvent users?

noughtmare commented 2 years ago

I think traceEvent can be used in much the same way as trace it might even be superior, because you can turn it off without recompiling and you get timestamps. So I see no reason not to have consistency between trace and traceEvent.

However, I think it is fine to drop the traceEvent variants, because they are not used as much and traceEvent already doesn't have all the variants that the normal trace has. Also, I think that traceEvent is usually used in more specific ways, like traceEventIO "START <eventName>" and traceEventIO "STOP <eventName>" see the instrumentation section of this blog post.

At least don't let that block this proposal. If people want consistency between trace and traceEvent then that can still be requested via another CLC proposal later.

Bodigrim commented 2 years ago

All right, let's have a vote as is. Dear CLC members, please vote on https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7729/diffs, which adds traceWith, traceShowWith and traceEventWith. CC @cigsender @tomjaguarpaw @emilypi @cgibbard @chessai


+1 from me.

tomjaguarpaw commented 2 years ago

+1


traceWithShow and traceEvent

I think you mean traceShowWith and traceEventWith

Bodigrim commented 2 years ago

Thanks @tomjaguarpaw, updated.

cgibbard commented 2 years ago

+1

mixphix commented 2 years ago

+1

ChickenProp commented 2 years ago

Right now there's four +1s, two yet to vote, and it's been two weeks since the last vote. Is that sufficient?

Bodigrim commented 2 years ago

Right, 4 out of 6 are sufficient. Approved.

chshersh commented 1 year ago

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field Value
Authors @ChickenProp
Status merged
base version 4.18.0.0
Merge Request (MR) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7729
Blocked by nothing
CHANGELOG entry present
Migration guide not needed

Please, let me know if you find any mistakes ๐Ÿ™‚


Something weird happened to this change. There're two PRs that add an entry to CHANGELOG and update the documentation but there's still no entry in the changelog and the current @since annotation is wrong (these functions appeared finally only in base-4.18 while the doc says base-4.17).

@ChickenProp I'm really confused about how this got lost in the mergeageddon ๐Ÿ˜•

ChickenProp commented 1 year ago

Hm, it looks like this is in the changelog now?

I guess part of this confusion is because 4.17 was upcoming when I made 7729 so I assumed it would make it into that, but the branch that finalized 4.17 had already been split off. I'm not sure if there's something I could/should have done to make things easier.