Fixes #14492 - Check types of Allocator functions

[leoconst]

Hi 👋

This is my first attempt at an open-source contribution - hope it's okay! I'm also very new to Zig so I'm sure there's a better way to do this, looking forward to hearing it. I'm also also happy for for this to be rejected outright if it's not what's wanted, of course 😅.

The change

Raises a compile error for non-pointers passed to Allocator functions. Fixes #14492, but also applies the same to other functions in Allocator.zig with the same requirement.

A potential problem

Notably, when running the following with my patch:

const std = @import("std");

const MyStruct = struct {
    foo: i32,
    bar: bool,
};

pub fn main() !void {
    const allocator = std.heap.page_allocator;

    const my_struct = MyStruct{
        .foo = 42,
        .bar = false,
    };

    allocator.free(my_struct);
}

The compile error unfortunatly doesn't point to the sample code, only highlighting Allocator.zig:

...\Zig\lib\std\mem\Allocator.zig:324:17: error: free expects a slice but received value of type `main.MyStruct`
        else => @compileError(function_name ++ " expects a slice but received value of type `" ++ @typeName(Type) ++ "`"),
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...\Zig\lib\std\mem\Allocator.zig:296:30: note: called from here
    const Slice = EnsureSlice(@TypeOf(memory), "free");
                  ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~

I don't know how to fix this, or indeed whether I need to fix it? For what it's worth. the current (pre this patch) error message doesn't mention the callsite either, but it would be nice to solve that here.

Tests

Tests pass when running zig build test-std -Dskip-release -Dskip-non-native:
2294 passed; 114 skipped; 0 failed.

[InKryption]

Try these changes:

diff --git a/lib/std/heap/general_purpose_allocator.zig b/lib/std/heap/general_purpose_allocator.zig
index 4f8be3804..15427dc1c 100644
--- a/lib/std/heap/general_purpose_allocator.zig
+++ b/lib/std/heap/general_purpose_allocator.zig
@@ -397,7 +397,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
                         const prev = bucket.prev;
                         if (config.never_unmap) {
                             // free page that was intentionally leaked by never_unmap
-                            self.backing_allocator.free(bucket.page[0..page_size]);
+                            const array_ptr = bucket.page[0..page_size];
+                            comptime assert(@TypeOf(array_ptr) == *align(page_size) [page_size]u8);
+                            self.backing_allocator.free(@as([]align(page_size) u8, array_ptr));
                         }
                         // alloc_cursor was set to slot count when bucket added to empty_buckets
                         self.freeBucket(bucket, @divExact(page_size, bucket.alloc_cursor));
@@ -814,7 +816,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
                     self.buckets[bucket_index] = bucket.prev;
                 }
                 if (!config.never_unmap) {
-                    self.backing_allocator.free(bucket.page[0..page_size]);
+                    const array_ptr = bucket.page[0..page_size];
+                    comptime assert(@TypeOf(array_ptr) == *align(page_size) [page_size]u8);
+                    self.backing_allocator.free(@as([]align(page_size) u8, array_ptr));
                 }
                 if (!config.retain_metadata) {
                     self.freeBucket(bucket, size_class);

This should fix the compile errors, though I do wonder if this is desirable. Once upon a time we could have used std.mem.span, but that's not applicable here now.
Alternatively, could just replace the calls to free here with destroy, but that doesn't quite sit right with me.

[leoconst]

Try these changes:

diff --git a/lib/std/heap/general_purpose_allocator.zig b/lib/std/heap/general_purpose_allocator.zig
index 4f8be3804..15427dc1c 100644
--- a/lib/std/heap/general_purpose_allocator.zig
+++ b/lib/std/heap/general_purpose_allocator.zig
@@ -397,7 +397,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
                         const prev = bucket.prev;
                         if (config.never_unmap) {
                             // free page that was intentionally leaked by never_unmap
-                            self.backing_allocator.free(bucket.page[0..page_size]);
+                            const array_ptr = bucket.page[0..page_size];
+                            comptime assert(@TypeOf(array_ptr) == *align(page_size) [page_size]u8);
+                            self.backing_allocator.free(@as([]align(page_size) u8, array_ptr));
                         }
                         // alloc_cursor was set to slot count when bucket added to empty_buckets
                         self.freeBucket(bucket, @divExact(page_size, bucket.alloc_cursor));
@@ -814,7 +816,9 @@ pub fn GeneralPurposeAllocator(comptime config: Config) type {
                     self.buckets[bucket_index] = bucket.prev;
                 }
                 if (!config.never_unmap) {
-                    self.backing_allocator.free(bucket.page[0..page_size]);
+                    const array_ptr = bucket.page[0..page_size];
+                    comptime assert(@TypeOf(array_ptr) == *align(page_size) [page_size]u8);
+                    self.backing_allocator.free(@as([]align(page_size) u8, array_ptr));
                 }
                 if (!config.retain_metadata) {
                     self.freeBucket(bucket, size_class);

This should fix the compile errors, though I do wonder if this is desirable. Once upon a time we could have used std.mem.span, but that's not applicable here now. Alternatively, could just replace the calls to free here with destroy, but that doesn't quite sit right with me.

That worked 😀. Tests now build and all pass (discounting the skipped ones). I've pushed that to leoconst#1, but as it's now working should I merge it into this branch?

[leoconst]

I've merged that in :-)

[leoconst]

@InKryption What's the next step for this PR?

[InKryption]

Well, now I imagine it's just a matter of waiting for a workflow approval from someone on the team, and possibly a review of the changes from the same (concerning whether these changes are desirable or not).

[andrewrk]

Reverted in 3c2a43f