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.

Sign up for GitHub

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

Jump to bottom

What should '. |= try . catch .' do in jq 1.6, and why is it different from jq 1.5? #2011

Closed
sshine opened this issue Nov 15, 2019 · 9 comments
Closed

What should '. |= try . catch .' do in jq 1.6, and why is it different from jq 1.5? #2011

sshine opened this issue Nov 15, 2019 · 9 comments

Comments

@sshine
Copy link

sshine commented Nov 15, 2019 •
edited
Loading

Describe the bug
Unless |= in combination with try-catch have changed semantics, this is probably a bug?

$ jq-1.5 '. |= try . catch .' <<< 1
1

$ jq-1.6 '. |= try . catch .' <<< 1
{
  "__jq": 0
}

Update: I understand from #658 that {"__jq": 0} is some internal control mechanism.

To Reproduce
See above or visit https://jqplay.org/s/M_RpdNHvHF for jq 1.6.

Expected behavior
Unless I'm mistaken, this should return 1 in both versions.

Note that neither . |= . nor try . catch . alone produce this effect.

Environment (please complete the following information):

Additional context
Discovered here:

https://stackoverflow.com/questions/58879588/jq-try-catch-goes-directly-to-catch-even-when-no-error
@wtlangford
Copy link
Contributor

This... does seem incorrect. I'll see what I can figure out.

@wtlangford
Copy link
Contributor

The behavior change would have been introduced here: bd7b48c

It's a weird interaction of nesting labels and a try/catch that trips up a bug in the way we generate code for the catch handler: https://github.com/stedolan/jq/blob/master/src/compile.c#L1041

The issue is that when we look for the internal errors jq uses to handle certain types of control flow jumps (in particular around labels), we intend to check for the type of the error to be an object, but instead we compare the error to "object". You can see this by running try error("object") catch ., which attempts to index into that string to test if we've got an internal error. That results in the internal error getting returned as the value to you.

I'll push a fix later tonight.

@wtlangford
Copy link
Contributor

I pushed e74eab8, which fixes this. Thanks for the report!

@pkoppstein
Copy link
Contributor

@wtlangford - Great work as always. Thanks.

I thought it was time to recreate an instance of jq "from scratch" and sure enough ran into some issues. After the first failure, I installed pipenv (brew install pipenv), and reran make, which failed as follows:

✔ Successfully created virtual environment! 
Virtualenv location: /Users/peter/.local/share/virtualenvs/docs-utiv-7IS
Traceback (most recent call last):
  File "build_manpage.py", line 4, in <module>
    from lxml import etree
ModuleNotFoundError: No module named 'lxml'
make[2]: *** [jq.1] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

I suppose I'm supposed to install somehow but pip3 says it's already installed:

pip3 install lxml
Requirement already satisfied: lxml in /usr/local/lib/python3.7/site-packages (4.4.1)

Maybe I need to dig deeper into the (revised?) installation instructions, but I'm disappointed that what used to be a painless process has seemingly become somewhat fraught. Am I missing something?

@wtlangford
Copy link
Contributor

@pkoppstein- yeah, that's an issue I'm aware of. It also prevents people from doing brew install jq --HEAD.
Essentially, I didn't set up that whole ruby replacement the way I meant to. Can you try the branch in #1995 and see how it works for you? It changes a few things about how the build process works and only requires a python environment if you make changes to the manpage yml.

@wtlangford
Copy link
Contributor

wtlangford commented Nov 22, 2019 via email

@pkoppstein
Copy link
Contributor

@wtlangford: Whoops - I just let SourceTree clone the repo without checking which branch it was checking out.

So, after checking out the relevant version, things ran successfully for a while,
but then the dreaded 'lxml' dragon raised its fearsome head:

....
checking for Python dependencies... Warning: the environment variable LANG is not set!
We recommend setting this in ~/.profile (or equivalent) for proper expected behavior.
Creating a virtualenv for this project…
...
✔ Successfully created virtual environment!
...
  GEN      jq.1
Traceback (most recent call last):
  File "build_manpage.py", line 4, in <module>
    from lxml import etree
ModuleNotFoundError: No module named 'lxml'
make[2]: *** [jq.1] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

@wtlangford
Copy link
Contributor

wtlangford commented Nov 22, 2019 via email

@pkoppstein
Copy link
Contributor

@wtlangford - git show begins:

commit dd87c3b9d7acf4efaced0a8eccb534df82c6cf27 (HEAD -> fix-brew-head-build, origin/fix-brew-head-build)
Author: William Langford <[email protected]>
Date:   Tue Oct 22 12:37:19 2019 -0400

    Rework pipenv requirement to be more relaxed
    
    Keep a cached copy of the man tests that we can use when no manpage
    changes are made. This allows automated systems that might not have
    easy access to a pipenv to build and run tests.

@wtlangford wtlangford mentioned this issue Nov 22, 2019
Merged
@wtlangford wtlangford mentioned this issue Jun 11, 2020
Closed
nicowilliams added a commit to nicowilliams/jq that referenced this issue Jul 24, 2023
@nicowilliams
Fix try/catch catches more than it should jqlang#1859
db93945 
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).
emanuele6 pushed a commit that referenced this issue Jul 24, 2023
@nicowilliams
Fix try/catch catches more than it should #1859
1cf6515 
Close #1885, #2140, #2011, #2220, #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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sshine @pkoppstein @wtlangford