date:-5/1 not including forecasted transaction on last day of April

[jolmg]

How to reproduce

$ f="$(mktemp)"
$ cat > "$f" << EOF
~ every friday
  foo      1.00 USD
  bar     -1.00 USD
EOF
$ LEDGER_FILE="$f" hledger register --forecast date:-5/1 foo
2021-04-23                      foo                       1.00 USD      1.00 USD
$ LEDGER_FILE="$f" hledger register --forecast date:-6/1 foo
2021-04-23                      foo                       1.00 USD      1.00 USD
2021-04-30                      foo                       1.00 USD      2.00 USD
2021-05-07                      foo                       1.00 USD      3.00 USD
2021-05-14                      foo                       1.00 USD      4.00 USD
2021-05-21                      foo                       1.00 USD      5.00 USD
2021-05-28                      foo                       1.00 USD      6.00 USD
$ rm "$f"

Expected

The 2021-04-30 entry to appear when querying date:-5/1.

Further notes

• This also affects balance.
• 2021-04-30 is also excluded when using the period expression every 30th day.

Platform

hledger: 1.21
OS: Archlinux

[jolmg]

Specifying the period using --forecast instead of date: as in --forecast=-5/1 works as a workaround:

$ LEDGER_FILE="$f" hledger register --forecast=-5/1 foo
2021-04-23                      foo                       1.00 USD      1.00 USD
2021-04-30                      foo                       1.00 USD      2.00 USD

[simonmichael]

Thanks for the report. It does look a bit odd. More info:

$ cat a.j
~ every friday
  (foo)      1.00 USD
$ hledger -f a.j reg --forecast date:-5/1 --debug=3
requestedspan: DateSpan .. 2021 - 04 - 30
forecastspan flag: Just DateSpan ..
journalEndDate: Nothing
forecastbeginDefault: 2021 - 04 - 22
specifieddates: Just 2021 - 04 - 30
forecastendDefault: 2021 - 04 - 30
forecastspan: DateSpan 2021 - 04 - 22. . 2021 - 04 - 29
alltxnspans:
 [ DateSpan 2021 - 04 - 16. . 2021 - 04 - 22
 , DateSpan 2021 - 04 - 23. . 2021 - 04 - 29
 ]
journalspan: DateSpan 2021 - 04 - 23
pricespan:  DateSpan ..
requestedspan': DateSpan 2021 - 04 - 23. . 2021 - 04 - 30
intervalspans: [ DateSpan 2021 - 04 - 23. . 2021 - 04 - 30 ]
reportspan: DateSpan 2021 - 04 - 23. . 2021 - 04 - 30
beforestartq: Date DateSpan .. 2021 - 04 - 22
2021-04-23                                 (foo)                               1.00 USD      1.00 USD

Related code: hledger:Hledger.Cli.Utils.journalAddForecast

[jolmg]

Thanks for the pointers.

In case it helps, here's the diff of that debug output between using --forecast date:-5/1, and --forecast=-5/1:

--- with-date.debug	2021-04-22 12:36:31.319139142 -0700
+++ with-forecast.debug	2021-04-22 12:36:16.002403999 -0700
@@ -1,15 +1,14 @@
-requestedspan: DateSpan .. 2021 - 04 - 30
-forecastspan flag: Just DateSpan ..
+requestedspan: DateSpan ..
+forecastspan flag: Just DateSpan .. 2021 - 04 - 30
 journalEndDate: Nothing
 forecastbeginDefault: 2021 - 04 - 22
-specifieddates: Just 2021 - 04 - 30
-forecastendDefault: 2021 - 04 - 30
-forecastspan: DateSpan 2021 - 04 - 22. . 2021 - 04 - 29
+forecastspan: DateSpan 2021 - 04 - 22. . 2021 - 04 - 30
 alltxnspans:
  [ DateSpan 2021 - 04 - 16. . 2021 - 04 - 22
  , DateSpan 2021 - 04 - 23. . 2021 - 04 - 29
+ , DateSpan 2021 - 04 - 30. . 2021 - 05 - 06
  ]
-journalspan: DateSpan 2021 - 04 - 23
+journalspan: DateSpan 2021 - 04 - 23. . 2021 - 04 - 30
 pricespan:  DateSpan ..
 requestedspan': DateSpan 2021 - 04 - 23. . 2021 - 04 - 30
 intervalspans: [ DateSpan 2021 - 04 - 23. . 2021 - 04 - 30 ]

[jolmg]

Isn't this it?

reportPeriodLastDay = fmap (addDays (-1)) . queryEndDate False . rsQuery

I can't seem to find the reason for that -1, but it seems to be what causes the difference in forecastspan.

[jolmg]

Or not.. forecastendDefault is right after all, and it's obtained from subtracting one from 5/1. Nevermind.

[simonmichael]

reportPeriodLastDay = fmap (addDays (-1)) . queryEndDate False . rsQuery I can't seem to find the reason for that -1

From what I can remember, it was an intentional slight deviation from the usual "exclusive end date" rule, that seemed necessary for intuitive behaviour / display in this case.

[jolmg]

Sounds like 2 different end-exclusivity mechanisms adding up on each other. One is the one I quoted and the other I guess is a general treatment of DateSpan, including, apparently, its Show instance which ends up causing the display of 2021-04-29 in the debug output.

Since other code might be counting on reportPeriodLastDay's addDays (-1), how about simply avoiding its current use in journalAddForecast?:

diff --git a/hledger/Hledger/Cli/Utils.hs b/hledger/Hledger/Cli/Utils.hs
index 404898a9e..95989f13f 100644
--- a/hledger/Hledger/Cli/Utils.hs
+++ b/hledger/Hledger/Cli/Utils.hs
@@ -59,6 +59,7 @@ import Hledger.Data
 import Hledger.Read
 import Hledger.Reports
 import Hledger.Utils
