Turn std.rand.Random into a Mixin

[daurnimator]

After chatting in IRC, I spied that std.rand.Random was a great candidate to be the first of what I've started calling "Mixin"s in the standard library.

A "Mixin" is a new pattern where you have a function that takes a struct and returns a struct with no fields, intended to be used from the definition of another struct with pub usingnamespace MyMixin(@This()). This act of "mixing-in" adds "helper" methods to the struct you're currently defining, and are expect to take a self: var parameter and use method(s) from the mixed-into struct. In this case of Random, we expect the base struct to implement a method bytes that looks like pub fn bytes(self: ........., buf: []u8) void) void.

Compared to the existing pattern, a Mixin:

• doesn't have the runtime requirement of a function pointer
• allows the base functions to be either sync or async
• allows the base functions (if they return an error union) to have differing error sets
• sidesteps design flaw: using a struct as an interface and @fieldParentPtr can lead to bad pointer dereference #591
• allows optimisation (e.g. inlining) of the base function into the helper function

[andrewrk]

One thing that I want to check - maybe you can help me with this @daurnimator - is how does this affect bloat? For example, what is the size of the std lib test binary with everything as mixins (including streams and sd.rand.Random) vs with master branch?

Also, what would be your plan for dealing with function name conflicts?

[daurnimator]

what would be your plan for dealing with function name conflicts?

You should get the error:

error: import of 'a' overrides existing definition

Test case:

const Foo = struct {
    pub fn a() void {}
};

const Bar = struct {
    pub usingnamespace Foo;
    pub fn a() void {}
};

pub fn main() void {
    const x = Bar{};
}

[andrewrk]

Right but do you see how this is problematic? There could easily be 2 interfaces that both have a reset() function, for example.

[daurnimator]

Right but do you see how this is problematic? There could easily be 2 interfaces that both have a reset() function.

Not really: if you wanted to have two different methods both called .foo()... bad luck. That would be confusing for the creator and the user, and the error is a good thing.

Good mixin design involves documenting the functions/fields you expect on the object, as well as the methods you add (note that we should get this latter part for free with documentation generation).

[daurnimator]

One thing that I want to check - maybe you can help me with this @daurnimator - is how does this affect bloat? For example, what is the size of the std lib test binary with everything as mixins

Testing lib/std/rand.zig:
Before stripping:

• master: 912K
• this branch: 900K

After stripping:

• master: 268K
• this branch: 260K

Testing just lib/std/std.zig for now:
Before stripping:

• master: 14720K
• this branch: 14712K

After stripping:

• master: 7332K
• this branch: 7324K

So... this decreased binary size. That was unexpected!

[andrewrk]

Which build modes did you use? Can you try Debug, ReleaseFast, and ReleaseSmall?

[fengb]

Good mixin design involves documenting the functions/fields you expect on the object, as well as the methods you add (note that we should get this latter part for free with documentation generation).

I personally consider this as a failure of mixins in language design. In every language with mixins, I wish there's a way to explicitly declare the dependencies at the consumption site as well as being able to push it into a separate namespace. Something like:

# before
class Banana
    include Enumerable

    def each
        # body
    end
end

# after
class Banana
    enum = include Enumerable(:each)

    def each
        # body
    end
end

[daurnimator]

explicitly declare the dependencies at the consumption site

So that's one reason why mixins take Self as a parameter:

fn MyMixin(Self: type) type {
    assert(@hasDecl(Self, "foo"));
    assert(@typeInfo(Self.foo).Fn. ........ );
    assert(@hasDecl(Self.FooError));
    return struct {
        pub fn bar(self: var) Self.FooError!void {
            do_something_with(try self.foo());
        }
    }
}

I don't use this in the implementation today because I got stuck on #3699. But once fixed, this should be easy to do with zig.

able to push it into a separate namespace.

You can do this today, but it brings #591 back into the picture.

[daurnimator]

Which build modes did you use? Can you try Debug, ReleaseFast, and ReleaseSmall?

I just used: ./build/zig test --override-lib-dir lib lib/std/std.zig

Testing again with ReleaseFast:

• master: 19536K
• master-stripped: 3196K
• 5718090: 19504K
• 5718090-stripped: 3192K

[daurnimator]

including streams

	master	rand branch	stream branch
debug	14752	14712	14812
debug stripped	7340	7324	7348
release-fast	19576	19504	20164
release-fast stripped	3204	3192	3304
release-small	14464	14408	15152
release-small stripped	2440	2432	2544

i.e. there is a minor increase in binary size for the stream branch.

[LemonBoy]

You may want to use google's Bloaty McBloatface tool to have an insight into why/how the binary sizes change.

[daurnimator]

Interesting note: with the mixin style, you can get the old style back quite easily:

const ArbitraryRandom = struct {
    fillFn: fn (self: *ArbitraryRandom, buf: []u8) void,

    pub fn bytes(self: *ArbitraryRandom, buf: []u8) void {
        return self.fillFn(self, buf);
    }

    pub usingnamespace Random(ArbitraryRandom);
};

test "ArbitraryRandom" {
    const MyPRNG = struct {
        random: ArbitraryRandom,
        next_value: u8,

        pub fn init() @This() {
            return .{
                .next_value = 0,
                .random = ArbitraryRandom {
                    .fillFn = fillFn,
                },
            };
        }

        fn fillFn(random: *ArbitraryRandom, buf: []u8) void {
            const self = @fieldParentPtr(@This(), "random", random);
            for (buf) |*b| {
                b.* = self.next_value;
            }
            self.next_value +%= 1;
        }
    };
    var r = MyPRNG.init();

    expect(r.random.int(u0) == 0);

    r.next_value = 0;
    expect(r.random.int(u1) == 0);
    expect(r.random.int(u1) == 1);
    expect(r.random.int(u2) == 2);
    expect(r.random.int(u2) == 3);
    expect(r.random.int(u2) == 0);
}

[frmdstryr]

I think the usingnamespace approach to only exacerbates the inheritance problem from a code readability standpoint.

Instead of having all "used namespaces" at one place at the top of the scope (eg in the typical extends (A, B, C) pattern) collisions can be buried anywhere in the struct.

[tamaskenez]

Right but do you see how this is problematic? There could easily be 2 interfaces that both have a reset() function, for example.

@andrewrk, @daurnimator In Rust they can qualify with the traits's name if two traits have the same function. That can happen easily, it's not a bad design, the "bad luck" argument doesn't hold.

[andrewrk]

This is another good example of providing another userland interface pattern (#1829) provide more insight into the possible language feature for this (#130).

All of the things you noted are significant improvements over status quo. The downsides of this pattern compared to status quo are:

• Required use of var as a parameter type for all of the interface methods
• Namespace collisions
• Does not account for runtime dispatch. For example, when calling r.bytes(), bytes cannot be a function pointer field.
• Abuse of usingnamespace causing code to be more difficult to read. Multiple mixins causes a situation where it is difficult to find out where identifiers are being imported from. I've personally experienced this pain in a ruby on rails codebase, where mixins were widely used.

Let this example stand as a list of requirements when considering #130. It should solve all the issues that mixins solves, as well as the downsides I noted above. Until then, we'll stick with status quo.