From f38a29995322e64622fee1267049dd78fb1c447f Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Mon, 29 Apr 2024 18:09:45 +0300 Subject: [PATCH 01/10] Add correct column metadata to all warning checks --- lib/credo/check/refactor/io_puts.ex | 9 ++++---- .../application_config_in_module_attribute.ex | 21 ++++++++++--------- .../warning/bool_operation_on_same_values.ex | 9 ++++---- .../warning/expensive_empty_enum_check.ex | 7 ++++--- lib/credo/check/warning/forbidden_module.ex | 13 ++++++------ lib/credo/check/warning/iex_pry.ex | 7 ++++--- lib/credo/check/warning/lazy_logging.ex | 9 ++++---- lib/credo/check/warning/leaky_environment.ex | 18 +++++++++------- .../check/warning/map_get_unsafe_pass.ex | 9 ++++---- .../missed_metadata_key_in_logger_config.ex | 9 ++++---- lib/credo/check/warning/mix_env.ex | 9 ++++---- .../check/warning/operation_on_same_values.ex | 9 ++++---- .../warning/operation_with_constant_result.ex | 9 ++++---- .../check/warning/raise_inside_rescue.ex | 9 ++++---- lib/credo/check/warning/spec_with_struct.ex | 15 ++++++------- lib/credo/check/warning/unsafe_exec.ex | 7 ++++--- .../warning/unused_function_return_helper.ex | 4 ++-- lib/credo/check/warning/unused_operation.ex | 9 ++++---- test/credo/check/refactor/io_puts_test.exs | 18 ++++++++++++++++ ...cation_config_in_module_attribute_test.exs | 18 ++++++++-------- .../bool_operation_on_same_values_test.exs | 2 ++ .../expensive_empty_enum_check_test.exs | 16 ++++++++++++-- .../check/warning/forbidden_module_test.exs | 19 ++++++++++++++--- test/credo/check/warning/iex_pry_test.exs | 18 ++++++++++++++++ .../credo/check/warning/lazy_logging_test.exs | 13 +++++++++++- .../check/warning/leaky_environment_test.exs | 3 +++ .../warning/map_get_unsafe_pass_test.exs | 11 ++++++++-- ...sed_metadata_key_in_logger_config_test.exs | 15 +++++++++++-- test/credo/check/warning/mix_env_test.exs | 18 ++++++++++++++++ .../warning/operation_on_same_values_test.exs | 7 ++++++- .../operation_with_constant_result_test.exs | 10 +++++++-- .../warning/raise_inside_rescue_test.exs | 3 +++ .../check/warning/spec_with_struct_test.exs | 17 ++++++++++++--- test/credo/check/warning/unsafe_exec_test.exs | 6 +++++- .../warning/unused_enum_operation_test.exs | 4 +++- 35 files changed, 272 insertions(+), 108 deletions(-) diff --git a/lib/credo/check/refactor/io_puts.ex b/lib/credo/check/refactor/io_puts.ex index 352b0acb9..af89bd8a9 100644 --- a/lib/credo/check/refactor/io_puts.ex +++ b/lib/credo/check/refactor/io_puts.ex @@ -24,7 +24,7 @@ defmodule Credo.Check.Refactor.IoPuts do end defp traverse( - {{:., _, [{:__aliases__, _, [:IO]}, :puts]}, meta, _arguments} = ast, + {{:., _, [{:__aliases__, meta, [:IO]}, :puts]}, _, _arguments} = ast, issues, issue_meta ) do @@ -36,15 +36,16 @@ defmodule Credo.Check.Refactor.IoPuts do end defp issues_for_call(meta, issues, issue_meta) do - [issue_for(issue_meta, meta[:line], @call_string) | issues] + [issue_for(issue_meta, meta, @call_string) | issues] end - defp issue_for(issue_meta, line_no, trigger) do + defp issue_for(issue_meta, meta, trigger) do format_issue( issue_meta, message: "There should be no calls to `IO.puts/1`.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/application_config_in_module_attribute.ex b/lib/credo/check/warning/application_config_in_module_attribute.ex index cdedcc899..d3da94cce 100644 --- a/lib/credo/check/warning/application_config_in_module_attribute.ex +++ b/lib/credo/check/warning/application_config_in_module_attribute.ex @@ -65,44 +65,45 @@ defmodule Credo.Check.Warning.ApplicationConfigInModuleAttribute do end defp get_forbidden_call( - {{:., _, [{:__aliases__, _, [:Application]}, :fetch_env]}, _meta, _args} = ast, + {{:., _, [{:__aliases__, meta, [:Application]}, :fetch_env]}, _meta, _args} = ast, _acc ) do - {ast, {"Application.fetch_env/2", "Application.fetch_env"}} + {ast, {meta, "Application.fetch_env/2", "Application.fetch_env"}} end defp get_forbidden_call( - {{:., _, [{:__aliases__, _, [:Application]}, :fetch_env!]}, _meta, _args} = ast, + {{:., _, [{:__aliases__, meta, [:Application]}, :fetch_env!]}, _meta, _args} = ast, _acc ) do - {ast, {"Application.fetch_env!/2", "Application.fetch_env"}} + {ast, {meta, "Application.fetch_env!/2", "Application.fetch_env"}} end defp get_forbidden_call( - {{:., _, [{:__aliases__, _, [:Application]}, :get_all_env]}, _meta, _args} = ast, + {{:., _, [{:__aliases__, meta, [:Application]}, :get_all_env]}, _meta, _args} = ast, _acc ) do - {ast, {"Application.get_all_env/1", "Application.get_all_env"}} + {ast, {meta, "Application.get_all_env/1", "Application.get_all_env"}} end defp get_forbidden_call( - {{:., _, [{:__aliases__, _, [:Application]}, :get_env]}, _meta, args} = ast, + {{:., _, [{:__aliases__, meta, [:Application]}, :get_env]}, _meta, args} = ast, _acc ) do - {ast, {"Application.get_env/#{length(args)}", "Application.get_env"}} + {ast, {meta, "Application.get_env/#{length(args)}", "Application.get_env"}} end defp get_forbidden_call(ast, acc) do {ast, acc} end - defp issues_for_call(attribute, {call, trigger}, meta, issue_meta, issues) do + defp issues_for_call(attribute, {call_meta, call, trigger}, _meta, issue_meta, issues) do [ format_issue(issue_meta, message: "Module attribute @#{Atom.to_string(attribute)} makes use of unsafe Application configuration call #{call}", trigger: trigger, - line_no: meta[:line] + line_no: call_meta[:line], + column: call_meta[:column] ) | issues ] diff --git a/lib/credo/check/warning/bool_operation_on_same_values.ex b/lib/credo/check/warning/bool_operation_on_same_values.ex index 341f866ca..579c911be 100644 --- a/lib/credo/check/warning/bool_operation_on_same_values.ex +++ b/lib/credo/check/warning/bool_operation_on_same_values.ex @@ -43,8 +43,8 @@ defmodule Credo.Check.Warning.BoolOperationOnSameValues do op_not_redefined? = unquote(op) not in redefined_ops if op_not_redefined? && Credo.Code.remove_metadata(lhs) === Credo.Code.remove_metadata(rhs) do - new_issue = issue_for(issue_meta, meta[:line], unquote(op)) - {ast, issues ++ [new_issue]} + new_issue = issue_for(issue_meta, meta, unquote(op)) + {ast, [new_issue | issues]} else {ast, issues} end @@ -86,13 +86,14 @@ defmodule Credo.Check.Warning.BoolOperationOnSameValues do {ast, acc} end - defp issue_for(issue_meta, line_no, trigger) do + defp issue_for(issue_meta, meta, trigger) do format_issue( issue_meta, message: "There are identical sub-expressions to the left and to the right of the '#{trigger}' operator.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/expensive_empty_enum_check.ex b/lib/credo/check/warning/expensive_empty_enum_check.ex index 537831ba0..8722305ff 100644 --- a/lib/credo/check/warning/expensive_empty_enum_check.ex +++ b/lib/credo/check/warning/expensive_empty_enum_check.ex @@ -63,7 +63,7 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheck do end defp issues_for_call(meta, trigger, issues, issue_meta, ast) do - [issue_for(issue_meta, meta[:line], trigger, suggest(ast)) | issues] + [issue_for(issue_meta, meta, trigger, suggest(ast)) | issues] end defp suggest({_op, _, [0, {_pattern, _, args}]}), do: suggest_for_arity(Enum.count(args)) @@ -72,12 +72,13 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheck do defp suggest_for_arity(2), do: "`not Enum.any?/2`" defp suggest_for_arity(1), do: "`Enum.empty?/1` or `list == []`" - defp issue_for(issue_meta, line_no, trigger, suggestion) do + defp issue_for(issue_meta, meta, trigger, suggestion) do format_issue( issue_meta, message: "#{trigger} is expensive, prefer #{suggestion}.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/forbidden_module.ex b/lib/credo/check/warning/forbidden_module.ex index ebedd1161..3ba07bb3d 100644 --- a/lib/credo/check/warning/forbidden_module.ex +++ b/lib/credo/check/warning/forbidden_module.ex @@ -43,7 +43,7 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse({:__aliases__, meta, modules} = ast, issues, forbidden_modules, issue_meta) do module = Name.full(modules) - issues = put_issue_if_forbidden(issues, issue_meta, meta[:line], module, forbidden_modules) + issues = put_issue_if_forbidden(issues, issue_meta, meta, module, forbidden_modules) {ast, issues} end @@ -56,12 +56,12 @@ defmodule Credo.Check.Warning.ForbiddenModule do ) do modules = Enum.map(aliases, fn {:__aliases__, meta, module} -> - {Name.full([base_alias, module]), meta[:line]} + {Name.full([base_alias, module]), meta} end) issues = - Enum.reduce(modules, issues, fn {module, line}, issues -> - put_issue_if_forbidden(issues, issue_meta, line, module, forbidden_modules) + Enum.reduce(modules, issues, fn {module, meta}, issues -> + put_issue_if_forbidden(issues, issue_meta, meta, module, forbidden_modules) end) {ast, issues} @@ -83,7 +83,7 @@ defmodule Credo.Check.Warning.ForbiddenModule do Enum.member?(forbidden_module_names, module) end - defp issue_for(issue_meta, line_no, module, forbidden_modules) do + defp issue_for(issue_meta, meta, module, forbidden_modules) do trigger = Name.full(module) message = message(forbidden_modules, module) || "The `#{trigger}` module is not allowed." @@ -91,7 +91,8 @@ defmodule Credo.Check.Warning.ForbiddenModule do issue_meta, message: message, trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end diff --git a/lib/credo/check/warning/iex_pry.ex b/lib/credo/check/warning/iex_pry.ex index f908251ce..b2ede1f3d 100644 --- a/lib/credo/check/warning/iex_pry.ex +++ b/lib/credo/check/warning/iex_pry.ex @@ -24,8 +24,8 @@ defmodule Credo.Check.Warning.IExPry do defp traverse( { - {:., _, [{:__aliases__, _, [:IEx]}, :pry]}, - meta, + {:., _, [{:__aliases__, meta, [:IEx]}, :pry]}, + _, _arguments } = ast, issues, @@ -44,7 +44,8 @@ defmodule Credo.Check.Warning.IExPry do issue_meta, message: "There should be no calls to `IEx.pry/0`.", trigger: @call_string, - line_no: meta[:line] + line_no: meta[:line], + column: meta[:column] ) [new_issue | issues] diff --git a/lib/credo/check/warning/lazy_logging.ex b/lib/credo/check/warning/lazy_logging.ex index a0240abc8..5f7e87611 100644 --- a/lib/credo/check/warning/lazy_logging.ex +++ b/lib/credo/check/warning/lazy_logging.ex @@ -46,7 +46,7 @@ defmodule Credo.Check.Warning.LazyLogging do end defp traverse( - {{:., _, [{:__aliases__, _, [:Logger]}, fun_name]}, meta, arguments} = ast, + {{:., _, [{:__aliases__, meta, [:Logger]}, fun_name]}, _, arguments} = ast, state, issue_meta ) @@ -99,7 +99,7 @@ defmodule Credo.Check.Warning.LazyLogging do end defp issue_for_call([{:<<>>, _, [_ | _]} | _] = _args, meta, fun_name, issue_meta) do - issue_for(issue_meta, meta[:line], fun_name) + issue_for(issue_meta, meta, fun_name) end defp issue_for_call(_args, _meta, _trigger, _issue_meta) do @@ -109,11 +109,12 @@ defmodule Credo.Check.Warning.LazyLogging do defp logger_import?([{:__aliases__, _meta, [:Logger]}]), do: true defp logger_import?(_), do: false - defp issue_for(issue_meta, line_no, trigger) do + defp issue_for(issue_meta, meta, trigger) do format_issue( issue_meta, message: "Prefer lazy Logger calls.", - line_no: line_no, + line_no: meta[:line], + column: meta[:column], trigger: trigger ) end diff --git a/lib/credo/check/warning/leaky_environment.ex b/lib/credo/check/warning/leaky_environment.ex index dda853845..663f786ee 100644 --- a/lib/credo/check/warning/leaky_environment.ex +++ b/lib/credo/check/warning/leaky_environment.ex @@ -32,8 +32,11 @@ defmodule Credo.Check.Warning.LeakyEnvironment do nil -> {ast, issues} + {trigger, meta} -> + {ast, [issue_for(issue_meta, meta, trigger) | issues]} + trigger -> - {ast, [issue_for(issue_meta, meta[:line], trigger) | issues]} + {ast, [issue_for(issue_meta, meta, trigger) | issues]} end end @@ -41,14 +44,14 @@ defmodule Credo.Check.Warning.LeakyEnvironment do {ast, issues} end - defp get_forbidden_call([{:__aliases__, _, [:System]}, :cmd], [_, _]) do - "System.cmd" + defp get_forbidden_call([{:__aliases__, meta, [:System]}, :cmd], [_, _]) do + {"System.cmd", meta} end - defp get_forbidden_call([{:__aliases__, _, [:System]}, :cmd], [_, _, opts]) + defp get_forbidden_call([{:__aliases__, meta, [:System]}, :cmd], [_, _, opts]) when is_list(opts) do if not Keyword.has_key?(opts, :env) do - "System.cmd" + {"System.cmd", meta} end end @@ -63,12 +66,13 @@ defmodule Credo.Check.Warning.LeakyEnvironment do nil end - defp issue_for(issue_meta, line_no, trigger) do + defp issue_for(issue_meta, meta, trigger) do format_issue( issue_meta, message: "When using #{trigger}, clear or overwrite sensitive environment variables.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/map_get_unsafe_pass.ex b/lib/credo/check/warning/map_get_unsafe_pass.ex index e8d2d62ad..405dbc6c1 100644 --- a/lib/credo/check/warning/map_get_unsafe_pass.ex +++ b/lib/credo/check/warning/map_get_unsafe_pass.ex @@ -57,9 +57,9 @@ defmodule Credo.Check.Warning.MapGetUnsafePass do {next_expr, _} = Enum.at(pipe, idx + 1, {nil, nil}) case {expr, nil_safe?(next_expr)} do - {{{{:., meta, [{_, _, [:Map]}, :get]}, _, args}, _}, false} + {{{{:., _, [{_, meta, [:Map]}, :get]}, _, args}, _}, false} when length(args) != required_length -> - acc ++ [issue_for(issue_meta, meta[:line], @call_string)] + [issue_for(issue_meta, meta, @call_string) | acc] _ -> acc @@ -80,13 +80,14 @@ defmodule Credo.Check.Warning.MapGetUnsafePass do end end - defp issue_for(issue_meta, line_no, trigger) do + defp issue_for(issue_meta, meta, trigger) do format_issue( issue_meta, message: "`Map.get` with no default return value is potentially unsafe in pipes, use `Map.get/3` instead.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex b/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex index b4a8a617f..1fbc58075 100644 --- a/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex +++ b/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex @@ -78,7 +78,7 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfig do end defp traverse( - {{:., _, [{:__aliases__, _, [:Logger]}, fun_name]}, meta, arguments} = ast, + {{:., _, [{:__aliases__, meta, [:Logger]}, fun_name]}, _, arguments} = ast, state, issue_meta, metadata_keys @@ -151,7 +151,7 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfig do nil missed -> - issue_for(issue_meta, meta[:line], Keyword.keys(missed)) + issue_for(issue_meta, meta, Keyword.keys(missed)) end end end @@ -176,10 +176,11 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfig do |> Keyword.get(:metadata) end - defp issue_for(issue_meta, line_no, [trigger | _] = missed_keys) do + defp issue_for(issue_meta, meta, [trigger | _] = missed_keys) do format_issue(issue_meta, message: "Logger metadata key #{Enum.join(missed_keys, ", ")} not found in Logger config.", - line_no: line_no, + line_no: meta[:line], + column: meta[:column], trigger: trigger ) end diff --git a/lib/credo/check/warning/mix_env.ex b/lib/credo/check/warning/mix_env.ex index eb2f542a3..6fddba684 100644 --- a/lib/credo/check/warning/mix_env.ex +++ b/lib/credo/check/warning/mix_env.ex @@ -66,7 +66,7 @@ defmodule Credo.Check.Warning.MixEnv do end defp traverse_defs( - {{:., _, [{:__aliases__, _, [:Mix]}, :env]}, meta, _arguments} = ast, + {{:., _, [{:__aliases__, meta, [:Mix]}, :env]}, _, _arguments} = ast, issues, issue_meta ) do @@ -78,15 +78,16 @@ defmodule Credo.Check.Warning.MixEnv do end defp issues_for_call(meta, issues, issue_meta) do - [issue_for(issue_meta, meta[:line]) | issues] + [issue_for(issue_meta, meta) | issues] end - defp issue_for(issue_meta, line_no) do + defp issue_for(issue_meta, meta) do format_issue( issue_meta, message: "There should be no calls to Mix.env in application code.", trigger: "Mix.env", - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/operation_on_same_values.ex b/lib/credo/check/warning/operation_on_same_values.ex index 0abda9148..7ec8f0b5f 100644 --- a/lib/credo/check/warning/operation_on_same_values.ex +++ b/lib/credo/check/warning/operation_on_same_values.ex @@ -62,13 +62,13 @@ defmodule Credo.Check.Warning.OperationOnSameValues do new_issue = issue_for( issue_meta, - meta[:line], + meta, unquote(op), unquote(operation_name), unquote(constant_result) ) - {ast, issues ++ [new_issue]} + {ast, [new_issue | issues]} else {ast, issues} end @@ -88,12 +88,13 @@ defmodule Credo.Check.Warning.OperationOnSameValues do {ast, issues} end - defp issue_for(issue_meta, line_no, trigger, operation, constant_result) do + defp issue_for(issue_meta, meta, trigger, operation, constant_result) do format_issue( issue_meta, message: "#{operation} will always return #{constant_result}.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/operation_with_constant_result.ex b/lib/credo/check/warning/operation_with_constant_result.ex index 4da87fbbd..7c77b2ad4 100644 --- a/lib/credo/check/warning/operation_with_constant_result.ex +++ b/lib/credo/check/warning/operation_with_constant_result.ex @@ -49,12 +49,12 @@ defmodule Credo.Check.Warning.OperationWithConstantResult do new_issue = issue_for( issue_meta, - meta[:line], + meta, unquote(op), unquote(constant_result) ) - {ast, issues ++ [new_issue]} + {ast, [new_issue | issues]} end end @@ -62,12 +62,13 @@ defmodule Credo.Check.Warning.OperationWithConstantResult do {ast, issues} end - defp issue_for(issue_meta, line_no, trigger, constant_result) do + defp issue_for(issue_meta, meta, trigger, constant_result) do format_issue( issue_meta, message: "Operation will always return #{constant_result}.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/raise_inside_rescue.ex b/lib/credo/check/warning/raise_inside_rescue.ex index 957ef4bec..86e182564 100644 --- a/lib/credo/check/warning/raise_inside_rescue.ex +++ b/lib/credo/check/warning/raise_inside_rescue.ex @@ -71,19 +71,20 @@ defmodule Credo.Check.Warning.RaiseInsideRescue do defp traverse(ast, issues, _issue_meta), do: {ast, issues} defp find_issues({:raise, meta, _arguments} = ast, issues, issue_meta) do - issue = issue_for(issue_meta, meta[:line]) + issue = issue_for(issue_meta, meta) - {ast, issues ++ [issue]} + {ast, [issue | issues]} end defp find_issues(ast, issues, _), do: {ast, issues} - defp issue_for(issue_meta, line_no) do + defp issue_for(issue_meta, meta) do format_issue( issue_meta, message: "Use `reraise` inside a rescue block to preserve the original stacktrace.", trigger: "raise", - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/spec_with_struct.ex b/lib/credo/check/warning/spec_with_struct.ex index 021740abe..497054c53 100644 --- a/lib/credo/check/warning/spec_with_struct.ex +++ b/lib/credo/check/warning/spec_with_struct.ex @@ -29,15 +29,15 @@ defmodule Credo.Check.Warning.SpecWithStruct do Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta)) end - defp traverse({:@, meta, [{:spec, _, args}]}, issues, issue_meta) do + defp traverse({:@, _, [{:spec, _, args}]}, issues, issue_meta) do case Macro.prewalk(args, [], &find_structs/2) do {ast, []} -> {ast, issues} {ast, structs} -> issues = - Enum.reduce(structs, issues, fn curr, acc -> - [issue_for(issue_meta, meta[:line], curr) | acc] + Enum.reduce(structs, issues, fn {curr, meta}, acc -> + [issue_for(issue_meta, meta, curr) | acc] end) {ast, issues} @@ -48,19 +48,20 @@ defmodule Credo.Check.Warning.SpecWithStruct do {ast, issues} end - defp find_structs({:%, _, [{:__aliases__, _, _} = aliases | _]} = ast, acc) do - {ast, [Name.full(aliases) | acc]} + defp find_structs({:%, meta, [{:__aliases__, _, _} = aliases | _]} = ast, acc) do + {ast, [{Name.full(aliases), meta} | acc]} end defp find_structs(ast, acc) do {ast, acc} end - defp issue_for(issue_meta, line_no, struct) do + defp issue_for(issue_meta, meta, struct) do format_issue(issue_meta, message: "Struct %#{struct}{} found in `@spec`.", trigger: "%#{struct}{", - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/unsafe_exec.ex b/lib/credo/check/warning/unsafe_exec.ex index 1b8a18528..f1fb71738 100644 --- a/lib/credo/check/warning/unsafe_exec.ex +++ b/lib/credo/check/warning/unsafe_exec.ex @@ -37,7 +37,7 @@ defmodule Credo.Check.Warning.UnsafeExec do defp traverse({{:., _loc, call}, meta, args} = ast, issues, issue_meta) do case get_forbidden_call(call, args) do {bad, suggestion, trigger} -> - {ast, [issue_for(bad, suggestion, trigger, meta[:line], issue_meta) | issues]} + {ast, [issue_for(bad, suggestion, trigger, meta, issue_meta) | issues]} nil -> {ast, issues} @@ -65,11 +65,12 @@ defmodule Credo.Check.Warning.UnsafeExec do nil end - defp issue_for(call, suggestion, trigger, line_no, issue_meta) do + defp issue_for(call, suggestion, trigger, meta, issue_meta) do format_issue(issue_meta, message: "Prefer #{suggestion} over #{call} to prevent command injection.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/lib/credo/check/warning/unused_function_return_helper.ex b/lib/credo/check/warning/unused_function_return_helper.ex index bb4fda254..d0f05f8b3 100644 --- a/lib/credo/check/warning/unused_function_return_helper.ex +++ b/lib/credo/check/warning/unused_function_return_helper.ex @@ -40,7 +40,7 @@ defmodule Credo.Check.Warning.UnusedFunctionReturnHelper do nil ) do if mods == required_mod_list do - {ast, acc ++ [ast]} + {ast, [ast | acc]} else {ast, acc} end @@ -53,7 +53,7 @@ defmodule Credo.Check.Warning.UnusedFunctionReturnHelper do restrict_fun_names ) do if mods == required_mod_list and fun_name in restrict_fun_names do - {ast, acc ++ [ast]} + {ast, [ast | acc]} else {ast, acc} end diff --git a/lib/credo/check/warning/unused_operation.ex b/lib/credo/check/warning/unused_operation.ex index b82a466fd..125b280f9 100644 --- a/lib/credo/check/warning/unused_operation.ex +++ b/lib/credo/check/warning/unused_operation.ex @@ -26,7 +26,7 @@ defmodule Credo.Check.Warning.UnusedOperation do ) Enum.reduce(all_unused_calls, [], fn invalid_call, issues -> - {_, meta, _} = invalid_call + {{:., _, [{:__aliases__, meta, _}, _fun_name]}, _, _} = invalid_call trigger = invalid_call @@ -34,16 +34,17 @@ defmodule Credo.Check.Warning.UnusedOperation do |> String.split("(") |> List.first() - issues ++ [issue_for(format_issue_fun, issue_meta, meta[:line], trigger, checked_module)] + [issue_for(format_issue_fun, issue_meta, meta, trigger, checked_module) | issues] end) end - defp issue_for(format_issue_fun, issue_meta, line_no, trigger, checked_module) do + defp issue_for(format_issue_fun, issue_meta, meta, trigger, checked_module) do format_issue_fun.( issue_meta, message: "There should be no unused return values for #{checked_module} functions.", trigger: trigger, - line_no: line_no + line_no: meta[:line], + column: meta[:column] ) end end diff --git a/test/credo/check/refactor/io_puts_test.exs b/test/credo/check/refactor/io_puts_test.exs index 42046c80f..642f63729 100644 --- a/test/credo/check/refactor/io_puts_test.exs +++ b/test/credo/check/refactor/io_puts_test.exs @@ -37,6 +37,24 @@ defmodule Credo.Check.Refactor.IoPutsTest do |> assert_issue() end + test "it should report a violation with two on the same line" do + """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + IO.puts(parameter1); IO.puts(parameter2) + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn [one, two] -> + assert one.line_no == 3 + assert one.column == 26 + assert two.line_no == 3 + assert two.column == 5 + end) + end + test "it should report a violation /2" do """ defmodule CredoSampleModule do diff --git a/test/credo/check/warning/application_config_in_module_attribute_test.exs b/test/credo/check/warning/application_config_in_module_attribute_test.exs index b4b0e07b9..a79efc7a8 100644 --- a/test/credo/check/warning/application_config_in_module_attribute_test.exs +++ b/test/credo/check/warning/application_config_in_module_attribute_test.exs @@ -67,41 +67,41 @@ defmodule Credo.Check.Warning.ApplicationConfigInModuleAttributeTest do assert_issues = [ { "Module attribute @config_1 makes use of unsafe Application configuration call Application.fetch_env/2", - 2, + {2, 13}, "Application.fetch_env" }, { "Module attribute @config_3 makes use of unsafe Application configuration call Application.fetch_env!/2", - 4, + {4, 13}, "Application.fetch_env" }, { "Module attribute @config_5 makes use of unsafe Application configuration call Application.get_all_env/1", - 6, + {6, 13}, "Application.get_all_env" }, { "Module attribute @config_7 makes use of unsafe Application configuration call Application.get_env/2", - 8, + {8, 13}, "Application.get_env" }, { "Module attribute @config_9 makes use of unsafe Application configuration call Application.get_env/3", - 10, + {10, 13}, "Application.get_env" } ] assert length(issues) == 5 - Enum.each(assert_issues, fn {error_message, line_no, trigger} -> - assert error_exists?(issues, error_message, line_no, trigger) + Enum.each(assert_issues, fn {error_message, pos, trigger} -> + assert error_exists?(issues, error_message, pos, trigger) end) end - defp error_exists?(errors, error_message, line_no, trigger) do + defp error_exists?(errors, error_message, {line_no, column}, trigger) do Enum.any?(errors, fn - %Credo.Issue{message: ^error_message, line_no: ^line_no, trigger: ^trigger} -> true + %Credo.Issue{message: ^error_message, line_no: ^line_no, column: ^column, trigger: ^trigger} -> true _ -> false end) end diff --git a/test/credo/check/warning/bool_operation_on_same_values_test.exs b/test/credo/check/warning/bool_operation_on_same_values_test.exs index 4bdc839e2..58085022f 100644 --- a/test/credo/check/warning/bool_operation_on_same_values_test.exs +++ b/test/credo/check/warning/bool_operation_on_same_values_test.exs @@ -98,6 +98,8 @@ defmodule Credo.Check.Warning.BoolOperationOnSameValuesTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.trigger == "and" + assert issue.line_no == 5 + assert issue.column == 7 end) end end diff --git a/test/credo/check/warning/expensive_empty_enum_check_test.exs b/test/credo/check/warning/expensive_empty_enum_check_test.exs index ec4f4336b..05b131ca5 100644 --- a/test/credo/check/warning/expensive_empty_enum_check_test.exs +++ b/test/credo/check/warning/expensive_empty_enum_check_test.exs @@ -131,6 +131,8 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.trigger == "length" + assert issue.line_no == 3 + assert issue.column == 26 end) end @@ -148,7 +150,11 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.trigger == "length" + assert issue.line_no == 3 + assert issue.column == 10 + end) end test "it should report when checking if Enum.count/1 is 0" do @@ -168,6 +174,8 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do |> assert_issue(fn issue -> assert issue.trigger == "Enum.count" assert issue.message =~ "Enum.empty" + assert issue.line_no == 3 + assert issue.column == 30 end) end @@ -185,7 +193,11 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issue(fn issue -> assert issue.message =~ "Enum.empty" end) + |> assert_issue(fn issue -> + assert issue.message =~ "Enum.empty" + assert issue.line_no == 3 + assert issue.column == 10 + end) end test "it should report when checking if Enum.count/2 is 0" do diff --git a/test/credo/check/warning/forbidden_module_test.exs b/test/credo/check/warning/forbidden_module_test.exs index ee713a68e..39504b681 100644 --- a/test/credo/check/warning/forbidden_module_test.exs +++ b/test/credo/check/warning/forbidden_module_test.exs @@ -50,7 +50,10 @@ defmodule Credo.Check.Warning.ForbiddenModuleTest do """ |> to_source_file |> run_check(@described_check, modules: [CredoSampleModule.ForbiddenModule]) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 2 + assert issue.column == 26 + end) end test "it should report on aliases" do @@ -62,7 +65,10 @@ defmodule Credo.Check.Warning.ForbiddenModuleTest do """ |> to_source_file |> run_check(@described_check, modules: [CredoSampleModule.ForbiddenModule]) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 2 + assert issue.column == 9 + end) end test "it should report on grouped aliases" do @@ -76,7 +82,14 @@ defmodule Credo.Check.Warning.ForbiddenModuleTest do |> run_check(@described_check, modules: [CredoSampleModule.ForbiddenModule, CredoSampleModule.ForbiddenModule2] ) - |> assert_issues() + |> assert_issues(fn [one, two] -> + assert one.trigger == "CredoSampleModule.ForbiddenModule2" + assert one.line_no == 2 + assert one.column == 60 + assert two.trigger == "CredoSampleModule.ForbiddenModule" + assert two.line_no == 2 + assert two.column == 43 + end) end test "it should report on import" do diff --git a/test/credo/check/warning/iex_pry_test.exs b/test/credo/check/warning/iex_pry_test.exs index d9f0f1c07..fc386aeb8 100644 --- a/test/credo/check/warning/iex_pry_test.exs +++ b/test/credo/check/warning/iex_pry_test.exs @@ -40,4 +40,22 @@ defmodule Credo.Check.Warning.IExPryTest do assert issue.trigger == "IEx.pry" end) end + + test "it should report a violation with two on the same line" do + """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + IEx.pry(); IEx.pry() + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn [one, two] -> + assert one.line_no == 3 + assert one.column == 16 + assert two.line_no == 3 + assert two.column == 5 + end) + end end diff --git a/test/credo/check/warning/lazy_logging_test.exs b/test/credo/check/warning/lazy_logging_test.exs index 98a5caa79..6822b5f72 100644 --- a/test/credo/check/warning/lazy_logging_test.exs +++ b/test/credo/check/warning/lazy_logging_test.exs @@ -120,7 +120,16 @@ defmodule Credo.Check.Warning.LazyLoggingTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues() + |> assert_issues(fn [three, two, one] -> + assert one.line_no == 5 + assert one.column == 5 + + assert two.line_no == 6 + assert two.column == 5 + + assert three.line_no == 7 + assert three.column == 5 + end) end test "it should report a violation with imported :debug from Logger" do @@ -137,6 +146,8 @@ defmodule Credo.Check.Warning.LazyLoggingTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.trigger == "debug" + assert issue.line_no == 5 + assert issue.column == 5 end) end end diff --git a/test/credo/check/warning/leaky_environment_test.exs b/test/credo/check/warning/leaky_environment_test.exs index 0838c584e..c5ada3f70 100644 --- a/test/credo/check/warning/leaky_environment_test.exs +++ b/test/credo/check/warning/leaky_environment_test.exs @@ -40,6 +40,7 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 + assert issue.column == 5 assert issue.trigger == "System.cmd" end) end @@ -56,6 +57,7 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 + assert issue.column == 5 assert issue.trigger == "System.cmd" end) end @@ -72,6 +74,7 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 + assert issue.column == 13 assert issue.trigger == ":erlang.open_port" end) end diff --git a/test/credo/check/warning/map_get_unsafe_pass_test.exs b/test/credo/check/warning/map_get_unsafe_pass_test.exs index 250f5f3a8..502bb964c 100644 --- a/test/credo/check/warning/map_get_unsafe_pass_test.exs +++ b/test/credo/check/warning/map_get_unsafe_pass_test.exs @@ -76,7 +76,10 @@ defmodule Credo.Check.Warning.MapGetUnsafePassTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 5 + assert issue.column == 8 + end) end test "it should report a violation /2" do @@ -93,7 +96,10 @@ defmodule Credo.Check.Warning.MapGetUnsafePassTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 5 + assert issue.column == 5 + end) end test "it should report a violation /3" do @@ -116,6 +122,7 @@ defmodule Credo.Check.Warning.MapGetUnsafePassTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 7 + assert issue.column == 22 assert issue.trigger == "Map.get" end) end diff --git a/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs b/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs index a8d242d62..e8e45e5a6 100644 --- a/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs +++ b/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs @@ -136,7 +136,10 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfigTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 5 + assert issue.column == 5 + end) end end @@ -153,7 +156,15 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfigTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues() + |> assert_issues(fn [two, one] -> + assert one.trigger == "user_id" + assert one.line_no == 4 + assert one.column == 5 + + assert two.trigger == "key" + assert two.line_no == 6 + assert two.column == 5 + end) end test "it should report a violation when Logger.log/3 is used with disallowed metadata" do diff --git a/test/credo/check/warning/mix_env_test.exs b/test/credo/check/warning/mix_env_test.exs index f0f6d3d16..d2604cfbc 100644 --- a/test/credo/check/warning/mix_env_test.exs +++ b/test/credo/check/warning/mix_env_test.exs @@ -128,6 +128,24 @@ defmodule Credo.Check.Warning.MixEnvTest do end) end + test "it should report a violation with two on the same line" do + """ + defmodule CredoSampleModule do + def some_function(parameter1, parameter2) do + Mix.env(); Mix.env() + end + end + """ + |> to_source_file + |> run_check(@described_check) + |> assert_issues(fn [one, two] -> + assert one.line_no == 3 + assert one.column == 16 + assert two.line_no == 3 + assert two.column == 5 + end) + end + test "it should report violations from variables named like def operations" do """ defmodule CredoSampleModule do diff --git a/test/credo/check/warning/operation_on_same_values_test.exs b/test/credo/check/warning/operation_on_same_values_test.exs index 6c5eae444..20d57b482 100644 --- a/test/credo/check/warning/operation_on_same_values_test.exs +++ b/test/credo/check/warning/operation_on_same_values_test.exs @@ -67,7 +67,10 @@ defmodule Credo.Check.Warning.OperationOnSameValuesTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 5 + assert issue.column == 14 + end) end test "it should report a violation for module attributes" do @@ -82,6 +85,8 @@ defmodule Credo.Check.Warning.OperationOnSameValuesTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.trigger == "-" + assert issue.line_no == 4 + assert issue.column == 29 end) end diff --git a/test/credo/check/warning/operation_with_constant_result_test.exs b/test/credo/check/warning/operation_with_constant_result_test.exs index 7957793c3..dca81674a 100644 --- a/test/credo/check/warning/operation_with_constant_result_test.exs +++ b/test/credo/check/warning/operation_with_constant_result_test.exs @@ -74,8 +74,14 @@ defmodule Credo.Check.Warning.OperationWithConstantResultTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues(fn issues -> - assert 2 == Enum.count(issues) + |> assert_issues(fn [two, one] -> + assert one.trigger == "*" + assert one.line_no == 5 + assert one.column == 7 + + assert two.trigger == "*" + assert two.line_no == 6 + assert two.column == 7 end) end end diff --git a/test/credo/check/warning/raise_inside_rescue_test.exs b/test/credo/check/warning/raise_inside_rescue_test.exs index e83187569..bde932f29 100644 --- a/test/credo/check/warning/raise_inside_rescue_test.exs +++ b/test/credo/check/warning/raise_inside_rescue_test.exs @@ -71,6 +71,7 @@ defmodule Credo.Check.Warning.RaiseInsideRescueTest do |> assert_issue(fn issue -> assert "raise" == issue.trigger assert 10 == issue.line_no + assert 9 == issue.column end) end @@ -93,6 +94,7 @@ defmodule Credo.Check.Warning.RaiseInsideRescueTest do |> assert_issue(fn issue -> assert "raise" == issue.trigger assert 9 == issue.line_no + assert 7 == issue.column end) end @@ -115,6 +117,7 @@ defmodule Credo.Check.Warning.RaiseInsideRescueTest do |> assert_issue(fn issue -> assert "raise" == issue.trigger assert 8 == issue.line_no + assert 53 == issue.column end) end diff --git a/test/credo/check/warning/spec_with_struct_test.exs b/test/credo/check/warning/spec_with_struct_test.exs index f0108c898..417af9607 100644 --- a/test/credo/check/warning/spec_with_struct_test.exs +++ b/test/credo/check/warning/spec_with_struct_test.exs @@ -99,6 +99,7 @@ defmodule Credo.Check.Warning.SpecWithStructTest do |> assert_issue(fn issue -> assert %{line_no: 2, message: "Struct %MyApp.MyStruct{} found in `@spec`."} = issue assert issue.trigger == "%MyApp.MyStruct{" + assert issue.column == 16 end) end @@ -118,6 +119,7 @@ defmodule Credo.Check.Warning.SpecWithStructTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 2 + assert issue.column == 16 assert issue.trigger == "%AStruct{" end) end @@ -137,7 +139,10 @@ defmodule Credo.Check.Warning.SpecWithStructTest do ] |> to_source_files() |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 3 + assert issue.column == 24 + end) end test "it should report an issue if a struct is used as an argument in a spec" do @@ -155,7 +160,10 @@ defmodule Credo.Check.Warning.SpecWithStructTest do ] |> to_source_files() |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 3 + assert issue.column == 33 + end) end test "it should report an issue if a struct has an argument name in a spec" do @@ -172,7 +180,10 @@ defmodule Credo.Check.Warning.SpecWithStructTest do ] |> to_source_files() |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 2 + assert issue.column == 24 + end) end test "it should report multiple issues in separate specs" do diff --git a/test/credo/check/warning/unsafe_exec_test.exs b/test/credo/check/warning/unsafe_exec_test.exs index 1c6fbb4d2..b1e1e0bed 100644 --- a/test/credo/check/warning/unsafe_exec_test.exs +++ b/test/credo/check/warning/unsafe_exec_test.exs @@ -42,7 +42,10 @@ defmodule Credo.Check.Warning.UnsafeExecTest do """ |> to_source_file() |> run_check(@described_check) - |> assert_issue() + |> assert_issue(fn issue -> + assert issue.line_no == 3 + assert issue.column == 9 + end) end test "it should report a violation /2" do @@ -73,6 +76,7 @@ defmodule Credo.Check.Warning.UnsafeExecTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 + assert issue.column == 13 assert issue.trigger == ":erlang.open_port" end) end diff --git a/test/credo/check/warning/unused_enum_operation_test.exs b/test/credo/check/warning/unused_enum_operation_test.exs index 81bc8f7c6..058d14c37 100644 --- a/test/credo/check/warning/unused_enum_operation_test.exs +++ b/test/credo/check/warning/unused_enum_operation_test.exs @@ -885,7 +885,9 @@ defmodule Credo.Check.Warning.UnusedEnumOperationTest do |> to_source_file |> run_check(@described_check) |> assert_issue(fn issue -> - assert "Enum.map" == issue.trigger + assert issue.trigger == "Enum.map" + assert issue.line_no == 5 + assert issue.column == 7 end) end end From db720f8dbefc2fbc3b871489c080f1d8a97ad599 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Tue, 30 Apr 2024 00:46:22 +0300 Subject: [PATCH 02/10] Remove unused argument --- .../warning/application_config_in_module_attribute.ex | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/credo/check/warning/application_config_in_module_attribute.ex b/lib/credo/check/warning/application_config_in_module_attribute.ex index d3da94cce..e1e36f5a3 100644 --- a/lib/credo/check/warning/application_config_in_module_attribute.ex +++ b/lib/credo/check/warning/application_config_in_module_attribute.ex @@ -36,13 +36,13 @@ defmodule Credo.Check.Warning.ApplicationConfigInModuleAttribute do Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta)) end - defp traverse({:@, meta, [attribute_definition]} = ast, issues, issue_meta) do + defp traverse({:@, _meta, [attribute_definition]} = ast, issues, issue_meta) do case traverse_attribute(attribute_definition) do nil -> {ast, issues} {attribute, call} -> - {ast, issues_for_call(attribute, call, meta, issue_meta, issues)} + {ast, issues_for_call(attribute, call, issue_meta, issues)} end end @@ -96,14 +96,14 @@ defmodule Credo.Check.Warning.ApplicationConfigInModuleAttribute do {ast, acc} end - defp issues_for_call(attribute, {call_meta, call, trigger}, _meta, issue_meta, issues) do + defp issues_for_call(attribute, {meta, call, trigger}, issue_meta, issues) do [ format_issue(issue_meta, message: "Module attribute @#{Atom.to_string(attribute)} makes use of unsafe Application configuration call #{call}", trigger: trigger, - line_no: call_meta[:line], - column: call_meta[:column] + line_no: meta[:line], + column: meta[:column] ) | issues ] From 47b2000ef1c8d01568b6f519e472b17be0893cc9 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Tue, 30 Apr 2024 18:47:40 +0300 Subject: [PATCH 03/10] Fix formatting --- .../application_config_in_module_attribute_test.exs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/credo/check/warning/application_config_in_module_attribute_test.exs b/test/credo/check/warning/application_config_in_module_attribute_test.exs index a79efc7a8..6759de6f0 100644 --- a/test/credo/check/warning/application_config_in_module_attribute_test.exs +++ b/test/credo/check/warning/application_config_in_module_attribute_test.exs @@ -101,8 +101,11 @@ defmodule Credo.Check.Warning.ApplicationConfigInModuleAttributeTest do defp error_exists?(errors, error_message, {line_no, column}, trigger) do Enum.any?(errors, fn - %Credo.Issue{message: ^error_message, line_no: ^line_no, column: ^column, trigger: ^trigger} -> true - _ -> false + %Credo.Issue{message: ^error_message, line_no: ^line_no, column: ^column, trigger: ^trigger} -> + true + + _ -> + false end) end end From 346953b79af5b4384799a67bf543d599995512a7 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Wed, 1 May 2024 19:08:28 +0300 Subject: [PATCH 04/10] Fix column length for erlang stdlib functions --- lib/credo/check/warning/lazy_logging.ex | 9 +++++---- lib/credo/check/warning/leaky_environment.ex | 6 ++++++ lib/credo/check/warning/unsafe_exec.ex | 12 +++++++++--- test/credo/check/warning/lazy_logging_test.exs | 3 +++ test/credo/check/warning/leaky_environment_test.exs | 2 +- test/credo/check/warning/unsafe_exec_test.exs | 5 +++-- 6 files changed, 27 insertions(+), 10 deletions(-) diff --git a/lib/credo/check/warning/lazy_logging.ex b/lib/credo/check/warning/lazy_logging.ex index 5f7e87611..ec340453e 100644 --- a/lib/credo/check/warning/lazy_logging.ex +++ b/lib/credo/check/warning/lazy_logging.ex @@ -51,7 +51,8 @@ defmodule Credo.Check.Warning.LazyLogging do issue_meta ) when fun_name in @logger_functions do - issue = find_issue(fun_name, arguments, meta, issue_meta) + trigger = "Logger.#{fun_name}" + issue = find_issue(fun_name, arguments, meta, issue_meta, trigger) {ast, add_issue_to_state(state, issue)} end @@ -62,7 +63,7 @@ defmodule Credo.Check.Warning.LazyLogging do issue_meta ) when fun_name in @logger_functions do - issue = find_issue(fun_name, arguments, meta, issue_meta) + issue = find_issue(fun_name, arguments, meta, issue_meta, fun_name) {ast, add_issue_to_state(state, issue)} end @@ -89,12 +90,12 @@ defmodule Credo.Check.Warning.LazyLogging do {module_contains_import?, [issue | issues]} end - defp find_issue(fun_name, arguments, meta, issue_meta) do + defp find_issue(fun_name, arguments, meta, issue_meta, trigger) do params = IssueMeta.params(issue_meta) ignored_functions = Params.get(params, :ignore, __MODULE__) unless Enum.member?(ignored_functions, fun_name) do - issue_for_call(arguments, meta, fun_name, issue_meta) + issue_for_call(arguments, meta, trigger, issue_meta) end end diff --git a/lib/credo/check/warning/leaky_environment.ex b/lib/credo/check/warning/leaky_environment.ex index 663f786ee..29fd31cf5 100644 --- a/lib/credo/check/warning/leaky_environment.ex +++ b/lib/credo/check/warning/leaky_environment.ex @@ -27,6 +27,7 @@ defmodule Credo.Check.Warning.LeakyEnvironment do Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta)) end + @offset 2 defp traverse({{:., _, call}, meta, args} = ast, issues, issue_meta) do case get_forbidden_call(call, args) do nil -> @@ -36,6 +37,11 @@ defmodule Credo.Check.Warning.LeakyEnvironment do {ast, [issue_for(issue_meta, meta, trigger) | issues]} trigger -> + [module, _function] = call + len = module |> Atom.to_string() |> String.length() + column = meta[:column] - len - @offset + meta = Keyword.put(meta, :column, column) + {ast, [issue_for(issue_meta, meta, trigger) | issues]} end end diff --git a/lib/credo/check/warning/unsafe_exec.ex b/lib/credo/check/warning/unsafe_exec.ex index f1fb71738..05cf51f65 100644 --- a/lib/credo/check/warning/unsafe_exec.ex +++ b/lib/credo/check/warning/unsafe_exec.ex @@ -37,7 +37,8 @@ defmodule Credo.Check.Warning.UnsafeExec do defp traverse({{:., _loc, call}, meta, args} = ast, issues, issue_meta) do case get_forbidden_call(call, args) do {bad, suggestion, trigger} -> - {ast, [issue_for(bad, suggestion, trigger, meta, issue_meta) | issues]} + [module, _function] = call + {ast, [issue_for(bad, suggestion, trigger, meta, module, issue_meta) | issues]} nil -> {ast, issues} @@ -65,12 +66,17 @@ defmodule Credo.Check.Warning.UnsafeExec do nil end - defp issue_for(call, suggestion, trigger, meta, issue_meta) do + # offset 2 characters for the dot call and the atom syntax + @offset 2 + defp issue_for(call, suggestion, trigger, meta, module, issue_meta) do + len = module |> Atom.to_string() |> String.length() + column = meta[:column] - len - @offset + format_issue(issue_meta, message: "Prefer #{suggestion} over #{call} to prevent command injection.", trigger: trigger, line_no: meta[:line], - column: meta[:column] + column: column ) end end diff --git a/test/credo/check/warning/lazy_logging_test.exs b/test/credo/check/warning/lazy_logging_test.exs index 6822b5f72..5995ebbb0 100644 --- a/test/credo/check/warning/lazy_logging_test.exs +++ b/test/credo/check/warning/lazy_logging_test.exs @@ -121,12 +121,15 @@ defmodule Credo.Check.Warning.LazyLoggingTest do |> to_source_file |> run_check(@described_check) |> assert_issues(fn [three, two, one] -> + assert one.trigger == "Logger.debug" assert one.line_no == 5 assert one.column == 5 + assert two.trigger == "Logger.debug" assert two.line_no == 6 assert two.column == 5 + assert three.trigger == "Logger.debug" assert three.line_no == 7 assert three.column == 5 end) diff --git a/test/credo/check/warning/leaky_environment_test.exs b/test/credo/check/warning/leaky_environment_test.exs index c5ada3f70..e1efedeb9 100644 --- a/test/credo/check/warning/leaky_environment_test.exs +++ b/test/credo/check/warning/leaky_environment_test.exs @@ -74,7 +74,7 @@ defmodule Credo.Check.Warning.LeakyEnvironmentTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 - assert issue.column == 13 + assert issue.column == 5 assert issue.trigger == ":erlang.open_port" end) end diff --git a/test/credo/check/warning/unsafe_exec_test.exs b/test/credo/check/warning/unsafe_exec_test.exs index b1e1e0bed..bb93d7a87 100644 --- a/test/credo/check/warning/unsafe_exec_test.exs +++ b/test/credo/check/warning/unsafe_exec_test.exs @@ -44,7 +44,7 @@ defmodule Credo.Check.Warning.UnsafeExecTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 - assert issue.column == 9 + assert issue.column == 5 end) end @@ -60,6 +60,7 @@ defmodule Credo.Check.Warning.UnsafeExecTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 + assert issue.column == 5 assert issue.trigger == ":os.cmd" end) end @@ -76,7 +77,7 @@ defmodule Credo.Check.Warning.UnsafeExecTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 3 - assert issue.column == 13 + assert issue.column == 5 assert issue.trigger == ":erlang.open_port" end) end From 4f37ddb13d64e51254b2f23cef3afb0cac525e4e Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 2 May 2024 03:23:26 +0300 Subject: [PATCH 05/10] Fix triggers for forbidden module and empty enum --- .../warning/expensive_empty_enum_check.ex | 12 +++- lib/credo/check/warning/forbidden_module.ex | 59 ++++++++----------- .../expensive_empty_enum_check_test.exs | 8 +-- .../check/warning/forbidden_module_test.exs | 11 ++-- 4 files changed, 43 insertions(+), 47 deletions(-) diff --git a/lib/credo/check/warning/expensive_empty_enum_check.ex b/lib/credo/check/warning/expensive_empty_enum_check.ex index 8722305ff..afa53991e 100644 --- a/lib/credo/check/warning/expensive_empty_enum_check.ex +++ b/lib/credo/check/warning/expensive_empty_enum_check.ex @@ -50,11 +50,11 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheck do for {lhs, rhs, trigger} <- @comparisons, operator <- @operators do defp traverse( - {unquote(operator), meta, [unquote(lhs), unquote(rhs)]} = ast, + {unquote(operator), _meta, [unquote(lhs), unquote(rhs)]} = ast, issues, issue_meta ) do - {ast, issues_for_call(meta, unquote(trigger), issues, issue_meta, ast)} + {ast, issues_for_call(unquote(trigger), issues, issue_meta, ast)} end end @@ -62,13 +62,19 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheck do {ast, issues} end - defp issues_for_call(meta, trigger, issues, issue_meta, ast) do + defp issues_for_call(trigger, issues, issue_meta, ast) do + meta = get_meta(ast) [issue_for(issue_meta, meta, trigger, suggest(ast)) | issues] end defp suggest({_op, _, [0, {_pattern, _, args}]}), do: suggest_for_arity(Enum.count(args)) defp suggest({_op, _, [{_pattern, _, args}, 0]}), do: suggest_for_arity(Enum.count(args)) + defp get_meta({_op, _, [0, {{:., _, [{:__aliases__, meta, _}, _]}, _, _}]}), do: meta + defp get_meta({_op, _, [0, {_, meta, _}]}), do: meta + defp get_meta({_op, _, [{{:., _, [{:__aliases__, meta, _}, _]}, _, _}, 0]}), do: meta + defp get_meta({_op, _, [{_, meta, _}, 0]}), do: meta + defp suggest_for_arity(2), do: "`not Enum.any?/2`" defp suggest_for_arity(1), do: "`Enum.empty?/1` or `list == []`" diff --git a/lib/credo/check/warning/forbidden_module.ex b/lib/credo/check/warning/forbidden_module.ex index 3ba07bb3d..615209fcd 100644 --- a/lib/credo/check/warning/forbidden_module.ex +++ b/lib/credo/check/warning/forbidden_module.ex @@ -29,9 +29,12 @@ defmodule Credo.Check.Warning.ForbiddenModule do modules = if Keyword.keyword?(modules) do - Enum.map(modules, fn {key, value} -> {Name.full(key), value} end) + Map.new(modules, fn {key, value} -> {Name.full(key), value} end) else - Enum.map(modules, fn key -> {Name.full(key), nil} end) + Map.new(modules, fn module -> + full = Name.full(module) + {full, "The `#{Name.full(module)}` module is not allowed."} + end) end Credo.Code.prewalk( @@ -43,9 +46,11 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse({:__aliases__, meta, modules} = ast, issues, forbidden_modules, issue_meta) do module = Name.full(modules) - issues = put_issue_if_forbidden(issues, issue_meta, meta, module, forbidden_modules) - - {ast, issues} + if found_module?(module, forbidden_modules) do + {ast, [issue_for(issue_meta, meta, module, forbidden_modules[module]) | issues]} + else + {ast, issues} + end end defp traverse( @@ -54,14 +59,17 @@ defmodule Credo.Check.Warning.ForbiddenModule do forbidden_modules, issue_meta ) do - modules = - Enum.map(aliases, fn {:__aliases__, meta, module} -> - {Name.full([base_alias, module]), meta} - end) - issues = - Enum.reduce(modules, issues, fn {module, meta}, issues -> - put_issue_if_forbidden(issues, issue_meta, meta, module, forbidden_modules) + Enum.reduce(aliases, issues, fn {:__aliases__, meta, module}, issues -> + full_name = Name.full([base_alias, module]) + + if found_module?(full_name, forbidden_modules) do + message = forbidden_modules[full_name] + trigger = Name.full(module) + [issue_for(issue_meta, meta, trigger, message) | issues] + else + issues + end end) {ast, issues} @@ -69,24 +77,12 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse(ast, issues, _, _), do: {ast, issues} - defp put_issue_if_forbidden(issues, issue_meta, line_no, module, forbidden_modules) do - forbidden_module_names = Enum.map(forbidden_modules, &elem(&1, 0)) + defp found_module?(module, forbidden_modules) when is_map_key(forbidden_modules, module), + do: true - if found_module?(forbidden_module_names, module) do - [issue_for(issue_meta, line_no, module, forbidden_modules) | issues] - else - issues - end - end - - defp found_module?(forbidden_module_names, module) do - Enum.member?(forbidden_module_names, module) - end - - defp issue_for(issue_meta, meta, module, forbidden_modules) do - trigger = Name.full(module) - message = message(forbidden_modules, module) || "The `#{trigger}` module is not allowed." + defp found_module?(_, _), do: false + defp issue_for(issue_meta, meta, trigger, message) do format_issue( issue_meta, message: message, @@ -95,11 +91,4 @@ defmodule Credo.Check.Warning.ForbiddenModule do column: meta[:column] ) end - - defp message(forbidden_modules, module) do - Enum.find_value(forbidden_modules, fn - {^module, message} -> message - _ -> nil - end) - end end diff --git a/test/credo/check/warning/expensive_empty_enum_check_test.exs b/test/credo/check/warning/expensive_empty_enum_check_test.exs index 05b131ca5..8d389f005 100644 --- a/test/credo/check/warning/expensive_empty_enum_check_test.exs +++ b/test/credo/check/warning/expensive_empty_enum_check_test.exs @@ -132,7 +132,7 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do |> assert_issue(fn issue -> assert issue.trigger == "length" assert issue.line_no == 3 - assert issue.column == 26 + assert issue.column == 8 end) end @@ -153,7 +153,7 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do |> assert_issue(fn issue -> assert issue.trigger == "length" assert issue.line_no == 3 - assert issue.column == 10 + assert issue.column == 13 end) end @@ -175,7 +175,7 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do assert issue.trigger == "Enum.count" assert issue.message =~ "Enum.empty" assert issue.line_no == 3 - assert issue.column == 30 + assert issue.column == 8 end) end @@ -196,7 +196,7 @@ defmodule Credo.Check.Warning.ExpensiveEmptyEnumCheckTest do |> assert_issue(fn issue -> assert issue.message =~ "Enum.empty" assert issue.line_no == 3 - assert issue.column == 10 + assert issue.column == 13 end) end diff --git a/test/credo/check/warning/forbidden_module_test.exs b/test/credo/check/warning/forbidden_module_test.exs index 39504b681..827007d6c 100644 --- a/test/credo/check/warning/forbidden_module_test.exs +++ b/test/credo/check/warning/forbidden_module_test.exs @@ -82,13 +82,13 @@ defmodule Credo.Check.Warning.ForbiddenModuleTest do |> run_check(@described_check, modules: [CredoSampleModule.ForbiddenModule, CredoSampleModule.ForbiddenModule2] ) - |> assert_issues(fn [one, two] -> - assert one.trigger == "CredoSampleModule.ForbiddenModule2" + |> assert_issues(fn [two, one] -> + assert one.trigger == "ForbiddenModule" assert one.line_no == 2 - assert one.column == 60 - assert two.trigger == "CredoSampleModule.ForbiddenModule" + assert one.column == 43 + assert two.trigger == "ForbiddenModule2" assert two.line_no == 2 - assert two.column == 43 + assert two.column == 60 end) end @@ -127,6 +127,7 @@ defmodule Credo.Check.Warning.ForbiddenModuleTest do |> run_check(@described_check, modules: [{CredoSampleModule.ForbiddenModule, "my message"}]) |> assert_issue(fn issue -> assert issue.line_no == 3 + assert issue.column == 5 assert issue.trigger == "CredoSampleModule.ForbiddenModule" assert issue.message == "my message" end) From d8011ee5886c53db17101293fe1c50784db5ef0d Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 2 May 2024 03:29:04 +0300 Subject: [PATCH 06/10] Revert missing metadata key column metadata --- .../warning/missed_metadata_key_in_logger_config.ex | 9 ++++----- .../missed_metadata_key_in_logger_config_test.exs | 3 --- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex b/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex index 1fbc58075..b4a8a617f 100644 --- a/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex +++ b/lib/credo/check/warning/missed_metadata_key_in_logger_config.ex @@ -78,7 +78,7 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfig do end defp traverse( - {{:., _, [{:__aliases__, meta, [:Logger]}, fun_name]}, _, arguments} = ast, + {{:., _, [{:__aliases__, _, [:Logger]}, fun_name]}, meta, arguments} = ast, state, issue_meta, metadata_keys @@ -151,7 +151,7 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfig do nil missed -> - issue_for(issue_meta, meta, Keyword.keys(missed)) + issue_for(issue_meta, meta[:line], Keyword.keys(missed)) end end end @@ -176,11 +176,10 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfig do |> Keyword.get(:metadata) end - defp issue_for(issue_meta, meta, [trigger | _] = missed_keys) do + defp issue_for(issue_meta, line_no, [trigger | _] = missed_keys) do format_issue(issue_meta, message: "Logger metadata key #{Enum.join(missed_keys, ", ")} not found in Logger config.", - line_no: meta[:line], - column: meta[:column], + line_no: line_no, trigger: trigger ) end diff --git a/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs b/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs index e8e45e5a6..139d983fe 100644 --- a/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs +++ b/test/credo/check/warning/missed_metadata_key_in_logger_config_test.exs @@ -138,7 +138,6 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfigTest do |> run_check(@described_check) |> assert_issue(fn issue -> assert issue.line_no == 5 - assert issue.column == 5 end) end end @@ -159,11 +158,9 @@ defmodule Credo.Check.Warning.MissedMetadataKeyInLoggerConfigTest do |> assert_issues(fn [two, one] -> assert one.trigger == "user_id" assert one.line_no == 4 - assert one.column == 5 assert two.trigger == "key" assert two.line_no == 6 - assert two.column == 5 end) end From 450f9e9e418e6e1e5679a7c8bd9fcea5afd6e5c9 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 2 May 2024 19:22:27 +0300 Subject: [PATCH 07/10] Rename module attribute --- lib/credo/check/warning/leaky_environment.ex | 4 ++-- lib/credo/check/warning/unsafe_exec.ex | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/credo/check/warning/leaky_environment.ex b/lib/credo/check/warning/leaky_environment.ex index 29fd31cf5..1a56628f5 100644 --- a/lib/credo/check/warning/leaky_environment.ex +++ b/lib/credo/check/warning/leaky_environment.ex @@ -27,7 +27,7 @@ defmodule Credo.Check.Warning.LeakyEnvironment do Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta)) end - @offset 2 + @colon_and_dot_length 2 defp traverse({{:., _, call}, meta, args} = ast, issues, issue_meta) do case get_forbidden_call(call, args) do nil -> @@ -39,7 +39,7 @@ defmodule Credo.Check.Warning.LeakyEnvironment do trigger -> [module, _function] = call len = module |> Atom.to_string() |> String.length() - column = meta[:column] - len - @offset + column = meta[:column] - len - @colon_and_dot_length meta = Keyword.put(meta, :column, column) {ast, [issue_for(issue_meta, meta, trigger) | issues]} diff --git a/lib/credo/check/warning/unsafe_exec.ex b/lib/credo/check/warning/unsafe_exec.ex index 05cf51f65..373b81074 100644 --- a/lib/credo/check/warning/unsafe_exec.ex +++ b/lib/credo/check/warning/unsafe_exec.ex @@ -66,11 +66,10 @@ defmodule Credo.Check.Warning.UnsafeExec do nil end - # offset 2 characters for the dot call and the atom syntax - @offset 2 + @colon_and_dot_length 2 defp issue_for(call, suggestion, trigger, meta, module, issue_meta) do len = module |> Atom.to_string() |> String.length() - column = meta[:column] - len - @offset + column = meta[:column] - len - @colon_and_dot_length format_issue(issue_meta, message: "Prefer #{suggestion} over #{call} to prevent command injection.", From 636cd9d1231ad3f680f9652200a889a54f1198e0 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 2 May 2024 19:25:43 +0300 Subject: [PATCH 08/10] Make issue assertions consistent --- test/credo/check/refactor/io_puts_test.exs | 6 +++--- test/credo/check/warning/dbg_test.exs | 6 +++--- test/credo/check/warning/iex_pry_test.exs | 6 +++--- test/credo/check/warning/mix_env_test.exs | 6 +++--- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/test/credo/check/refactor/io_puts_test.exs b/test/credo/check/refactor/io_puts_test.exs index 642f63729..6fb147b29 100644 --- a/test/credo/check/refactor/io_puts_test.exs +++ b/test/credo/check/refactor/io_puts_test.exs @@ -47,11 +47,11 @@ defmodule Credo.Check.Refactor.IoPutsTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues(fn [one, two] -> + |> assert_issues(fn [two, one] -> assert one.line_no == 3 - assert one.column == 26 + assert one.column == 5 assert two.line_no == 3 - assert two.column == 5 + assert two.column == 26 end) end diff --git a/test/credo/check/warning/dbg_test.exs b/test/credo/check/warning/dbg_test.exs index 8311a62d8..226f2207f 100644 --- a/test/credo/check/warning/dbg_test.exs +++ b/test/credo/check/warning/dbg_test.exs @@ -81,11 +81,11 @@ defmodule Credo.Check.Warning.DbgTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues(fn [one, two] -> + |> assert_issues(fn [two, one] -> assert one.line_no == 3 - assert one.column == 23 + assert one.column == 5 assert two.line_no == 3 - assert two.column == 5 + assert two.column == 23 end) end diff --git a/test/credo/check/warning/iex_pry_test.exs b/test/credo/check/warning/iex_pry_test.exs index fc386aeb8..6c5d2dbc0 100644 --- a/test/credo/check/warning/iex_pry_test.exs +++ b/test/credo/check/warning/iex_pry_test.exs @@ -51,11 +51,11 @@ defmodule Credo.Check.Warning.IExPryTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues(fn [one, two] -> + |> assert_issues(fn [two, one] -> assert one.line_no == 3 - assert one.column == 16 + assert one.column == 5 assert two.line_no == 3 - assert two.column == 5 + assert two.column == 16 end) end end diff --git a/test/credo/check/warning/mix_env_test.exs b/test/credo/check/warning/mix_env_test.exs index d2604cfbc..32e8aa18a 100644 --- a/test/credo/check/warning/mix_env_test.exs +++ b/test/credo/check/warning/mix_env_test.exs @@ -138,11 +138,11 @@ defmodule Credo.Check.Warning.MixEnvTest do """ |> to_source_file |> run_check(@described_check) - |> assert_issues(fn [one, two] -> + |> assert_issues(fn [two, one] -> assert one.line_no == 3 - assert one.column == 16 + assert one.column == 5 assert two.line_no == 3 - assert two.column == 5 + assert two.column == 16 end) end From cdee25fd75aac6980995aa919fcfe14870e45e3b Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 2 May 2024 19:31:30 +0300 Subject: [PATCH 09/10] Remove found_module?/2 --- lib/credo/check/warning/forbidden_module.ex | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/lib/credo/check/warning/forbidden_module.ex b/lib/credo/check/warning/forbidden_module.ex index 615209fcd..8c19b5983 100644 --- a/lib/credo/check/warning/forbidden_module.ex +++ b/lib/credo/check/warning/forbidden_module.ex @@ -46,7 +46,7 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse({:__aliases__, meta, modules} = ast, issues, forbidden_modules, issue_meta) do module = Name.full(modules) - if found_module?(module, forbidden_modules) do + if forbidden_modules[module] do {ast, [issue_for(issue_meta, meta, module, forbidden_modules[module]) | issues]} else {ast, issues} @@ -63,7 +63,7 @@ defmodule Credo.Check.Warning.ForbiddenModule do Enum.reduce(aliases, issues, fn {:__aliases__, meta, module}, issues -> full_name = Name.full([base_alias, module]) - if found_module?(full_name, forbidden_modules) do + if forbidden_modules[full_name] do message = forbidden_modules[full_name] trigger = Name.full(module) [issue_for(issue_meta, meta, trigger, message) | issues] @@ -77,11 +77,6 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse(ast, issues, _, _), do: {ast, issues} - defp found_module?(module, forbidden_modules) when is_map_key(forbidden_modules, module), - do: true - - defp found_module?(_, _), do: false - defp issue_for(issue_meta, meta, trigger, message) do format_issue( issue_meta, From 7a4d7d9b4b0af3efb23376b2b1cb88457e497c09 Mon Sep 17 00:00:00 2001 From: Nikola Jichev Date: Thu, 2 May 2024 23:30:10 +0300 Subject: [PATCH 10/10] Revert refactor and just pass the trigger --- lib/credo/check/warning/forbidden_module.ex | 52 +++++++++++++-------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/lib/credo/check/warning/forbidden_module.ex b/lib/credo/check/warning/forbidden_module.ex index 8c19b5983..ab9a747d1 100644 --- a/lib/credo/check/warning/forbidden_module.ex +++ b/lib/credo/check/warning/forbidden_module.ex @@ -29,12 +29,9 @@ defmodule Credo.Check.Warning.ForbiddenModule do modules = if Keyword.keyword?(modules) do - Map.new(modules, fn {key, value} -> {Name.full(key), value} end) + Enum.map(modules, fn {key, value} -> {Name.full(key), value} end) else - Map.new(modules, fn module -> - full = Name.full(module) - {full, "The `#{Name.full(module)}` module is not allowed."} - end) + Enum.map(modules, fn key -> {Name.full(key), nil} end) end Credo.Code.prewalk( @@ -46,11 +43,9 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse({:__aliases__, meta, modules} = ast, issues, forbidden_modules, issue_meta) do module = Name.full(modules) - if forbidden_modules[module] do - {ast, [issue_for(issue_meta, meta, module, forbidden_modules[module]) | issues]} - else - {ast, issues} - end + issues = put_issue_if_forbidden(issues, issue_meta, meta, module, forbidden_modules, module) + + {ast, issues} end defp traverse( @@ -61,15 +56,9 @@ defmodule Credo.Check.Warning.ForbiddenModule do ) do issues = Enum.reduce(aliases, issues, fn {:__aliases__, meta, module}, issues -> - full_name = Name.full([base_alias, module]) - - if forbidden_modules[full_name] do - message = forbidden_modules[full_name] - trigger = Name.full(module) - [issue_for(issue_meta, meta, trigger, message) | issues] - else - issues - end + full_module = Name.full([base_alias, module]) + module = Name.full(module) + put_issue_if_forbidden(issues, issue_meta, meta, full_module, forbidden_modules, module) end) {ast, issues} @@ -77,7 +66,23 @@ defmodule Credo.Check.Warning.ForbiddenModule do defp traverse(ast, issues, _, _), do: {ast, issues} - defp issue_for(issue_meta, meta, trigger, message) do + defp put_issue_if_forbidden(issues, issue_meta, meta, module, forbidden_modules, trigger) do + forbidden_module_names = Enum.map(forbidden_modules, &elem(&1, 0)) + + if found_module?(forbidden_module_names, module) do + [issue_for(issue_meta, meta, module, forbidden_modules, trigger) | issues] + else + issues + end + end + + defp found_module?(forbidden_module_names, module) do + Enum.member?(forbidden_module_names, module) + end + + defp issue_for(issue_meta, meta, module, forbidden_modules, trigger) do + message = message(forbidden_modules, module) || "The `#{trigger}` module is not allowed." + format_issue( issue_meta, message: message, @@ -86,4 +91,11 @@ defmodule Credo.Check.Warning.ForbiddenModule do column: meta[:column] ) end + + defp message(forbidden_modules, module) do + Enum.find_value(forbidden_modules, fn + {^module, message} -> message + _ -> nil + end) + end end