Assertion failure when using group-by and amount

[taviso]

Test ledger File:

2024/01/01 * Test1
    Expenses:Test                            $1.00
    Assets:Checking
2024/01/01 * Test2
    Expenses:Test                            $1.00
    Assets:Checking

$ ledger --args-only --file test.ldg bal --group-by 'payee' --invert ^Expenses:
terminate called after throwing an instance of 'ledger::assertion_failed'
  what():  Assertion failed in "ledger/src/op.h", line 260:void ledger::expr_t::op_t::release() const: refc > 0

$ ledger --version
Ledger 3.3.2-20230330, the command-line accounting tool

[taviso]

I would like to fix this bug, I can see the problem is that in post_t::add_to_value, it tries to create a bind_scope_t where scope->parent == scope, so that just recurses infinitely in ledger::bind_scope_t::define.

I don't understand how it got to this state.

This is where it starts to recurse forever:

https://github.com/ledger/ledger/blob/master/src/scope.h#L188

(gdb) list
185
186       virtual void define(const symbol_t::kind_t kind, const string& name,
187                           expr_t::ptr_op_t def) {
188         parent->define(kind, name, def);
189         grandchild.define(kind, name, def);
190       }
(gdb) p this == parent
$1 = true

Here is the debug output before that happens:

   54ms  [DEBUG] value = (Expenses:Test)
              $-1.00  Expenses:Test
   54ms  [DEBUG] Binding scope 0x7ffffffebe80 with 0x81b5330 <-- This is creating the loop
   54ms  [DEBUG] Before compilation:
0x81b8510         O_SEQ (1)
0x81b8330          VALUE: null (1)
0x81b84c0          IDENT: __tmp_amount_expr (1)
0x81b8280           SCOPE: 0x81b7b00 (3)
0x81b8230            O_SEQ (1)
0x81b7cd0             VALUE: null (1)
0x81b81e0             O_SEQ (1)
0x81b8040              VALUE: null (1)
0x81b8190              IDENT: amount_expr (1)
0x81b7ff0               SCOPE: 0x81b7f00 (5)
0x81b7fa0                O_NEG (1)
0x81b7f50                 IDENT: amount_expr (1)
0x81b7c80                  SCOPE: 0x81b7b50 (1)
0x81b7c30                   IDENT: amount (1)
0x81b7be0                    FUNCTION (1)
   55ms  [DEBUG] Compiling: __tmp_amount_expr=(amount_expr=(amount);amount_expr=-amount_expr;amount_expr);__tmp_amount_expr
   55ms  [DEBUG] Compiled expr: __tmp_amount_expr=(amount_expr=(amount);amount_expr=-amount_expr;amount_expr);__tmp_amount_expr
   55ms  [DEBUG] 
   55ms  [DEBUG] .
   55ms  [DEBUG] ..
   55ms  [DEBUG] Binding scope 0x7ffffffebe80 with 0x81b8330
   55ms  [DEBUG] ...
   55ms  [DEBUG] ....
   56ms  [DEBUG] .....
   56ms  [DEBUG] Binding scope 0x81baa60 with 0x81b84c0
   56ms  [DEBUG] ......
   56ms  [DEBUG] Lookup: amount in 0x81baa40
   56ms  [DEBUG] Found definition:
0x81b30d0         FUNCTION (1)
   56ms  [DEBUG] ......
   56ms  [DEBUG] .....
   56ms  [DEBUG] Defining amount_expr in 0x81baa60

This is pretty deep in the weeds, any hints @jwiegley 🤣

[igbanam]

Could you share the call stack until the #define call here, please?

[taviso]

@igbanam I can't parse what you're asking, what #define? There is a reproducer in comment 1.

It looks like @xkikeg spotted the problem using ASan in #2343, but didn't have a solution.

Is the issue just that we need to restore the context after the calc(), because the expr->calc -> expr_t::compile() process will set context to the bound scope on the stack? It sounds plausible, I don't know something like this:

diff --git a/src/post.cc b/src/post.cc
index 72930b46..e2063a36 100644
--- a/src/post.cc
+++ b/src/post.cc
@@ -642,11 +642,12 @@ void post_t::add_to_value(value_t& value, const optional<expr_t&>& expr) const
       add_or_set_value(value, xdata_->compound_value);
   }
   else if (expr) {
-    bind_scope_t bound_scope(*expr->get_context(),
-                             const_cast<post_t&>(*this));
+    scope_t *ctx = expr->get_context();
+    bind_scope_t bound_scope(*ctx, const_cast<post_t&>(*this));
 #if 1
     value_t temp(expr->calc(bound_scope));
     add_or_set_value(value, temp);
+    expr->set_context(ctx);
 #else
     if (! xdata_) xdata_ = xdata_t();
     xdata_->value = expr->calc(bound_scope);

That does seem to fix it, and all the tests parse... what do you think @jwiegley @xkikeg @tbm?

I'll send you a pull request for that.

[igbanam]

Oh I'm glad you two could figure it out. I was asking for more information. 2343 seems to have provided that

[xkikeg]

Thanks! That looks like a reasonable fix, LGTM

If we can invent a way to have a temporal evaluation, that'll be even cleaner, but I don't know if it's easy with the current expr_t lazy compile design.