Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compilation warning when using super with unused, overridable function via using module #14094

Closed
acco opened this issue Dec 20, 2024 · 8 comments

Comments

@acco
Copy link

acco commented Dec 20, 2024

Elixir and Erlang/OTP versions

Elixir 1.18.0 (compiled with Erlang/OTP 27)

Operating system

MacOS Sequoia 15.1.1 (24B91)

Current behavior

Thanks for a great release!

When using a library with super, this implementation:

defmodule Sequin.Encrypted.Field do
  use Cloak.Ecto.Binary, vault: Sequin.Vault

  def load(nil), do: {:ok, nil}

  def load(value) do
    super(Base.decode64!(value))
  end
end

Results in a compilation error:

  warning: this clause of defp load (overridable 1)/1 is never used
    │
  8 │   use Cloak.Ecto.Binary, vault: Sequin.Vault
    │   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    │
    └─ lib/sequin/encrypted/field.ex:8: Sequin.Encrypted.Field."load (overridable 1)"/1

The compilation error goes away if I call super/1 in the first load/1:

defmodule Sequin.Encrypted.Field do
  use Cloak.Ecto.Binary, vault: Sequin.Vault

  def load(nil), do: super(nil)

  def load(value) do
    super(Base.decode64!(value))
  end
end

The library defines two overridable load/1 functions:

 defmacro __using__(opts) do
    vault = Keyword.fetch!(opts, :vault)
    label = opts[:label]
    closure = !!opts[:closure]

    quote location: :keep do
    
    
      @doc false
      @impl Ecto.Type
      def load(nil) do
        {:ok, nil}
      end

      def load(value) do
        with {:ok, value} <- decrypt(value) do
          value = after_decrypt(value)

          if unquote(closure) do
            {:ok, fn -> value end}
          else
            {:ok, value}
          end
        else
          _other ->
            :error
        end
      end

So, it appears I need to use both overridable functions in order to not get a compilation warning.

Expected behavior

Because I don't "own" the module I'm using, I'd expect the compiler to not care that one of the clauses is unused?

@acco
Copy link
Author

acco commented Dec 20, 2024

To reproduce:

  1. Clone Sequin
  2. Modify the first load in lib/sequin/encrypted/field.ex:
def load(nil), do: {:ok, nil}
  1. mix compile

@josevalim
Copy link
Member

Does it work if you change quote location: :keep do to mark the generated functions as generated: true?

@acco
Copy link
Author

acco commented Dec 20, 2024

@josevalim I followed these steps:

  1. Edited deps/cloak_ecto. I changed the using from this:
quote location: :keep do

To this:

quote location: :keep, generated: true do
  1. Ran this:
mix deps.compile cloak_ecto --force

When I run mix compile, the warning persists.

@josevalim
Copy link
Member

Thank you. We need to decide to not emit these warnings for overriddable functions (that may be the best option) or to generated ones.

@josevalim
Copy link
Member

@acco I believe you forgot mix compile --force in the last step, that addresses the issue here. We need to decide if that's how we want to move forward though. :)

@acco
Copy link
Author

acco commented Dec 22, 2024

@josevalim Ah, that's right! I was using --force with the dep, but not with my repo. Running mix compile --force after adding generated: true indeed fixes 🙏

@josevalim
Copy link
Member

Just so you know, we decided to fix this without requiring you to annotate them as :generated. This is because :generated would turn off other warnings.

@acco
Copy link
Author

acco commented Dec 22, 2024

@josevalim Very cool, thank you! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants