diff --git a/lib/elixir/lib/list.ex b/lib/elixir/lib/list.ex index ab758ad287c..f62df412fb9 100644 --- a/lib/elixir/lib/list.ex +++ b/lib/elixir/lib/list.ex @@ -1046,6 +1046,7 @@ defmodule List do :"🌢 Elixir" """ + @doc since: "1.21.0" @spec to_unsafe_atom(charlist) :: atom def to_unsafe_atom(charlist) do :erlang.list_to_atom(charlist) @@ -1104,6 +1105,7 @@ defmodule List do ** (ArgumentError) unexpected value: ~c\"unknown\", the allowed atoms are: [:foo, :bar] """ + @doc since: "1.21.0" @spec to_existing_atom(charlist, nonempty_list(a)) :: a when a: atom() def to_existing_atom(charlist, [_ | _] = allowed_atoms) when is_list(charlist) do atom = :erlang.list_to_existing_atom(charlist) diff --git a/lib/elixir/lib/string.ex b/lib/elixir/lib/string.ex index c148577da6e..6c32ee54636 100644 --- a/lib/elixir/lib/string.ex +++ b/lib/elixir/lib/string.ex @@ -2988,6 +2988,7 @@ defmodule String do :my_atom """ + @doc since: "1.21.0" @spec to_unsafe_atom(String.t()) :: atom def to_unsafe_atom(string) when is_binary(string) do :erlang.binary_to_atom(string, :utf8) @@ -3046,6 +3047,7 @@ defmodule String do ** (ArgumentError) unexpected value: \"unknown\", the allowed atoms are: [:foo, :bar] """ + @doc since: "1.21.0" @spec to_existing_atom(String.t(), nonempty_list(a)) :: a when a: atom() def to_existing_atom(string, [_ | _] = allowed_atoms) when is_binary(string) do atom = :erlang.binary_to_existing_atom(string, :utf8) diff --git a/lib/elixir/pages/anti-patterns/code-anti-patterns.md b/lib/elixir/pages/anti-patterns/code-anti-patterns.md index 002768837e7..bcaca5ce163 100644 --- a/lib/elixir/pages/anti-patterns/code-anti-patterns.md +++ b/lib/elixir/pages/anti-patterns/code-anti-patterns.md @@ -169,71 +169,49 @@ When we use the `String.to_unsafe_atom/1` function to dynamically create an atom #### Refactoring -To eliminate this anti-pattern, developers must either perform explicit conversions by mapping strings to atoms or replace the use of `String.to_unsafe_atom/1` with `String.to_existing_atom/1`. An explicit conversion could be done as follows: +To eliminate this anti-pattern, developers must either: -```elixir -defmodule MyRequestHandler do - def parse(%{"status" => status, "message" => message} = _payload) do - %{status: convert_status(status), message: message} - end - - defp convert_status("ok"), do: :ok - defp convert_status("error"), do: :error - defp convert_status("redirect"), do: :redirect -end -``` + * replace the use of `String.to_unsafe_atom/1` with a list of expected atoms in `String.to_existing_atom/2` -```elixir -iex> MyRequestHandler.parse(%{"status" => "status_not_seen_anywhere", "message" => "all good"}) -** (FunctionClauseError) no function clause matching in MyRequestHandler.convert_status/1 -``` + * use pattern-matching to map strings to their equivalent atoms -By explicitly listing all supported statuses, you guarantee only a limited number of conversions may happen. Passing an invalid status will lead to a function clause error. + * use `String.to_existing_atom/1` when the former options are not possible -An alternative is to use `String.to_existing_atom/1`, which will only convert a string to atom if the atom already exists in the system: +An explicit conversion could be done as follows: ```elixir defmodule MyRequestHandler do def parse(%{"status" => status, "message" => message} = _payload) do - %{status: String.to_existing_atom(status), message: message} + status = String.to_existing_atom(status, [:ok, :error, :redirect]) + %{status: status, message: message} end end ``` +This will only accept valid values and convert `"ok"` as `:ok`, `"error"` as `:error`, and `"redirect"` as `:redirect`. Any other value would result in an `ArgumentError`, either because the atom doesn't exist or because it isn't part of the list: + ```elixir iex> MyRequestHandler.parse(%{"status" => "status_not_seen_anywhere", "message" => "all good"}) -** (ArgumentError) errors were found at the given arguments: - - * 1st argument: not an already existing atom +** (ArgumentError) ... ``` -In such cases, passing an unknown status will raise as long as the status was not defined anywhere as an atom in the system. However, assuming `status` can be either `:ok`, `:error`, or `:redirect`, how can you guarantee those atoms exist? You must ensure those atoms exist somewhere **in the same module** where `String.to_existing_atom/1` is called. For example, if you had this code: +By explicitly listing all supported statuses, you guarantee that only a limited number of conversions may happen, and that all expected atoms already exist within the system. + +An alternative is to use pattern matching and explicitly convert each string to its respective atom. This is useful when you need additional logic on differnt branches: ```elixir defmodule MyRequestHandler do def parse(%{"status" => status, "message" => message} = _payload) do - %{status: String.to_existing_atom(status), message: message} - end - - def handle(%{status: status}) do case status do - :ok -> ... - :error -> ... - :redirect -> ... + "ok" -> %{status: :ok, message: message} + "error" -> %{status: :error, message: message} + "redirect" -> %{status: :redirect, message: message, code: 302} end end end ``` -All valid statuses are defined as atoms within the same module, and that's enough. If you want to be explicit, you could also have a function that lists them: - -```elixir -def valid_statuses do - [:ok, :error, :redirect] -end -``` - -However, keep in mind using a module attribute or defining the atoms in the module body, outside of a function, are not sufficient, as the module body is only executed during compilation and it is not necessarily part of the compiled module loaded at runtime. +You may alternatively use `String.to_existing_atom/1`, but keep in mind it does have pitfalls related to code loading. Read the documentation for more information. ## Long parameter list