HackerExperience / Helix

GNU Affero General Public License v3.0
53 stars 10 forks source link

Make Helix compatible with Elixir 1.6.x #394

Open renatomassaro opened 6 years ago

renatomassaro commented 6 years ago

So, Elixir 1.6 is out and has some interesting new features (though none I plan to adopt in the near future, hence the low priority).

The main change for us was the deprecation of Map.replace/3, which I've migrated to using Map.replace!/3 instead (which is, indeed, the desired behaviour).

But some low level compiler change (I'm assuming) happened that modified the final binary output, resulting in some compilation warnings and dialyzer errors[1].

The main low level change that affected us is that the code snippet below, that automatically handles the fallback, yields a "the above clause will always match" warning.

quote do
  @doc false
  def get_objects(unquote(event)) do
    unquote(block)
  end

  # Fallback
  def get_objects(_),
    do: []
end

Mind you, the warning is absolutely correct! But remember that the snippet above is inside a macro, and as such is a generated code. If the event variable is a pattern match like %{foo: :bar}, the fallback clause will be valid and no warning is generated. But if event is a catch-all variable, then the fallback is useless and a warning will be emitted (though that did not happen on 1.5.x or before).

There's a simple fix for this: put generated: true on the quote/2 macro. But that comes with a caveat: since we are telling Elixir that code is generated, typespec verifications from dialyzer will be ignored.

It is possible to conditionally generate the fallbacks (based on the input of event or whatever variable the macro receives) without using generated: true. Something like this:

fallback_block =
  if elem(event, 0) == := do
    quote do
      def get_objects(_),
        do: []
    end
  else
    []
  end

quote do
  @doc false
  def get_objects(unquote(event)) do
    unquote(block)
  end

  unquote(fallback_block)
end

The problem with this approach is that the code will be significantly more complex and harder to read.

Not to mention the several fallback cases we have on e.g. Process.Executable, which are generated at __before_compile__ level (and thus we'd need to track the input variables with module attributes).

So, while deprecation warnings were fixed, I'm still delaying the Fallback/Dialyzer fix because I don't like neither option. Maybe I'll have some insight that enables a simple fix without generated: true, but so far that seems unlikely.

Progress

Incidental

Notes

[1] - It may be related to https://github.com/elixir-lang/elixir/issues/7224, since Phoenix relies heavily on macros / generated code, but I did not look further.


This change is Reviewable

sourcelevel-bot[bot] commented 6 years ago

Ebert has finished reviewing this Pull Request and has found:

But beware that this branch is 6 commits behind the HackerExperience:master branch, and a review of an up to date branch would produce more accurate results.

You can see more details about this review at https://ebertapp.io/github/HackerExperience/Helix/pulls/394.

themaxhero commented 6 years ago

I think this PR should be named: Make Helix compatible with Elixir 1.7.x, because Elixir 1.6.x isn't the most recent anymore

renatomassaro commented 6 years ago

I think this PR should be named: Make Helix compatible with Elixir 1.7.x, because Elixir 1.6.x isn't the most recent anymore

The incompatibilities are on 1.6; switching from 1.6 to 1.7 should be OK. Anyway, the name really does not matter.