Skip to content

Fix: empty array doesn't override an array of tables in toml_merge - #53

Closed
afonsojanu wants to merge 1 commit into
cktan:mainfrom
afonsojanu:fix-empty-array-merge-override
Closed

afonsojanu wants to merge 1 commit into
cktan:mainfrom
afonsojanu:fix-empty-array-merge-override

Conversation

@afonsojanu

Copy link
Copy Markdown
Contributor

`is_array_of_tables()` returns true for an empty array as well, since a loop over zero elements never runs into one that fails the `TOML_TABLE` check. On its own that's harmless, but `datum_merge()`'s `TOML_ARRAY` case uses "both sides are arrays of tables" as the signal to append src onto dst instead of overriding it, and an empty array satisfies that check vacuously.

So merging in a document that explicitly resets an existing array-of-tables key to an empty array doesn't clear it like it should. The merge takes the append branch, appends zero elements from the empty array, and leaves dst's original array of tables untouched:

const char *s1 = "[[arr]]\nx = 1\n";
const char *s2 = "arr = []\n";

toml_result_t r1 = toml_parse(s1, strlen(s1));
toml_result_t r2 = toml_parse(s2, strlen(s2));
toml_result_t merged = toml_merge(&r1, &r2);

toml_datum_t x = toml_get(merged.toptab, "arr");
printf("%d\n", x.u.arr.size);
// prints 1, still [{x=1}], even though r2 asked for arr = []

Excluding the empty-array case from `is_array_of_tables()` makes that combination fall through to the plain override path instead, same as any other src value that isn't a genuine array of tables. I checked this doesn't change behavior for any of the other size/type combinations the previous `is_array_of_tables(*dst) && is_array_of_tables(src)` check (from #44) was meant to distinguish, since an empty array on either side that's genuinely being appended-to or replaced by a real array of tables gets the same end result either way.

Added two cases to `test/merge/test1.c`: one for the array-of-tables-overridden-by-empty-array direction (fails without the fix), and the reverse direction as a sanity check that it still works.

Ran `make clean test` locally; everything passes except `test-stdtest`, which needs the `toml-test` Go binary that wasn't available in my environment and is unrelated to this change.

is_array_of_tables() returns true for an empty array too, since a loop
over zero elements never finds one that fails the TOML_TABLE check.
That was fine on its own, but datum_merge()'s TOML_ARRAY case treats
"both sides are arrays of tables" as a signal to append src onto dst
rather than override it, and an empty array happens to satisfy that
check vacuously.

So merging a document that explicitly sets an existing array-of-tables
key to an empty array (arr = []) doesn't clear it as intended. Instead
the merge takes the append branch, appends zero elements from the
empty array, and leaves dst's original array of tables completely
untouched:

    r1: [[arr]]
        x = 1
    r2: arr = []

    toml_merge(r1, r2) -> arr still has one table, size stays 1,
                          when the override to [] should have taken effect

Excluding the empty case from is_array_of_tables() makes it fall
through to the plain override path instead, same as any other
non-array-of-tables src value.

Added two tests to test/merge/test1.c covering both directions: an
array of tables overridden by an empty array (the bug), and an empty
array overridden by an array of tables (already worked, kept as a
sanity check since the fix touches the same condition).
@cktan cktan closed this in ae0ce2f Sep 11, 2026
@cktan

cktan commented Sep 11, 2026

Copy link
Copy Markdown
Owner

Thanks! This landed on main as ae0ce2f (squash-merged locally, so GitHub shows the PR as closed rather than merged). Authorship is preserved on the commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants