Fix Js.String.match_ return type

[renatoalencar]

Javscript String.prototype.match function can return undefined for optional capture groups that are not found, which breaks the type annotations since this is not added as a Js.nullable, it's not possible to deal with those undefined values since it can't be passed to Js.Nullable.isNullable.

Fixes #5066.

I used the following code to test this.

let m = Js.String.match_ [%re "/hello (world)?/"] "hello word";;

let n =
  match m with
  | Some(n) -> Js.Array.filter (fun a -> not (Js.Nullable.isNullable a)) n
  | None -> [||];;

Js.log n;;

[renatoalencar]

I also mentioned that, there are a few files that have been modified, but I'm not sure if I should commit that also.

And I don't know how I add the test case above to the codebase (neither if I should).

[bobzhang]

hi @renatoalencar , thanks your contribution.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match
according to this, it will never return null. so its return type could be simplified as t option array option.
Also would you also do the same change in js_string2.match?

And I don't know how I add the test case above to the codebase (neither if I should).

Feel free to skip it

[renatoalencar]

hi @renatoalencar , thanks your contribution.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match
according to this, it will never return null. so its return type could be simplified as t option array option.
Also would you also do the same change in js_string2.match?

And I don't know how I add the test case above to the codebase (neither if I should).

Feel free to skip it

Sure. Done that.

The new test case I wrote for this:

let m = Js.String.match_ [%re "/hello (world)?/"] "hello word";;

let n =
 match m with
 | Some(n) -> Js.Array.filter Js.Option.isSome n
 | None -> [||];;

Js.log n;;

let o = Js.String2.match_ "hello word" [%re "/hello (world)?/"];;

let p =
 match o with
 | Some(q) -> Js.Array.filter Js.Option.isSome q
 | None -> [||];;

Js.log p;;

[bobzhang]

can you add the test case in jscomp/test/js_string_test.ml?

[renatoalencar]

can you add the test case in jscomp/test/js_string_test.ml?

Sure

[bobzhang]

Error message:

../linux/bsc.exe -bs-cmi -bs-cmj -bs-no-version-header  -bs-cross-module-opt -make-runtime-test -bs-package-output commonjs:jscomp/test  -w -3-6-26-27-29-30-32..40-44-45-52-60-9-106+104 -warn-error A  -I runtime -I stdlib-406 -I others   -I test  test/js_string_test.ml
File "test/js_string_test.ml", line 87, characters 44-61:
Error: This expression has type Js.String2.t option array option
       but an expression was expected of type string array option
       Type Js.String2.t option is not compatible with type string 

For the record, for CI, we should set -k 1

[renatoalencar]

Error message:

../linux/bsc.exe -bs-cmi -bs-cmj -bs-no-version-header  -bs-cross-module-opt -make-runtime-test -bs-package-output commonjs:jscomp/test  -w -3-6-26-27-29-30-32..40-44-45-52-60-9-106+104 -warn-error A  -I runtime -I stdlib-406 -I others   -I test  test/js_string_test.ml
File "test/js_string_test.ml", line 87, characters 44-61:
Error: This expression has type Js.String2.t option array option
       but an expression was expected of type string array option
       Type Js.String2.t option is not compatible with type string 

For the record, for CI, we should set -k 1

Yeah, I fixed that changed the following test case

From

"match", (fun _ ->
    Eq(Some [| "na"; "na" |], "banana" |. Js.String2.match_ [%re "/na+/g"])
  );

To

"match", (fun _ ->
    Eq(Some [| Some "na"; Some "na" |], "banana" |. Js.String2.match_ [%re "/na+/g"])
  );

And added another one:

 "match - not found capture groups", (fun _ ->
      Eq(Some [| Some "hello "; None |], "hello word" |. Js.String2.match_ [%re "/hello (world)?/"])
    );

But I'm getting the following error

      AssertionError [ERR_ASSERTION]: Expected values to be loosely deep-equal:

[
  'hello ',
  undefined
]

should loosely deep-equal

[
  'hello ',
  undefined,
  groups: undefined,
  index: 0,
  input: 'hello word'
]

The MDN docs mention extra properties for some cases, I think that's something special for groups.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match#return_value

[fakenickels]

That's harsh! I can't think of anything else that would make it possible to bind those extra properties in the array while keeping the zero cost. Maybe just ignore them for now.

[renatoalencar]

I passed the result of the match through another step like this

"hello word" |. Js.String2.match_ [%re "/hello (world)?/"] |. Belt.Option.map Js.Array.copy

So the runtime extra props are removed and the test passes. I'm not sure if this is the best solution, but let me know if there's anything I can do in order to improve that.

[bobzhang]

But I'm getting the following error

@renatoalencar That's expected, since polymorphic comparision is best effort. I will merge it after we roll out 9.1, thanks

[renatoalencar]

But I'm getting the following error

@renatoalencar That's expected, since polymorphic comparision is best effort. I will merge it after we roll out 9.1, thanks

Ok, thanks.