Translate with/1 as a closure #13299
Conversation
sabiwara
force-pushed
the
with-closure
branch
from
a376d01 to
eaad87a
Compare
| @@ -5,7 +5,6 @@ defmodule Kernel.WithTest do | |||
|
|
|||
| test "basic with" do | |||
| assert with({:ok, res} <- ok(41), do: res) == 41 | |||
| assert with(res <- four(), do: res + 10) == 14 | |||
There was a problem hiding this comment.
Was triggering a warning which I think is legit since this clause is always matching indeed:
Maybe what could be improved could be to emit a different warning (e.g. "var <- clause will always match in with statement, use var = instead) during expansion and replace <- by a = so that we don't actually emit the else clause, WDYT?
There was a problem hiding this comment.
@sabiwara 👍 to omit the else clause in this case and generate leaner code. :)
sabiwara
force-pushed
the
with-closure
branch
from
eaad87a to
e408364
Compare
lib/elixir/src/elixir_clauses.erl
Outdated
| {{'<-', Meta, [ELeft, ERight]}, {SL, EL, true}}; | ||
| case ELeft of | ||
| {Var, _, Ctx} when is_atom(Var), is_atom(Ctx) -> | ||
| {{'=', Meta, [ELeft, ERight]}, {SL, EL, HasMatch}}; |
There was a problem hiding this comment.
This is nice, it is even able to emit a warning in this case
which is indeed on point.
There was a problem hiding this comment.
However, we should probably do the conversion when translating to Erlang code, to avoid doing many changes to the original AST. The original AST is used for type checking and in error reports, so we want to preserve semantics when we can.
There was a problem hiding this comment.
Makes sense! Addressed in 4e11ff2
| Ann = ?ann(Meta), | ||
| {_, FunErlVar, SC} = elixir_erl_var:assign(Meta, S), | ||
| {_, ArgErlVar, SA} = elixir_erl_var:assign(Meta, SC), | ||
| FunAssign = {match, Ann, FunErlVar, {'fun', Ann, {clauses, TranslatedClauses}}}, |
There was a problem hiding this comment.
Just to confirm, we generate one function for all else clauses, right?
There was a problem hiding this comment.
That is correct 👍
Including the :else_clause exception one.
There was a problem hiding this comment.
Basically here is how it gets translated:
No closure if no else:
sabiwara
force-pushed
the
with-closure
branch
from
e408364 to
4e11ff2
Compare
|
Hello @sabiwara I would like to know if this change is supposed to fix a dialyzer problem when the To me, this shows me that it would indeed fix the problem, but we still have it. We use Thank you. |
|
Hi @lud-wj, the main idea was to avoid emitting the same block of code repeatedly and be closer to what erlang's new I had hopes that this change would make dialyzer more useful and that we could now mark less code as generated, but unfortunately the kind of issues you mentioned didn't disappear so we had to mark most of it as generated again (making Dialyzer unable to track The second one has not been released yet, it should be fixed with 1.17.4. Could you try the |
|
Hey @sabiwara :) Thank you! Hmmm strange that it did not work, it was a nice solution. Tried on 1.17.3 with a demo app and it seems to be ok. I'll try to keep you posted when the elixir version of the app is upgraded. |
As discussed in #6738.
A nice side-effect is that we are able to remove a bunch of
generatedexcept from theraiseclause.The compiler and dialyzer should be able to track actual issues without false positives.