1.22.1: breaks auto-deduced amounts of forecasted balance assertions

[nobodyinperson]

Hi,

hledger-1.22.1 introduces a regression preventing auto-deduced amounts of forecasted balance assertions of being calculated.

How To Reproduce

Take this log.hledger file:

2021-09-01   Normal Balance Assertion Works
    Checking   = -60
    Costs

~ 2021-10-01       explicit forecasted assertion
    Checking      = -100
    Costs             40

~ 2021-10-02       auto-deduced forecasted assertion
    Checking      = -100
    Costs

# Works with v1.22
➤  hledger-1.22 reg -f log.hledger --forecast 
2021-09-01 Normal Balance Assertion Works          Checking                  -60           -60
                                                   Costs                      60             0
2021-10-01 explicit forecasted assertion           Checking                  -40           -40
                                                   Costs                      40             0
2021-10-02 auto-deduced forecasted assertion       Checking                    0             0
                                                   Costs                       0             0
# Doesn't work with v1.22.1
➤  hledger-1.22.1 reg -f log.hledger --forecast
hledger-1.22.1: "" (lines 1-1)
could not balance this transaction:
can't have more than one real posting with no amount
(remember to put two or more spaces between account and amount)
2021-10-02 auto-deduced forecasted assertion
    ; generated-transaction: ~ 2021-10-02
    Checking                 = -100
    Costs

CallStack (from HasCallStack):
  error, called at ./Hledger/Cli/Utils.hs:125:27 in hledger-1.22.1-inplace:Hledger.Cli.Utils

Is this intended? Auto-deduced amounts for forecasted balance assertions are really handy to say e.g. ”I'll move all of the money from that account to another one at that time”.

Platform: up-to-date Manjaro x86_64 with hledger 1.22 and 1.22.1

[simonmichael]

Ack. It seems we broke this use case with

• Fixes: Auto posting rules now also see inferred amounts, not just explicit amounts.
  (Auto-Postings don't work on auto-deduced remaining amounts of periodic transactions #1412)

Thanks for the report! You have earned the https://hledger.org/regressionbounty.

[Xitian9]

sigh I'll look into it. I seem to be doing great damage to hledger project finances.

[simonmichael]

LOL, it's money well spent! :-) Thanks for looking into it.

FWIW I'm feeling cautiously optimistic on quality:

• as long as we keep adding tests and don't let our changes outrun them I think the tests must be, and are, gaining ground
• meanwhile there's no other choice but to wade through the breakages, they are to be expected
• we'll keep trying to ship a regression-free version of every major release (so that we never have to say "avoid BADVERSION", Debian doesn't ship with BADVERSION, etc.)
• catching regressions early is something worth spending project funds on, I will adjust bounty amounts when needed
• if it becomes necessary, we might have to set expectations: "all features described in [manual] and covered by [these test suites] are expected to work. If you find other cases/combinations not working optimally, please report and we'll try to improve them in next release."

[simonmichael]

PS and thinking aloud, if it becomes the norm to see frequent bounty money flowing to bug reporters and none to the hard-working bug fixers, that might become a problem. There could be a fixed bounty budget, lasting one release cycle, and any unclaimed is distributed to release contributors at the end of the cycle..

[Xitian9]

Okay, the problem is specifically with balance assignments in forecast transactions. While this seems like a weird feature to me (making statements about what balances should be in the future seems likely to break), I can understand that in some restricted cases this may be useful. In @nobodyinperson's example above, forecast transactions are basically being used as a normal transaction which is only included in the journal when given the --forecast option.

Here's a summary:

• Forecast transactions are not added during the normal journal finalisation process, but as an extra post-processing step afterwards.
• In order for balance assignments to be calculated, they need access to the balancing state of the journal up to this point.
• Previously this was handled by adding the forecast transactions, then re-balancing the whole journal in order to properly assess assertions.
• In order to fix Auto-Postings don't work on auto-deduced remaining amounts of periodic transactions #1412, it was realised that two balancing passes were needed: one to get any inferred prices, and another to check the balances after auto-postings were added.
• 2da0e67 tried to be more efficient by just balancing the forecast transactions, rather than the whole journal. This worked well except when balance assignments were used, as we have lost access to the overall balancing state by not re-assessing the whole journal.

So a proper solution using the most obvious method seems to require four different balancing passes over the whole journal:

1. Balance all transactions (ignoring balance assertions/assignments) to infer prices
2. Add auto transactions, and then balance all transactions.
3. Add forecast transactions, and then balance the whole journal to infer prices on these.
4. add auto transactions on the forecast transactions, and then balance all transactions.

This is not only inefficient, but also error-prone. We need to make sure the proper balancing procedure is followed in all places where it has to happen (at least two, currently), and any bugs in one will not necessarily be fixed in the other (which is basically what we're seeing here).

My proposed fix is to change the structure so that forecast transactions are added during the journal finalisation pass. This will require moving forecast_ to InputOpts, as it will now be needed during journal parsing. The overall flow would then be unified, and only require two balancing passes:

1. Add forecast transactions.
2. Balance all transactions (ignoring balance assertions/assignments) to infer prices
3. Add auto transactions, and then balance all transactions.

Any objections?

[simonmichael]

Four passes sounds like too many. Yes adding forecast transactions earlier so they can be checked within journalFinalise's usual two passes sounds good. I don't remember why they are not there, I think it was just implementation convenience as they were added later.

[Xitian9]

I'm sure you'll love to hear this. There is precisely one place where forecast_ is used that does not have access to InputOpts: in accountTransactionsReport, where it needs to check whether we've called --forecast to determine whether to exclude future transactions. Since we were just discussing this the other day, here's another data point to decide whether to keep this feature.

[simonmichael]

Aha.. I'm fine with dropping aregister's hiding of future txns, let it work like other commands for now.

[Xitian9]

Second issue: hledger-ui allows you to toggle viewing forecast transactions. Previously this just required running the appropriate report with an updated ReportOpts. If we make this change, it will require re-reading and re-parsing the entire journal.

I guess this doesn't actually differ too much from the status quo, since we have to rebalance the entire journal (twice) anyway. But it is something to note.

[Xitian9]

Update: Generating the default half-a-year's worth of forecast transactions and toggling visibility based on the flag seems like a reasonable option, and actually what is currently done. I'll just put a note here for myself, as I'm likely to mess up to logic here if I'm not careful.