Enhance Regex Support in Standard Library for JS Backend #592

leonfuss · 2024-09-16T14:27:21Z

This pull request significantly improves regex support in Effekt's standard library for the JavaScript backend. It introduces capture groups, group indices, and regex flags, addressing current limitations and providing a more powerful regex interface.

Key Enhancements:

Support for capture groups and their indices
Implementation of regex flags
Stateful regex object for sequential matching

Current Limitations:
Effekt's regex support is currently limited, lacking support for capture groups and advanced regex features. This implementation bridges this gap for JavaScript environments.

New Features:

Capture Groups: Ability to use and access capture groups in regex patterns
Group Indices: Retrieve the starting index of each capture group
Regex Flags: Support for standard regex flags (e.g., 'i' for case-insensitive matching)
Stateful Matching: Global flag ('g') set by default for sequential matching

Backwards compatibility
This change is not backwards compatible.

Feedback on the API design and usability from experienced Effekt developers is highly appreciated.

jiribenes

Thanks for the contribution! :)
I can't review it fully, but I have a few nitpicks I pointed out in the review: most of them are just stylistic.

What I would really like to see before merging are a few relevant tests in effekt/examples/stdlib/, feel free to make a new folder.
Our tests are always regression tests, so there's an .effekt file and a corresponding .check file. You can either just print out the results (see here) or use the test suite written in Effekt (see here).
The tests are especially important if you're planning to depend on this library going forward -- otherwise we won't notice a regression.

libraries/common/option.effekt

libraries/js/text/regex.effekt

jiribenes · 2024-09-16T15:24:17Z

The tests are failing since examples/casestudies/lexer.effekt.md uses text/regex.
Here are some more modules importing text/regex: https://github.com/search?q=repo%3Aeffekt-lang%2Feffekt+%22text%2Fregex%22+path%3Aexamples%2F**%2F*.effekt&type=code

leonfuss · 2024-09-17T12:13:26Z

The tests are failing since examples/casestudies/lexer.effekt.md uses text/regex. Here are some more modules importing text/regex: https://github.com/search?q=repo%3Aeffekt-lang%2Feffekt+%22text%2Fregex%22+path%3Aexamples%2F**%2F*.effekt&type=code

I refactored the lexer example to account for the changes. The other occurrences shouldn't be affected since I'm only changing the JS backend? Please correct me if I oversaw anything.

leonfuss · 2024-09-17T12:17:16Z

Thank you for your thorough review. I've implemented your suggestions, making the code more idiomatic. The addition of typed flags should notably enhance usability. I plan to add the test cases tomorrow.

libraries/js/text/regex.effekt

jiribenes

I went through the PR again and LGTM
Thank you for the contribution :)

leonfuss · 2024-09-18T08:56:16Z

I ran into some problems designing the test. While the LSP server and I cannot find any error in the code, the compiler fails with the error [error] Cannot typecheck call. Unfortunately it doesn't specify the failed type check. I experimented now for one hour and could not get it working.

@jiribenes If you see any obvious problems why it doesn't type check I really would appreciate your help :)
Otherwise I will just experiment around until I get a more expressive error :/

jiribenes · 2024-09-18T08:59:21Z

I'll take a look. This error usually means that UFCS (foo.bar(baz) standing for bar(foo, baz)) failed.

examples/stdlib/string/regex.effekt

leonfuss · 2024-09-18T09:18:32Z

Is there a way to run certain tests only for the JS-Backend? I would like to add test for the new features, but it seems that the chez-backend is not happy with that. At least I have no trouble running the test succesful on my machine using only the JS-Backend.

b-studios · 2024-09-18T09:25:32Z

It looks like this test even fails on JS:

https://github.com/effekt-lang/effekt/actions/runs/10919082132/job/30306075187?pr=592#step:11:402

jiribenes · 2024-09-18T09:26:07Z

Is there a way to run certain tests only for the JS-Backend? I would like to add test for the new features, but it seems that the chez-backend is not happy with that. At least I have no trouble running the test succesful on my machine using only the JS-Backend.

You can add the file into ignored here for the backends it should not run on together with a comment why it's disabled: https://github.com/effekt-lang/effekt/blob/master/effekt/jvm/src/test/scala/effekt/StdlibTests.scala

So for example for Chez-$something backends:

abstract class StdlibChezTests extends StdlibTests {
  override def ignored: List[File] = List(
    // Not implemented yet
    examplesDir / "stdlib" / "bytes",
    examplesDir / "stdlib" / "io",

    // Not implemented: advanced regex features
    examplesDir / "stdlib" / "string" / "regex.effekt",
  )
}

jiribenes · 2024-09-18T09:28:33Z

It looks like this test even fails on JS:

https://github.com/effekt-lang/effekt/actions/runs/10919082132/job/30306075187?pr=592#step:11:402

It seems to work for me locally so it might be a NodeJS version issue?
Did the API for regexes change there?

EDIT: it seems that the last part of the test labelled "capture groups" throws some error somewhere.

EDIT 2: I think that NodeJS 12.x does not support the d (GenerateIndices()) flag since it was only introduced in ES9 and implemented in NodeJS 16.x according to MDN.

jiribenes · 2024-09-18T09:57:15Z

Don't know how to resolve this if we want to keep the functionality. We could update NodeJS: that might be a wise move anyway since even the extended support for NodeJS 12.x ended 2 years and 4 months ago (30 Apr 2022). Current oldest somewhat supported LTS version is NodeJS 18.x (where security support ends in 7 months).
[I'd also be fine with updating to "just" NodeJS 16.x]

leonfuss · 2024-09-18T09:57:20Z

EDIT 2: I think that NodeJS 12.x does not support the d (GenerateIndices()) flag since it was only introduced in ES9 and implemented in NodeJS 16.x according to MDN.

I just tested it locally and can confirm that. I will try to add a condition that the d flag will only works if the node version is >16.x and change the test to only run if node is >16.x

Do you see any problems with that?

b-studios · 2024-09-23T12:34:36Z

We can just require a newer node version, that shouldn't be a problem.

jiribenes · 2024-09-23T12:34:36Z

libraries/js/text/regex.effekt

+  function process$version() {
+    const v = process.version
+    const parts = v.split('.')
+    return parseInt(parts[0].replace('v', ''))
+  }


As a tiny nitpick: according to StackOverflow, process.versions.node should have the version without v :)
https://stackoverflow.com/questions/6656324/check-for-current-node-version

