fix: SignalError.As reports success but never assigns the target - #42
Open
dualfroz wants to merge 1 commit into
Open
fix: SignalError.As reports success but never assigns the target#42dualfroz wants to merge 1 commit into
dualfroz wants to merge 1 commit into
Conversation
errors.As calls SignalError.As when it cannot match the concrete type directly, e.g. for `var target SignalError; errors.As(err, &target)`. The previous implementation matched target's type and returned true but never wrote to it, so callers got a successful errors.As result with a zero-valued SignalError and no indication the signal was lost. Add a regression test asserting the target is actually populated.
dualfroz
force-pushed
the
dualfroz/fix-signalerror-as-assign
branch
from
September 5, 2026 22:42
996a174 to
bbbbc56
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Staleness note
This repository merges infrequently: the last code merge to
mainwasPR #27 ("Fix bug in SignalHandler", 2024-01-27), which is also the commit
that introduced the bug fixed here. A later commit (#38, 2025-06-27) only
touched
.github/workflows/test.yamland the README, no Go source. There isno CONTRIBUTING.md, CODE_OF_CONDUCT.md, or any AI-authorship policy in the
repo or its
.github/directory as of this branch. This PR is submitted asa draft against a fork; it has not been opened upstream.
Problem
SignalError.As(target interface{}) bool(actors.go) is the customimplementation
errors.Ascalls when it can't matchSignalErrorbyconcrete type directly (see the
errors.Ascontract: "sets target to thaterror value and returns true"). The current implementation only inspects
the target's type and returns
true/falseaccordingly - it neverassigns anything to
target:So the idiomatic usage pattern
reports success (
ok == true) while silently discarding the actualos.Signalthat triggered the error. Callers who trust theerrors.Ascontract get a zero-valued
SignalErrorand no indication anything wentwrong. This is a regression in the maintainer's own prior fix (#27) for a
previously-documented design error in
SignalError(see the type's doccomment, which predates this change and is unrelated to the bug fixed here).
The project's own test suite did not catch this:
TestSignalErrorinactors_test.gocallserrors.As(err, &(SignalError{}))(line 52) andchecks only the boolean return value, never the populated struct.
Root cause
actors.go:86-93(upstream commiteee6e04, "Fix bug in SignalHandler",#27) -
SignalError.Asmatches ontarget's type but never performs theassignment
errors.Aspromises.Reproduction (empirical)
Added
TestSignalErrorAsAssignsTargettoactors_test.goand ran itagainst the unmodified source:
errors.Asreturnedtrue;target.Signalstayednil. Confirms the bugas described.
Fix
Replace the type-only switch with the standard
errors.Ascustom-implementation pattern: type-assert
targetto the only type thatcan meaningfully be assigned through it, and assign into it.
Note on the dropped
case SignalError(bare, non-pointer) branch: thestandard library's
errors.Aspanics before calling any customAsmethodunless its
targetargument is itself a non-nil pointer, so a bareSignalErrorvalue can never reach this method through the publicerrors.AsAPI - and even if it did, there would be no addressablelocation to write a result back into (an
interface{}holding a valuetype is a copy). That branch was dead code that could only return a
misleading
truewith no possibility of ever assigning; removing it doesnot change any reachable behavior and removes a second copy of the same
class of bug this PR fixes.
The
*SignalErrorcase is the one real path: it's whaterrors.Ascallswhen a caller does
var target SignalError; errors.As(err, &target), sincethe concrete type returned by
SignalHandler(*SignalError) is notdirectly assignable to
target's pointed-to type (SignalError), forcingerrors.Asdown theAs(interface{}) boolpath.(The other common pattern,
var target *SignalError; errors.As(err, &target),already worked correctly before this fix and is untouched by it:
errors.Asresolves it via a direct concrete-type match before ever calling
As.)