Repository navigation
468a3da broke income statements that use -X, -e and -M #2385
Description
@Xitian9 468a3da broke income statements that use -X, -e and -M:
$ git rev-parse HEAD
468a3da1b70bbcc8499ba398a6904ef86f9eea20
$ cabal run hledger -- incomestatement -f ~/finances/all-years.journal -X CHF -e thismonth -M
Configuration is affected by the following files:
- cabal.project
hledger: Error: journalApplyValuationFromOpts: expected all spans to have an end date
CallStack (from HasCallStack):
error, called at ./Hledger/Reports/ReportOptions.hs:646:11 in hledger-lib-1.42.99-inplace:Hledger.Reports.ReportOptions
Reacted by Simon Michael
Activity
simonmichael commentedon May 11, 2025
Woah, surprising. This is a bug with master, right @basvandijk ?
basvandijk commentedon May 11, 2025
Yes. I did a git bisect which identified 468a3da as the culprit. Interesting that the strict data types cause this error to be thrown.
basvandijk commentedon May 11, 2025
If I trace latestSpanContaining like:
and run it without
StrictDataI get:but with
StrictDataI get:So with
StrictDatalatestSpanContainingis called on the extra day2025-04-02outside the given datespans range.basvandijk commentedon May 11, 2025
Ok going for bed now so I'm leaving my last debugging notes here:
The Day
2025-04-02is returned by the following (censored) invocation of postingDateOrDate2:When trying to show the
ptransactionfield I noticed this comment about co-recursive types. I'm just wondering, are these co-recursive types safe in the context ofStrictData?For the record, I indeed have a transaction on that date but I wonder why it's being processed when it falls outside the specified period (
-b 2025-03 -e 2025-04 -M):basvandijk commentedon May 11, 2025
Another interesting observation about my trace of
latestSpanContainingabove is that the function is not just applied to one Day outside of the specified period but to many whenStrictDatais enabled.Xitian9 commentedon May 12, 2025
I think in this particular case it's safe, as the co-recursive type is wrapped in a
Maybewhich is not itself strict. TheStrictDatawill evaluateptransactionto weak head normal form, which means instead ofptransaction = thunkit will evaluate to eitherptransaction = Nothingorptransaciton = Just thunk. The actual corecursion won't appear until you evaluate one more layer of thunks.basvandijk commentedon May 12, 2025
So the following patch on master fixes the issue but I have no idea why:
basvandijk commentedon May 12, 2025
In case it's useful I attached a stack trace of when
hledger(built from the latest master 00442a6) crashes:hledger-stderr.txt
Xitian9 commentedon May 12, 2025
To be honest, I think the best approach to resolving this is to rewrite the code to avoid the possibility of encountering that error condition in the first place. Looking a bit at the logic, we go through a lot of contortions to work around the fact that 1. DateSpans do not necessarily contain either start or end dates and 2. DateSpans do not necessarily form a partition of time. As a result, we keep adding or subtracting 1 from start or end dates, constructing artificial DateSpans which have certain properties with respect to the ordering, and so on. My guess is there was a subtle bug in all that logic which laziness hid from us and is now being dredged up.
I can gave a go at this, but the general approach I'll take is to replace
DateSpanwith(Day, Day)when we know they must both exist. Maybe even create a new data type representing a partition of time (to replace[DateSpan]) with appropriate functions to access start and end dates.simonmichael commentedon May 13, 2025
What are you thinking of @igs-raiderler ?
fix: Ensure valuation happens after filtration (hledgerorg#2385)
Xitian9 commentedon May 13, 2025
I've found the problem, and have one proposed solution #2387. I'll note that my proposed solution conflicts with the proposed change in #2373, so I think we need some discussion about whether that's the correct approach.
The nub of the problem is as follows. Valuation currently happens before filtration, so we are valuing all postings even if they don't appear in the final report. Since we have a report with an early end date, we are not providing interval end dates out to the end of the journal, and so we encounter this error. Previously this wasn't a problem, because the thunks were never evaluated before being filtered out, but with
StrictDatathey're now being evaluated and encountering the error condition.Xitian9 commentedon May 13, 2025
Other potential solutions:
pamountfield lazy.simonmichael commentedon May 13, 2025
Great debugging info. I haven't been able to make a repro though. Could anyone make a small one ?
I did some more benchmarking of #2381, compiled with ghc 9.10.1 on mac.
Reductions (-) are good:
So while this won't be noticeable to most users, it does seem like a win and worth updating the code for.
simonmichael commentedon May 13, 2025
Ah! Found the repro in #2387.
simonmichael commentedon May 13, 2025
We now have three versions of journalValueAndFilterPostingsWith:
current: filter by amt: and cur:, then do valuation, then other filtering.
pr 2373: do valuation, then filtering.
This is a WIP proposal of mine to fix bug 2371 (filtering breaks boolean queries, caused by 1). It also fixes bug 2385.
pr 2387: do filtering, then valuation.
This is Xitian9's fix for bug 2385, caused by pr 2381. It probably also fixes bug 2371 ?
2 is not finished; I was planning to add pre-valuation origamt: and origcur: queries there, to preserve the old behaviour for people relying on that.
3 does the filtering first, intending to be more efficient, avoiding "we are valuing all postings even if they don't appear in the final report". Have we really been doing wasted work here ? Perhaps laziness was avoiding it ? Perhaps until pr 2381 added StrictData ?
3 changes the long-standing query/valuation order, and I was expecting that to break something. But we were already doing cur: and amt: filtering before valuation, and I guess the other query types aren't affected by valuation. (Or if they are, tests don't cover it ?)
It seems like 3 must also fix bug 2371, in a simpler and better way. I have a question mark only because I don't seem to have a specific test handy.
basvandijk commentedon May 15, 2025
I can confirm #2387 fixes the issue for me.
fix: Ensure valuation happens after filtration (#2385)
;dev: simplify pre-valuation query tests [#2371] [#2385] [#2387]
simonmichael commentedon May 17, 2025
I have merged #2387 which fixes this bug, and some tests confirming that it also fixes #2371.
Now we do all query-filtering before valuation (not just amt: and cur:).
Let's watch out for any breakage or docs that need updating.
Thanks!
;dev: journalValueAndFilterPostingsWith notes [#2371], [#2385]