Skip to content

feature: variable substitution - #14

Open
zazedd wants to merge 2 commits into
paurkedal:masterfrom
ahrefs:upstream/var-subst
Open

feature: variable substitution#14
zazedd wants to merge 2 commits into
paurkedal:masterfrom
ahrefs:upstream/var-subst

Conversation

@zazedd

@zazedd zazedd commented Aug 1, 2025

Copy link
Copy Markdown

Related to #12

This PR adds variable substitution to the %pcre extension.

Details

Allows defining variables with let%pcre, which could then be used inside a few patterns for substitution:

  • (?&var) - Named substitution (doesn't capture)
  • (?&name:var) - Named substitution with rename

Only 1 AST pass is needed.

If a user tries to define let%pcre var = (?N<var>), the ppx is smart enough to warn the user about having unbounded recursion.

Note

This updates the opam package to use ppxlib <= "0.35.0" only, as ppxlib.0.36.0 has breaking changes in the Parsetree AST.

@zazedd
zazedd marked this pull request as ready for review August 1, 2025 17:17
@Drup

Drup commented Aug 5, 2025

Copy link
Copy Markdown
Collaborator

If I understand correctly, this is similar to the call feature available for tyre ? IIRC, I tried to choose a fairly standard syntax, could you use the same one ?

Comment thread ppx_regexp/ppx_regexp.ml Outdated
@zazedd

zazedd commented Aug 7, 2025

Copy link
Copy Markdown
Author

I tried to choose a fairly standard syntax, could you use the same one

Sure thing, I'll change the syntax!

@paurkedal

Copy link
Copy Markdown
Owner

Nice addition, one I recall this was on my wish-list if I had spent more time on it.

If a user tries to define let%pcre var = (?N<var>), the ppx is smart enough to warn the user about having unbounded recursion.

Shouldn't that, without rec qualification, shadow rather than recurse.

@zazedd
zazedd force-pushed the upstream/var-subst branch from f4d6a69 to 603c634 Compare August 13, 2025 18:29
@zazedd

zazedd commented Aug 13, 2025

Copy link
Copy Markdown
Author

Shouldn't that, without rec qualification, shadow rather than recurse.

You're right, I fixed the symptom and not the cause 😅.


The latest commit is an intermediate to a final version, there are still some improvements to be made.

I went back and looked at how ppx_tyre does it. The updated solution here uses Call and the syntax from ppx_tyre.

Important change: the AST is now translated into base Re calls, instead of into a string and then compiled using Re.Perl. This should make the change mentioned in #15 easier.

@zazedd
zazedd force-pushed the upstream/var-subst branch 3 times, most recently from b12c891 to c1b28cd Compare August 13, 2025 20:50
@paurkedal

Copy link
Copy Markdown
Owner

Important change: the AST is now translated into base Re calls, instead of into a string and then compiled using Re.Perl. This should make the #15 (comment) in #15 easier.

Nice! We should maybe merge this first to avoid conflicts. There seems to be a bug, though, involving group indexing:

$ OCAMLRUNPARAM=b dune runtest -p ppx_regexp
File "tests/dune", line 13, characters 9-24:
13 |   (names test_ppx_regexp test_ppx_regexp_unused)
              ^^^^^^^^^^^^^^^
(cd _build/default/tests && ./test_ppx_regexp.exe)
Fatal error: exception Not_found
Raised at Re__Group.offset in file "lib/group.ml", line 12, characters 12-27
Called from Re__Group.get in file "lib/group.ml", line 17, characters 15-25
Called from Test_ppx_regexp.test4 in file "tests/test_ppx_regexp.ml", lines 64-67, characters 12-20
Called from Test_ppx_regexp in file "tests/test_ppx_regexp.ml", line 94, characters 10-29

@zazedd

zazedd commented Sep 9, 2025

Copy link
Copy Markdown
Author

Much thanks for catching this! Planning to update the branch this week and fix this issue.

@paurkedal paurkedal left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

In addition to the two corrections, can you add a test case to cover the three cases of new syntax?

Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants