Skip to content

Issues found by claude #233

Description

@yebai

Verified on Julia 1.10, 1.11, and 1.12 where a runtime claim is made.


Bug

  • produce of a non-Int literal whose result is used across a later produce crashes TapedTask construction. src/refelim.jl:234,250 (emitted at src/transformation.jl:812).

    function model()
        s = produce("hi")   # constant literal as produce arg
        t = produce(s)      # result `s` live across a later produce -> ref is kept
        return (s, t)
    end
    TapedTask(nothing, model)
    # ERROR: Unexpected value argument to set_ref_at!: hi

    eliminate_refs only handles set_ref_at! value-args that are ID, GlobalRef, or
    Argument; a literal that survives constant-propagation into a kept cross-suspension ref
    hits error(...). The Int case is masked because Core.Const folds the literal into all
    use sites, leaving the produce-result SSA unused (no ref kept). Fix: handle a non-ID
    constant value-arg like the existing GlobalRef/Argument arm (store it as-is for a kept
    set / substitute it for a dropped get) instead of erroring, in both branches.


Docs / CI

  • Doc build fails: BasicBlockCode.remake_enter docstring not indexed. docs/src/internals.md.
    makedocs(; modules=[Libtask]) enforces checkdocs=:all; remake_enter has a docstring
    but no @docs entry → [:missing_docs], deterministically on all versions, so Documentation
    CI is red. Fix: add Libtask.BasicBlockCode.remake_enter to the @docs block.
    (Already fixed on the support-trycatch-194 branch; listed for completeness.)

  • replace_captures docstring references nonexistent build_rrule (a Mooncake
    leftover) and has a typo "intepreter". src/utils.jl:10. It's a documented internal, so the
    stale text renders in the docs. Fix: reword to Libtask's actual use (reusing a compiled
    MistyClosure's captures on copy/fresh_copy to avoid recompilation).

  • optimise_ir! docstring signature is wrong. src/utils.jl:35. Shows
    optimise_ir!(ir, show_ir=false) (positional) but the real signature is
    optimise_ir!(ir; show_ir=false, do_inline=true) (keyword-only; do_inline undocumented).
    Documented internal → the rendered call form throws MethodError. Fix: correct the
    signature and document do_inline.


Robustness

  • consume leaves taped_globals in the calling task's TLS. src/copyable_task.jl:520.
    consume sets task_local_storage(TASK_VARIABLE_KEY, …) on the caller and never clears it,
    so after any consume(t), get_taped_globals(::Type{T}) called outside a TapedTask stops
    throwing NotInTapedTaskError and returns the stale value — contradicting both docstrings.
    (The existing test passes only because it is ordered first.) Fix: save/clear the key in a
    try/finally around the inner call, or weaken the docstrings to state the key persists.

  • get_function(x::Expr) = eval(x) runs arbitrary IR sub-expressions. src/transformation.jl:67,
    from stmt_might_produce's :call branch. Result only feeds a conservative
    isa(f, Union{IntrinsicFunction,Builtin,DataType}) test, so eval is both unnecessary and a
    side-effect/world-age hazard. Fix: for an unresolved Expr callee, conservatively return
    true (might-produce) instead of evaluating it.

  • No _find_id_uses!(::Switch) method. src/bbcode.jl:699-724. characterise_used_ids
    would under-report IDs used only in Switch.conds (latent today: Switch is synthesized after
    the single call site and removed before lowering, but replace_ids does handle Switch, so
    the asymmetry reads as a bug). Fix: add a Switch method mirroring IDPhiNode, or comment
    the invariant.

  • necessary_ref_ids dedup relies on a non-local invariant. src/refelim.jl:184.
    vcat(length(refs), …) force-includes the resume-block ref; correct only because that ref is
    never touched via get_ref_at/set_ref_at!. Fix: use a set union so dedup is explicit.


Concurrency / contract

  • set_taped_globals! over-restricts the new value's type, contradicting the docs.
    src/copyable_task.jl:499,217. The method …(t::TapedTask{T}, ::T) and the parametric field
    taped_globals::Ttaped_globals pin the type to construction time, so
    set_taped_globals!(TapedTask(nothing, f), 5) throws a MethodError. The docstring says "set …
    to anything you like." Fix: either widen the field/signature, or correct the docstring (and
    the wrong signature shown in the set_taped_globals! docstring) to state the type is fixed.

  • generate_ir mutates the shared _id_count outside build_callable_lock.
    src/copyable_task.jl:138. It calls seed_id!() and runs the full IR-deriving pipeline
    unlocked — a second derivation path outside the lock the concurrency note (Serialize build_callable under a global lock to prevent segfaults in threaded usage #227) forbids.
    Debug-only, but violates the invariant. Fix: wrap the ID-generating body in
    build_callable_lock.

  • GlobalMCCache's internal ReentrantLock is dead and its docstring misattributes
    thread-safety.
    src/copyable_task.jl:228-243. All cache access is already under
    build_callable_lock; only setindex! takes the inner lock (haskey/getindex don't), so it
    never contends and the comment points at the wrong mechanism. Fix: drop the inner lock (thin
    Dict wrapper) or lock all accessors + correct the docstring to credit build_callable_lock.


Performance (build-time only)

  • get_block_id_from_int linear-scans per resume site → ~O(blocks × resume_sites).
    src/refelim.jl:57. Fix: build an int → ID dict once before the loop.

  • Redundant copy of an already-fresh slice. src/refelim.jl:321
    vcat(typeof(new_refs), copy(ir.argtypes[2:end])); the slice already copies. Fix: drop copy.


Tests

  • allocs perf flag is a silent no-op for empty-result cases. src/test_utils.jl:52-64.
    The allocation loop iterates iteration_results, so cases with [] expected results assert
    nothing — yet "no produce", "kwarg tester 1/2", and both "default kwarg tester" are marked
    allocs. Fix: loop 1:(length(results)+1) (also measure the terminating consume), or
    downgrade those cases to none.

  • Duplicate test name + redundant case. src/test_utils.jl:217-227. Two cases share the
    name "default kwarg tester"; kwargs=nothing and kwargs=(;) construct the identical task.
    Fix: rename / drop the redundant one.


Trivial

  • Dead assignment new_argtypes = copy(ir.argtypes) overwritten on the next line. src/transformation.jl:979 — delete it.
  • throw(error(msg)) double-wraps (and is inconsistent with sibling bare error(...) sites). src/transformation.jl:77,692 — use error(msg).
  • might_produce opt-in API documented but not exported/public. src/Libtask.jl / docs/src/index.md:31-35. Consider public might_produce, might_produce_if_sig_contains, @might_produce (guarded for Julia ≥ 1.11).
  • 1.12.0-prerelease @warn has no maxlog → floods logs in tight TapedTask-construction loops. src/copyable_task.jl:437 — add maxlog=1.
  • Typo "hanled" → "handled". src/bbcode.jl:646 (inherited from Mooncake).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions