Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/elixir/lib/list.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions lib/elixir/lib/string.ex
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
56 changes: 17 additions & 39 deletions lib/elixir/pages/anti-patterns/code-anti-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukaszsamson What about something like:

Suggested change
Finally, keep in mind that dynamic atoms could happen indirectly even if you are not calling `String.to_unsafe_atom/1` yourself. A typical example would be code parsing, using `Code.string_to_quoted/2` without the `:existing_atoms_only` or `:static_atoms_encoder` option.

@josevalim Also, this made me realize, would it be an interesting idea to support a list of atoms in existing_atoms_only? Assuming you're writing a sth like a calculator: [:cos, :sin, :exp, ...].

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sabiwara I don't think we need to talk about that here. But we can add disclaimers to string_to_quoted if necessary.

Also, this made me realize, would it be an interesting idea to support a list of atoms in existing_atoms_only? Assuming you're writing a sth like a calculator: [:cos, :sin, :exp, ...].

In this case, I think simply defining the atoms upfront (as suggested here) is enough, because you need to validate what was parsed anyway. The big benefit of to_existing_atom/2 is to parse and validate at once.

## Long parameter list

Expand Down
Loading