Disallow use of unquote when quote is used in pattern#15469
Conversation
| Unquote = case E of | ||
| #{context := nil} -> proplists:get_value(unquote, EOpts, DefaultUnquote); | ||
| % unquote=raise when quote/1 is called in a guard or pattern | ||
| _ -> raise |
There was a problem hiding this comment.
We may want to preserve unquote: false though.
One alternative is to call has_unquote_calls separately, if you don’t to couple those. Given it is extremely unlikely to have quote in patterns and guards anyway, maybe that’s the best call?
There was a problem hiding this comment.
We may also need to change this too: https://github.com/sabiwara/elixir/blob/d7f22b71fd9d8d02535d68e39b461222c0e2fdaf/lib/elixir/src/elixir_quote.erl#L386
So perhaps doing a separate check would indeed be cleaner with less duplication?
There was a problem hiding this comment.
Oh nice, yeah I agree performance isn't a concern in that case and it will be much more robust 👍
There was a problem hiding this comment.
Pushed - much simpler indeed 😌
| maybe | ||
| true ?= map_get(context, E) /= nil, | ||
| true ?= Unquote, | ||
| true ?= elixir_quote:has_unquotes(Exprs), | ||
| file_error(Meta, E, ?MODULE, quote_in_pattern_with_unquote) | ||
| end, |
There was a problem hiding this comment.
This is a style thing but I would write it rather as:
| maybe | |
| true ?= map_get(context, E) /= nil, | |
| true ?= Unquote, | |
| true ?= elixir_quote:has_unquotes(Exprs), | |
| file_error(Meta, E, ?MODULE, quote_in_pattern_with_unquote) | |
| end, | |
| (map_get(context, E) /= nil) andalso Unquote andalso elixir_quote:has_unquotes(Exprs) andalso | |
| file_error(Meta, E, ?MODULE, quote_in_pattern_with_unquote), |
There was a problem hiding this comment.
Agreed it's better!
(Sorry for that, I haven't done any erlang recently and I'm a bit rusty, couldn't remember if it had to be guard syntax only or sth like that 😄)
|
@josevalim will backport after merge |
2 participants
Better error message for: #15461
The only thing I don't love is that
unquotecomes from the public option, and this is an internal-only mechanism, but I think it's probably fine?