This repository was archived by the owner on Jun 11, 2023. It is now read-only.
Make Helix compatible with Elixir 1.6.x#394
Open
renatomassaro wants to merge 1 commit intoHackerExperience:masterfrom
Open
Make Helix compatible with Elixir 1.6.x#394renatomassaro wants to merge 1 commit intoHackerExperience:masterfrom
renatomassaro wants to merge 1 commit intoHackerExperience:masterfrom
Conversation
|
Ebert has finished reviewing this Pull Request and has found:
But beware that this branch is 6 commits behind the You can see more details about this review at https://ebertapp.io/github/HackerExperience/Helix/pulls/394. |
|
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 |
Member
Author
The incompatibilities are on 1.6; switching from 1.6 to 1.7 should be OK. Anyway, the name really does not matter. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 usingMap.replace!/3instead (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.
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
eventvariable is a pattern match like%{foo: :bar}, the fallback clause will be valid and no warning is generated. But ifeventis 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: trueon thequote/2macro. 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
eventor whatever variable the macro receives) without usinggenerated: true. Something like this: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 elixir-lang/elixir#7224, since Phoenix relies heavily on macros / generated code, but I did not look further.
This change is