From 924ec1c98d65c1de11964045b67fbc77ab507ba3 Mon Sep 17 00:00:00 2001
From: Simon Michael <simon@joyful.com>
Date: Fri, 29 May 2020 13:09:17 -0700
Subject: [PATCH] journal: clearer transaction balancing errors; refactor

As mentioned by netvor on IRC, the unbalanced transaction error was
not too clear when postings all have the same sign.

Some other wording has been clarified, and the main error message is
now shown on multiple lines for readability (at the cost of
predictability/grepability..)

There's also a probably unnoticeable change: selecting which parts of
the error to show is now based on display precisions (reusing the
balanced check logic), rather than original precisions.
---
 hledger-lib/Hledger/Data/Transaction.hs | 67 +++++++++++++------
 tests/journal/parse-errors.test         | 88 ++++++++++++++++++++-----
 tests/journal/precision.test            |  2 +-
 tests/journal/unbalanced.test           |  7 --
 4 files changed, 119 insertions(+), 45 deletions(-)
 delete mode 100644 tests/journal/unbalanced.test

diff --git a/hledger-lib/Hledger/Data/Transaction.hs b/hledger-lib/Hledger/Data/Transaction.hs
index 4eac78d00..4c9cc0979 100644
--- a/hledger-lib/Hledger/Data/Transaction.hs
+++ b/hledger-lib/Hledger/Data/Transaction.hs
@@ -59,6 +59,7 @@ module Hledger.Data.Transaction (
 )
 where
 import Data.List
+import Data.List.Extra (nubSort)
 import Data.Maybe
 import Data.Text (Text)
 import qualified Data.Text as T
@@ -339,31 +340,55 @@ transactionPostingBalances t = (sumPostings $ realPostings t
                                ,sumPostings $ virtualPostings t
                                ,sumPostings $ balancedVirtualPostings t)
 
--- | Check that this transaction appears balanced when rendered,
--- returning an appropriate error message if it is not.
--- In more detail: after converting amounts to cost using explicit
--- transaction prices if any; and summing the real postings; and
--- summing the balanced virtual postings; and applying the given
--- display styles, if provided (maybe affecting decimal places):
--- do both posting totals appear to be zero when rendered ?
+-- | Check that this transaction would appear balanced to a human when displayed,
+-- and return an appropriate error message otherwise.
+--
+-- In more detail:
+-- For the real postings, and separately for the balanced virtual postings:
+--
+-- 1. Convert amounts to cost where possible
+--
+-- 2. When there are multiple amounts which appear non-zero when displayed
+--    (using the given display styles if provided),
+--    are they a mix of positives and negatives ?
+--    This is checked separately to give a clearer error message.
+--    (Best effort, could be confused by postings with multicommodity amounts.)
+--
+-- 3. Does the amounts' sum appear non-zero when displayed ?
+--    (using the given display styles if provided)
+--
 transactionCheckBalanced :: Maybe (M.Map CommoditySymbol AmountStyle) -> Transaction -> Maybe String
 transactionCheckBalanced mstyles t 
-  | rbalanced && bvbalanced = Nothing
-  | otherwise = Just $ printf "could not balance this transaction (%s%s%s)" rmsg sep bvmsg
+  | rsignsok && bvsignsok && rzerosum && bvzerosum = Nothing
+  | otherwise = Just msg
   where
-    (rsum, _, bvsum)            = transactionPostingBalances t
+    (rps, bvps) = (realPostings t, balancedVirtualPostings t)
+    canonicalise = maybe id canonicaliseMixedAmount mstyles
+
+    -- check mixed signs, detecting nonzeros at display precision
+    nonZeros ps = filter (not.isZeroMixedAmount) $ map (canonicalise.costOfMixedAmount.pamount) ps
+    signsOk ps = 
+      case nonZeros ps of
+        nzs | length nzs > 1 -> length (nubSort $ mapMaybe isNegativeMixedAmount nzs) > 1
+        _ -> True
+    (rsignsok, bvsignsok)       = (signsOk rps, signsOk bvps)
+
+    -- check zero sum, at display precision
+    (rsum, bvsum)               = (sumPostings rps, sumPostings bvps)
     (rsumcost, bvsumcost)       = (costOfMixedAmount rsum, costOfMixedAmount bvsum)
     (rsumdisplay, bvsumdisplay) = (canonicalise rsumcost, canonicalise bvsumcost)
-      where canonicalise = maybe id canonicaliseMixedAmount mstyles
-    -- for checking balanced, use the display precision
-    (rbalanced, bvbalanced)     = (isZeroMixedAmount rsumdisplay, isZeroMixedAmount bvsumdisplay)
-    -- for selecting and generating error messages, use the uncanonicalised full precision
-    -- XXX always correct ?
-    rmsg | isReallyZeroMixedAmount rsumcost = ""
-         | otherwise = "real postings are off by " ++ showMixedAmount rsumcost
-    bvmsg | isReallyZeroMixedAmount bvsumcost = ""
-          | otherwise = "balanced virtual postings are off by " ++ showMixedAmount bvsumcost
-    sep = if not (null rmsg) && not (null bvmsg) then "; " else "" :: String
+    (rzerosum, bvzerosum)       = (isZeroMixedAmount rsumdisplay, isZeroMixedAmount bvsumdisplay)
+
+    -- select & generate error messages, showing amounts with their original precision
+    rmsg
+      | not rsignsok  = "real postings all have the same sign"
+      | not rzerosum  = "real postings' sum should be 0 but is: " ++ showMixedAmount rsumcost
+      | otherwise     = "" 
+    bvmsg
+      | not bvsignsok = "balanced virtual postings all have the same sign"
+      | not bvzerosum = "balanced virtual postings' sum should be 0 but is: " ++ showMixedAmount bvsumcost
+      | otherwise     = "" 
+    msg = intercalate "\n" $ ["could not balance this transaction:"] ++ filter (not.null) [rmsg, bvmsg]
 
 -- | Legacy form of transactionCheckBalanced.
 isTransactionBalanced :: Maybe (M.Map CommoditySymbol AmountStyle) -> Transaction -> Bool
@@ -403,7 +428,7 @@ balanceTransactionHelper mstyles t = do
     Just err -> Left $ annotateErrorWithTransaction t' err
 
 annotateErrorWithTransaction :: Transaction -> String -> String
-annotateErrorWithTransaction t s = intercalate "\n" [showGenericSourcePos $ tsourcepos t, s, showTransaction t]
+annotateErrorWithTransaction t s = unlines [showGenericSourcePos $ tsourcepos t, s, showTransaction t]
 
 -- | Infer up to one missing amount for this transactions's real postings, and
 -- likewise for its balanced virtual postings, if needed; or return an error
diff --git a/tests/journal/parse-errors.test b/tests/journal/parse-errors.test
index 87295df28..fa4aa6310 100644
--- a/tests/journal/parse-errors.test
+++ b/tests/journal/parse-errors.test
@@ -19,23 +19,18 @@ expecting date separator or digit
 
 # A journal with an unbalanced transaction.
 <
-2018/1/1
-  a  1
+2009/1/1
+  a   1
 
-# 2. When read from stdin with no reader prefix, the journal reader is used,
-# and fails here. (Before 1.17, all readers were tried and the timedot reader
-# would succeed.)
+# 2. When read from stdin with no reader prefix, the journal reader is used
+# (before 1.17, all readers were tried and the timedot reader would succeed),
+# giving an unbalanced error in this case.
 $ hledger -f - print
->2 /could not balance/
+>2 /real postings' sum should be 0 but is: 1/
 >=1
 
