Rework anti-pattern doc about unsafe atom creation#15495
Conversation
|
Should we mention here other ways that can lead to dynamic atom creation like interpolation |
|
Good call, also
|
|
|
Updated the refactoring section to clarify the use of explicit conversions and pattern matching for string to atom conversions. Added examples and emphasized the importance of defining valid statuses within the same module.
josevalim
left a comment
There was a problem hiding this comment.
I pushed some changes cause I think we can be more direct as we don't need to talk about to_existing_atom's pitfalls.
I think that's fair, it's probably one of the common vectors where it can happen.
Looks much better indeed, thanks! |
|
|
||
| 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. | ||
|
|
There was a problem hiding this comment.
@lukaszsamson What about something like:
| 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, ...].
There was a problem hiding this comment.
@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.
Following the introduction of
to_exising_atom/2variations in #15483 and #15493.