Skip to content

Assert scope is not match/guard when using escaped regexes #14780

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 27, 2025
Merged

Assert scope is not match/guard when using escaped regexes #14780

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2ac95e9
Assert scope is not match/guard when using escaped regexes
sabiwara Sep 20, 2025
c1dee89
Use Regex.__compile_pattern__ macro instead
sabiwara Sep 20, 2025
4012cd0
Add nil check for regex pattern in pattern AST
josevalim Sep 25, 2025
0e671f4
Fix missing require using required: true meta
sabiwara Sep 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions lib/elixir/lib/kernel.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7004,6 +7004,12 @@ defmodule Kernel do

## Shared functions

@doc false
defmacro __assert_assert_no_match_or_guard_scope__(exp) do
Copy link
Member

Choose a reason for hiding this comment

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

I think in this case some code duplication is fine. Let's just reimplement the error message directly in the Regex module, since that's what we would expect library authors to do too.

assert_no_match_or_guard_scope(__CALLER__.context, exp)
nil
end

defp assert_module_scope(env, fun, arity) do
case env.module do
nil -> raise ArgumentError, "cannot invoke #{fun}/#{arity} outside module"
Expand Down
1 change: 1 addition & 0 deletions lib/elixir/lib/regex.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,7 @@ defmodule Regex do
{:ok, exported} = :re.compile(regex.source, [:export] ++ regex.opts)

quote do
__assert_assert_no_match_or_guard_scope__("an escaped Regex")
:re.import(unquote(Macro.escape(exported)))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of generating additional code, we should call Regex.__import__(unquote(...)), where __import__ itself is a macro which also checks the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds much better indeed 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arg, this might actually not work, since unlike Kernel we need to require Regex.

       quote do
            require Regex
            Regex.__import_pattern__(unquote(Macro.escape(exported)))
          end

Which then makes it impossible to compile elixir:

==> elixir (compile)
error: you are trying to use/import/require the module Regex which is currently being defined.

(although it seems weird to warn here, since it's happening within quote 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the broken commit just in case: c1dee89

Copy link
Member

Choose a reason for hiding this comment

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

I believe the issue happens because we escape the struct defined by defstruct inside Regex. An easy fix is to add this clause to the top of the cond in __escape__:

        is_nil(regex.re_pattern) ->
          nil

And then it compiles fine!

Copy link
Contributor Author

@sabiwara sabiwara Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Another idea to keep the emitted AST small 💡

We could leave a special meta when we escape a struct with __escape__, and add an extra hint when we emit the scope error if the meta is there? (Hint: This happened because you tried to escape a Regex).

Another benefit is that custom structs need to do nothing to benefit from this.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think that metadata would work? The __escape__ function can return any node and it would be hard to know which node to annotate/which node would fail.

Another option is for us to allow this to work:

require Regex

quote do
  Regex.__import__(...)
end

or even just this:

quote do
  Regex.__import__(...)
end

Especially as I thought we already annotated within quote that the module was already required...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that metadata would work? The escape function can return any node and it would be hard to know which node to annotate/which node would fail.

That's true, I didn't think this through, sorry.

or even just this:

quote do
  Regex.__import__(...)
end

That would be ideal, but isn't there a risk of allowing implicit requires?
Wouldn't this mess with the lexical tracker?

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, the way it works is that we annotate it is required and consider the require in both caller and callee. At least that’s how we handle imports and they are more complex than require.

another option is to allow requiring the current module as long as it happens inside a function? As you can assume that when the function is executed the module is indeed defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL about the required: true meta, I think it fixed it nicely 0e671f4.

end

Expand Down
10 changes: 8 additions & 2 deletions lib/elixir/test/elixir/macro_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,14 @@ defmodule MacroTest do
[],
[
__struct__: Regex,
re_pattern:
{{:., [], [:re, :import]}, [], [{:{}, [], [:re_exported_pattern | _]}]},
re_pattern: {
:__block__,
[],
[
{:__assert_assert_no_match_or_guard_scope__, _, _},
{{:., [], [:re, :import]}, [], [{:{}, [], [:re_exported_pattern | _]}]}
]
},
source: "foo",
opts: []
]
Expand Down
19 changes: 19 additions & 0 deletions lib/elixir/test/elixir/regex_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,25 @@ defmodule RegexTest do
end
end

@tag :re_import
test "module attribute in match context" do
assert_raise(
ArgumentError,
~r/invalid expression in match, an escaped Regex is not allowed in patterns such as function clauses/,
fn ->
Code.eval_quoted(
quote do
defmodule ModAttrGuard do
@regex ~r/example/
def example?(@regex), do: true
def example?(_), do: false
end
end
)
end
)
end

test "multiline" do
refute Regex.match?(~r/^b$/, "a\nb\nc")
assert Regex.match?(~r/^b$/m, "a\nb\nc")
Expand Down