exercises(all-your-base): add missing frees

[stgatilov]

The comment in user code says:

/// Caller owns the returned memory.

However, the testing code does not free returned memory if the answer is zero = {0}. As the result, users get memory leaks with wrong code due to testing code breaking the contract. Some awful hacks are necessary to workaround it.

Closes: #319

[stgatilov]

Note that this is breaking change: solutions which previously did pass by adding a workaround will no longer pass after merging this.

As a non-breaking alternative, it might be OK to disable memory leak check in these three tests, so that both the old solutions pass, and the proper solutions pass despite memory leak in testing code.

[ee7]

Thanks for the PR.

As-is, this PR may remove memory leak errors when testing an invalid solution. But it causes a panic when testing a valid solution, right?

Running tests for all-your-base...
1/17 test.single bit one to decimal... OK
2/17 test.binary to single decimal... OK
3/17 test.single decimal to binary... OK
4/17 test.binary to multiple decimal... OK
5/17 test.decimal to binary... OK
6/17 test.trinary to hexadecimal... OK
7/17 test.hexadecimal to trinary... OK
8/17 test.15-bit integer... OK
9/17 test.empty list... thread 1782 panic: Invalid free
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/std/heap/general_purpose_allocator.zig:609:21: 0x22cb5e in freeLarge (test)
                   @panic("Invalid free");
                   ^
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/std/heap/general_purpose_allocator.zig:824:31: 0x22c0db in free (test)
               self.freeLarge(old_mem, log2_old_align, ret_addr);
                             ^
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/std/mem/Allocator.zig:98:28: 0x22800f in free__anon_3795 (test)
   return self.vtable.free(self.ptr, buf, log2_buf_align, ret_addr);
                          ^
/home/runner/work/zig/zig/build/all-your-base/test_all_your_base.zig:94:33: 0x22881a in test.empty list (test)
   defer testing.allocator.free(actual);
                               ^
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/test_runner.zig:176:28: 0x23a559 in mainTerminal (test)
       } else test_fn.func();
                          ^
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/test_runner.zig:36:28: 0x22f00a in main (test)
       return mainTerminal();
                          ^
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/std/start.zig:564:22: 0x2294e2 in posixCallMainAndExit (test)
           root.main();
                    ^
