phoenixframework / phoenix

Peace of mind from prototype to production
https://www.phoenixframework.org
MIT License
21.45k stars 2.88k forks source link

Verified routes & compile time dependencies #5893

Closed zachdaniel closed 3 months ago

zachdaniel commented 3 months ago

Environment

Actual behavior

I’ve noticed that verified routes incurs a bunch of compile time dependencies that I can’t figure out. I’m trying to unravel how this works, but if the router references a module that does a use MyAppWeb, :verified_routes, this causes the router to be recompiled for modules that are seemingly unrelated.

For example, the router in this project will recompile whenever TestWeb.Gettext is recompiled. If you comment out the compile time link to Test in the router, the router will no longer recompile when TestWeb.Gettext is recompiled.

I’m only using TestWeb.Gettext as an example. People probably don’t modify that module often if at all, but it’s occurring in other projects on modules that do change quite often.

I'm reporting this as a "bug" but I understand it could be a flaw in my understanding and/or just a "downside" of some existing design decision. However, if it is something that can be addressed at the framework level, it could make significant differences on many projects

I'm also pretty good at working with xref and tracking down compile time dependencies, so the fact that I can't figure this out might mean that, if it is working as intended, a blurb could be added to the docs explaining it. (I'm happy to do this once its clear to me).

Expected behavior

Referring a module that uses verified routes doesn't incur new additional compile time dependencies that the other module doesn't have.

zachdaniel commented 3 months ago

My current theory (that isn't quite holding up?):

So if a router ever has a compile time dependency on a module that uses verified routes, then the router has a compile time dependency on all of its own runtime dependencies?

unfortunately, if this were the actual case, it would mean recompiling any controller that uses verified routes, for example, would recompile all other modules that use verified routes. So I'm still missing something.

zachdaniel commented 3 months ago

At a minimum it feels like there is something missing from mix xref. Because mix xref graph --source lib/test_web/router.ex --label compile doesn't show Test.Gettext nor does using --label compile-connected. So I have no way to trace the cause of the compilation here? Or maybe there is an option to xref I don't know about?

zachdaniel commented 3 months ago

So, something in the above is true, however what I've ultimately determined is that this is unlikely a bug, and just that you should not have a compile time dependency from the router to (well, almost anything) a module that uses verified routes. If there is a bug here, I think it may actually be in xref. Specifically that, given the provided example repo, I cannot find any xref command that will list gettext.ex as a dependency of router.ex. So there is either something I'm missing there, or a bug in xref.

josevalim commented 3 months ago

—label compile will only find direct compile dependencies. You probably want to use —source and —sink, without —label, and see if there is a path there between router and Gettext. You can also use mix xref trace to see what the router is depending on.

zachdaniel commented 3 months ago

I was using --label compile-connected, should that not find this kind of dependency?

❯ mix xref graph --source lib/test_web/router.ex --sink lib/test_web/gettext.ex
lib/test_web/router.ex
├── lib/test.ex (compile)
│   ├── lib/test_web/endpoint.ex
│   │   └── lib/test_web/router.ex
│   └── lib/test_web/router.ex
├── lib/test_web/components/layouts.ex
│   ├── lib/test_web/components/core_components.ex (export)
│   │   └── lib/test_web/gettext.ex (compile)
│   ├── lib/test_web/endpoint.ex
│   ├── lib/test_web/gettext.ex (export)
│   └── lib/test_web/router.ex
└── lib/test_web/controllers/page_controller.ex
    ├── lib/test_web/components/layouts.ex
    ├── lib/test_web/endpoint.ex
    ├── lib/test_web/gettext.ex (export)
    └── lib/test_web/router.ex
❯ mix xref graph --source lib/test_web/router.ex --label compile-connected
lib/test_web/router.ex
└── lib/test.ex (compile)

So the only way to see (from xref) that gettext would recompile the router is to parse the first output manually?

zachdaniel commented 3 months ago

In practice, router.ex recompiles when changing gettext.ex, but gettext.ex does not appear in either the --label compile or --label compile-connected variants of mix xref. That seems strange to me.

josevalim commented 3 months ago

What I am saying is that --label compile is going to filter for direct dependencies, but gettext is not a direct dependency on the router, so you won't see it there. But now we need to understand why this dependency is not showing up.

I am back at the computer now. So the reason why this was tricky to spot was because of you have a cyclic dependency between lib/test.ex and lib/test/router.ex. This happens because of scope Test in router. If you rename defmodule Test do to defmodule Test.Foo do, then the problem disappears.

But because this cyclic dependency has a "compile" time component, this means that anytime the router has any runtime dependency, the router itself becomes a compile-time dependency, because it indirectly depends on itself at compile-time.

So this dependency is enough to trigger the router compilation.

lib/test_web/router.ex
├── lib/test_web/components/layouts.ex
│   ├── lib/test_web/gettext.ex (export)
josevalim commented 3 months ago

The simplest way to say this is that the cyclic compile-time dependency in the router transforms all runtime dependencies into a compile-time dependency. This is of course really harmful, so Elixir should probably make it much easier to spot these. I will open up an issue.

zachdaniel commented 3 months ago
josevalim commented 3 months ago

I have said one thing wrong here that I want to clarify, --label compile WILL NOT filter for direct dependencies. This was the default behaviour but it was changed at some point. It only filter direct dependencies if --only-direct is given. The issue is that the cycles functionality was treating all labels as direct by default, this will be fixed on Elixir v1.18.