+import Hledger.Query (queryEndDate)
 import Control.Monad (when)
 
 -- | Standard error message for a bad output format specified with -O/-o.
@@ -133,7 +134,7 @@ journalAddForecast CliOpts{inputopts_=iopts, reportspec_=rspec} j =
     forecastbeginDefault = dbg2 "forecastbeginDefault" $ fromMaybe today mjournalend
 
     -- "They end on or before the specified report end date, or 180 days from today if unspecified."
-    mspecifiedend = dbg2 "specifieddates" $ reportPeriodLastDay rspec
+    mspecifiedend = dbg2 "specifieddates" $ queryEndDate False $ rsQuery rspec
     forecastendDefault = dbg2 "forecastendDefault" $ fromMaybe (addDays 180 today) mspecifiedend
 
     forecastspan = dbg2 "forecastspan" $

I don't know if this would be an acceptable fix for you. If it is, would you like me to submit this as a PR?

[simonmichael]

@jolmg thanks! I'd like to understand the problem and the larger impact of such a change in detail. It will take me a while to make time for it unfortunately. If you want to experiment and run tests, you are welcome to.

[Xitian9]

The ‘shift the end day back by one’ requirement is very error-prone. My inclination would be to change the way DateSpans work so that they are always inclusive of both start and end points. The ‘exclude the endpoint’ convenience can be taken care of at the point where the DateSpan is created, not at the points where it's handled. Thoughts?

[jolmg]

Just to clarify my proposal with the diff above first, the bug can be seen almost completely with this excerpt:

    mspecifiedend = dbg2 "specifieddates" $ reportPeriodLastDay rspec
    forecastendDefault = dbg2 "forecastendDefault" $ fromMaybe (addDays 180 today) mspecifiedend

    forecastspan = dbg2 "forecastspan" $
      spanDefaultsFrom
        (fromMaybe nulldatespan $ dbg2 "forecastspan flag" $ forecast_ ropts)
        (DateSpan (Just forecastbeginDefault) (Just forecastendDefault))

forecast_ ropts gives a DateSpan with the end-date equal to that provided to --forecast=, while forecastendDefault is the end-date provided to date: subtracted by 1 day by reportPeriodLastDay. So, the DateSpan that results from spanDefaultsFrom has an exclusive end-date when using --forecast= and an inclusive end-date when using date:.

Given that DateSpans' second arguments right now are generally treated as exclusive, that they're generally set with values that should be treated as exclusive, and that reportPeriodLastDay is generally treated as an inclusive end-date as its name ("last day") implies, avoiding the use of reportPeriodLastDay to set a DateSpan seems like the most localized and least error-prone fix.

Also, neither forecastendDefault nor mspecifiedend are used for anything else, so I don't see any adverse effects beyond fixing this bug.

Getting back to sharing thoughts on your proposal, correct me if I'm wrong, but I'm understanding you're proposing creating something like this:

mkDateSpan :: Maybe Day -> Maybe Day -> DateSpan
mkDateSpan b e = DateSpan b $ addDays (-1) <$> e

, replacing all uses of the DateSpan constructor in expressions (not patterns) with it, and changing the handling of DateSpan so everything treats the second argument as an inclusive value rather than exclusive.

I don't think that would eliminate the need to sometimes shift end-dates. Some code will want to work with inclusive end-dates and other code with exclusive end-dates.

This example has an inclusive end-date (that it's turning into an exclusive one):

        DateSpan (Just $ minimum dates) (Just $ addDays 1 $ maximum dates)

also this one:

    DateSpan (minimumMay dates) (addDays 1 <$> maximumMay dates)

This one has an exclusive end-date:

    beforestartq = dbg3 "beforestartq" $ dateqtype $ DateSpan Nothing mstart

and this other one, too:

        valuedStart = avalue (DateSpan Nothing historicalDate) startingBalance
...
    historicalDate = minimumMay $ mapMaybe spanStart colspans

At most, you shift the decision from either using or not using addDays 1 to either using mkDateSpan or straight DateSpan. You'd still need to make the choice; you'd just be hiding it.

Also, between having the DateSpan end-dates being inclusive or exclusive, having them be exclusive means that you can take the end-date of one and use it as the start-date of another or vice versa without having the DateSpan overlap. That property is used, for example, in the last 2 excerpts I shared. That means having them be exclusive is not just for convenience on creation (which it isn't always), it's also for convenience at the handlers.

[jolmg]

I've realized that making a written test for this is easier than I initially thought:

diff --git a/hledger/test/forecast.test b/hledger/test/forecast.test
index cf2e3c071..635b9a907 100644
--- a/hledger/test/forecast.test
+++ b/hledger/test/forecast.test
@@ -218,3 +218,18 @@ commodity 1,000.00 USD
 2020-01-28                      (a)                   1,000.00 USD  2,000.00 USD
 >>>2
 >>>=0
+
+# 11. End-date provided to date: includes the day before it like with --forecast=
+hledger -f - reg --forecast date:-20200103
+<<<
+2020-01-01
+  (a)          1.00 USD
+
+~ every day
+  (a)          1.00 USD
+
+>>>
+2020-01-01                      (a)                       1.00 USD      1.00 USD
+2020-01-02                      (a)                       1.00 USD      2.00 USD
+>>>2
+>>>=0

The test fails on master right now and succeeds after applying the provided patch. No other test fails.

[simonmichael]

@jolmg, thanks. I have not followed every detail of the later posts, but I believe this was fixed in 1.22.2 (cf #1633) and we can close this ?

[simonmichael]

You might also have thoughts on #1648.

[Xitian9]

Yes, I believe we can close this.