Skip to content

Use Error type for exceptions#1

Merged
paf31 merged 3 commits into
masterfrom
error
Aug 7, 2014
Merged

Use Error type for exceptions#1
paf31 merged 3 commits into
masterfrom
error

Conversation

@paf31

@paf31 paf31 commented Aug 7, 2014

Copy link
Copy Markdown
Contributor

@garyb

garyb commented Aug 7, 2014

Copy link
Copy Markdown
Member

This should perhaps use the global Error type? https://github.com/purescript/purescript-globals/blob/master/src/Global.purs#L29-L42

I know this fixes the type safety problem, but it doesn't fix the issue of catching exceptions from pattern matches and the like. I guess that doesn't matter if we're saying exceptions are "anything that could go wrong in JavaScript land or explicitly thrown PureScript exceptions"?

@paf31

paf31 commented Aug 7, 2014

Copy link
Copy Markdown
Contributor Author

Does anything currently use that type? Can we move it into exceptions? If you accept the premise that "Errors were made to be thrown" then they sort of live here naturally :)

@garyb

garyb commented Aug 7, 2014

Copy link
Copy Markdown
Member

node-fs does, but actually I'd probably change that to use exceptions anyway (stuff in Sync returns Eff (...) (Either Error a) at the moment, moving it into the effect would be much nicer obviously), and foreign has a readError instance.

paf31 added a commit that referenced this pull request Aug 7, 2014
Use Error type for exceptions
@paf31 paf31 merged commit b45edc4 into master Aug 7, 2014
@paf31 paf31 deleted the error branch August 7, 2014 23:22

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the expected semantics here?

When would e not be an Error? If someone throws something that isn't in the Error hierarchy? Could we wrap that thing in an Error and handle it immediately, rather than throwing something we can't actually catch?

if (e instanceof Error) {
  return c(e)();
} else {
  return c(new Error(e))();
}

As it is now. This is a runtime exception we can't actually handle.

Unfortunately, it's also not a matter of wrapping the library throwing the non-error and calling it a day. This check will fail in NodeWebKit, since it uses two different contexts with different prototypes. So, what is thrown as an Error on the node side, will not be an instance of the window's Error because the prototypes are different. There's no way around this prototypal difference

I understand you shouldn't be concerned with what some random library does, but I think this happens for anything with a different context, like iframes, so, this might be more painful than first thought.

@paf31

paf31 commented Sep 1, 2014

Copy link
Copy Markdown
Contributor Author

I'm not sure I understand your NodeWebkit example, sorry. The reason I check the type is that someone might just throw "Error!" in library code, which would become a type error at runtime if someone tries to use the caught value to read the stack trace, for example.

@garyb

garyb commented Sep 1, 2014

Copy link
Copy Markdown
Member

I think he's saying there are two different Error constructors in some circumstances (due to sandboxing), depending on whether you're on the node or webkit side of the fence, so trying to catch errors from one in the other will mean they go through uncaught.

@paf31

paf31 commented Sep 1, 2014

Copy link
Copy Markdown
Contributor Author

Hmm. Is there any way we can identify those? Using toString maybe?

Maybe rethrowing everything as an Error is better, but I guess you don't get a proper stacktrace.

@joneshf

joneshf commented Sep 2, 2014

Copy link
Copy Markdown
Member

@garyb that's exactly it. The NodeWebKit example is just one, but any time you have more than one window you run into the same thing.

@paf31, that should work for Error thrown from different . Object.prototype.toString.call(e).slice(8, 1) === 'Error'

For other things, can we wrap them in a new Error and be on our way? I think that's more what people are expecting when looking at the type of things than a runtime error because some library down the chain is doing bad things. Also, without it you're not getting any stack trace or message at all.

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.

Fix Exceptions

3 participants