Initial RegExp #184

lemueldls · 2024-08-07T01:55:18Z

Fixes #183
Fixes #175

kaleidawave · 2024-08-07T08:13:23Z

checker/Cargo.toml

@@ -39,6 +39,7 @@ path-absolutize = { version = "3.0", features = ["use_unix_paths_on_wasm"] }
 either = "1.6"
 levenshtein = "1"
 ordered-float = "4.2"
+regress = { version = "0.10.0", features = [] }


Looks like an awesome library!

kaleidawave · 2024-08-07T08:17:48Z

checker/definitions/overrides.d.ts

+    exec(input: string): RegExpExecArray | null;
+}
+
+interface RegExpExecArray extends Array<string> {


Man JS spec is crazy. So the returned result is an array with a bunch of added string/named properties! I think this definition where an interface extends a class works 🤞

kaleidawave · 2024-08-07T08:35:40Z

checker/src/features/constant_functions.rs

+					arguments.first().unwrap().non_spread_type().expect("pattern");
+				let pattern_type = types.get_type_by_id(pattern_type_id);
+				let pattern = {
+					let Type::Constant(Constant::String(pattern)) = pattern_type else {


This is great, if the exec is run with a constant string argument then we get a constant result.

I assume this example gets always true or false warnings under this branch (where TSC doesn't)

I wonder whether this can be extended for general (ie via Type::RootPolyType or Type::Constructor) strings not through the constant result but so that .groups has named members. So new RegExp("Hi (?<name>.*)").exec(event.target.value) would have groups as { name: string }. I think TS added this, but I can't find it?

I don't know whether regress allows introspection? I don't know whether this could be a super simple scan function to pick out groups from the pattern?

If not simple then that is fine. Can merge without

Oh wait, I think this is being collected below? https://github.com/kaleidawave/ezno/pull/184/files#diff-be53d15efbd9ffdb15e107f75ef4867a569eb1ba61ecbd6cc1c468d3f49e8193R53

I will have a think about how this could work

The TS Playground example works:

kaleidawave · 2024-08-07T08:38:17Z

checker/src/types/regexp.rs

+		if let Some(flags) = flags {
+			for flag in flags.chars() {
+				match flag {
+					'd' => unimplemented!("indices for substring matches are not supported"),


Is this not something regress supports?

regress does not yet support all flags:
https://github.com/ridiculousfish/regress/blob/9f39399b0e61bee0602c791df8be468e00738100/src/api.rs#L59C1-L89C6

kaleidawave · 2024-08-07T08:40:24Z

checker/src/types/regexp.rs

+			for flag in flags.chars() {
+				match flag {
+					'd' => unimplemented!("indices for substring matches are not supported"),
+					'g' => unimplemented!("stateful regex is not supported"),


I think at the moment this can be ignored rather than panicking.

I think the eventful design of Ezno can support this in the future. For example evaluating a stateful RegExp could set a regexp.#used = true property. That would be a diagnostic that I don't think any other tool does today!

kaleidawave · 2024-08-07T08:44:17Z

checker/src/synthesis/expressions.rs

-		Expression::RegexLiteral { pattern, flags: _, position: _ } => {
-			return checking_data.types.new_regex(pattern.clone());
+		Expression::RegexLiteral { pattern, flags, position } => {
+			return checking_data.types.new_regex(pattern, flags, position);


👍 nice. RegExp constructor and regex literal have the same logic

kaleidawave · 2024-08-07T08:58:47Z

Nice! Didn't expect this so soon after I wrote that issue. This is great, really impressed that you got it a nice working example. It is a great use case for constant_functions, where the logic extends something representable in JS.

Minor things that I haven't already brought up in the review

Could types/regexp be renamed/moved to features/regexp?
Is is it possible to test whether the syntax is valid. Maybe RegExp::new could return Result (replacing the .unwrap here https://github.com/kaleidawave/ezno/pull/184/files#diff-be53d15efbd9ffdb15e107f75ef4867a569eb1ba61ecbd6cc1c468d3f49e8193R41)
Somewhere near "constant operations" in specification.md, could you add a simple match example for RegExp (+any more independent features you think could be tested)

Looks great at the moment, if any of those things are too difficult LMK. Happy to just add the constant stuff as that is a really neat feature!

checker/src/types/regexp.rs

lemueldls · 2024-08-07T21:30:08Z

I've written a constant and variable version of the exec function, though I am not sure how to test non-constant strings (like Type::RootPolyType or Type::Constructor)..

I am experimenting with utf16 matching but am running into some correctness issues. regress is very standard compliant, so I'll find a test suite to find how to correctly match ranges.

I am also running into issues with optional property access of RegExpExecArray | null types on the result of the non-constant version of exec. Not sure if I am doing unions correctly.

kaleidawave · 2024-08-08T10:01:50Z

I've written a constant and variable version of the exec function, though I am not sure how to test non-constant strings (like Type::RootPolyType or Type::Constructor)..

Awesome. If you try

function greetingWho() {
	const r = new RegExp("Hi (?<name>.*)")
	print_type(r.exec("Hi Ben")!.groups)
}

Does it print { name : string }? For the dynamic version is it creating a result result = types.new_or_type(*result of object builder that contains special .group property*, TypeId::NULL)

I am experimenting with utf16 matching but am running into some correctness issues. regress is very standard compliant, so I'll find a test suite to find how to correctly match ranges.

Awesome, I think basic support is good for now. Maybe put a comment in checker/README.md that regex support is based on regress and mention any slight differences?

I am also running into issues with optional property access of RegExpExecArray | null types on the result of the non-constant version of exec. Not sure if I am doing unions correctly.

From last week I think property access on an or (left, right) didn't check whether both left and right contained that property (aka declare let a: { b: 2 } | null; print_type(a.b) printed 2 rather than an error). I have just merged #157 (which is a huge PR where I redid a lot of the property things) which should have fixed that case (and also added non null assertions so the above should work once main is merged in).

kaleidawave · 2024-08-25T08:02:11Z

Hey, putting out a release this week. Would love to have this feature in! Do you think you can maybe cut it back to what was working in the screenshot?

lemueldls · 2024-08-25T15:28:14Z

Non-constant examples work, but I'm still not sure how I should write specification tests for it.

Does it print { name : string }?

It prints { name: "Ben" } (but doing !. causes problems)

Issue with !.:

For the dynamic version is it creating a result result = types.new_or_type(*result of object builder that contains special .group property*, TypeId::NULL)

Yes that was the original implementation. It was an error until I pulled from main, now there's a Expression is always false warning with optional property access of constants.

However another error still happens with non-constant optional property access:

Only when I also remove the new_or_type does it give the expected result:

The original example still works fine with or without optional property access, I'll write some working specifications soon.

- Add new specification test for above - Fix clippy lints - Don't print [null] as prototype of object

kaleidawave · 2024-08-26T07:44:59Z

Awesome!

It prints { name: "Ben" } (but doing !. causes problems)

Yep its a case of #165. The parser thinks treats ! as something before an expression and so it thinks the . means the start of a decimal number like .25. Will fix eventually. I think it works if you split it between lines with a variable.

Only when I also remove the new_or_type does it give the expected result:

Hmm yes I think there are some problems with ?. and narrowing at the moment...

- Update regexp.rs to use crate::Map

kaleidawave · 2024-08-26T08:37:17Z

Merged!

Made a few changes before merge

Fixed things clippy was whining about
Added a function error for new RegExp with a bad input (should rename ConstantFunctionError::BadCall to ConstantFunctionError::CannotComputeConstant in the future)
Added a test for bad expressions and the new bad function call above
The Constant `RegExp.exec` groups greedy test was testing against a type checker error that printed the .groups object. However it looks like the order of items in .groups is unpredictable because regress uses a HashMap along the way. I have made the changes this side to use my own crate::Map for groups which I already use in the checker for small things and importantly has a stable order. To get stable ordering it needs a (hopefully small) change on the regress side: Stable ordering of named_group_indices ridiculousfish/regress#96. In the meantime I have changed the body of the test to not rely on ordering.

I did try adding String.prototype.match and String.prototype.split but there is something wrong with events on my side. I will investigate and let you when I get it working! Also I was worried it would add a big perf drop integrating this external process in, but looks okay so far!

lemueldls added 2 commits August 6, 2024 21:52

feat: initial regexp

0632d90

fix: extras

439ea2c

kaleidawave reviewed Aug 7, 2024

View reviewed changes

kaleidawave added enhancement New feature or request checking Issues around checking labels Aug 7, 2024

kaleidawave reviewed Aug 7, 2024

View reviewed changes

checker/src/types/regexp.rs Outdated Show resolved Hide resolved

lemueldls added 2 commits August 7, 2024 17:16

refactor: move regexp into features/

f73e685

feat: handle errors and variable exec

6b3f153

fix: exec function signature

a5665f2

lemueldls added 2 commits August 9, 2024 02:12

Merge branch 'main' into regexp

a0b08a9

Merge branch 'main' into regexp

660889e

lemueldls added 2 commits August 25, 2024 11:08

Merge branch 'main' into regexp

516f24e

chore: cleanup

6bfca7e

lemueldls added 2 commits August 25, 2024 18:15

chore: regexp specifications

1bd7121

chore: cleanup specification

2cbc571

lemueldls marked this pull request as ready for review August 25, 2024 22:36

Add constant function bad RegExp error

7dcc189

- Add new specification test for above - Fix clippy lints - Don't print [null] as prototype of object

kaleidawave added 2 commits August 26, 2024 09:07

Update specification to have consistent test

8570d65

- Update regexp.rs to use crate::Map

fix formatting

7834e73

kaleidawave merged commit c952b6d into kaleidawave:main Aug 26, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial RegExp #184

Initial RegExp #184

lemueldls commented Aug 7, 2024

kaleidawave Aug 7, 2024

kaleidawave Aug 7, 2024

kaleidawave Aug 7, 2024 •

edited

Loading

kaleidawave Aug 7, 2024

lemueldls Aug 25, 2024

kaleidawave Aug 7, 2024

lemueldls Aug 7, 2024

kaleidawave Aug 7, 2024

kaleidawave Aug 7, 2024

kaleidawave commented Aug 7, 2024

lemueldls commented Aug 7, 2024

kaleidawave commented Aug 8, 2024

kaleidawave commented Aug 25, 2024

lemueldls commented Aug 25, 2024 •

edited

Loading

kaleidawave commented Aug 26, 2024

kaleidawave commented Aug 26, 2024

Initial RegExp #184

Initial RegExp #184

Conversation

lemueldls commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaleidawave Aug 7, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaleidawave commented Aug 7, 2024

lemueldls commented Aug 7, 2024

kaleidawave commented Aug 8, 2024

kaleidawave commented Aug 25, 2024

lemueldls commented Aug 25, 2024 • edited Loading

kaleidawave commented Aug 26, 2024

kaleidawave commented Aug 26, 2024

kaleidawave Aug 7, 2024 •

edited

Loading

lemueldls commented Aug 25, 2024 •

edited

Loading