-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Adds Kernel.defguard/1 macro to build guard-safe macros. #5854
Conversation
It occurred to me to try to raise the exact same error invalids guards do, but I don't really see any other calls to |
All the other guards that Elixir offers can be implemented with normal one liner macros, since they are relatively simple and don't need special unquoting, so only |
This more precisely checks for the things conceptually allowed in guards from http://erlang.org/doc/reference_manual/expressions.html#id84508, and if there are any special forms that raises in `:assert_no_guard_scope` in `:elixir_expand` (https://github.com/elixir-lang/elixir/blob/c55f92d78b04baaa274fb2d54bea557c444165d4/lib/elixir/src/elixir_expand.erl). It ultimately lets more forms through, like pins and variable map heads and bad variables references and the like, but these will be caught by other compiler checks when the `defguard` generated macro is used. This is not ideal, but without formalizing more of elixir's syntax and semantics in predicate functions like `:erl_internal` and `:erl_syntax`, we would either have to re-implement the elixir compiler in elixir (fun for some) or just try to compile the guard expression and catch errors in `defguard`. More importantly, it has fewer (none I can find) false positives for complicated guards in previous iterations of this approach. I think this is a reasonable compromise.
Thank you @christhekeele. After looking at the implementation, it seems it would be best if we could use Elixir's existing :elixir_expand pass instead of reimplementing everything in Elixir land? |
Any thought of making defguard be a special syntax instead of a macro? Would open a lot more possibilities. |
@OvermindDL1 What do you mean by special syntax? If you are referring to making it support things like |
Plus |
Ah I meant that it is worth adding a a 'defguard' if it brings something more powerful than just making a macro of a guard. For example |
That's a fair opinion but not one that the core team shares. For me the main value of |
@josevalim I think I shied away from Working on that now. |
@christhekeele let me clarify one thing before you move on to do more work though. We still have not decided if we are going to merge this feature or not. :) It seems you are having fun though and we will be glad to review it regardless. |
Yep, it's loads of fun and I have no expectations. :) |
Closes #2469.
Public API
(Checks denote that documentation has been updated where necessary to the satisfaction of involved reviewers. Uncheck if they need work.)
Public functions modified
Macro.validate/1
is now implemented in terms ofMacro.validate/2
Macro.expand(_once)/2
is now implemented in terms ofMacro.expand(_once)/3
Public functions added
Macro.validate/2
allows you to verify an AST tree against an additional custom validator functionMacro.expand(_once)/3
allows you to provide optionrewrite: true
to perform the expansions inelixir_rewrite.erl
Macro.validate_guard/1
allows you to check if an AST tree is guard-safeMacro.validate_guard/2
does so after a rewrite-expansion over the providedMacro.Env.t
Macro.guard/2
prepares an AST tree to be used in a guard by handling the provided list of variable names differently inside and outside of guardsPublic macros added
Kernel.defguard/1
creates a guard macro that behaves correctly inside and outside of guards, provided the macro implementation is valid in guardsPublic macros modified
Record.is_record/{1,2}
now usesdefguard
How it works
The heart of this implementation is that it makes few hard-coded assumptions about what is allowed in a guard.
The criteria used to validate a guard is that:
assert_no_guard_scope
in:elixir_expand
The only hardcoded values in the algorithm are the list of special forms that
assert_no_guard_scope
. The rest uses erlang introspection and Elixir compiler helpers to inform the validation.This is only a rough algorithm. It's still possible to create an invalid guard with
defguard
that passes this test and is syntactically correct, but semantically invalid. For example, odd pins, using variables in map keys, other funny business. The errors in those guards won't be discovered until the macro thatdefguard
generates is invoked, and the AST proves to be subtly invalid.To get any further into those woods, we would have either have access to more metaprogramming predicates like those found in
:erl_internal
and:erl_syntax
, or start emulating the low-level compiler translations of things like binary literals that expand into forms that are only valid erlang, but not valid Elixir, AST.This is the best algorithm I could come up with that is
Here are the semi-private erlang introspection and Elixir compiler calls I make:
In
Kernel.defguard/1
:elixir_utils.extract_guards/1
to pull out the guard definition in the macroIn
Macro.validate_guard/1
:erl_internal.guard_bif/2
to check for guard bifs:elixir_utils.guard_op/2
to more concisely make other erlang guard checksIn
Macro.expand(_once)/3
:elixir_rewrite.inline/3
to perform simple compiler rewrites of the AST in advance to check for validity:elixir_rewrite.rewrite/5
to perform more complicated ones that involve changing arities or changing AST formsTests
(Please uncheck any items that need better/more/any tests, or check off unchecked things you determine are sufficiently covered.)
For changed functions
Macro.validate/1
tests still all passMacro.expand(_once)/2
tests still all passFor added functions
Macro.validate/2
is indirectly tested by the above and belowMacro.expand(_once)/3
is indirectly tested by the above and belowMacro.validate_guard/1
is indirectly tested by the belowMacro.validate_guard/2
explores 45 forms in guards, exercisingMacro.validate/2
andMacro.expand(_once)/3
in the process.Macro.guard/2
is not really tested beyond usageFor added macros
Kernel.defguard/1
is not really tested beyond usageFor changed macros
Record.is_record/{1,2}
tests still all passErrata: I have a rather lengthy explanation of how I reached this implementation here.