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

Fixes variable binding in object destructuring #78

Conversation

wheene
Copy link
Contributor

@wheene wheene commented May 18, 2021

ref #77

This change makes ObjectMatcher keep entries put into accumulator
unchanged until they are emitted. This makes entries put into the accumulator
by nested ObjectMatcher visible to subsequent matchers and eventually
to the expression where whole accumulated capture is emitted.

From the four tests I added two failed before the change and now pass to demonstrate the fix,
the other two I added as sanity check to make sure I did not break any previous behavior.

If I understand your use of accumulator in the PatternMatcher correctly the only problem
was that it got cleared too eagerly in ObjectMatcher which made bindings
made in nested matchers invisible after returning from the recursive call.

ref eiiches#77

This change makes ObjectMatcher keep entries put into accumulator
unchanged until they are emitted. This makes entries put into the accumulator
by nested ObjectMatcher visible to subsequent matchers and eventually
to the expression where whole accumulated capture is emitted.
@wheene
Copy link
Contributor Author

wheene commented Jun 7, 2021

Hi @eiiches
have you had time to look at the proposed fix?
Please let me know if there are some problems I need to address before it can be accepted.

@eiiches
Copy link
Owner

eiiches commented Jun 7, 2021

Sorry it's taking me so long to respond. I'll take a look tomorrow.

@eiiches
Copy link
Owner

eiiches commented Jun 8, 2021

These still seems to be an issue with the pattern matching:

$ java -jar jackson-jq-cli/target/jackson-jq-cli-0.0.13-SNAPSHOT.jar -c -n '{outer: {a: 1, b: 2}, c: 3} as {outer: {("a", "b"): $x}, c: $y} | [$x, $y]'
[1,3]

$ jq-1.6 -c -n '{outer: {a: 1, b: 2}, c: 3} as {outer: {("a", "b"): $x}, c: $y} | [$x, $y]'
[1,3]
[2,3]

, which I think could be a separate issue and I might as well just merge this PR for now. But before that, let me look into this a bit more.

@eiiches
Copy link
Owner

eiiches commented Jun 8, 2021

I think now I understand the issue correctly. The proper fix is probably like:

        private void recursive(final Scope scope, final JsonNode in, final Functional.Consumer<List<Pair<String, JsonNode>>> out, final Stack<Pair<String, JsonNode>> accumulate, final boolean emit, int index) throws JsonQueryException {
-               if (index >= matchers.size())
+               if (index >= matchers.size()) {
+                       out.accept(accumulate);
                        return;
+               }

                final Pair<JsonQuery, PatternMatcher> kvexpr = matchers.get(index);
                final JsonQuery keyexpr = kvexpr._1;
@@ -36,10 +38,10 @@ public class ObjectMatcher implements PatternMatcher {
                        final JsonNode value = in.get(key.asText());

                        final int size = accumulate.size();
-                       matcher.match(scope, value != null ? value : NullNode.getInstance(), out, accumulate, emit && index == matchers.size() - 1);
-                       recursive(scope, in, out, accumulate, emit, index + 1);
-                       if(emit) // value in accumulate needs to remain visible until it is emitted
-                               accumulate.setSize(size);
+                       matcher.match(scope, value != null ? value : NullNode.getInstance(), (match) -> {
+                               recursive(scope, in, out, accumulate, emit, index + 1);
+                       }, accumulate, true);
+                       accumulate.setSize(size);

Can you update the PR so I can merge? I think I'm gonna add a couple of tests later.

@eiiches
Copy link
Owner

eiiches commented Jun 8, 2021

Oh, there's the same problem in ArrayMatcher as well...

$ java -jar jackson-jq-cli/target/jackson-jq-cli-0.0.13-SNAPSHOT.jar -c -n '[{outer: {a: 1, b: 2}, c: 3}, 2] as [{outer: {("a", "b"): $x}, c: $y}, $z] | [$x, $y, $z]'
jq: error: Undefined variable: $z

$ jq-1.6 -c -n '[{outer: {a: 1, b: 2}, c: 3}, 2] as [{outer: {("a", "b"): $x}, c: $y}, $z] | [$x, $y, $z]'
[1,3,2]
[2,3,2]

@eiiches
Copy link
Owner

eiiches commented Jun 8, 2021

Let me merge this PR with my commit added. Thanks for reporting the issue and sending this PR, @wheene !

@eiiches eiiches merged commit a6c35dc into eiiches:develop/0.x Jun 8, 2021
@eiiches
Copy link
Owner

eiiches commented Jun 8, 2021

I'm going to port this fix to 1.x branch as well and release both 0.x and 1.x versions.

@wheene
Copy link
Contributor Author

wheene commented Jun 10, 2021

Thanks for merging the PR and quick release!

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.

2 participants