-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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.
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
base: main
Are you sure you want to change the base?
Conversation
lib/elixir/lib/kernel.ex
Outdated
## Shared functions | ||
|
||
@doc false | ||
defmacro __assert_assert_no_match_or_guard_scope__(exp) do |
There was a problem hiding this comment.
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.
lib/elixir/lib/regex.ex
Outdated
__assert_assert_no_match_or_guard_scope__("an escaped Regex") | ||
:re.import(unquote(Macro.escape(exported))) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
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
🤔)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @josevalim !
Are we happy with the fact we need to emit require
though?
We end up with as much code as __assert_assert_no_match_or_guard_scope__
, but the benefit of the latter is that users could reuse it in their own code.
Will fix the CI for the require approach though if we want to go this way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arf, it doesn't work either invalid pattern in match, require is not allowed in matches
.
Following on #14720, I've been thinking if we could/should improve the error message when trying to use regex module attributes in guards/match scope.
This PR explores the idea, but feel free to close if this is overthinking on my part.
Before (confusing:

:re.compile
mentioned but not in the code)After
