If the compiler exits in an `@after_verify` hook, all compiled modules in that context will produce the "redefining module" error on all subsequent compilations

[simpers]

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.2.7.1] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [jit]

Elixir 1.19.0-rc.0 (2a9a4f2) (compiled with Erlang/OTP 27)

Operating system

macOS

Current behavior

With the help of @zachdaniel, who gave me the necessary context finally figure this out, we tracked it down to the @after_verify hook.

Verified that the issue exists in Elixir 1.18.4 and 1.19.0-rc.0, but @zachdaniel also tried the main branch, which seemed to not have this issue. So this might be a non-issue for 1.20+, but a fix for 1.18 and 1.19 would be great!

If the compiler exits in an @after_verify hook, all compiled modules in that context will produce the "redefining module" error on all subsequent compilations

A reproduction repository with instructions can be found at: https://github.com/simpers/redefining-reprod

In bigger projects where a change to a module affects a whole list of dependents, this will cause all of the modules affected to henceforth output this redefining module error, until mix clean is called.

Example output from reprod repo after the issue has been triggered:

$ mix compile
    warning: redefining module Reprod (current version loaded from _build/dev/lib/reprod/ebin/Elixir.Reprod.beam)
    │
  1 │ defmodule Reprod do
    │ ~~~~~~~~~~~~~~~~~~~
    │
    └─ lib/reprod.ex:1: Reprod (module)

    warning: redefining module Reprod.Sad (current version loaded from _build/dev/lib/reprod/ebin/Elixir.Reprod.Sad.beam)
    │
  1 │ defmodule Reprod.Sad do
    │ ~~~~~~~~~~~~~~~~~~~~~~~
    │
    └─ lib/reprod/sad.ex:1: Reprod.Sad (module)

Expected behavior

The warnings should not get stuck like this, and also they are false warnings.

[zachdaniel]

Hmm...the choice to change from errors to warnings is pretty significant. I had considered changing them to warnings myself anyway, and I'll probably just do that now to get around the issue for current versions, but it is a change to how pretty much all Ash apps compile. Right now if you misconfigure it in certain ways it won't compile.

Should we document that you can't stop compile with an error in an after_verify hook?

[josevalim]

Good call, I am going to document it but basically I didn't expect people to raise on after_verify since, as the name says, it is for verification and it happens after compilation, so some of the artifacts were already written to disk (and therefore already compiled).

[josevalim]

Hrm, I am guessing this comes with another issue where the user explicitly halts compilation by calling Ctrl+C twice and now we are in a dirty state. The fix above is not correct.

[josevalim]

I will push another fix but the restriction that you should not raise is still there. Ash needs to be changed accordingly.

[zachdaniel]

It's an interesting issue because I'm happy to add a try/catch there but there should probably be a system level try/catch if raising in that callback will put things in a bad state, otherwise someone else will just do it either on purpose or by accident.

[josevalim]

I have made the code resilient to try/catch but it is more of a last defense scenario as we should assume the VM or the code may fail for unrelated reasons. The point is that you should not proactively raise as part of verification. If it happens because of a bug, that's fine.

[zachdaniel]

Sounds good, thanks! I'll update accordingly. I've been considering making these warnings anyway because of how it affects incremental compilation.

Want to bring it up with you separately that it seems like there is a big win on incremental compilation where modules that were successfully compiled could stay successfully compiled even if there are errors. Right now if any module fails they all fail right?

[josevalim]

Yes. It is something that could be done but we would likely have to reimplement how we store the manifest in disk. It is non trivial.

[zachdaniel]

Awesome, thanks for the explanation! 🙇‍♂️

[sorentwo]

Thanks for opening this issue and highlighting the problems with raising in an after_verify hook. Oban (Pro) was doing this in a few places, but no longer!

[simpers]

Thanks for opening this issue and highlighting the problems with raising in an after_verify hook. Oban (Pro) was doing this in a few places, but no longer!

I'm just glad it was found! It has haunted me for a while, but it was one of those things that seemed to hit you when you had the least amount of time to look at it... so I'd just shrug it off. But then it became unbearable after I started working on a slower machine over SSH, and a mix clean would take up to 2 minutes 😓

So then I started digging and brought it up on the Ash Discord, and with @zachdaniel's help during Goatmire we quickly found the reason 😅