(though I haven't tried it on NodeJS 12.x, it might be too new 🤔)

b-studios · 2024-09-23T12:38:31Z

Thanks for the PR! I think it is great to have improved regex support in the language.

I have one major concern, which is the compatibility with the LLVM backend. In general, I do not care so much anymore about the Chez and ML backends. However, we should aim for feature completeness of the LLVM backend.

Adding JS-only features now will make this more and more difficult in the future.

@phischu what is your opinion when viewing this from the LLVM perspective?

b-studios · 2024-09-23T12:42:18Z

Which of the extensions that are proposed in this PR are compatible with the regular expressions in POSIX?

(https://www.man7.org/linux/man-pages/man3/regex.3.html)

jiribenes · 2024-09-23T12:52:58Z

master is updated to NodeJS 16.x via #599, you can just revert the last commit and rebase :)

b-studios · 2024-09-23T13:22:39Z

To clarify: I am not proposing that you should implement the LLVM part. I just want to know what the intersection between posix regex and your proposed features is.

libraries/common/option.effekt

libraries/js/text/regex.effekt

b-studios · 2024-09-23T13:35:55Z

libraries/js/text/regex.effekt

-record Match(matched: String, index: Int)
+// matches and groups
+record Range(start: Int, end: Int)
+record Group(matched: String, index: Option[Range])


Can we just always generate indices and turn this into index: Range?

That's definitely possible and should decrease complexity

libraries/js/text/regex.effekt

b-studios · 2024-09-23T13:41:22Z

libraries/js/text/regex.effekt

+namespace internal {
+  extern io def unsafeExec(reg: Regex, str: String): RegexMatch = js "regex$exec(${reg}, ${str})"
+
+  extern pure def unsafeRegex(str: String, flags: String): Regex = js "new RegExp(${str}, ${flags})"


This needs to be io (or global, not sure which one this should be), because otherwise the same regex will potentially be inlined over and over again and not have the "stateful" behavior.

Thank you for the clarification. I sometimes still struggle with FFI boundaries in effekt

b-studios · 2024-09-23T13:42:34Z

libraries/js/text/regex.effekt

+ * The returned Match object contains:
+ * - matched: The full matched string.
+ * - start: The starting index of the match.
+ * - end: The ending index of the match if `Global` flag is set, otherwise 0
+ * - groups: A list of Group objects for each capturing group.


Maybe move this documentation to Match and refer to it here?

Also, why is end 0 if global isn' set?

libraries/js/text/regex.effekt

leonfuss · 2024-10-28T14:15:54Z

Which of the extensions that are proposed in this PR are compatible with the regular expressions in POSIX?

(https://www.man7.org/linux/man-pages/man3/regex.3.html)

The POSIX standard for Regex does only include basic support for capture groups. Only the extraction of the content is possible here, but generating indices for those is not. Stateful matching is definitely not known in the POSIX world.

Flags are a bit more difficult in compatibility. Usually flags are passed as additional command line arguments which makes it dependent on the used program if they are compatible or not.

Does that answer the question?

leonfuss · 2024-10-28T14:17:07Z

u can just revert the last commit and rebase

Thank you :)

Update libraries/js/text/regex.effekt Update libraries/common/option.effekt Co-authored-by: Jonathan Immanuel Brachthäuser <jonathan@b-studios.de>

leonfuss · 2024-11-06T15:08:26Z

I have implemented the following changes as suggested previously:

The Global Flag is now handled directly. The exec function returns a Regex object that captures the string in the capture field. This allows subsequent calls on the returned object to iterate (lazily) through all matches. A shorthand for map on the Regex Object is provided, which maps directly onto the captured Match for an ergonomic API. This also eliminates the need for a global or io modifier to prevent inlining. Both attributes previously prevented the use of regex queries in areas where these effects are not allowed.
The examples have been added to the tests and verified to work as expected.
The GenerateIndices flag has been removed as an option and is now enabled by default.

Please feel free to share your thoughts on the current design.

leonfuss · 2024-11-06T15:22:24Z

Currently, the tests fail for both LLVM and the Chez backend because they lack the flags interface. To proceed, I will disable the regex tests on those backends. As long as the regex engine of a backend supports flags, implementing those should be straightforward, as the only stateful Flag (Global) is now directly expressed in Effekt.

jiribenes reviewed Sep 16, 2024

View reviewed changes

jiribenes added the area:js label Sep 16, 2024

jiribenes reviewed Sep 17, 2024

View reviewed changes

libraries/js/text/regex.effekt Outdated Show resolved Hide resolved

jiribenes previously approved these changes Sep 18, 2024

View reviewed changes

leonfuss commented Sep 18, 2024

View reviewed changes

examples/stdlib/string/regex.effekt Outdated Show resolved Hide resolved

jiribenes reviewed Sep 23, 2024

View reviewed changes

jiribenes mentioned this pull request Sep 23, 2024

Update NodeJS to 16.x #599

Merged

b-studios reviewed Sep 23, 2024

View reviewed changes

libraries/common/option.effekt Outdated Show resolved Hide resolved

b-studios reviewed Sep 23, 2024

View reviewed changes

libraries/js/text/regex.effekt Outdated Show resolved Hide resolved

b-studios reviewed Sep 23, 2024

View reviewed changes

libraries/js/text/regex.effekt Outdated Show resolved Hide resolved

b-studios reviewed Sep 23, 2024

View reviewed changes

libraries/js/text/regex.effekt Outdated Show resolved Hide resolved

jiribenes force-pushed the master branch from ee9d209 to 58c8510 Compare October 1, 2024 18:11

leonfuss force-pushed the feat/regex/capture_groups branch 2 times, most recently from e981f37 to 5d34ec2 Compare October 28, 2024 14:36

leonfuss and others added 16 commits October 30, 2024 13:33

added js regex capturegroups

183a04a

refactor option/zip

d57c4ff

adhere to indent styling of two spaces

3ffbb1a

rename debug to show and move internals into own namespace

a0cf0d8

use explicit types for regex flags

68d89d7

refactor Match.show()

884a4e9

adapt casestudies/lexer to new regex interface

d577e62

use const for js regex invocation

eae9693

change default behaviour to not use global flags

413c1d1

Rename regex group 'content' to 'matched'

179564a

add test cases

23ac87c

fix test cases and require Global flag for end-index

5a4bbac

Apply style suggestions

5e7d8cd

Update libraries/js/text/regex.effekt Update libraries/common/option.effekt Co-authored-by: Jonathan Immanuel Brachthäuser <jonathan@b-studios.de>

change pure to io to prevent inlining

c24d09b

always use 'd' flag

16c68f2

add example as test case

aab54e3

leonfuss force-pushed the feat/regex/capture_groups branch from 6459232 to aab54e3 Compare October 30, 2024 12:34

leonfuss added 3 commits November 6, 2024 14:20

redesign interface for handling global flag in effekt

6bd26f8

improved api for capture string

aa87f6a

update documentation

41e208c

leonfuss requested a review from b-studios November 6, 2024 15:14

disable regex tests for llvm and chez

c6997f0

leonfuss force-pushed the feat/regex/capture_groups branch from 95eebc4 to c6997f0 Compare November 6, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Regex Support in Standard Library for JS Backend #592

Enhance Regex Support in Standard Library for JS Backend #592

leonfuss commented Sep 16, 2024 •

edited

Loading

jiribenes left a comment •

edited

Loading

jiribenes commented Sep 16, 2024

leonfuss commented Sep 17, 2024

leonfuss commented Sep 17, 2024

jiribenes left a comment

leonfuss commented Sep 18, 2024

jiribenes commented Sep 18, 2024

leonfuss commented Sep 18, 2024

b-studios commented Sep 18, 2024

jiribenes commented Sep 18, 2024 •

edited

Loading

jiribenes commented Sep 18, 2024 •

edited

Loading

jiribenes commented Sep 18, 2024 •

edited

Loading

leonfuss commented Sep 18, 2024

b-studios commented Sep 23, 2024

jiribenes Sep 23, 2024 •

edited

Loading

b-studios commented Sep 23, 2024

b-studios commented Sep 23, 2024

jiribenes commented Sep 23, 2024 •

edited

Loading

b-studios commented Sep 23, 2024

b-studios Sep 23, 2024

leonfuss Oct 28, 2024

b-studios Sep 23, 2024 •

edited

Loading

leonfuss Oct 28, 2024 •

edited

Loading

b-studios Sep 23, 2024

leonfuss commented Oct 28, 2024

leonfuss commented Oct 28, 2024

leonfuss commented Nov 6, 2024

leonfuss commented Nov 6, 2024

Enhance Regex Support in Standard Library for JS Backend #592

Are you sure you want to change the base?

Enhance Regex Support in Standard Library for JS Backend #592

Conversation

leonfuss commented Sep 16, 2024 • edited Loading

jiribenes left a comment • edited Loading

Choose a reason for hiding this comment

jiribenes commented Sep 16, 2024

leonfuss commented Sep 17, 2024

leonfuss commented Sep 17, 2024

jiribenes left a comment

Choose a reason for hiding this comment

leonfuss commented Sep 18, 2024

jiribenes commented Sep 18, 2024

leonfuss commented Sep 18, 2024

b-studios commented Sep 18, 2024

jiribenes commented Sep 18, 2024 • edited Loading

jiribenes commented Sep 18, 2024 • edited Loading

jiribenes commented Sep 18, 2024 • edited Loading

leonfuss commented Sep 18, 2024

b-studios commented Sep 23, 2024

jiribenes Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

b-studios commented Sep 23, 2024

b-studios commented Sep 23, 2024

jiribenes commented Sep 23, 2024 • edited Loading

b-studios commented Sep 23, 2024

b-studios Sep 23, 2024

Choose a reason for hiding this comment

leonfuss Oct 28, 2024

Choose a reason for hiding this comment

b-studios Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

leonfuss Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

b-studios Sep 23, 2024

Choose a reason for hiding this comment

leonfuss commented Oct 28, 2024

leonfuss commented Oct 28, 2024

leonfuss commented Nov 6, 2024

leonfuss commented Nov 6, 2024

leonfuss commented Sep 16, 2024 •

edited

Loading

jiribenes left a comment •

edited

Loading

jiribenes commented Sep 18, 2024 •

edited

Loading

jiribenes commented Sep 18, 2024 •

edited

Loading

jiribenes commented Sep 18, 2024 •

edited

Loading

jiribenes Sep 23, 2024 •

edited

Loading

jiribenes commented Sep 23, 2024 •

edited

Loading

b-studios Sep 23, 2024 •

edited

Loading

leonfuss Oct 28, 2024 •

edited

Loading