Revamp sub/3 to resolve most issues with gsub (and sub with "g") #2641

pkoppstein · 2023-06-29T06:05:49Z

The primary purpose of this commit (which supercedes PR #2624) is to rectify most problems with gsub (and also sub with the "g" option), in particular fix #1425 ('\b'), fix #2354 (lookahead), and fix #2532 (regex == "^(?!cd ).*$|^cd ";"")).

This commit also partly resolves #2148 and also resolves #1206 in that gsub no longer loops infinitely; however, because the new gsub depends critically on match(_;"g"), the behavior when regex == "" is sometimes non-standard. [*1]

Since the new sub/3 relies on uniq/1, that has been added as well [*2].

The documentation has been updated to reflect the fact that sub and gsub are intended to be regular in the second argument. [*3]

Also, _nwise/1 has been tweaked to take advantage of TCO.

Footnotes:

[*1] Using the new gsub, '"a" | gsub( ""; "a")' emits "aa" rather than "aaa" as would be standard. This is nevertheless better than the infinite loop behavior of jq 1.6 in this case.

With one exception (as explained in [*2]), the new gsub is implemented as though match/2 behavior is correct. That is, bugs in gsub behavior will most likely have their origin in match/2.

[*2] uniq/1 adopts the Unix/Linux name and semantics; it is needed for the following test case:

gsub("(?=u)"; "u")
"qux"
"quux"

Without this functionality:

Test #23: 'gsub("(?=u)"; "u")' at line number 100
*** Expected "quux", but got "quuux" for test at line number 102: gsub("(?=u)"; "u")

The root of the problem here is match: if match is fixed, then gsub would not need untie.

The addition of uniq as a top-level function should be a non-issue relative to general concern about builtins.jq bloat: the line count of the new builtin.jq is significantly reduced overall, and the number of defs is actually reduced by 1 (from 111 (ignoring a redundant def) to 110).

[*3] See e.g. #513 (comment)

… uniq(stream) The primary purpose of this commit (which supercedes PR jqlang#2624) is to rectify most problems with `gsub` (and also `sub` with the "g" option), in particular jqlang#1425 ('\b'), jqlang#2354 (lookahead), and jqlang#2532 (regex == "^(?!cd ).*$|^cd ";"")). This commit also partly resolves jqlang#2148 and jqlang#1206 in that `gsub` no longer loops infinitely; however, because the new `gsub` depends critically on match(_;"g"), the behavior when regex == "" is sometimes non-standard. [*1] Since the new sub/3 relies on uniq/1, that has been added as well [*2]. The documentation has been updated to reflect the fact that `sub` and `gsub` are intended to be regular in the second argument. [*3] Also, _nwise/1 has been tweaked to take advantage of TCO. Footnotes: [*1] Using the new gsub, '"a" | gsub( ""; "a")' emits "aa" rather than "aaa" as would be standard. This is nevertheless better than the infinite loop behavior of jq 1.6 in this case. With one exception (as explained in [*2]), the new gsub is implemented as though match/2 behavior is correct. That is, bugs in `gsub` behavior will most likely have their origin in `match/2`. [*2] `uniq/1` adopts the Unix/Linux name and semantics; it is needed for the following test case: gsub("(?=u)"; "u") "qux" "quux" Without this functionality: Test jqlang#23: 'gsub("(?=u)"; "u")' at line number 100 *** Expected "quux", but got "quuux" for test at line number 102: gsub("(?=u)"; "u") The root of the problem here is `match`: if `match` is fixed, then gsub would not need `untie`. The addition of `uniq` as a top-level function should be a non-issue relative to general concern about builtins.jq bloat: the line count of the new builtin.jq is significantly reduced overall, and the number of defs is actually reduced by 1 (from 111 (ignoring a redundant def) to 110). [*3] See e.g. jqlang#513 (comment)

pkoppstein · 2023-06-29T07:16:00Z

@itchyny - Please note that the failing checks almost certainly have nothing to do with the changes in this PR.

Please also note that in the case of "no match" examples such as "p" | gsub("q"; "a","b"), this PR currently follows gojq, but that it would only require a small change to one line to produce what I would argue is actually the "correct" behavior (in the sense of regularity in the second position).

pkoppstein · 2023-07-01T19:42:56Z

@itchyny - as expected, the failing tests had nothing to do with this PR. Since “all checks have passed”, it would be a good time to pull, before merging becomes very complex, wouldn’t it?

itchyny · 2023-07-01T23:10:03Z

docs/content/manual/manual.yml

+          named captures. The named captures are, in effect, presented
+          as a JSON object (as constructed by `capture`) to
+          `tostring`, so a reference to a captured variable named "x"
+          would take the form: `"\(.x)"`.

        example:
          - program: 'sub("^[^a-z]*(?<x>[a-z]*).*")'
            input: '"123abc456"'
            output: '"ZabcZabc"'


This example is invalid and somehow not included in man.test. There is no sub/1 and output should always be an array. Since you added examples, would you rebuild man.test to check if the CI passes?

As per notes elsewhere, this example is very old, and was always wrong. It has been replaced.

itchyny · 2023-07-01T23:13:39Z

docs/content/manual/manual.yml

@@ -1428,6 +1428,30 @@ sections:
            input: '[{"foo":1, "bar":14}, {"foo":2, "bar":3}]'
            output: ['{"foo":2, "bar":3}']

+      - title: "`uniq(stream)`"


I'm still disagree with this naming, I'm very worried about this confuses the users. Consistency within the builtin functions is much important than external command name conventions. We need to remember that uniq is a stream oriented and unique is an array oriented. There're already filters having both array and stream versions like first so that's good.

itchyny · 2023-07-01T23:16:35Z

docs/content/manual/manual.yml

        body: |

          `gsub` is like `sub` but all the non-overlapping occurrences of the regex are
-          replaced by the string, after interpolation.
+          replaced by `tostring`, after interpolation. If the second argument is a stream
+          of jq strings, then `gsub` will produce a corresponding stream of JSON strings.


Adding an example will help users understand this sentence.

Example added in 5373191

itchyny · 2023-07-01T23:17:45Z

tests/onig.test

@@ -75,6 +75,45 @@ gsub( "(.*)"; "";  "x")
 ""
 ""

+gsub( ""; "a";  "g")


Nitpick but inconsistent spacing.

itchyny · 2023-07-01T23:30:24Z

src/builtin.jq

+# in mind that s could be a stream
+def sub($re; s; $flags):
+   . as $in
+   | (reduce uniq(match($re; $flags)) as $edit


Following patch isn't enough to fix the duplicate bug?

diff --git a/src/builtin.c b/src/builtin.c index 9b2d9a2..03ab4d5 100644 --- a/src/builtin.c +++ b/src/builtin.c @@ -930,7 +930,7 @@ static jv f_match(jq_state *jq, jv input, jv regex, jv modifiers, jv testmode) { match = jv_object_set(match, jv_string("string"), jv_string("")); match = jv_object_set(match, jv_string("captures"), jv_array()); result = jv_array_append(result, match); - start += 1; + start = (const UChar*)(input_string+region->end[0]+1); continue; }

$ ./jq -nc '"qqqqqqux" | match("(?=u)"; "g")' {"offset":6,"length":0,"string":"","captures":[]}

itchyny · 2023-07-01T23:32:58Z

src/builtin.jq

-def _nwise(a; $n): if a|length <= $n then a else a[0:$n] , _nwise(a[$n:]; $n) end;
-def _nwise($n): _nwise(.; $n);
+def _nwise($n):
+  def n: if length <= $n then . else .[0:$n] , (.[$n:] | n) end;


We can use omitting else . by flipping the condition.

def _nwise($n): def _nwise: if length > $n then .[:$n], (.[$n:] | _nwise) end; _nwise;

Being able to omit else is not in jq 1.6, so my reason for sticking with the 1.6 syntax is that it should be easy for someone familiar with 1.6 to understand code that might well be "copied and pasted" into a jq program, or indeed a jaq program.

itchyny · 2023-07-01T23:54:13Z

src/builtin.jq

+            | [reduce ( $edit | .captures | .[] | select(.name != null) | { (.name) : .string } ) as $pair
+                 ({}; . + $pair) | s ] as $inserts
+            | reduce range(0; $inserts|length) as $ix (.; .result[$ix] += $gap + $inserts[$ix])
+	    | .previous = ($edit | .offset + .length ) )


Inconsistent tab indentation.

Hmmm. This seems to me to be a case of apples and oranges. The first reduce occurs in the form [reduce ... ] as $_. Anyway, in my view, it's fine to put a reduce (or an if..then..else..end) on one line if its fits. But feel free to tweak :-)

I just noticed you're mixing space and tab indentations...

Sorry about that. Two tabs in builtin.jq removed in pkoppstein@3015ec8

itchyny · 2023-07-01T23:57:13Z

src/builtin.jq

+            | reduce range(0; $inserts|length) as $ix (.; .result[$ix] += $gap + $inserts[$ix])
+	    | .previous = ($edit | .offset + .length ) )
+          | .result[] + $in[.previous:] )
+      // $in;


Removing // $in passes the tests, but this is necessary. Would you add a test that doesn't match?

↑ I thought this but this is actually unnecessary!

Please see detailed notes elsewhere regarding the three variants of this line. Assuming you want gsub fixed in the next release, I'd suggest merging and then tweaking, otherwise this "branch" will fall too far behind and be quite difficult to manage later as there are so many files involved, two or three of which are the subject of many commits.

builtin.c: f_match: Zero-width match : ensure '"qux" | match("(?=u)"; "g")' matches just once rm uniq/1 as it is no longer needed In manual.yml, replace nonsensical sub/1 example

Correct one gsub example, and add another

Remove two tab characters

itchyny · 2023-07-02T23:22:27Z

src/builtin.c

@@ -930,7 +930,8 @@ static jv f_match(jq_state *jq, jv input, jv regex, jv modifiers, jv testmode) {
        match = jv_object_set(match, jv_string("string"), jv_string(""));
        match = jv_object_set(match, jv_string("captures"), jv_array());
        result = jv_array_append(result, match);
-        start += 1;
+	// ensure '"qux" | match("(?=u)"; "g")' matches just once
+	start = (const UChar*)(input_string+region->end[0]+1);


There is tab indentation here but we prefer spaces.

Fixed (in 832734b); also fixed the editor setting.

untabify

itchyny

LGTM!

pkoppstein · 2023-07-03T02:35:52Z

@itchyny wrote:

LGTM!

Thanks. I manually forced the restarting of the checking process, and now there's a sign "All checks have passed" again. But github also tells me:

You’re not authorized to merge this pull request

itchyny · 2023-07-03T03:37:33Z

@pkoppstein Can you kick the ci again (by rebasing again?) to make sure you can merge?

pkoppstein · 2023-07-03T04:11:55Z

@itchyny - As expected, my git/github skills are not up to the task.

In fact, I may have made things worse. Using GitHub Desktop, I clicked on "Update from upstream/master"
which resulted in two new commits that I didn't intend.

However, GitHub tells me now that "This branch [gsub] is not behind the upstream jqlang:master".
That looks promising, doesn't it?

itchyny · 2023-07-03T04:32:25Z

@pkoppstein That's good. However , if you want to drop the merge commit, you can use git reset --hard HEAD^ and git rebase upstream/master.

pkoppstein · 2023-07-03T04:59:07Z

@itchyny wrote:

That's good.

Apparently, good enough for you to merge into jqlang:

I'll try to avoid messing things up in the meantime.

If something needs adjusting, please feel free to make changes: "Maintainers are allowed to edit this pull request."

itchyny · 2023-07-03T07:44:04Z

If you don't mind, I'll merge this PR. Would you rephrase the PR description (which contains the outdated descriptions about uniq)? Note that fix #XXX links the issue toe be closed by this PR so the that's important. Thanks.

pkoppstein · 2023-07-03T13:42:27Z

@itchyny wrote:

If you don't mind, I'll merge this PR

Since I don’t have the permissions to do so, that would be great. I think it would be best if you made any adjustments when doing so. I will undoubtedly just make a mess of things. Tx.

emanuele6 · 2023-07-18T05:03:54Z

I think there is a bug in this implementation: If you use the "g" flag, and the pattern matches up to the end of the string, it will replace both that match and the empty string at the end of the string if the pattern is able to match the empty string:

"1" | gsub("1*"; "2")
# "22"

EDIT: actually, that is a bug with _match_impl/3

pkoppstein · 2023-07-18T05:36:20Z

I think there is a bug in this implementation ... "1" | gsub("1*"; "2")

Interestingly, 7 of the 8 regex engines at regex101.com indicate that "1" ~ /1*/ twice, evidently because
/1*/ eats the 1, and then matches the EOS as well. I wonder why it's Rust that's the odd one out here.

Perhaps the the moral of the story is that users should avoid using "g" in cases like this.

Mention jqlang#2641

jqlang#2641

pkoppstein requested a review from itchyny June 29, 2023 06:05

pkoppstein mentioned this pull request Jun 29, 2023

revamp sub/3 to resolve most issues with gsub (and sub with "g") #2624

Closed

itchyny requested changes Jul 2, 2023

View reviewed changes

itchyny added this to the 1.7 release milestone Jul 2, 2023

pkoppstein added 3 commits July 2, 2023 00:29

builtin.c: fix zero-width bug, so uniq/1 is no longer needed

d8cd39e

builtin.c: f_match: Zero-width match : ensure '"qux" | match("(?=u)"; "g")' matches just once rm uniq/1 as it is no longer needed In manual.yml, replace nonsensical sub/1 example

manual.yml: gsub examples

5373191

Correct one gsub example, and add another

untabify builtin.jq

3015ec8

Remove two tab characters

itchyny changed the title ~~revamp sub/3 to resolve most issues with gsub (and sub with "g"); add…~~ Revamp sub/3 to resolve most issues with gsub (and sub with "g") Jul 2, 2023

itchyny reviewed Jul 2, 2023

View reviewed changes

builtin.c: untabify

832734b

untabify

itchyny approved these changes Jul 3, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into gsub

37f7ec5

itchyny merged commit 83f375c into jqlang:master Jul 3, 2023

baodrate mentioned this pull request Jul 6, 2023

sub and gsub when the second argument is a stream 01mf02/jaq#89

Open

itchyny mentioned this pull request Jul 9, 2023

Attempt to fix gsub with zero length matches #2564

Closed

emanuele6 mentioned this pull request Jul 18, 2023

Initialise debug_cb{,_data} in jq_init(); Populate captures also for zero-width matches; In the manual, define examples in the "examples" field not in "example" #2724

Merged

pkoppstein added a commit to pkoppstein/jq that referenced this pull request Jul 29, 2023

NEWS.md: sub/3 and gsub

5a4cbf4

Mention jqlang#2641

pkoppstein mentioned this pull request Jul 31, 2023

faster version of _nwise #1572

Closed

itchyny linked an issue Jul 31, 2023 that may be closed by this pull request

faster version of _nwise #1572

Closed

pkoppstein added a commit to pkoppstein/jq that referenced this pull request Aug 1, 2023

NEWS.md - sub and gsub

b6d434c

jqlang#2641

pkoppstein mentioned this pull request Aug 1, 2023

NEWS.md - sub and gsub #2809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp sub/3 to resolve most issues with gsub (and sub with "g") #2641

Revamp sub/3 to resolve most issues with gsub (and sub with "g") #2641

pkoppstein commented Jun 29, 2023 •

edited by itchyny

Loading

pkoppstein commented Jun 29, 2023

pkoppstein commented Jul 1, 2023

itchyny Jul 1, 2023

pkoppstein Jul 2, 2023

itchyny Jul 1, 2023

itchyny Jul 1, 2023

pkoppstein Jul 2, 2023

itchyny Jul 1, 2023

itchyny Jul 1, 2023

itchyny Jul 1, 2023

pkoppstein Jul 2, 2023

itchyny Jul 1, 2023

pkoppstein Jul 2, 2023

itchyny Jul 2, 2023

pkoppstein Jul 2, 2023

itchyny Jul 1, 2023

itchyny Jul 2, 2023 •

edited

Loading

pkoppstein Jul 2, 2023

itchyny Jul 2, 2023

pkoppstein Jul 3, 2023

itchyny left a comment

pkoppstein commented Jul 3, 2023 •

edited

Loading

itchyny commented Jul 3, 2023

pkoppstein commented Jul 3, 2023 •

edited

Loading

itchyny commented Jul 3, 2023

pkoppstein commented Jul 3, 2023 •

edited

Loading

itchyny commented Jul 3, 2023

pkoppstein commented Jul 3, 2023

emanuele6 commented Jul 18, 2023 •

edited

Loading

pkoppstein commented Jul 18, 2023

Revamp sub/3 to resolve most issues with gsub (and sub with "g") #2641

Revamp sub/3 to resolve most issues with gsub (and sub with "g") #2641

Conversation

pkoppstein commented Jun 29, 2023 • edited by itchyny Loading

pkoppstein commented Jun 29, 2023

pkoppstein commented Jul 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itchyny Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itchyny left a comment

Choose a reason for hiding this comment

pkoppstein commented Jul 3, 2023 • edited Loading

itchyny commented Jul 3, 2023

pkoppstein commented Jul 3, 2023 • edited Loading

itchyny commented Jul 3, 2023

pkoppstein commented Jul 3, 2023 • edited Loading

itchyny commented Jul 3, 2023

pkoppstein commented Jul 3, 2023

emanuele6 commented Jul 18, 2023 • edited Loading

pkoppstein commented Jul 18, 2023

pkoppstein commented Jun 29, 2023 •

edited by itchyny

Loading

itchyny Jul 2, 2023 •

edited

Loading

pkoppstein commented Jul 3, 2023 •

edited

Loading

pkoppstein commented Jul 3, 2023 •

edited

Loading

pkoppstein commented Jul 3, 2023 •

edited

Loading

emanuele6 commented Jul 18, 2023 •

edited

Loading