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

hasX might hide exceptions other than XException #2450

Closed
DigitalBrains1 opened this issue Apr 14, 2023 · 1 comment · Fixed by #2451
Closed

hasX might hide exceptions other than XException #2450

DigitalBrains1 opened this issue Apr 14, 2023 · 1 comment · Fixed by #2451
Labels

Comments

@DigitalBrains1
Copy link
Member

If the value evaluated with hasX contains multiple exceptions, only the first one encountered will determine the outcome of hasX.

>>> let a = errorX "XException" :> error "ErrorCall" :> Nil
>>> let b = error "ErrorCall" :> errorX "XException" :> Nil
>>> hasX a
Left "X: XException\nCallStack (from HasCallStack):\n  errorX, called at <interactive>:1:9 in interactive:Ghci2"
>>> hasX b
*** Exception: ErrorCall
CallStack (from HasCallStack):
  error, called at <interactive>:2:9 in interactive:Ghci3

In my opinion, both should have thrown an ErrorCall. The documentation for hasX describes it as fully evaluate a value. It is currently not evaluating anything beyond the XException.

Currently, hasX reduces its argument to normal form using NFData, and catches XExceptions. This checks only one of the exceptions the argument might throw. Instead, we probably need to first check for exceptions other than XException and only after that check for all exceptions (which boils down to just the XExceptions of course):

hasX :: (NFDataX a, NFData a) => a -> Either String a
hasX a =
  unsafeDupablePerformIO
    (catch
      (evaluate (rnfX a `seq` rnf a) >> return (Right a))
      (\(XException msg) -> return (Left msg)))
@christiaanb
Copy link
Member

We've released v1.8.0, which includes a fix for this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants