Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Credo.Check.Warning.UnsafeToAtom warning on compile time created atom #1148

Closed
Wigny opened this issue Aug 26, 2024 · 5 comments
Closed

Credo.Check.Warning.UnsafeToAtom warning on compile time created atom #1148

Wigny opened this issue Aug 26, 2024 · 5 comments

Comments

@Wigny
Copy link

Wigny commented Aug 26, 2024

Environment

  • Credo version (mix credo -v): 1.7.7-ref.dependabot-hex-gettext-0.26.1.58e9c1b08
  • Erlang/Elixir version (elixir -v): Elixir 1.17.2 (compiled with Erlang/OTP 26)
  • Operating system: Ubuntu 22.04.3 LTS

What were you trying to do?

Running the Credo.Check.Warning.UnsafeToAtom check (mix credo -c Credo.Check.Warning.UnsafeToAtom) is reporting right now issues for dynamically created atoms passed to unquote().

Expected outcome

AFAIK dynamically created atoms passed to unquote() are generated in compile time and thus should not be reported by this check, given they cannot be exploited in runtime. Thus the following code should not emit the check warning:

defmodule Test do
  for n <- 1..4 do
    def unquote(:"fun_#{n}")(), do: unquote(n)
  end
end

Actual outcome

Running the check on the code above returns

Prefer :erlang.binary_to_existing_atom/2 over :erlang.binary_to_atom/2 to avoid creating atoms at runtime.
@Wigny
Copy link
Author

Wigny commented Aug 26, 2024

Rewriting the code to the following avoids the warning, but I'm unsure if that is the right solution.

defmodule Test do
  for n <- 1..4 do
    def unquote(:erlang.binary_to_atom("fun_#{n}"))(), do: unquote(n)
  end
end

rrrene added a commit that referenced this issue Sep 22, 2024
@rrrene
Copy link
Owner

rrrene commented Sep 22, 2024

@Wigny Thanks for reporting this 😀 Sorry for the delay. It is now fixed on master.

You can try this by setting the Credo dep to

{:credo, github: "rrrene/credo"}

Please report back if your issue is solved! 👍

@Wigny
Copy link
Author

Wigny commented Sep 23, 2024

Yes, it worked great, thanks!

The only case the change didn't cover in our codebase was

unquote(context).unquote(:"get_#{type}_by")(id: id)

which I believe could have been written as this instead

apply(unquote(context), unquote(:"get_#{type}_by"), [[id: id]])

So it is all good IMO. Many thanks for solving it!

rrrene added a commit that referenced this issue Sep 23, 2024
@rrrene
Copy link
Owner

rrrene commented Sep 23, 2024

@Wigny Patched the check to cover that case as well. Thx for the input 👍

@Wigny Wigny closed this as completed Sep 25, 2024
@rrrene
Copy link
Owner

rrrene commented Sep 26, 2024

@Wigny This is part of Credo 1.7.8. Thx! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants
@rrrene @Wigny and others