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

doAssertRaises: stmt catch all, including foreign exceptions #15940

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Nov 12, 2020

this PR adds an overload for doAssertRaises that catches all exceptions (including foreign ones which wasn't possible before this PR with doAssertRaises, which is useful for cpp and js backends):

doAssertRaises: fn()

#15765 (comment)

This overload overloads untyped on an inconsistent position so it's unlikely to work well.

overloading works in this case, each overload has a distinct number of non-optional parameters; tests check for this.

@Araq
Copy link
Member

Araq commented Nov 13, 2020

overloading works in this case, each overload has a distinct number of non-optional parameters; tests check for this.

But it violates the spec... Also, why do we need it? If you write a test case with doAssertRaises why not specify which exception should be raised?

@timotheecour
Copy link
Member Author

timotheecour commented Nov 13, 2020

If you write a test case with doAssertRaises why not specify which exception should be raised?

because this won't work with foreign exceptions as I explained in #15765 (comment) (see P2)

But it violates the spec...

I'm not sure which section of spec explicitly forbids this case but I think you'll agree the spec should allow overriding in this case:

EDIT: I just remembered #14827 ; let me think whether there's 0 ambiguity still holds in this case (or whether this statement stays true with more precise wording)

EDIT: https://nim-lang.github.io/Nim/manual.html#overloading-resolution-lazy-type-resolution-for-untyped says: "But one has to watch out because other overloads might trigger the argument's resolution:"
in the case of this PR, it's fine since each argument is essentially a disguised typed param; maybe that could be the distinction that makes this overloading sane: when untyped params are disguised typed params (undergo semantic analysis), then overloading can happen even if position of untyped params don't match

@Clyybber
Copy link
Contributor

Clyybber commented Nov 13, 2020

because this won't work with foreign exceptions as I explained in #15765 (comment) (see P2)

But those work with doAssertRaises(void): ... no?

@timotheecour
Copy link
Member Author

But those work with doAssertRaises(void): ... no?

no:

when defined case2:
  {.emit:"""
  #include <stdexcept>
  void fn(){throw std::runtime_error("asdf");}""".}
  proc fn(){.importcpp.}
  doAssertRaises(void): fn()

gives: Error: only a 'ref object' can be raised

(note that in 1st commit of #15765 (see ab6dc1f) I was supporting void to fit this bill, but @Araq decided against it)

@timotheecour timotheecour force-pushed the pr_doAssertRaises_catch_all branch from e22d1f4 to 91834fa Compare January 15, 2021 08:30
@timotheecour
Copy link
Member Author

ping @Araq I'd like to use this in #16727:

    var ok = false
    try: discard big"2" ** big"-1" # raises foreign `RangeError`
    except: ok = true
    doAssert ok

=>

    doAssertRaises: discard big"2" ** big"-1"

@stale
Copy link

stale bot commented Jan 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Jan 16, 2022
@stale stale bot closed this Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants