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

Fix try/catch catches more than it should #1859 #2750

Merged

Conversation

nicowilliams
Copy link
Contributor

@nicowilliams nicowilliams commented Jul 22, 2023

Rename the FORK_OPT opcode to TRY_BEGIN, add a TRY_END opcode, and wrap errors when raising through a TRY_END so that they will not be caught by the matching TRY_BEGIN.

Now a try exp catch handler expression generates code like:

TRY_BEGIN handler
<exp>
TRY_END
JUMP past_handler
handler: <handler>
past_handler:
...

On backtrack through TRY_BEGIN it just backtracks.

If anything past the whole thing raises when produced a value, then the TRY_END will catch the error, wrap it in another, and backtrack. The TRY_BEGIN will see a wrapped error and then it will unwrap and re-raise the error.

If raises, then TRY_BEGIN will catch the error and jump to the handler, but the TRY_BEGIN will not stack_save() in that case, so on raise/backtrack the TRY_BEGIN will not execute again (nor will the TRY_END).


Resolves #1859, resolves #1885, resolves #2140, resolves #2220, resolves #2485, resolves #2073.

@nicowilliams
Copy link
Contributor Author

This is a straight-forward cherry-pick of ee36e9a from my old dlopen branch. There were minimal conflicts. Let's see if it works (er, well, it's failing jqtest for me locally, so it needs more work).

@nicowilliams nicowilliams added this to the 1.7 release milestone Jul 22, 2023
@emanuele6
Copy link
Member

It does not matter much, but, probably because of the way you have ./configure-ed your build locally, in parser.c, all the #line directives have the hardcoded full path to your parser.y/parser.c files (/home/nico/ws/jq/src/parser.y) instead of just src/parser.y; and instead of the boring #ifndef YY_YY_SRC_PARSER_H_INCLUDED, it now uses #ifndef YY_YY__HOME_NICO_WS_JQ_SRC_PARSER_H_INCLUDED. =D

tests/jq.test Outdated Show resolved Hide resolved
src/parser.c Outdated Show resolved Hide resolved
@nicowilliams
Copy link
Contributor Author

It does not matter much, but, probably because of the way you have ./configure-ed your build locally, in parser.c, all the #line directives have the hardcoded full path to your parser.y/parser.c files (/home/nico/ws/jq/src/parser.y) instead of just src/parser.y; and instead of the boring #ifndef YY_YY_SRC_PARSER_H_INCLUDED, it now uses #ifndef YY_YY__HOME_NICO_WS_JQ_SRC_PARSER_H_INCLUDED. =D

😆

That's pretty funny. It's probably because I used --srcdir=/home/nico/ws/jq, except using --srcdir=.. didn't help. Hmm.

@nicowilliams nicowilliams force-pushed the fix_try_catch_catches_too_much branch from 15c5fbe to 8a60d40 Compare July 23, 2023 04:41
@nicowilliams
Copy link
Contributor Author

I think the issues above are fixed.

@nicowilliams nicowilliams force-pushed the fix_try_catch_catches_too_much branch from 8a60d40 to 6052fee Compare July 23, 2023 04:53
@nicowilliams nicowilliams marked this pull request as ready for review July 23, 2023 22:06
@nicowilliams nicowilliams requested review from emanuele6 and wader July 23, 2023 22:07
@itchyny
Copy link
Contributor

itchyny commented Jul 23, 2023

@itchyny
Copy link
Contributor

itchyny commented Jul 23, 2023

Here's also an example; jq -n 'first(.?,.?)' that might have been fixed by this PR.

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Jul 23, 2023

@itchyny nice finds! I'll check them out and add tests based on them.

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Jul 23, 2023

Here's also an example; jq -n 'first(.?,.?)' that might have been fixed by this PR.

Oh, that's ugly, but no, this PR wouldn't fix that. This PR is strictly about try/catch, and first/1 doesn't use try/catch. and yes, this fixes it.

@nicowilliams
Copy link
Contributor Author

nicowilliams commented Jul 24, 2023

Looks like I might have cherry-picked the wrong commit. See #1885 (comment).

Close jqlang#1885, jqlang#2140, jqlang#2011, jqlang#2220, jqlang#2485, 2073

Rename the FORK_OPT opcode to TRY_BEGIN, add a TRY_END opcode, and wrap
errors when raising through a TRY_END so that they will not be caught by
the matching TRY_BEGIN.

Now a `try exp catch handler` expression generates code like:

    TRY_BEGIN handler
    <exp>
    TRY_END
    JUMP past_handler
    handler: <handler>
    past_handler:
    ...

On backtrack through TRY_BEGIN it just backtracks.

If anything past the whole thing raises when <exp> produced a value,
then the TRY_END will catch the error, wrap it in another, and
backtrack.  The TRY_BEGIN will see a wrapped error and then it will
unwrap and re-raise the error.

If <exp> raises, then TRY_BEGIN will catch the error and jump to the
handler, but the TRY_BEGIN will not stack_save() in that case, so on
raise/backtrack the TRY_BEGIN will not execute again (nor will the
TRY_END).
@nicowilliams nicowilliams force-pushed the fix_try_catch_catches_too_much branch from 6052fee to db93945 Compare July 24, 2023 01:30
@nicowilliams
Copy link
Contributor Author

@itchyny I think this is ready now.

Copy link
Member

@emanuele6 emanuele6 left a comment

Choose a reason for hiding this comment

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

LGTM

@emanuele6 emanuele6 merged commit 1cf6515 into jqlang:master Jul 24, 2023
@emanuele6
Copy link
Member

I merged fixing 2073 => #2073 in the commit message.

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