Fix false positives for SpaceInParentheses

Author: rrrene

lib/credo/check/consistency/space_in_parentheses.ex

@@ -40,11 +40,9 @@ defmodule Credo.Check.Consistency.SpaceInParentheses do
     issue_meta = IssueMeta.for(source_file, params)
     allow_empty_enums = Params.get(params, :allow_empty_enums, __MODULE__)
 
-    lines_with_issues =
-      @collector.find_locations_not_matching(expected, source_file, allow_empty_enums)
+    lines_with_issues = @collector.find_locations_not_matching(expected, source_file, allow_empty_enums)
 
-    lines_with_issues
-    |> Enum.map(fn location ->
+    Enum.map(lines_with_issues, fn location ->
       format_issue(issue_meta, [{:message, message_for(expected)} | location])
     end)
   end

lib/credo/check/consistency/space_in_parentheses/collector.ex

@@ -3,24 +3,29 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.Collector do
 
   use Credo.Check.Consistency.Collector
 
+  def collect_from_source_file(source_file) do
+    Credo.Code.Token.reduce(source_file, &spaces(&1, &2, &3, &4), %{})
+  end
+
   def collect_matches(source_file, _params) do
-    # dbg(Credo.Code.to_tokens(source_file), limit: :infinity)
+    collection = collect_from_source_file(source_file)
 
-    Credo.Code.Token.reduce(source_file, &spaces(&1, &2, &3, &4), %{})
-    |> Map.new(fn {key, value} ->
+    Map.new(collection, fn {key, value} ->
       {key, Enum.count(value)}
     end)
   end
 
   def find_locations_not_matching(expected, source_file, allow_empty_enums) do
+    collection = collect_from_source_file(source_file)
+
     actual =
       case expected do
         :with_space when allow_empty_enums == true -> :without_space_allow_empty_enums
         :with_space -> :without_space
         :without_space -> :with_space
       end
 
-    Credo.Code.Token.reduce(source_file, &spaces(&1, &2, &3, &4), %{})
+    collection
     |> Map.get(actual)
     |> List.wrap()
   end
@@ -37,6 +42,14 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.Collector do
     do_spaces(no_space_between?, empty_enum?, location, acc)
   end
 
+  defp spaces(_prev, {:"[", {line, col, _}}, {:"]", {line, col2, _}} = _next, acc) when col2 - col == 1 do
+    do_spaces(true, true, [trigger: "[]", line_no: line, column: col], acc)
+  end
+
+  defp spaces(_prev, {:"{", {line, col, _}}, {:"}", {line, col2, _}} = _next, acc) when col2 - col == 1 do
+    do_spaces(true, true, [trigger: "{}", line_no: line, column: col], acc)
+  end
+
   # ignores `, ]` at the end of a list
   defp spaces({:",", {line, _col, _}}, {:"]", {line, _col2, _}}, _next, acc), do: acc
 
@@ -45,9 +58,7 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.Collector do
   defp spaces({:"{", {line, _col, _}}, {:"}", {line, _col2, _}}, _next, acc), do: acc
 
   defp spaces(_prev, {paren, {_line, _col, _}} = t1, next, acc) when paren in [:"(", :"[", :"{"] do
-    # dbg(:opening)
     {line_no, col_start, _line_no_end, col_end} = Credo.Code.Token.position(t1)
-    # dbg(next)
     {line_no2, col_start2, _line_no_end, _col_end} = Credo.Code.Token.position(next)
 
     empty_enum? =
@@ -58,17 +69,19 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.Collector do
         _ -> false
       end
 
-    no_space_between? = line_no == line_no2 && col_end == col_start2
-    location = [trigger: "#{paren}", line_no: line_no, column: col_start]
+    if Credo.Code.Token.eol?(next) || line_no != line_no2 do
+      acc
+    else
+      no_space_between? = line_no == line_no2 && col_end == col_start2
+      location = [trigger: "#{paren}", line_no: line_no, column: col_start]
 
-    do_spaces(no_space_between?, empty_enum?, location, acc)
+      do_spaces(no_space_between?, empty_enum?, location, acc)
+    end
   end
 
   defp spaces(prev, {paren, {_, _, _}} = t1, _next, acc) when paren in [:"}", :"]", :")"] do
-    # dbg(:closing)
-
-    {line_no, _col_start, _line_no_end, col_end} = Credo.Code.Token.position(prev)
-    {line_no2, col_start2, _line_no_end, _col_end} = Credo.Code.Token.position(t1)
+    {line_no, _col_start, _line_no_end, col_end} = prev |> Credo.Code.Token.position()
+    {line_no2, col_start2, _line_no_end, _col_end} = t1 |> Credo.Code.Token.position()
 
     empty_enum? =
       case prev do
@@ -78,10 +91,14 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.Collector do
         _ -> false
       end
 
-    no_space_between? = line_no == line_no2 && col_end == col_start2
-    location = [trigger: "#{paren}", line_no: line_no, column: col_start2]
+    if Credo.Code.Token.eol?(prev) || line_no != line_no2 do
+      acc
+    else
+      no_space_between? = col_end == col_start2 || col_end > col_start2
+      location = [trigger: "#{paren}", line_no: line_no, column: col_start2]
 
-    do_spaces(no_space_between?, empty_enum?, location, acc)
+      do_spaces(no_space_between?, empty_enum?, location, acc)
+    end
   end
 
   defp spaces(_prev, _current, _next, acc), do: acc

lib/credo/code/token.ex

@@ -62,6 +62,12 @@ defmodule Credo.Code.Token do
     position_tuple_for_heredoc(atom_or_charlist, line_no, col_start)
   end
 
+  def position({:char, {line_no, col_start, charlist}, _number}) when is_list(charlist) do
+    col_end = col_start + String.length(to_string(charlist))
+
+    {line_no, col_start, line_no, col_end}
+  end
+
   def position({:atom, {line_no, col_start, atom_or_charlist}, _atom}) do
     # +1 for the `:` of the atom
     col_end = col_start + String.length(to_string(atom_or_charlist)) + 1
@@ -84,11 +90,10 @@ defmodule Credo.Code.Token do
   def position({:sigil, {line_no, col_start, nil}, sigil_name, charlist, _list, _number, _binary})
       when is_list(charlist) do
     case position_tuple_for_quoted_string(charlist, line_no, col_start) do
-      {line_no, col_start, line_no, col_end} ->
+      {line1, col_start, line1, col_end} ->
         sigil_tag = String.replace("~#{sigil_name}", ~r/sigil_/, "")
 
-        # col_end + 1 for opening delimiter
-        {line_no, col_start, line_no, col_end + String.length(sigil_tag) + 1}
+        {line1, col_start, line1, col_end + String.length(sigil_tag)}
 
       value ->
         value
@@ -158,9 +163,40 @@ defmodule Credo.Code.Token do
   end
 
   @doc false
+  def position_tuple_for_quoted_string([string], line_no, col_start)
+      when is_binary(string) do
+    # a simple string with double quotes (note the brackets in the fun head match)
+    case String.split(string, "\n") do
+      [string] ->
+        # no line break
+        col_end = col_start + String.length(string) + 2
+
+        {line_no, col_start, line_no, col_end}
+
+      [_ | _] = list ->
+        # line breaks
+        newlines = Enum.count(list) - 1
+        last_line = List.last(list)
+
+        {line_no_end, col_end, terminator} = convert_to_col_end(line_no + newlines, 1, last_line)
+
+        {line_no_end, col_end} =
+          case terminator do
+            :eol ->
+              # move to next line
+              {line_no_end + 1, 1}
+
+            _ ->
+              # add 1 for " (closing double quote)
+              {line_no_end, col_end + 1}
+          end
+
+        {line_no, col_start, line_no_end, col_end}
+    end
+  end
+
   def position_tuple_for_quoted_string(atom_or_charlist, line_no, col_start)
       when is_list(atom_or_charlist) or is_atom(atom_or_charlist) do
-    # add 1 for " (closing double quote)
     {line_no_end, col_end, terminator} = convert_to_col_end(line_no, col_start, atom_or_charlist)
 
     {line_no_end, col_end} =

test/credo/check/consistency/space_in_parentheses/collector_test.exs

@@ -26,19 +26,19 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.CollectorTest do
   end
   """
   @with_spaces_empty_enum """
-    defmodule Credo.Sample2 do
-      defmodule InlineModule do
-        def foobar do
-          exists = File.exists?(filename)
-          { result, %{} } = File.read( filename )
-        end
+  defmodule Credo.Sample2 do
+    defmodule InlineModule do
+      def foobar do
+        exists = File.exists?(filename)
+        { result, %{} } = File.read( filename )
+      end
 
-        def barfoo do
-          exists = File.exists?(filename)
-          { result, [] } = File.read( filename )
-        end
+      def barfoo do
+        exists = File.exists?(filename)
+        { result, [] } = File.read( filename )
       end
     end
+  end
   """
 
   @heredoc_example """
@@ -51,27 +51,31 @@ defmodule Credo.Check.Consistency.SpaceInParentheses.CollectorTest do
   \"\"\"
   """
 
-  test "it should report correct frequencies" do
+  test "it should report correct frequencies for without_spaces" do
     without_spaces =
       @without_spaces
       |> to_source_file()
       |> Collector.collect_matches([])
 
-    assert %{without_space: 2, without_space_allow_empty_enums: 2} == without_spaces
+    assert %{without_space: 6, without_space_allow_empty_enums: 6} == without_spaces
+  end
 
+  test "it should report correct frequencies for with_spaces" do
     with_spaces =
       @with_spaces
       |> to_source_file()
       |> Collector.collect_matches([])
 
-    assert %{with_space: 1} == with_spaces
+    assert %{with_space: 4} == with_spaces
+  end
 
+  test "it should report correct frequencies for empty enums" do
     empty_enum =
       @with_spaces_empty_enum
       |> to_source_file()
       |> Collector.collect_matches([])
 
-    assert %{with_space: 2, without_space: 4, without_space_allow_empty_enums: 2} == empty_enum
+    assert %{with_space: 8, without_space: 6, without_space_allow_empty_enums: 4} == empty_enum
   end
 
   test "it should NOT report heredocs containing sigil chars" do

test/credo/check/consistency/space_in_parentheses_test.exs

@@ -118,6 +118,34 @@ defmodule Credo.Check.Consistency.SpaceInParenthesesTest do
     |> refute_issues()
   end
 
+  test "it should NOT report with config on empty params" do
+    [
+      @with_spaces_empty_params1,
+      @with_spaces_empty_params2
+    ]
+    |> to_source_files()
+    |> run_check(@described_check, allow_empty_enums: true)
+    |> refute_issues()
+  end
+
+  test "it should NOT report with without_spaces" do
+    [
+      ~S'''
+      defmodule Credo.Test.IntegrationTest do
+        def run(argv) do
+          if System.get_env("DEBUG") do
+            {suffix, remainder} = Enum.split_while(remainder, &(&1 != ?\n))
+          end
+        end
+      end
+
+      '''
+    ]
+    |> to_source_files()
+    |> run_check(@described_check)
+    |> refute_issues()
+  end
+
   #
   # cases raising issues
   #
@@ -144,7 +172,7 @@ defmodule Credo.Check.Consistency.SpaceInParenthesesTest do
     end)
   end
 
-  test "it should trigger error with no config on empty map" do
+  test "it should report with no config on empty map" do
     [
       @with_spaces_empty_params1
     ]
@@ -156,25 +184,15 @@ defmodule Credo.Check.Consistency.SpaceInParenthesesTest do
     end)
   end
 
-  test "it should trigger error with no config on empty array" do
+  test "it should report with no config on empty array" do
     [
       @with_spaces_empty_params2
     ]
     |> to_source_files()
     |> run_check(@described_check)
     |> assert_issue(fn issue ->
       assert 4 == issue.line_no
-      assert "[" == issue.trigger
+      assert "[]" == issue.trigger
     end)
   end
-
-  test "it should not trigger error with config on empty params" do
-    [
-      @with_spaces_empty_params1,
-      @with_spaces_empty_params2
-    ]
-    |> to_source_files()
-    |> run_check(@described_check, allow_empty_enums: true)
-    |> refute_issues()
-  end
 end

test/credo/check/design/tag_helper_test.exs

@@ -15,7 +15,7 @@ defmodule Credo.Check.Design.TagHelperTest do
       end
       """
       |> to_source_file
-      |> TagHelper.tags(:TODO, true)
+      |> TagHelper.do_tags(:TODO, true)
 
     assert [] == tags
   end
@@ -32,7 +32,7 @@ defmodule Credo.Check.Design.TagHelperTest do
       end
       """
       |> to_source_file
-      |> TagHelper.tags(:TODO, true)
+      |> TagHelper.do_tags(:TODO, true)
 
     expected = [
       {
@@ -57,7 +57,7 @@ defmodule Credo.Check.Design.TagHelperTest do
       end
       """
       |> to_source_file
-      |> TagHelper.tags(:TODO, true)
+      |> TagHelper.do_tags(:TODO, true)
 
     expected = [
       {
@@ -83,7 +83,7 @@ defmodule Credo.Check.Design.TagHelperTest do
       end
       """
       |> to_source_file
-      |> TagHelper.tags(:TODO, true)
+      |> TagHelper.do_tags(:TODO, true)
 
     expected = [
       {
@@ -117,7 +117,7 @@ defmodule Credo.Check.Design.TagHelperTest do
       end
       """
       |> to_source_file
-      |> TagHelper.tags(:TODO, true)
+      |> TagHelper.do_tags(:TODO, true)
 
     assert 3 == Enum.count(tags)
   end