Prevent crash on zero arity def in protocol #14191

sabiwara · 2025-01-15T23:42:21Z

We can feel the excitement in the commits 😄 1763865.

I tried main on a project and got a compiler crash due to this dependency which defines regular functions inside a protocol, basically:

defprotocol Msgpax.Packer do
  def pack(term)

  Kernel.def(pack_nan(), do: <<0xCB, -1::64>>)
end

The lib should probably be fixed and we should probably not allow this in the first place (same spirit as #14158), but right now this crashes the compiler on consolidation: (fun: :pack_nan, arity: 0)

08:40:08.671 [error] Task #PID<0.7509.0> started from #PID<0.94.0> terminating
** (FunctionClauseError) no function clause matching in :lists.duplicate/2
    (stdlib 6.2) lists.erl:510: :lists.duplicate(-1, :term)
    (elixir 1.19.0-dev) lib/protocol.ex:670: anonymous fn/3 in Protocol.new_signatures/3
    (elixir 1.19.0-dev) lib/enum.ex:2546: Enum."-reduce/3-lists^foldl/2-0-"/3
    (elixir 1.19.0-dev) lib/protocol.ex:669: Protocol.new_signatures/3
    (elixir 1.19.0-dev) lib/protocol.ex:620: Protocol.consolidate/5
    (elixir 1.19.0-dev) lib/protocol.ex:565: Protocol.consolidate/2
    (mix 1.19.0-dev) lib/mix/compilers/protocol.ex:153: Mix.Compilers.Protocol.consolidate_each/4
    (elixir 1.19.0-dev) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
Function: #Function<8.117671614/0 in Mix.Compilers.Protocol.consolidate/4>
    Args: []

I tried to add a test but consolidation tests are a bit high ceremony and I wasn't sure it was worth defining a fixture etc. especially if this is going to be forbidden.

josevalim · 2025-01-16T07:06:26Z

lib/elixir/lib/protocol.ex

@@ -666,7 +666,7 @@ defmodule Protocol do
      end

    new_signatures =
-      for {{fun, arity}, :def, _, _} <- definitions do
+      for {{fun, arity}, :def, _, _} when arity > 0 <- definitions do


Unfortunately this is not enough to fix it. If people are defining custom functions, then we need to do this modification only for protocol functions. So we need to store somewhere, either in a module attribute or perhaps in the function metadata, the protocol functions and traverse only them here.

Maybe we can replace this line:

elixir/lib/elixir/lib/protocol.ex

Line 292 in 7a1a3ab

Kernel.def(unquote(name)(unquote_splicing(args)))

By:

Kernel.def(unquote({name, [protocol: true], args}))

And see if [protocol: true]appears in the metadata here.

Another option, which perhaps I like most, is to traverse the implementation of the __protocol__ and find the return for the :functions clause:

elixir/lib/elixir/lib/protocol.ex

Line 974 in 7a1a3ab

Kernel.def(__protocol__(:functions), do: unquote(:lists.sort(@__functions__)))

And then we need to add a test here that the signature for the user generated function does not have a :strong triplet:

elixir/lib/elixir/test/elixir/protocol/consolidation_test.exs

Line 175 in 7a1a3ab

test "defines signatures without fallback to any" do

It should most likely be an :infer tuple (you can match on {:infer, _, _}).

Another option, which perhaps I like most, is to traverse the implementation of the protocol and find the return for the :functions clause:

I like it, seems straightforward. This is what you mean right? 609f7cd

And then we need to add a test here that the signature for the user generated function does not have a :strong triplet:

Will have a look!

@josevalim done ba95908 but I'm getting {sig: :none}.

Indeed it used to be :strong without the fix.

sabiwara · 2025-01-16T11:47:17Z

lib/elixir/lib/protocol.ex

@@ -665,10 +665,13 @@ defmodule Protocol do
          end
      end

+    fun_arities = :sets.from_list(protocol.__protocol__(:functions), version: 2)


The set might be overkill since most protocols should define very few functions, so perhaps just a list is fine?

Unfortunately we cannot load the code, that would add a bunch of unnecessary work, given we would have to purge it immediately after consolidation anyway. What I proposed is to find the {:__protocol__, 1} entry inside definitions, then find the :functions clauses in there, and get its return type, which should be a keyword list. :)

Got it! 6ff5011

lib/elixir/lib/protocol.ex

josevalim

Looks great, we just need updated tests and we can ship it!

Co-authored-by: José Valim <[email protected]>

sabiwara · 2025-01-17T02:19:20Z

lib/elixir/test/elixir/fixtures/consolidation/extra_def.ex

+defprotocol Protocol.ConsolidationTest.ExtraDef do
+  def protocol_fun(term)
+
+  Kernel.def(regular_fun(term), do: term + 1)


Or I can add it to NoImpl if you prefer to avoid fixture proliferation?

Yes, let's add it to to an existing one with a comment on top.

Done 9adad46

lib/elixir/test/elixir/fixtures/consolidation/sample.ex

josevalim

Ship it!!!!

Co-authored-by: José Valim <[email protected]>

Prevent crash on zero arity def in protocol

7a1a3ab

josevalim reviewed Jan 16, 2025

View reviewed changes

Check protocol functions only

609f7cd

sabiwara commented Jan 16, 2025

View reviewed changes

Read from __protocol__ clause return

6ff5011

josevalim reviewed Jan 16, 2025

View reviewed changes

lib/elixir/lib/protocol.ex Outdated Show resolved Hide resolved

josevalim reviewed Jan 16, 2025

View reviewed changes

sabiwara and others added 2 commits January 17, 2025 10:33

Raise when functions are not found

e9879e6

Co-authored-by: José Valim <[email protected]>

Add rest with regular functions

ba95908

sabiwara commented Jan 17, 2025

View reviewed changes

Re-use Sample fixture

9adad46

sabiwara force-pushed the fix-protocol-crash branch from aaa0100 to 9adad46 Compare January 17, 2025 11:43

josevalim reviewed Jan 17, 2025

View reviewed changes

lib/elixir/test/elixir/fixtures/consolidation/sample.ex Outdated Show resolved Hide resolved

josevalim approved these changes Jan 17, 2025

View reviewed changes

Add more info to comment

1644fe4

Co-authored-by: José Valim <[email protected]>

sabiwara merged commit 618d03b into elixir-lang:main Jan 17, 2025
9 checks passed

sabiwara deleted the fix-protocol-crash branch January 17, 2025 11:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent crash on zero arity def in protocol #14191

Prevent crash on zero arity def in protocol #14191

sabiwara commented Jan 15, 2025 •

edited

Loading

josevalim Jan 16, 2025

josevalim Jan 16, 2025

josevalim Jan 16, 2025

sabiwara Jan 16, 2025

sabiwara Jan 17, 2025 •

edited

Loading

sabiwara Jan 16, 2025

josevalim Jan 16, 2025

sabiwara Jan 16, 2025

josevalim left a comment

sabiwara Jan 17, 2025

josevalim Jan 17, 2025

sabiwara Jan 17, 2025

josevalim left a comment

Prevent crash on zero arity def in protocol #14191

Prevent crash on zero arity def in protocol #14191

Conversation

sabiwara commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sabiwara Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim left a comment

Choose a reason for hiding this comment

sabiwara commented Jan 15, 2025 •

edited

Loading

sabiwara Jan 17, 2025 •

edited

Loading