Fix dialyzer warnings on EEx.eval_string #14971
Conversation
lib/eex/lib/eex.ex
Outdated
| ### Helpers | ||
|
|
||
| defp do_eval(compiled, bindings, options) do | ||
| options = Keyword.new(options) |
There was a problem hiding this comment.
I think this is simply hiding the options, right? Wouldn't it be better if we called Keyword.split or Keyword.pop as we do in other places?
There was a problem hiding this comment.
As I was saying in the comment mentioned above, we're a bit lying to ourselves because Dialyzer doesn't really follow the logic of Keyword.pop.
Calling any function (even Function.identity) is equivalent to type-laundering and returns a broader type.
The real type of these functions is actually "open" keywords, where unknown keys are just ignored.
There was a problem hiding this comment.
👍🏻 Keyword.pop is also effectively only hiding the options. But I agree it would be better, I think it also makes the code more readable/intentional
I think here the difference is that we cannot easily take the "local" options because EEx compile_opts are "open" on purpose, I think custom EEx engines options can be arbitrary keys? But we could take the remote opts before calling Code.eval_quoted, should I do that?
There was a problem hiding this comment.
Yes, calling take would be the way to go then!
michallepicki
changed the title
michallepicki
force-pushed
the
michal/25-11-29-eex-dialyzer
branch
from
5d15496 to
9ac5f3b
Compare
|
💚 💙 💜 💛 ❤️ |
3 participants
See #14837 (comment)
As mentioned on #14928, this has a small performance cost. Alternatively the spec of
Code.eval_quotedcan be changed to be "open"edit: Actually, I think
EEx.eval_fileis probably not warning thanks toKeyword.put_newhere