-# 3. So in these tests we must sometimes force the desired format, like so.
-# Now we see the error from the journal reader.
-$ hledger -f journal:- print
->2 /could not balance this transaction \(real postings are off by 1\)/
->=1
-
-# 4. We expect to have reference to line number with last posting
+# 3. We expect to have reference to line number with last posting
+# NB: \< and \> match start and end of a word.
 $ hledger -f journal:- print
 >2 /\<2\>/
 >=1
@@ -45,7 +40,7 @@ $ hledger -f journal:- print
 2018/1/1
   (a) 1
 
-# 5. hledger doesn't detect this as an error directly, it parses account name "(a) 1" and
+# 4. hledger doesn't detect this as an error directly, it parses account name "(a) 1" and
 # amount 0 here.
 $ hledger -f - print -x
 2018-01-01
@@ -53,7 +48,7 @@ $ hledger -f - print -x
 
 >=
 
-# 6. Two (or more) postings with implicit amount cannot be balanced.
+# 5. Two (or more) postings with implicit amount cannot be balanced.
 <
 2018/1/1
   a  1
@@ -64,7 +59,7 @@ $ hledger -f journal:- print
 >2 /could not balance this transaction - can't have more than one real posting with no amount/
 >=1
 
-# 7. Two (or more) virtual postings with implicit amount cannot be balanced.
+# 6. Two (or more) virtual postings with implicit amount cannot be balanced.
 # (And the error message contains line numbers).
 <
 2018/1/1
@@ -75,3 +70,64 @@ $ hledger -f journal:- print
 $ hledger -f journal:- print
 >2 /lines 1-4/
 >=1
+
+# 7. Balancing error messages show the recorded precision, not the display precision.
+<
+2010/1/1
+   a  1C @ $1.0049
+   a  $-1.000
+$ hledger -f- print
+>2 /real postings' sum should be 0 but is: \$0.0049/
+>=1
+
+# 8. Multiple postings all with the same sign gets a distinct error message.
+<
+2020-01-01
+  a  1A
+  b  1A
+$ hledger -f- print
+>2 /real postings all have the same sign/
+>=1
+
+# 9. Zeros don't trigger the same sign error.
+<
+2020-01-01
+  a  A 0
+  b  B 0
+$ hledger -f- print
+2020-01-01
+    a               0
+    b               0
+
+>=0
+
+# 10. An inferred multicommodity amount doesn't trigger the same sign error.
+<
+2020-01-01
+    a  A 1
+    b  B 1
+    c
+$ hledger -f- print
+2020-01-01
+    a             A 1
+    b             B 1
+    c
+
+>=0
+
+# 11. For multiple postings all with the same sign but in different commodities,
+# the error should show the original amounts, without inferring transaction prices.
+<
+2020-01-01
+  a  1A
+  b  1B
+$ hledger -f- print
+>2
+hledger: "-" (lines 1-3)
+could not balance this transaction:
+real postings all have the same sign
+2020-01-01
+    a        1A @@ 1B
+    b              1B
+
+>=1
diff --git a/tests/journal/precision.test b/tests/journal/precision.test
index 2a5907b98..d6a83d8d3 100644
--- a/tests/journal/precision.test
+++ b/tests/journal/precision.test
@@ -38,7 +38,7 @@ hledger -f - balance --no-total --cost --empty
 2010/1/1
    a  1C @ $1.0049
    a  $-1.000
->>>2 /off by \$0.0049/
+>>>2 /should be 0 but is: \$0.0049/
 >>>= 1
 
 # 4. with $'s display precision at 2 or less, this txn should balance
diff --git a/tests/journal/unbalanced.test b/tests/journal/unbalanced.test
deleted file mode 100644
index 393e839a9..000000000
--- a/tests/journal/unbalanced.test
+++ /dev/null
@@ -1,7 +0,0 @@
-hledger -f - register
-<<<
-2009/1/1 a
-  b  1.1
-  c  -1
->>>2 /could not balance this transaction \(real postings are off by 0.1\)/
->>>= 1