Split routes by verb during compilation

[josevalim]

This pull request groups compiled routes by verb, which speeds up compilation times for large routers up to 3x. A side-effect is that match/forward routes are always matched last.

Therefore, this pull request changes the semantics of dead routes. If you had this code:

match "/foo"
get "/foo"

The second route would never be matched but it is now as part of this pull request. However, get "/foo" should not be there in the first place. It is effectively a dead route.

Currently WIP. We should either document this pitfall or put this change behind an option. I am fine with documenting this in the CHANGELOG as the code above should either not exist or be correct anyway.

[SteffenDE]

I think it's fine to just mention it in the changelog. Alternatively, maybe we could detect such dead routes at compile time and drop them to keep the same behavior, emitting a deprecation warning? Then we drop that extra code for 2.0.

[SteffenDE]

why did we remove that test?

[josevalim]

We already have a match :* above which is tested through other paths and this was emitting a warning with the new changes, so I moved it to a separate warning test!

[SteffenDE]

oh, but wait. In this example

match :*, "/any", UserController, :any
get "/*path", UserController, :not_found

the get route is not a dead route. It is only dead for the very specific case that path is /any. So the changelog is wrong when it says

which changes the semantics of dead routes

[josevalim]

You are right. Perhaps what we do is to change the warning to always warn if any route is used after :*?

[rhcarvalho]

It's wonderful how many optimization possibilities are open :)

... or put this change behind an option.

Considering this could cause unintended change in behavior during an upgrade and be hard to detect (warnings can be missed before and after pushing to production, we may miss some corner case), I think an opt-in approach would be safer for existing production apps.

[rhcarvalho]

I think there are a few more cases where behavior changed and we're not triggering a warning. Failing tests:

    test "warns on overlapping concrete route after :*" do
      assert ExUnit.CaptureIO.capture_io(:stderr, fn ->
        defmodule MatchConcreteOverlap do
          use Phoenix.Router
          import ExUnit.Assertions, except: [trace: 3]

          match :*, "/foo/:bar", UserController, :index
          get "/foo/bar", UserController, :show
        end

        conn = call(MatchConcreteOverlap, :get, "foo/bar")
        assert conn.resp_body == "users show"
      end) =~ "found route matching on \"/foo/bar\" after match(:*, \"/foo/:bar\")"
    end

    test "warns on specific route after glob wildcard :*" do
      assert ExUnit.CaptureIO.capture_io(:stderr, fn ->
        defmodule MatchGlobOverlap do
          use Phoenix.Router
          import ExUnit.Assertions, except: [trace: 3]

          match :*, "/*path", UserController, :index
          get "/foo", UserController, :show
        end

        conn = call(MatchGlobOverlap, :get, "foo")
        assert conn.resp_body == "users show"
      end) =~ "found route matching on \"/foo\" after match(:*, \"/*path\")"
    end

    test "warns on specific route after forward" do
      assert ExUnit.CaptureIO.capture_io(:stderr, fn ->
        defmodule MatchForwardOverlap do
          use Phoenix.Router
          import ExUnit.Assertions, except: [trace: 3]

          defmodule AdminPlug do
            def init(opts), do: opts
            def call(conn, _opts), do: Plug.Conn.send_resp(conn, 200, "admin forward")
          end

          forward "/admin", AdminPlug
          get "/admin/login", UserController, :show
        end

        conn = call(MatchForwardOverlap, :get, "admin/login")
        assert conn.resp_body == "users show"
      end) =~ "found route matching on \"/admin/login\" after forward(\"/admin\")"
    end