Skip to content

Fix ArkRegex quantifier issue - #1535

Merged
ssalbdivad merged 13 commits into
arktypeio:mainfrom
codpro2005:fix/arkregex-quantifier-issue
Nov 12, 2025
Merged

Fix ArkRegex quantifier issue#1535
ssalbdivad merged 13 commits into
arktypeio:mainfrom
codpro2005:fix/arkregex-quantifier-issue

Conversation

@codpro2005

@codpro2005 codpro2005 commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

Fixed ArkRegex quantifier issues (#1531, #1534)

@codpro2005

codpro2005 commented Nov 3, 2025

Copy link
Copy Markdown
Contributor Author

To reduce ternary nesting here (and slightly improve scalability):

r["min"] extends never ? null
: r["max"] extends never ? null
: r

I was considering adding a someExtends<ts, t> generic utility type so it could be used like this:

someExtends<[r["min"], r["max"]], never> extends true
? null
: r

But I'm unsure whether this is worth it.

@codpro2005 codpro2005 changed the title Fix/arkregex quantifier issue Fix ArkRegex quantifier issue Nov 3, 2025
@codpro2005
codpro2005 marked this pull request as draft November 5, 2025 12:02
@codpro2005
codpro2005 marked this pull request as ready for review November 5, 2025 20:40

@ssalbdivad ssalbdivad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey thanks for submitting this! Would love to see these scenarios handled better and appreciate the detail mirroring the behavior of new RegExp 1:1 here, but we actually have the luxury of failing for cases like this that are almost certainly a mistake.

Can we adjust the handling here so that for invalid quantifiers, we write a descriptive error like Quantifier ${actual} must be a natural number?

We should be able to use the existing parseInteger type to determine this or tryParseInteger which has a builtin parameter for an error message. We could potentially prompt the user to use an escape char for the opening { if they really do want the literal string, although I think that will be quite rare and I don't want the message to be too verbose so up to you.

@ssalbdivad ssalbdivad moved this from To do to In progress in arktypeio Nov 10, 2025
@codpro2005

codpro2005 commented Nov 11, 2025

Copy link
Copy Markdown
Contributor Author

That makes sense to me, I'm not a huge fan of the implicit literal treatment for invalid quantifiers for native RegExp anyways. I too agree that the escaping hint may be avoided since it's an edge case and probably intuitive enough that it doesn't need to be mentioned if you did need it.

It's worth noting that in the test cases falls back to literal on empty and falls back to literal for non-numeric quantifiers are treated as literals too so maybe it could be considered to always request escaping the { when quantifiers aren't desired? Not sure though.

I'll try to get to it in the weekends. If I'll have any questions or notes I'll let you know. Thanks for the review!

@codpro2005

codpro2005 commented Nov 12, 2025

Copy link
Copy Markdown
Contributor Author

I fixed it, some things I was uncertain about:

  • Should the quantifier variable in the error message be the full quantifier (e.g., {-3,4}) or just the invalid number (e.g., -3)? I went for the first, which is why I adjusted the error message to Quantifier ${quantifier} must use natural numbers.
  • I'm assuming it's appropriate to show an error message on leading zeroes since there's no reason to keep them in quantifiers. If so:
    • Should I use the same error message for leading zeroes, or prefer something specific like Quantifier ${quantifier} should avoid leading zeroes?
  • Should the error be singular/plural based on quantifier form?
    • {n} | {n,} => Quantifier ${quantifier} must use a natural number
    • {n,m} => Quantifier ${quantifier} must use natural numbers

Edit: Looks like a step failed, that's not my strong suit so I'll give this a look on another day.

@ssalbdivad

Copy link
Copy Markdown
Member

Wow this is perfect I love it- great work 💯

The CI failure is just due to some recent issue in node 25 so I'm going to merge this as is and will publish in the next few days. Thanks again!

@ssalbdivad
ssalbdivad merged commit 56a53bf into arktypeio:main Nov 12, 2025
5 of 6 checks passed
@github-project-automation github-project-automation Bot moved this from In progress to Done (merged or closed) in arktypeio Nov 12, 2025
@codpro2005

Copy link
Copy Markdown
Contributor Author

Thanks a lot, David! Learned a lot working on this, so it's great to be able to contribute back 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done (merged or closed)

Development

Successfully merging this pull request may close these issues.

2 participants