Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change tryGet() API misuse to Defect #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

etan-status
Copy link
Contributor

Assuming a Result[T, E] with E: Exception, tryGet has the nasty side effect of triggering {.raises: [E, ResultError[void]].}. This is because it tries to play nice when tryGet() is called on a Result that has not yet been initialized.

That is not in line with how Nim typically operates. Accessing items that are not initialized may yield to SIGSEGV in many situations.

proc f() =
  var x: ref CatchableError
  raise x

f()

tryGet() semantics are well-defined for an initialized Result.

  • If a value has been set, it is returned.
  • If an error has been set, it is raised.

Calling tryGet() on un-initialized data is not a typical usecase and could instead become a ResultDefect. This way, it is not required anymore to except + discard the ResultError[void] for hypothetical API misuse, or to alternatively pollute the callstack with {.raises.}.

The alternatiev would be to raise E(msg: ...) instead, but that one may trigger unexpected behaviour as it hides the API misuse error.

Assuming a `Result[T, E]` with `E: Exception`, `tryGet` has the nasty
side effect of triggering `{.raises: [E, ResultError[void]].}`. This is
because it tries to play nice when `tryGet()` is called on a `Result`
that has not yet been initialized.

That is not in line with how Nim typically operates. Accessing items
that are not initialized may yield to SIGSEGV in many situations.

```nim
proc f() =
  var x: ref CatchableError
  raise x

f()
```

`tryGet()` semantics are well-defined for an initialized `Result`.
- If a value has been set, it is returned.
- If an error has been set, it is raised.

Calling `tryGet()` on un-initialized data is not a typical usecase
and could instead become a `ResultDefect`. This way, it is not required
anymore to `except + discard` the `ResultError[void]` for hypothetical
API misuse, or to alternatively pollute the callstack with `{.raises.}`.

The alternatiev would be to `raise E(msg: ...)` instead, but that one
may trigger unexpected behaviour as it hides the API misuse error.
etan-status added a commit to vacp2p/nim-libp2p that referenced this pull request Jul 4, 2024
`LPError` is the top level error type of libp2p, it makes more sense
to raise a multi address specific subtype to avoid requiring callers
to catch errors too broadly. As `MaError` inherits from `LPError`,
existing error handlers will still work fine.

Requires arnetheduck/nim-results#41
@arnetheduck
Copy link
Owner

Been thinking about this one - there are two ways to reason about it:

  • it's indeed a logic fault - in this school of thought, tryGet() is the same as raises which also crashes on nil exception
  • it's not a logic fault but rather something that "can happen" - the risk in particular is the default-initialized Result when you fall off some return path that doesn't actually set the exception

Originally, the latter story was chosen simply because nobody really cared about specific exception types and "correct" raises lists while the case of the default-initialized Result was deemed highly likely to occur due to how weak Nim is at detecting uninitialized variables / returns (somewhat a consequence of default-to-0 init).

Since then, usage has gone in the direction of more explicit raises lists while the language slowly but surely is getting better at detecting lack of init.

There are ways in which the impact of this PR could be diminished:

  • not nil annotations would detect at compile-time which exceptions are not nil. In particular, the built-in exception-to-result catch infrastructure sets a non-nil Exception. Unfortunately, not nil is not being worked on in an real capacity and remains experimental.
  • a NotNil[E] = distinct E as an emulation of the above
  • enforced explicit initialization of Result when E is an exception (maybe via requiresInit)

The interesting thing perhaps is that these methods of improving the safety of tryGet are useful regardless of whether this PR is accepted or not, but one could make the argument that raises lists should represent the worst case of which errors may happen and not just the ones that actually do, ie leave the status quo of the raises effect in "uncertain" cases but work to reduce the surface area via the above ideas (or others)

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