1279545 - Add a testing function that returns a tree for RegExp pattern.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

bug 1279467's case could be caught easily if we checked a parsed tree for RegExp pattern.

I'm thinking something like this

  js> JSON.stringify(parse_regexp("[123]+", "u"), null, 2);
  {
    "type": "Quantifier",
    "min": 1,
    "max": 2147483647,
    "greedy": true,
    "body": {
      "type": "CharacterClass",
      "ranges": [
        {
          "from": 0x0031,
          "to": 0x0031
        },
        {
          "from": 0x0032,
          "to": 0x0032
        },
        {
          "from": 0x0033,
          "to": 0x0033
        }
      ]
    }
  }

will make a prototype and see if it can be done without much code change to existing parser.

Comment 1

[Tooru Fujisawa [:arai]]

Also, there are some plan to add new syntax (\p{...} and \P{...}, and maybe some more?),
this function should help while adding/testing new syntax.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: (WIP) Add parse_regexp testing function and test RegExp parse tree.

fortunately, there is almost no change required in irregexp code (only one change to add const variant of a method),
and most part is added in shell/js.cpp.

it's already working, and now adding more the test coverage.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add parse_regexp shell-only testing function.

Separated code and test.

parse_regexp function parses given pattern string (not RegExp object), flags string,  and match_only bool value,
and returns parse tree (RegExpTree, not RegExpNode) as Object.

this can be done totally outside of RegExpObject or RegExpShared, so added whole code to shell/js.cpp

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add tests for RegExp parse tree.

Added tests for each tree kinds.

each test parses pattern, and compares the result object with expected object created by helper functions.

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8802870 [details] [diff] [review]
Part 1: Add parse_regexp shell-only testing function.

Review of attachment 8802870 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. What's the reason for having it in js.cpp instead of TestingFunctions.cpp? That way it'd be available in browser tests, too, should we ever want it there. More importantly, it seems like that's the right place for a function like this.

::: js/src/shell/js.cpp
@@ +6473,5 @@
>  "  to the 'wasmEval' function together with the specified imports object."),
>  
> +#ifdef DEBUG
> +    JS_FN_HELP("parse_regexp", ParseRegExp, 3, 0,
> +"parse_regexp(pattern[, flags[, match_only])",

nit: this should use camel-case: parseRegExp for the JS function, too.

Comment 6

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Till Schneidereit [:till] from comment #5)
> Looks good to me. What's the reason for having it in js.cpp instead of
> TestingFunctions.cpp? That way it'd be available in browser tests, too,
> should we ever want it there. More importantly, it seems like that's the
> right place for a function like this.

Just wanted to put this into fuzzing_unsafe_functions, as exposing this to fuzzing doesn't make sense.
maybe I could move this to TestingFunctions.cpp and check `fuzzingSafe` variable at the top of function.

Comment 7

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #6)
> Just wanted to put this into fuzzing_unsafe_functions, as exposing this to
> fuzzing doesn't make sense.
> maybe I could move this to TestingFunctions.cpp and check `fuzzingSafe`
> variable at the top of function.

Oh, I see. Perhaps we should have support for omitting fuzzing-unsafe functions in TestingFunctions.cpp in general?

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add parse_regexp testing function.

Okay, moved to TestingFunctions.cpp, with FuzzingUnsafeTestingFunctions array.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add tests for RegExp parse tree.

fixed function name.

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 8803834 [details] [diff] [review]
Part 1: Add parse_regexp testing function.

Review of attachment 8803834 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 11

[Till Schneidereit [:till]]

Comment on attachment 8803835 [details] [diff] [review]
Part 2: Add tests for RegExp parse tree.

Review of attachment 8803835 [details] [diff] [review]:
-----------------------------------------------------------------

This is amazing, thank you so much!

Comment 12

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b212d4a114fcab72161b2616957b4dc15308c13f
Bug 1279545 - Part 1: Add parseRegExp testing function. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/46e205ee383b94bc0b4e9c6a5ac0971c007c15da
Bug 1279545 - Part 2: Add tests for RegExp parse tree. r=till

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b212d4a114fc
https://hg.mozilla.org/mozilla-central/rev/46e205ee383b