Skip to content
This repository has been archived by the owner on Jun 11, 2023. It is now read-only.

Make Helix compatible with Elixir 1.6.x #394

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

renatomassaro
Copy link
Member

@renatomassaro renatomassaro commented Mar 10, 2018

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

  • Fix deprecation warnings
  • Fix dialyzer warnings

Incidental

  • Added support for Postgres 10.x

Notes

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


This change is Reviewable

@sourcelevel-bot
Copy link

Ebert has finished reviewing this Pull Request and has found:

  • 1 fixed issue! 🎉

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
Copy link
Member

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
Copy link
Member Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants