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

Interpreter: Catch Invalid_argument more locally #1736

Merged
merged 2 commits into from
Jul 17, 2020
Merged

Conversation

nomeata
Copy link
Collaborator

@nomeata nomeata commented Jul 17, 2020

previously, we would catch Invalid_argument and turn it into a Trap
very coarsly. The coarse catch around interpret_block was added in
1e1757a
but now also catches Invalid_argument exceptions that should not be
caught.

Instead, we need to to try … with precisely the points where we expect
an Invalid_argument to stand for something trapping. In this case
Prim.prim.

Prim.prim can't raise trap itself, as Trap is defined in the two
interpreters, and because it doesn't have accees to the exp.at.

So this wraps invocations to Prim.prim, following similar patters for
binop etc.

It also simplifies the type signature of Prim.num_conv_prim, making it
clear that this is just a self-contained call-by-value function.
Otherwise, we’d still catch Invalid_argument exceptions raised by the
continuation that num_conv_prim invokes.

The same should be applied to Prim.prim, but we can’t, because
Array.tabulate is higher order and inovkes a function. But still, this
is an improvement of the status quo.

See https://dfinity.slack.com/archives/CPL67E7MX/p1594979459058400?thread_ts=1594977500.055800&cid=CPL67E7MX

previously, we would catch `Invalid_argument` and turn it into a `Trap`
very coarsly. The coarse `catch` around `interpret_block` was added in
1e1757a
but now also catches `Invalid_argument` exceptions that should _not_ be
caught.

Instead, we need to to `try … with` precisely the points where we expect
an `Invalid_argument` to stand for something trapping. In this case
`Prim.prim.`

`Prim.prim` can't raise trap itself, as `Trap` is defined in the two
interpreters, and because it doesn't have accees to the `exp.at`.

So this wraps invocations to `Prim.prim`, following similar patters for
`binop` etc.

It also simplifies the type signature of `Prim.num_conv_prim`, making it
clear that this is just a self-contained call-by-value function.
Otherwise, we’d still catch `Invalid_argument` exceptions raised by the
_continuation_ that `num_conv_prim` invokes.

The same _should_ be applied to `Prim.prim`, but we can’t, because
`Array.tabulate` is higher order and inovkes a function. But still, this
is an improvement of the status quo.

See https://dfinity.slack.com/archives/CPL67E7MX/p1594979459058400?thread_ts=1594977500.055800&cid=CPL67E7MX
@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Jul 17, 2020
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

@crusso
Copy link
Contributor

crusso commented Jul 17, 2020

I think the correct thing to do here as @grreif has suggested in the past, is to pass success and failure continuations to the prims and do the right thing. Related to #1530

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

Changed my mind because I wanted to check something...

I was wondering whether we should actually give OtherPrims access to the failure continuation, but for now this is good.

@mergify mergify bot merged commit 4e4e347 into master Jul 17, 2020
@mergify mergify bot deleted the joachim/local-catch branch July 17, 2020 11:30
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 17, 2020
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.

3 participants