Skip to content

Matcher.groupCount behaviour inconsistent between JS and JVM #3280

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
Closed
nrinaudo opened this issue Feb 1, 2018 · 6 comments
Closed

Matcher.groupCount behaviour inconsistent between JS and JVM #3280

nrinaudo opened this issue Feb 1, 2018 · 6 comments
Labels
bug Confirmed bug. Needs to be fixed. help wanted The core team will not work on this issue. Help from the community is wanted.
Milestone
v0.6.26

Comments

@nrinaudo
Copy link

nrinaudo commented Feb 1, 2018

The following works fine under both platforms:

val matcher = Pattern.compile("\\d+").matcher("2147483647")

matcher.find()

matcher.groupCount

The following fails in js but not jvm:

val matcher = Pattern.compile("\\d+").matcher("2147483647")

//matcher.find()

matcher.groupCount

There's nothing in the Matcher scaladoc that seems to imply that calling find before groupCount is needed.

It does, however, seem like an unfixable bug. I'm no javascript expert, but it does rather look that regexes can only expose the group count for a given match, not for a matcher.
@sjrd sjrd added the bug Confirmed bug. Needs to be fixed. label Feb 2, 2018
@sjrd
Copy link
Member

sjrd commented Feb 2, 2018

Indeed, we need an existing match in JS to be able to obtain the group count. We could invent a matching string to produce a match, but producing such a string is non-trivial. However, we could in theory do a shallow parse of the regex to count the number of non-capturing groups.

I'll leave that open and up for grabs by the community.

@sjrd sjrd added the help wanted The core team will not work on this issue. Help from the community is wanted. label Feb 2, 2018
@sjrd sjrd changed the title Matcher.groupCount behaviour inconsistent between js and jvm Matcher.groupCount behaviour inconsistent between JS and JVM Jun 19, 2018
@neVERberleRfellerER
Copy link
Contributor

Since
https://www.ecma-international.org/ecma-262/6.0/#sec-regexpbuiltinexec
points 19 nad 20 are always hit and set result size to NcapturingParens+1 and
https://www.ecma-international.org/ecma-262/6.0/#sec-notation
states that "NcapturingParens is the total number of left capturing parentheses" then it should be actually easier to just create all-matching regexp by prepending "|". I have it here
https://github.com/neVERberleRfellerER/scala-js/commit/720a36b44bc9adc774f17138aa06e8eb9beae8e9
Tests passing

@sjrd
Copy link
Member

sjrd commented Oct 12, 2018

PR welcome 🙂

@neVERberleRfellerER
Copy link
Contributor

PR to master or 0.6.x? I am not sure if fix of this is breaking change. I can imagine that there is code somewhere in dark corner of the Galaxy that uses exception from calling groupCount to check, if find has been already called or not, calling find in catch and retrying.

@sjrd
Copy link
Member

sjrd commented Oct 12, 2018

0.6.x. It doesn't matter that it could be breaking someone's code, since we're aligning with the JVM. That always goes to 0.6.x.

@neVERberleRfellerER neVERberleRfellerER mentioned this issue Oct 12, 2018
Merged
sjrd added a commit that referenced this issue Oct 15, 2018
@sjrd
Merge pull request #3468 from neVERberleRfellerER/fix-matcher-group-c…
de34cee 
…ount

Fix #3280: Count capturing groups independently on find.
@sjrd
Copy link
Member

sjrd commented Oct 15, 2018

Fixed in e5d3d6b.

@sjrd sjrd closed this as completed Oct 15, 2018
@sjrd sjrd added this to the v0.6.26 milestone Oct 15, 2018
@sjrd sjrd mentioned this issue Mar 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Confirmed bug. Needs to be fixed. help wanted The core team will not work on this issue. Help from the community is wanted.
Projects
None yet
Milestone
v0.6.26
Development

No branches or pull requests
3 participants
@sjrd @nrinaudo @neVERberleRfellerER