/home/runner/work/zig/zig/zig-linux-x86_64-0.11.0/lib/std/start.zig:243:5: 0x229031 in _start (test)
   asm volatile (switch (native_arch) {
   ^
???:?:?: 0x0 in ??? (???)
error: the following test command crashed:
/home/runner/.cache/zig/o/15b2bfe7d08095a03c7bbdb222eb4aff/test

[ee7]

users get memory leaks with wrong code

I initially read this as "the problem only affects solutions that don't pass the tests". That is, "the tests where expected is {0} leak memory if the solution returns a slice that has length greater than 1".

But I see that testing your submitted solution produces memory leak errors if you comment out lines 72-75. I didn't immediately see why.

However, without this PR, there are no memory leak errors when testing the other published solutions:

• https://exercism.org/tracks/zig/exercises/all-your-base/solutions/Acaccia
• https://exercism.org/tracks/zig/exercises/all-your-base/solutions/mhennig4711
• https://exercism.org/tracks/zig/exercises/all-your-base/solutions/sezoka

and with this PR, testing each of the above solutions produces a panic due to invalid free.

Could you please investigate why your solution requires lines 72-75, and other solutions don't require the same?

---

Edit: I guess it's because one approach involves:

var zero = [_]u32{0};

[...]
    if (num == 0) {
        return &zero;
}

while your solution does:

if (res.items.len == 0)
    try res.append(0);

So I think we do want to add frees in these test cases, but only perform them when valid.

[FedericoStra]

However, without this PR, there are no memory leak errors when testing the other published solutions:

• https://exercism.org/tracks/zig/exercises/all-your-base/solutions/Acaccia
• https://exercism.org/tracks/zig/exercises/all-your-base/solutions/mhennig4711
• https://exercism.org/tracks/zig/exercises/all-your-base/solutions/sezoka

and with this PR, testing each of the above solutions produces a panic due to invalid free.

Could you please investigate why your solution requires lines 72-75, and other solutions don't require the same?

As I explained in #319, all the accepted solutions are currently broken as well (mine inclueded!). Some of them return a reference to a local variable (which is obviously wrong), whereas some others return a reference to a global array (which is wrong too because if can be modified to not be zero anymore).

The only options are:

1. have convert return a []const u32 instead, so that returning a slice of a global immutable array is fine;
2. fix the tests and force implementations to always return a newly allocated array.

Option 1 is not nice because it doesn't make much sense to sometimes return an owned slice and sometimes not.

[ee7]

whereas some others return a reference to a global array (which is wrong too because if can be modified to not be zero anymore).

@FedericoStra thanks for your thoughts. I agree that it'd be better to avoid the global array. But if actual contains exactly 0 at test time, shouldn't we consider that a "valid solution"? I'm concerned that users are likely to be confused if their solution passes the tests, but produces "invalid free" errors. We might even get PRs to remove the frees added by this PR :)

Can we do better than just adding comments to the tests, and adding explanation in the instructions? There's no reasonable and robust way to free actual only when it's on the heap, right?

That said, as @stgatilov pointed out, the doc comment does say "Caller owns the returned memory".

[ee7]

I pushed commits to fix the example solution.

I'll merge this soon, but later we could try to help the user that returns a reference to a global array for the tests that expect {0}. Exercism (unfortunately) disables merge commits org-wide, so I'll squash this.

Note that this is breaking change: solutions which previously did pass by adding a workaround will no longer pass after merging this.

As a non-breaking alternative, it might be OK to disable memory leak check in these three tests, so that both the old solutions pass, and the proper solutions pass despite memory leak in testing code.

I think it's better to break submitted solutions here. This exercise was added only recently.

[ee7]

@stgatilov @FedericoStra Sorry for the inconvenience, and thanks again for the PRs. Apparently I wasn't thinking when I implemented this exercise. You should be able to fix your solutions now.

[stgatilov]

@stgatilov @FedericoStra Sorry for the inconvenience, and thanks again for the PRs. Apparently I wasn't thinking when I implemented this exercise. You should be able to fix your solutions now.

Yes, I fixed the solution.
Thank you!

P.S. Actually, exercism does not automatically drop accepted solutions, it merely suggests updating to the new version and retrying tests. So there is no problem at all in breaking existing solutions, if this makes the problem better.

[FedericoStra]

There's no reasonable and robust way to free actual only when it's on the heap, right?

I don't think there is, apart from some scary dark magic...

I personally think that the solutions returning a reference to a mutable global array should be considered invalid (mine currently does so) and people should be forced to respect the docstring contract that

/// Caller owns the returned memory.

[ee7]

P.S. Actually, exercism does not automatically drop accepted solutions, it merely suggests updating to the new version and retrying tests. So there is no problem at all in breaking existing solutions, if this makes the problem better.

If I recall correctly, when a test file changes, Exercism really does put every corresponding solution in a queue to be retested. You can see here that old solutions are now marked with a red cross.

But it's fine to break solutions in this case anyway.

people should be forced to respect the docstring contract

Sure. But as an aside: I think we should generally allow the user to alter the doc string, or at least append to it, to the extent permitted by the tests (which they shouldn't change). In this case, that contract is enforced by the tests. In other cases, the contract will be enforced by future tests (e.g. when std.testing gains the ability to expect a panic for a particular input).

But for example, in the matching-brackets exercise, I think it's fine if a user wants to add their own contract like

/// Caller guarantees that the maximum nesting depth of brackets in `s` is at most 100

and then use a local array to determine whether brackets are matched, without using the given allocator.

[FedericoStra]

Sure. But as an aside: I think we should generally allow the user to alter the doc string, or at least append to it, to the extent permitted by the tests (which they shouldn't change). In this case, that contract is enforced by the tests.

I agree that users should be allowed to change the contract, but I disagree that in this case the tests are thorough enough. Returning a mutable global array is clearly wrong and should be tested. The following tests breaks all current solutions that return a reference to the same global var zero = [1]u32{0}:

test "does not return the same zero multiple times" {
    const expected = [_]u32{0};
    const digits = [_]u32{0};
    const input_base = 10;
    const output_base = 2;
    const actual = try convert(testing.allocator, &digits, input_base, output_base);
    try testing.expectEqualSlices(u32, &expected, actual);
    actual[0] = 1 // modify the output!
    const again = try convert(testing.allocator, &digits, input_base, output_base);
    try testing.expectEqualSlices(u32, &expected, again);
}

[ee7]

I disagree that in this case the tests are thorough enough.

@FedericoStra Thanks for your thoughts.

Well, "the tests enforce the contract" in the sense that they now panic for a solution that returns a reference to a global array. Every test now contains a free, and that memory can't be freed. So the current tests are "sufficient" in the sense that Exercism won't mark such a solution as passing.

But I agree the tests should be more thorough. Part of my concern above was that I'd like such a solution to fail a test, rather than only producing a panic.

I've created #322 to add the test case you proposed, with exactly these changes:

• Bikeshed the test name to better match the other test names
• Add comments
• Add frees for actual and again :)
• Capitalise the comment

I'll try to merge that PR before the weekend. If it's not merged when you read this: could you please review it? Otherwise, please yell at me if you have suggested improvements.

[ee7]

And nice work on ziglang/zig@4f952c7 :)

Was it from going down the rabbit hole on an Exercism exercise? I believe ziglang/zig#14827 was created due to the armstrong-numbers exercise.

[FedericoStra]

@ee7

But I agree the tests should be more thorough. Part of my concern above was that I'd like such a solution to fail a test, rather than only producing a panic.

I've created #322 to add the test case you proposed [...]

#322 looks good to me! With this test it should be indeed easier for a user to identify the problem with returning a global array.

---

And nice work on ziglang/zig@4f952c7 :)

Was it from going down the rabbit hole on an Exercism exercise? I believe ziglang/zig#14827 was created due to the armstrong-numbers exercise.

Yes, exactly! I used std.math.log10 on line 6 of my solution to the armstrong-numbers exercise, was curious to see how a specialized implementation of log10 looked like and then started scrolling around the source code, soon to realize that std.math.log returned incorrect results for integer arguments, and this prompted me to suggest ziglang/zig#17143. Unfortunately this only covers runtime integers and not comptime_int, see ziglang/zig#17142.

On top of that, the comment before std.math.log10_int reminded me to check how Rust implements integer logarithm, and I found out that it uses iterated divisions instead of multiplications; so now there are rust-lang/rust#115874 and rust-lang/rust#115875 trying to improve its performance.

I love how it's a back and forth between two lovely languages trying to help each other out.