Skip to content

fix(auto): register Base lock functions as non-consuming effects - #84

Closed
MilesCranmerBot wants to merge 3 commits into
MilesCranmer:mainfrom
MilesCranmerBot:bless-lockables
Closed

MilesCranmerBot wants to merge 3 commits into
MilesCranmer:mainfrom
MilesCranmerBot:bless-lockables

Conversation

@MilesCranmerBot

Copy link
Copy Markdown
Contributor

Problem

@safe misflags locked regions as violations. Reproduces on v0.4.6, Julia 1.12.6:

BorrowChecker.@safe function f(l::Base.Lockable{Vector{Int}})
    lock(l) do arr
        arr[2] += 1
    end
    return l.value[2]
end
# BorrowCheckError: value escapes/consumed by unknown call; it (or an alias) is used later

The callback functor and the Lockable both flow into lock, an unknown callee, so the conservative fallback treats the call as consuming/escaping its arguments.

Fix

Register Base.lock, Base.unlock, Base.trylock, Base.islocked in the builtin effect registry (_populate_registry!) with empty writes/consumes/ret_aliases. Locks serialize access; they never consume or write through their arguments' contents, so the conservative treatment is always a false positive for them.

A pleasant consequence: Base.Lockable (immutable struct, 1.12+) now works as a checked shared-mutable carrier. The wrapper itself is untracked (immutable), and payload access through the lock(f, l) do-block form checks cleanly at :function scope and under recursive scopes (:module/:user).

Testing

  • New testset: do-block at :function and :module scope passes; ordinary aliasing violations near blessed calls still throw.
  • Updated the registry-provenance testset to allow the four blessed Base functions.
  • Full suite: 1173 pass / 4 broken (the same 4 pre-existing broken as the v0.4.6 baseline), exit 0.

Known remaining gaps (not addressed here)

  • Bare lock(l); ...; unlock(l) regions still false-positive when the payload is written through field access in the same function (l.value[2] += 1): the write through getfield of the live immutable wrapper is flagged as an aliased write. Pre-existing, independent of locks; reads pass.
  • Captured mutables crossing Threads.@spawn boundaries are flagged as consumes regardless of whether the thunk reads or writes (thunk bodies are not summarized yet).

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd58e82ffb

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/auto/defs.jl Outdated
@MilesCranmerBot

Copy link
Copy Markdown
Contributor Author

Replaced the approach in 4bd8bd5 — registering Base.lock/unlock/trylock/islocked as non-consuming effects is the wrong model: it blesses a lock as a memory-safety boundary and makes locked regions a diagnostic void where aliased writes and escapes are silently accepted. A lock serializes task access; it says nothing about borrow rules.

Adversarial probe results (now pinned as tests): aliased write inside the lock block remains missed on both versions and is now pinned with a repro as @test_broken; escape into an outliving Ref is diagnosed on this branch via conservative consume-attribution of the unknown closure call; global array writes remain missed on both (globals untracked, pre-existing).

Root cause of the remaining holes: the do-block lowers to a fresh closure object captured through slot-merged Core.Boxes; the checker never enters the callback body, and static recovery of the captured bindings from the call site is defeated by the box reuse. The proper fix is checking the callback body against caller state at check time (closure inlining), which is follow-up-sized work - attempted variants (callback-tt substitution; union-find capture linking via def-chain walks) were evaluated and rejected because they either still miss the case or corrupt unrelated cache entries. The memoryrefget ret-alias adjustment from the original commit is retained.

@MilesCranmerBot

Copy link
Copy Markdown
Contributor Author

Heads-up to whoever is shepherding this PR: do not re-apply 4bd8bd5 / d3f31ce. Pushing 3b798de next (force-push), which supersedes the revert-and-pin approach:

  • The codex P1 scenario is now actually fixed, not documented as broken: lock(f, l) is modeled as a transparent higher-order call in _effects_for_call. The callback is summarized on its real signature; capture-field writes map to the functor argument (so mutating an aliased captured vector inside the callback throws), payload writes stay granted (the callback holds the lock), consumption of functor/payload is dropped (neither escapes), and callback return aliases propagate.
  • Verified on 1.12.6: codex case throws; legit payload mutation passes at :function and :module scope; ret-alias propagation passes. Full suite locally: 1174 pass / same 4 pre-existing broken, exit 0.
  • Registry entries for lock/unlock/trylock/islocked stay (bare forms need them); defs.jl comment updated to point at the structural handling.
  • The @test_broken pins from 4bd8bd5 describe the diagnostic void that no longer exists, so they are not carried over.

Per direct instruction from Miles to implement the fix rather than document the hole.

The lock-effect registry entries, the higher-order lock(f, l) model,
and all four lock testsets are merged into MilesCranmer#88 (ported to the
dissolved src/safe layout and verified green on 1.12 and 1.13-rc3).
The src/auto module layout this branch was built on was dissolved by
MilesCranmer#87, so the remaining rename is superseded upstream.
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