1.29: Valid entries with equity conversion postings are rejected if hledger can't detect balancing postings

[the-solipsist]

This entry is parsed without issue with hledger bal in v1.28:

2017-12-08 Giordano
    Expenses:Gift:XX-XXXXXXX                 HKD 118.00
    Expenses:Personal:Clothing               HKD 118.00
    Equity:Trading:Currency:INR-HKD:HKD     HKD -236.00
    Equity:Trading:Currency:INR-HKD:INR       ₹2,150.77
    Liabilities:Credit-Card:XXX              ₹-2,150.77

However, this is no longer the case with v1.29+, which throws up the error:

There is not a unique posting which matches the conversion posting pair:
    Equity:Trading:Currency:INR-HKD:INR       ₹2,150.77
    Equity:Trading:Currency:INR-HKD:HKD     HKD -236.00

This is essentially doing a strict --infer-costs check, rather than / in addition to whatever checks the journal parsing code was doing earlier.

This works fine:

2017-12-08 Giordano
    Expenses:Personal:Clothing               HKD 236.00
    Equity:Trading:Currency:INR-HKD:HKD     HKD -236.00
    Equity:Trading:Currency:INR-HKD:INR       ₹2,150.77
    Liabilities:Credit-Card:XXX              ₹-2,150.77
    Expenses:Gift:XX-XXXXXXX                 HKD 118.00
    Expenses:Personal:Clothing              HKD -118.00

[the-solipsist]

Strangely, I don't recall having had an issue with --infer-costs earlier, though the 5-6 problematic entries I still had remaining should have meant that --infer-costs wouldn't work. At any rate, I fixed those entries, and things are working again. So I'm actually thankful for this 'bug'. But given that journal parsing isn't supposed to be this strict, I think it should still qualify as a bug.

[simonmichael]

The problem occurs during this new pass that is done to allow redundant costs + conversion postings: https://github.com/simonmichael/hledger/blob/c2c668e3a73382e928a903c3165e814626f241b5/hledger-lib/Hledger/Read/Common.hs#L334

in addCostsToPostings, when dryrun' is true: https://github.com/simonmichael/hledger/blob/c2c668e3a73382e928a903c3165e814626f241b5/hledger-lib/Hledger/Data/Transaction.hs#L298

If I make that case do nothing instead, it fixes @the-solipsist's example but breaks the three tests starting at costs test 39: https://github.com/simonmichael/hledger/blob/c2c668e3a73382e928a903c3165e814626f241b5/hledger/test/journal/costs.test#L510

I for one need a fresh mind to see when exactly it should raise an error here.

[simonmichael]

I have pushed a fix (3357c27, meant to push a PR instead..). This changes the transaction-balancing and --infer-cost behaviour of 1.29 and 1.30, to be more forgiving, doing nothing rather than raise errors, which solves the present issue and I think will not cause problems, and is consistent with --infer-equity, infer-market-prices, -B and -V.

[simonmichael]

Comments welcome. If no-one sees a problem with this we should probably release it in hledger-1.30.2.

@the-solipsist you might be able to test the binary at https://github.com/simonmichael/hledger/actions/runs/5196929865.

[the-solipsist]

Is there any other way of figuring out which journal entries aren't in a format that --infer-cost can understand? If I use --strict or --debug, e.g.? If not, there needs to be. Also, it would be useful if all the entries that are causing problems are printed together instead of halting at each one.

This is going from a bug report to a feature wish, but I think the 'feature' aspect of the bug needs to be retained in a documented fashion.

[simonmichael]

Is there any other way of figuring out which journal entries aren't in a format that --infer-cost can understand?

@the-solipsist I haven't experienced it yet but I'm assuming two ways: either the transaction will fail to balance, or some reports will show that something is off, causing us to investigate - as with a failing balance assertion, a commodity that doesn't fully convert to cost or value, or a non-zero total in balancesheetequity.

If I use --strict or --debug, e.g.? If not, there needs to be.

We could certainly add the strict error checking back as an on-demand check. Possibly included in the --strict checks.

Also, it would be useful if all the entries that are causing problems are printed together instead of halting at each one.

Noted. That's a difficult feature, perhaps there's an open issue for it.

This is going from a bug report to a feature wish, but I think the 'feature' aspect of the bug needs to be retained in a documented fashion.

Which feature aspect is that ?

@Xitian9 , any thoughts on this (relaxed error checking when matching equity conversion postings with regular postings) ?

To review:

Currently, the valid entry @the-solipsist showed here is too similar to an entry that's invalid for inferring cost-from-equity; hledger can't distinguish them. Also, hledger reuses part of the cost-from-equity logic as part of basic journal checking (to help it accept redundant costs). So it wrongly raises an error with this example, even when --infer-cost is not used.

The easiest fix is to (1) relax the error checking, making cost-from-equity inference best-effort (like cost conversion and value conversion), not strictly checked. I like easy, so I'm proposing this, unless we discover a definite problem it would cause.

If 1 is too problematic, some alternatives are:

(2) Declare that entries like this example are now invalid, and require humans to write conversion entries in a form that hledger can more easily check. (I think we can rule this out.)

(3) Bring back the stricter error checking, as a check that can be run on demand with check, and possibly as one of the --strict checks.

(4) Make hledger's inferring cost-from-equity smarter, so it can handle this example. The code is already complex, but we could make it handle this case. There may be other cases that would cause this problem to resurface in future.

(5) Separate the logic for checking-for-redundant-costs and inferring-cost-from-equity. Let the former (done always) be best effort, while keeping the latter (done on demand with --infer-cost) strict. If the latter ever becomes the norm, some might want the option to relax its error checking.

(6) ?

[simonmichael]

Also, for the larger picture, we should have a global sense of

• whether automatic/inferring operations are on or off by default
• whether validation is relaxed (flagging only definite problems) or strict (flagging likely problems) by default (and which checks are always on, part of --strict, or fully optional)

and ideally keep these fairly consistent across the product. [Currently we mostly have auto features off by default and do relaxed checking by default.]

[simonmichael]

The easiest fix is to (1) relax the error checking, making cost-from-equity inference best-effort (like cost conversion and value conversion), not strictly checked. I like easy, so I'm proposing this, unless we discover a definite problem it would cause.

[simonmichael]

1 (ignoring transactions with unmatchable conversion equity postings) is the current fix.

[simonmichael]

By which I mean: "reminder, this is the fix currently in master which will be in the next bugfix release one of these days; more discussion, examples of problems with it, and PRs still welcome."

[simonmichael]

@the-solipsist I forgot to mention: please claim your https://hledger.org/regressionbounty .

[the-solipsist]

Thanks, @simonmichael. I submitted a claim.