Proposal: add assertLocked to std.debug.SafetyLock

[scottredig]

• Adds a function which asserts that a SatetyLock's state is locked when runtime_safety is on.
• Changes the state enum when runtime_safety is off to be unknown.
• Add a simple usage test.

In my code, I have several constructs with methods that are, roughly:

• Begin
• Add item
• End

Where begin reserves resources, add item adds a pending value, and end sends the queued items. As adding items is only valid between begin and end calls, I want an assert construct in my code. I started writing my own, realized there's probably one in the standard library, and found SafetyLock. However SafetyLock is currently only used in cases where a special state makes some calls invalid, and is not used anywhere that a special state makes calls valid. Therefor, I propose adding a method assertLocked.

In performing this change, I noticed that when runtime_safety is off then the only state is unlocked. This obviously clashes with a method called assertLocked. So I've changed the state in that case to be named unknown. As a side benefit, I think this better reflects the state. It'll have a better chance of correctness with any code inspecting the state, since outside code can't look at the SafetyLock and see that it's unlocked in runtime_safety=false code when, in the same overall program state, it would be locked in runtime_safety=true code.

There was one place where unlocked was mentioned outside of the definition. I've switched to using a default field value, though I could see adding SafetlyLock.unlocked instead. Which to do is up to reviewers style choice. A corresponding SafetyLock.locked would conflict with https://github.com/ziglang/zig/issues/19328

[mlugg]

[Halfway through writing this comment I've just noticed that you touched on this at the bottom of your comment!]

Counterproposal:

pub const SafetyLock = struct {
    state: State,

    pub const locked: SafetyLock = .{ .state = if (runtime_safety) .locked else {} };
    pub const unlocked: SafetyLock = .{ .state = if (runtime_safety) .unlocked else {} };

    pub const State = if (runtime_safety) enum { unlocked, locked } else void;

    pub fn lock(l: *SafetyLock) void {
        if (!runtime_safety) return;
        assert(l.state == .unlocked);
        l.state = .locked;
    }

    pub fn unlock(l: *SafetyLock) void {
        if (!runtime_safety) return;
        assert(l.state == .locked);
        l.state = .unlocked;
    }

    pub fn assertUnlocked(l: SafetyLock) void {
        if (!runtime_safety) return;
        assert(l.state == .unlocked);
    }
};

Existing fields which look like pointer_stability: std.debug.SafetyLock = .{} become pointer_stability: std.debug.SafetyLock = .unlocked, and for your use case, you can default to .locked instead. Your solution here seems like a hack, because you aren't trying to check that the lock is locked, but rather, you just want the initial state to be locked, and your begin method unlocks it.

This does not conflict with #19328; we just need to adjust how the stack traces are captured. We could use essentially a rolling buffer, and always include the last two stack traces when we panic. (There may be no stack trace if you try to lock an initially-locked SafetyLock.)

[mlugg]

@scottredig I'll wait a few days for you to respond to the above; if you don't get to it in time I'll probably open a PR with my one, but am happy to have a discussion about the two options if you'd like to!

[scottredig]

Thanks @andrewrk

Your solution here seems like a hack, because you aren't trying to check that the lock is locked, but rather, you just want the initial state to be locked, and your begin method unlocks it.

At a guess, I think we're interpreting locked and unlocked differently here.

The way I was taking it, was SafetyLock is like a mutex. lock means that you're inside some critical section of code/state, unlock means that your outside of it. In this way, the critical section in my example is building. So with this interpretation to have it start as locked, and unlock at the start of building, the variable would have to be not_building or something similar. This is analogous to a pretty common mistake in booleans where you have an inversion in the variable name, so inverting both the variable name and the value makes code much more clear.

Under my interpretation, I think it's reasonable to assert you're within a critical section and also (at other times) assert you're outside of the critical section. It's less motivated by my real usage example, but I might want to, eg, assert that using the data elsewhere that you're not in a building state. Or an algorithm using std.HashMap might want to assert the pointers are stable.

The other way I can see it (and how I'm guessing you're seeing it), is taking locked to mean fixed. For HashMap, inside the critical section is pointers to values are fixed and some operations become invalid. For my example, inside the critical section means a common buffer is reserved for my use, and some operations become valid.

Lmk if I'm understanding you right?

I did consider to have SafetyLock take a enum, so HashMap could use, eg, enum {fixed_pointers, unfixed_pointers}. And the assert would take which one you want. Essentially applying the same transformation that is done by locked: bool, -> state: enum { locked, unlocked }, However, then the questions about specifying valid state changes becomes real and this thing goes down a rabbit hole of being way more complicated.

This does not conflict with #19328; we just need to adjust how the stack traces are captured. We could use essentially a rolling buffer, and always include the last two stack traces when we panic. (There may be no stack trace if you try to lock an initially-locked SafetyLock.)

Ah, yeah you're right. My brain was stuck in .state = .locked implying having a stack trace. It makes more sense to just always store the latest transition stack trace, and if it breaks before a transition say that instead of a stack trace.

Existing fields which look like pointer_stability: std.debug.SafetyLock = .{} become pointer_stability: std.debug.SafetyLock = .unlocked, and for your use case, you can default to .locked instead.

I think you raise a good point that it's reasonable to want to start with .locked state (with whatever interpretation of what that means). So once we agree on the other details, I'll update the PR to add those definitions and remove the default value of state.

As far as void vs enum { unknown }, I have no strong preference. I mostly did the smaller delta from what existed before. It's kinda helpful to say "we don't know what this is", but not /that/ helpful. So your pick here.