Closed coot closed 12 months ago
While addressing this issue in #4660, I put together a document enumerating and contextualizing the uses of error
. In lieu of putting that document in the repo itself, I'm duplicating it here for posterity. If desired, it could be migrated it to a wiki page, but I have no issue leaving it here.
error
We began this work by branching at 77b54c71ca67a759f82872791749fea549ee3bc7
, committed 18 August 2023.
Per Mark's advice, I have classified files into two tiers.
The first tier consists of files in the following packages, excluding those used in testing within those packages:
ouroboros-network
ouroboros-network-api
ouroboros-network-protocols
ouroboros-network-framework
The second tier consists of files in the following packages, excluding those used in testing within those packages:
cardano-client
cardano-ping
monoidal-synchronisation
network-mux
ntp-client
ouroboros-network-mock
ouroboros-network-testing
For the moment, we are not focusing on uses of error
in testing files within these packages. We consider error
to be more plausibly useful in testing than in production code, and its removal to be less necessary. If need be, we can create a third tier of files consisting of those used in testing across all packages.
Tier 1 files contain 17 uses of error
.
ouroboros-network/demo/chain-sync.hs:577
nextState [] = error "chainSyncServer: impossible"
The list on which nextState
is called originates from chainGenerator
in the same file. nextState
will never see an empty list because chainGenerator
creates an infinite list.
To avoid mentioning error
here, I've lifted the chainGenerator
result into an Infinite
construction, courtesy of infinite-list
.
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor/Types.hs:674
error $ "A pick policy requested an attribute for peer address "
++ " which is outside of the set given to pick from"
This use of error
appears in pickPeers
, which is a "PickPolicy
wrapper function". pickPeers
is provided:
PeerSelectionState
containing i.a. localRootPeers
, publicRootPeers
, and knownPeers
PickPolicy
called pick
Set peeraddr
called available
A PickPolicy
is "an action that picks a subset of elements from a [collection] of peers". It makes this choice by examining each peer in this collection and examining them via several functions (generally of the shape peeraddr -> _
) it is provided.
pickPeers
provides pick
, its PickPolicy
, with functions of its own creation. Those functions check their input peer for membership in localRootPeers
, publicRootPeers
, and knownPeers
, and trigger the error if the peer is not found in knownPeers
, but pick
will only ever apply them to peers in available
. Therefore, the error may be triggered if a peer in available
is not in knownPeers
(i.e. if available
is partially disjoint from knownPeers
).
In practice, available
appears to be constructed from the various peer collections in a PeerSelectionState
, whose invariant (as described in assertPeerSelectionState
in this same module) ensures that knownPeers
is a superset of all other peer sets, so this error is quite unlikely to be triggered as long as any extant PeerSelectionState
adheres to its invariant.
I've added documentation at this error
site of the rationale I've derived, and have included a reference to the invariant-documenting function at the declaration site of PeerSelectionState
.
I've also added a precondition that checks whether available
is a subset of knownPeers
.
ouroboros-network/src/Ouroboros/Network/PeerSelection/Governor.hs:544
-- impossible since guardedDecisions always has something to wait for
error "peerSelectionGovernorLoop: impossible: nothing to do"
The comment is correct, but I will elaborate on why.
guardedDecisions
's result is of type Guarded
:
data Guarded m a =
GuardedSkip !(Maybe (Min Time))
| Guarded' !(Maybe (Min Time)) (FirstToFinish m a)
Guarded
has a Semigroup
instance:
instance Alternative m => Semigroup (Guarded m a) where
Guarded' ta a <> Guarded' tb b = Guarded' (ta <> tb) (a <> b)
Guarded' ta a <> GuardedSkip tb = Guarded' (ta <> tb) a
GuardedSkip ta <> Guarded' tb b = Guarded' (ta <> tb) b
GuardedSkip ta <> GuardedSkip tb = GuardedSkip (ta <> tb)
Note that the presence of any Guarded'
-constructed value in a <>
operation dominates - that is, will guarantee a Guarded'
-constructed value.
Here, "something to wait for" means "a Guarded
value constructed via Guarded'
rather than GuardedSkip
. guardedDecisions
constructs a Guarded
value by concatenating (via <>
) several Guarded
values. At least one of these values is always constructed via Guarded'
: Ouroboros.Network.PeerSelection.Governor.Monitor.connections
. (In fact, several others are as well.) The presence of this value guarantees that guardedDecisions
produces a Guarded'
value - i.e., "something to wait for".
ouroboros-network/src/Ouroboros/Network/PeerSelection/RootPeersDNS.hs:217
Nothing -> error "localRootPeersProvider: impossible happened"
The error is triggered only if waitAnyCatchSTM
were to disobey its contract.
A version of
waitAnyCatch
that can be used inside an STM transaction".
Wait for any of the supplied asynchronous operations to complete. The value returned is a pair of the
Async
that completed, and the result that would be returned bywait
on thatAsync
.If multiple
Async
s complete or have completed, then the value returned corresponds to the first completedAsync
in the list.
waitAnyCatchSTM
, then, will always yield an asynchronous action that was part of the original list, so not finding one should necessitate a panic
. (This analysis assumes a coherent instance of Eq (Async m Void)
, one that will "correctly" compare an action as equal with itself.) The index of this asynchronous action is used to get ahold of a DomainAccessPoint
from domains
, which formed the basis of the async computation list.
To avoid mentioning error
, I've implemented withAsyncAllWithCtx
and waitAnyCatchSTMWithCtx
, which are siblings of withAsyncAll
and waitAnyCatchSTM
that pass around a context value with the results of the async computations they process. The DomainAccessPoint
that the rest of the code expects is the context value, so it needn't be looked up.
ouroboros-network/src/Ouroboros/Network/PeerSelection/PeerMetric.hs:387
Nothing -> error "impossible empty pq"
The surrounding code documents well the impossibility of this error. The code inserts an element into an IntPSQ
, then gets the minimum value from the new IntPSQ
. The new queue is guaranteed not to be empty, by IntPSQ.insert
's contract, so there will always be a minimum to find.
ouroboros-network/src/Ouroboros/Network/BlockFetch/ClientRegistry.hs:301
unless ok $ error "setFetchClientContext: called more than once"
The surrounding code tries to set a StrictTMVar
, via tryPutTMVar
, and triggers this error if the operation fails, which it does iff the StrictTMVar
is already full. I believe this error is not impossible to trigger, but instead represents a failure of program logic - accidentaly resetting some state instead of initializing it from scratch - severe enough to warrant program termination.
I say "I believe" because the surrounding function (setFetchClientContext
) is undocumented. It should probably have some commentary attesting to this logic and the rationale for this error. I've opened issue #4678 for this.
ouroboros-network/src/Ouroboros/Network/Diffusion/P2P.hs:296
error ("socketAddressType: unexpected address " ++ show addr)
The error is triggered iff the surrounding function, socketAddressType
is called on a SockAddr
that isn't SockAddrInet
- or SockAddrInet6
-constructed. I presume these applications expect IP sockets and that program termination is warranted if they see something else, so this error is logically appropriate.
However, in addition to being partial via error
, socketAddressType
is also partial via Maybe
(SockAddr -> Maybe AddressType
), which seems to be to match a pattern of other record fields (diNtnAddressType
, cmAddressType
, perhaps inter alia) being similarly partial. I have explicitly matched the remaining AddressType
constructor and replaced this error with Nothing
, per this discussion.
ouroboros-network/src/Ouroboros/Network/KeepAlive.hs:103
-- 'decisionSTM' above cannot return 'Quiesce'
Quiesce -> error "keepAlive: impossible happened"
The comment is correct, decisionSTM
only returns a Terminate
- or Continue
-constructed ControlMessage
. After discussing the ramifications of introducing a local type that mirrors just the two possible constructors of ControlMessage
, Marcin and I decided to leave this as-is.
ouroboros-network/src/Ouroboros/Network/TxSubmission/Outbound.hs:142
(Nothing, TokNonBlocking) -> error "txSubmissionOutbound: impossible happend!"
The surrounding context makes clear that this error is impossible. The matched variables are mbtxs
and blocking
, and mbtxs
is initialized based on the state of blocking
. If blocking
is TokNonBlocking
, the only possible constructor for mbtxs
is Just
.
I've refactored the code to eliminate this error by lifting more functionality into the pattern-match on blocking
.
ouroboros-network/src/Ouroboros/Network/TxSubmission/Outbound.hs:163
Nothing -> error "txSubmissionOutbound: empty transaction's list"
-- Assert txs is non-empty: we blocked until txs was non-null,
-- and we know reqNo > 0, hence take reqNo txs is non-null.
The accompanying comment explains well the impossibility of this code path. No semantic changes are required, though I have bracketed the expression take reqNo txs
for clarity.
ouroboros-network-api/src/Ouroboros/Network/PeerSelection/PeerSharing.hs:91
encodeRemoteAddress (SockAddrUnix _) = error "Should never be encoding a SockAddrUnix!"
This error is similar to no. 7, above - the code expects and supports IP sockets, and does not expect or support Unix sockets. No change is warranted.
ouroboros-network-api/src/Ouroboros/Network/AnchoredSeq.hs:810
fromMaybe (error "could not join sequences") $
The enclosing function exposes a total/irrefutable interface for join
. It passes an always-passing predicate to join
, which guarantees a Just
-constructed result from join
, so the error will never trigger. The function is documented to describe why this irrefutability is sound.
ouroboros-network-api/src/Ouroboros/Network/NodeToNode/Version.hs:174
_ -> error "decodeTerm: impossible happened!",
The error is triggered when a peerSharing
value, in a context in which it is guarded to be between 0 and 2 (| ..., peerSharing >= 0, peerSharing <= 2
), pattern-matches to a number other than 0, 1, or 2. Failure of the primary guard already triggers failure via Left
, so I've eliminated the mention of error
by reorganized the pattern-matching while preserving the existing circumstances in which failure via Left
is indicated.
ouroboros-network-api/src/Ouroboros/Network/AnchoredFragment.hs:165
anchorFromPoint GenesisPoint _ = error "anchorFromPoint: genesis point"
I don't fully understand, logically, why calling anchorFromPoint
on GenesisPoint
warrants an error. It seems like the natural result would be AnchorGenesis
, an Anchor
constructor seemingly for this purpose, rather than an error. Such a change would bring anchorFromPoint
and anchorToPoint
closer to being logical inverses of one another, if this is desirable.
I note that this function is exported but does not appear to be used elsewhere within the IOG org (per an org-wide Github search). Perhaps it could be marked deprecated and/or removed? I've opened issue #4690 to discuss the merits of this.
ouroboros-network-framework/src/Ouroboros/Network/Protocol/Handshake/Version.hs:73
[] -> error "foldMapVersions: precondition violated"
This error appears in foldMapVersions
, which converts a collection of things into a collection of Version
s (represented as a Versions
value). The precondition in question is that the collection of things be nonempty.
foldMapVersions
is, almost precisely, foldMap1
from Data.Foldable1
. I opened issue #4677 to encourage leveraging this instance once the codebase is upgraded to a version of GHC that includes this class.
ouroboros-network-framework/src/Ouroboros/Network/Snocket.hs:543
toLocalAddress _ = error "localSnocket.toLocalAddr: impossible happened"
This error is triggered if a non-Unix address is encountered in the process of creating a local snocket. Local snockets seem to rely on Unix sockets or named pipes, and cannot be constructed from IP sockets, so the error seems justified. I've filled out the function clauses to explicitly match on other socket constructors, for robustness's sake and to improve the error messages.
ouroboros-network-framework/src/Ouroboros/Network/Channel.hs:231
if full then error failureMsg
This error is triggered when trying to write to a full buffer. This behavior is intentional: the enclosing function makes its potentially partial semantics clear in its documentation, and declares itself "primarily useful for testing protocols." No changes are warranted.
Tier 2 files contain 11 uses of error
.
cardano-ping/src/Cardano/Network/Ping.hs:183
handshakeReqEnc [] _ = error "null version list"
The error is triggered when handshakeReqEnc
is provided an empty list of NodeVersion
- but its only caller, handshakeReq
, also guards against providing an empty list. I've changed handshakeReqEnc
to expect a NonEmpty NodeVersion
to eliminate the error.
network-mux/src/Network/Mux/DeltaQ/TraceStats.hs:37
Nothing -> error "step: missing referenceTimePoint"
The error is triggered when the referenceTimePoint
is Nothing
- but this eventuality is handled in a prior function guard, so the error is never triggered. More careful pattern-matching eliminates the call to error
.
network-mux/src/Network/Mux/DeltaQ/TraceStats.hs:253
= error "Infeasible sampleInterval"
This error is triggered when a developer tries to provide an unsuitable sample interval. The existing interval will not trigger the error.
network-mux/src/Network/Mux/Ingress.hs:185
Nothing -> error ("setupDispatchTable: missing " ++ show miniProtocolNum)
The error occurs if, in the building of an array (ptclArray
), an intermediate array (pnumArray
) resolves an index (miniProtocolNum
) to an "empty" (represented as Nothing
) slot.
pnumArray
is constructed such that every miniProtocolNum
in ptcls
maps to a Just
-constructed slot. ptclArray
is constructed by using every miniProtocolNum
in ptcls
to index pnumArray
, and will trigger this error on encountering Nothing
as a result of that indexing. Therefore, by construction, the error will never trigger. I've included documentation to this effect.
ntp-client/src/Network/NTP/Client.hs:105
Right NtpSyncPending -> error "ntpClientThread: impossible happened"
ntpClientThread
calls ntpQuery
and decomposes its NtpStatus
result. A NtpSyncPending
-constructed value result triggers this error - but ntpQuery
will only ever produce NtpSyncUnavailable
or NtpDrift
values.
I've created a new abstraction for NTP statuses that can only represent the two "completed" versions of an NtpStatus
, and made ntpQuery
yield that instead, so the match for the impossible case no longer exists.
ouroboros-network-mock/src/Ouroboros/Network/Mock/ConcreteBlock.hs:222
partialField n = error ("mkPartialBlock: you didn't fill in field " ++ n)
This error is used to populate fields of an intentionally-partial BlockHeader
value. The value is used as the starting point in a carefully-constructed but somewhat-brittle blockchain generation procedure relying heavily on fix
and lazy evaluation, and I am taken to believe that these errors are overwritten or otherwise left unevaluated with standard usage of this function. (For example, it seems that results of mkPartialBlockHeader
, the enclosing function, are passed immediately to fixupBlock
, which overwrites the stubbed-out fields.)
I think it would be possible to refactor this chain generation to avoid having to utter error
, but it would not be a quick fix, and the testing-based nature and use of this package leads me to believe that such a fix may not provide substantial value.
Regardless, I've changed the error message to describe the function enclosing it, which is mkPartialBlockHeader
, not mkPartialBlock
.
ouroboros-network-mock/src/Ouroboros/Network/Mock/Chain.hs:235
go _ = error "successorBlock: point not on chain"
The enclosing function, successorBlock
, tries to find the successor block of a given point on a given chain. At the moment, this error is triggered iff the point is not on the chain.
All callers already check point membership on the chain, via pointOnChain
, before calling this function. I've documented successorBlock
to alert users of the need to perform this check.
ouroboros-network-testing/src/Ouroboros/Network/Testing/Data/Script.hs:221/223
= error "interpretPickScript: given empty map to pick from"
-- ...
= error "interpretPickScript: given invalid pickNum"
interpretPickScript
is used to create PickPolicy
s to initialize a PeerSelectionPolicy
. Its two errors both ensure the same precondition that Ouroboros.Network.PeerSelection.Governor.Types.pickPeers
ensures when executing a PickPolicy
- that there are peers available to pick from, and that we are picking more than zero of them.
ouroboros-network-testing/src/Ouroboros/Network/Testing/Data/Signal.hs:251/305
linger = error "TODO: Signal.linger"
-- ...
until _ = error "TODO: Signal.until"
These are stubs of unimplemented functions. As far as I can tell, these functions are unused.
Unrecoverable errors, such as raised by error, might fall into various categories (at least)
The result of the above would be the following: