elixir-lang / elixir

Elixir is a dynamic, functional language for building scalable and maintainable applications
https://elixir-lang.org/
Apache License 2.0
24.44k stars 3.37k forks source link

Elixir 1.17 should provide better feedback on dead code #13715

Open bugnano opened 3 months ago

bugnano commented 3 months ago

Elixir and Erlang/OTP versions

Erlang/OTP 27 [erts-15.0] [source] [64-bit] [smp:6:6] [ds:6:6:10] [async-threads:1] [jit:ns]

Elixir 1.17.1 (compiled with Erlang/OTP 27)

Operating system

Ubuntu 24.04

Current behavior

In our codebase we have a guard defined like so:

  defguardp conn_scope(conn, target)
            when is_atom(target) and is_struct(conn, Conn) and
                   is_atom(conn.assigns.auth_profile.scope) and
                   conn.assigns.auth_profile == target

  defguardp session_scope(session, target)
            when is_atom(target) and is_struct(session, Session) and
                   is_atom(session.scope) and
                   session.scope == target

  defguard auth_scope(conn, target)
           when conn_scope(conn, target) or session_scope(conn, target)

so that we can use the guard auth_scope either with a parameter of type Conn or a parameter of type Session, and you can see that we use is_struct to restrict further guards to access the correct members of the appropriate struct.

With Elixir 1.17 the compiler gives the following warning:

     warning: unknown key .assigns in expression:

         session.assigns

     where "session" was given the type:

         # type: dynamic(%DataLayer.Session{
           acked_server_sequence: term(),
           app_id: term(),
           authenticated: term(),
           default_api_version: term(),
           expires_at: term(),
           expiry_timer: term(),
           external_user_id: term(),
           id: term(),
           permissions: term(),
           protocol_version: term(),
           scope: term(),
           sdk_name: term(),
           server_sequence: term(),
           socket_pid: term(),
           transport: term(),
           user_id: term()
         })
         # from: lib/backend_web/calls/conversation_calls.ex:110
         %DataLayer.Session{app_id: app_id} = session

     typing violation found at:
     │
 112 │       when auth_scope(session, :app) do
     │       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     │
     └─ lib/backend_web/calls/conversation_calls.ex:112: BackendWeb.ConversationCalls.delete/2

because it checks also for the guards of the other data structure.

Expected behavior

No warning being emitted, because is_struct narrows down subsequent guards to only 1 data type.

josevalim commented 3 months ago

Correct. the type system does not do narrowing (or occurrence typing) yet.

Can you provide more details about the code though? In the code snippet that you shared, there is no narrowing. You have simply specified it can be both using or.

bugnano commented 3 months ago

The code where the warning is is like so:

  def delete(
        %Call{params: %{"conversation_params" => conversation_param_or_id}} = call,
        %Session{app_id: app_id} = session
      )
      when auth_scope(session, :app) do

So there's a Session struct passed as a parameter to the function, and auth_scope is designed to work either with Session or with Conn.

Given that in the function definition we know that the parameter is a Session, the guards after is_struct(conn, Conn) should be ignored by the type checker, because is_struct(conn, Conn) is known to be false.

josevalim commented 3 months ago

Oh, I got it. We need to perform type refinement on the guard and we plan to support this in the next Elixir version. However I am afraid the type system will still warn you have dead code (i.e. part of your guard is always false). You would need to use session_scope if you only have a session.

josevalim commented 3 months ago

I changed the title to reflect that the error is correct but the message is wrong. It should rather say about dead code.

bugnano commented 3 months ago

I don't know if it's dead code.

I mean, on other parts of our codebase we use that guard with a Conn struct, so both parts of the guard are used, just not at the same time.

josevalim commented 3 months ago

The type system knows one of the sides of your or is always false because you have pattern matched on the struct. This is fine:

  def delete(
        %Call{params: %{"conversation_params" => conversation_param_or_id}} = call,
        session
      )
      when auth_scope(session, :app) do

But this means there is dead code inside the auth_scope guard since is_struct(session, Conn) is always false:

  def delete(
        %Call{params: %{"conversation_params" => conversation_param_or_id}} = call,
        %Session{app_id: app_id} = session
      )
      when auth_scope(session, :app) do
bugnano commented 3 months ago

Got it, thank you😁

marcandre commented 3 months ago

I changed the title to reflect that the error is correct but the message is wrong. It should rather say about dead code.

I'm trying to understand why the "error is correct"? IMO, there is no typing violation, there is no error in the code, dead code generated in a macro should not be an issue, and there should be no warning.

If I understand correctly, the LiveView fix is more of a hack and works because there is no real need to call __live__, otherwise the hack would fail if a module was passed as a variable instead of a literal module.

Maybe the name should be changed again? I did look for an open bug that mentionned "typing violation" and found none. The tags should also be changed as I believe it is not an enhancement but a bug.

josevalim commented 3 months ago

Generally speaking, we only ignore warnings from macros if they are tagged as generated: true. Which is supported for types as of #13727. In the absence of the annotation, it is treated as regular code, which is indeed “dead”.

marcandre commented 3 months ago

Got it, thanks.