Rewrite SpaceBetweenParens linter to reduce false positives

lib/scss_lint/linter/space_between_parens.rb

@@ -1,36 +1,112 @@
 module SCSSLint
+  # rubocop:disable Metrics/AbcSize
+
   # Checks for the presence of spaces between parentheses.
   class Linter::SpaceBetweenParens < Linter
     include LinterRegistry
 
-    def visit_root(_node)
+    def check_node(node)
+      check(node, source_from_range(node.source_range))
+      yield
+    end
+
+    alias_method :visit_atroot,           :check_node
+    alias_method :visit_cssimport,        :check_node
+    alias_method :visit_function,         :check_node
+    alias_method :visit_media,            :check_node
+    alias_method :visit_mixindef,         :check_node
+    alias_method :visit_mixin,            :check_node
+    alias_method :visit_script_funcall,   :check_node
+
+    def feel_for_parens_and_check_node(node)
+      source = feel_for_enclosing_parens(node)
+      check(node, source)
+      yield
+    end
+
+    alias_method :visit_script_listliteral,  :feel_for_parens_and_check_node
+    alias_method :visit_script_mapliteral,  :feel_for_parens_and_check_node
+    alias_method :visit_script_operation,   :feel_for_parens_and_check_node
+    alias_method :visit_script_string,      :feel_for_parens_and_check_node
+
+  private
+
+    TRAILING_WHITESPACE = /\s*$/
+
+    def check(node, source) # rubocop:disable Metrics/MethodLength
       @spaces = config['spaces']
+      source = trim_right_paren(source)
+      return if source.count('(') != source.count(')')
+      source.scan(/
+          \(
+          (?<left>\s*)
+          (?<contents>.*)
+          (?<right>\s*)
+          \)
+      /x) do |left, contents, right|
+        right = contents.match(TRAILING_WHITESPACE)[0] + right
+        contents.gsub(TRAILING_WHITESPACE, '')
+
+        # We don't lint on multiline parenthetical source.
+        break if (left + contents + right).include? "\n"
 
-      engine.lines.each_with_index do |line, index|
-        line.gsub(%r{((//|/\*).*$)}, '').scan(/
-          (^(\t|\s)*\))?  # Capture leading spaces and tabs followed by a `)`
-          (
-            \([ ]*(?!$)   # Find `( ` as long as its not EOL )
-            |
-            [ ]*\)
-          )?
-        /x) do |match|
-          check(match[2], index) if match[2]
+        if contents.empty?
+          # If we're looking at empty parens (like `()`, `( )`, `(  )`, etc.),
+          # only report a possible lint on the left side.
+          right = ' ' * @spaces
+        end
+
+        if left != ' ' * @spaces
+          message = "#{expected_spaces} after `(` instead of `#{left}`"
+          add_lint(node, message)
+        end
+
+        if right != ' ' * @spaces
+          message = "#{expected_spaces} before `)` instead of `#{right}`"
+          add_lint(node, message)
         end
       end
-      yield
     end
 
-  private
+    # An expression enclosed in parens will include or not include each paren, depending
+    # on whitespace. Here we feel out for enclosing parens, and return them as the new
+    # source for the node.
+    def feel_for_enclosing_parens(node) # rubocop:disable Metrics/CyclomaticComplexity
+      range = node.source_range
+      original_source = source_from_range(range)
+      left_offset = -1
+      right_offset = 0
+
+      if original_source[-1] != ')'
+        right_offset += 1 while character_at(range.end_pos, right_offset) =~ /\s/
 
-    def check(str, index)
-      spaces = str.count ' '
-      return if spaces == @spaces
+        return original_source if character_at(range.end_pos, right_offset) != ')'
+      end
+
+      # At this point, we know that we're wrapped on the right by a ')'.
+      # Are we wrapped on the left by a '('?
+      left_offset -= 1 while character_at(range.start_pos, left_offset) =~ /\s/
+      return original_source if character_at(range.start_pos, left_offset) != '('
+
+      # At this point, we know we're wrapped on both sides by parens. However,
+      # those parens may be part of a parent function call. We don't care about
+      # such parens. This depends on whether the preceding character is part of
+      # a function name.
+      return original_source if character_at(range.start_pos, left_offset - 1) =~ /[A-Za-z0-9_]/
+
+      range.start_pos.offset += left_offset
+      range.end_pos.offset += right_offset
+      source_from_range(range)
+    end
+
+    # An unrelated right paren will sneak into the source of a node if there is no
+    # whitespace between the node and the right paren.
+    def trim_right_paren(source)
+      (source.count(')') == source.count('(') + 1) ? source[0..-2] : source
+    end
 
-      location = Location.new(index + 1)
-      message = "Expected #{pluralize(@spaces, 'space')} " \
-                "between parentheses instead of #{spaces}"
-      add_lint(location, message)
+    def expected_spaces
+      "Expected #{pluralize(@spaces, 'space')}"
     end
   end
 end

lib/scss_lint/sass/script.rb

@@ -74,5 +74,24 @@ def children
         [value]
       end
     end
+
+    # This monkey patch can be removed once scss-lint depends on a minimum
+    # version of the sass gem that includes a fix for
+    # https://github.com/sass/sass/issues/1799
+    class ListLiteral
+      def source_range
+        return @source_range if @elements.empty?
+        @source_range.end_pos = @elements.last.source_range.end_pos
+        @source_range
+      end
+    end
+
+    class MapLiteral
+      def source_range
+        return @source_range if @pairs.empty?
+        @source_range.end_pos = @pairs.last.last.source_range.end_pos
+        @source_range
+      end
+    end
   end
 end

spec/scss_lint/linter/space_between_parens_spec.rb

@@ -119,6 +119,26 @@
     it { should_not report_lint }
   end
 
+  context 'when a 0-argument function has nothing between parens' do
+    let(:scss) { <<-SCSS }
+      p {
+        property: unique-id();
+      }
+    SCSS
+
+    it { should_not report_lint }
+  end
+
+  context 'when a 0-argument function has space between parens' do
+    let(:scss) { <<-SCSS }
+      p {
+        property: unique-id(  );
+      }
+    SCSS
+
+    it { should report_lint line: 2 }
+  end
+
   context 'when parens exist in a silent comment' do
     let(:scss) { <<-SCSS }
       p {
@@ -132,13 +152,167 @@
   context 'when parens exist in an outputted comment' do
     let(:scss) { <<-SCSS }
       p {
-        margin: 0; /* Some comment ( with parens ) */
+        /*
+         * Some comment ( with parens )
+         */
+        margin: 0;
       }
     SCSS
 
     it { should_not report_lint }
   end
 
+  context 'when an import uses parens' do
+    let(:scss) { <<-SCSS }
+      @import url( foo );
+      @import url(bar);
+    SCSS
+
+    it { should report_lint line: 1, count: 2 }
+  end
+
+  context 'when a variable definition uses parens' do
+    let(:scss) { <<-SCSS }
+      $family: unquote( "Droid+Sans" );
+      $family: unquote("Droid+Sans");
+    SCSS
+
+    it { should report_lint line: 1, count: 2 }
+  end
+
+  context 'when a variable definition is wrapped in parens' do
+    let(:scss) { <<-SCSS }
+      $value-map: (text: #00ff00, background: #0000ff, border: #ff0000);
+      $value-map: ( text: #00ff00, background: #0000ff, border: #ff0000 );
+    SCSS
+
+    it { should report_lint line: 2, count: 2 }
+  end
+
+  context 'when a url uses parens' do
+    let(:scss) { <<-SCSS }
+      p {
+        background-image: url( 'bg1.png' );
+      }
+
+      code {
+        background-image: url('bg2.png');
+      }
+    SCSS
+
+    it { should report_lint line: 2, count: 2 }
+  end
+
+  context 'when a @media directive uses parens' do
+    let(:scss) { <<-SCSS }
+      @media screen and (orientation: landscape) {
+        // Stuff.
+      }
+
+      @media screen and ( orientation: landscape ) {
+        // Stuff.
+      }
+    SCSS
+
+    it { should report_lint line: 5, count: 2 }
+  end
+
+  context 'when an @at-root directive uses parens' do
+    let(:scss) { <<-SCSS }
+      @media screen {
+        @at-root (without: media) {
+          // Stuff.
+        }
+        @at-root ( with: media ) {
+          // Stuff.
+        }
+      }
+    SCSS
+
+    it { should report_lint line: 5, count: 2 }
+  end
+
+  context 'when an @if directive uses parens' do
+    let(:scss) { <<-SCSS }
+      p {
+        @if (1 + 2 == 2) { border: 1px; }
+
+        @if ( 1 + 2 == 2 ) { border: 1px; }
+      }
+    SCSS
+
+    it { should report_lint line: 4, count: 2 }
+  end
+
+  context 'when an @each directive uses parens around a list' do
+    let(:scss) { <<-SCSS }
+      @each $animal in (puma, sea-slug, egret) {
+        // Stuff.
+      }
+
+      @each $animal in ( puma, sea-slug, egret ) {
+        // Stuff.
+      }
+    SCSS
+
+    it { should report_lint line: 5, count: 2 }
+  end
+
+  context 'when an @each directive uses parens' do
+    let(:scss) { <<-SCSS }
+      /*@each $color, $cursor in (black, default), (blue, pointer) {
+        // Stuff.
+      }*/
+
+      @each $color, $cursor in ( black, default ), ( blue, pointer ) {
+        // Stuff.
+      }
+    SCSS
+
+    it { should report_lint line: 5, count: 4 }
+  end
+
+  context 'when a @mixin uses parens' do
+    let(:scss) { <<-SCSS }
+      @mixin sexy-border($color, $width) {
+        // Stuff.
+      }
+
+      @mixin rockin-border( $color, $width ) {
+        // Stuff.
+      }
+    SCSS
+
+    it { should report_lint line: 5, count: 2 }
+  end
+
+  context 'when an @include uses parens' do
+    let(:scss) { <<-SCSS }
+      @mixin sexy-border($color, $width) {
+        // Stuff.
+      }
+
+      a { @include sexy-border(blue, 1in); }
+      p { @include sexy-border( blue, 1in ); }
+    SCSS
+
+    it { should report_lint line: 6, count: 2 }
+  end
+
+  context 'when a @function uses parens' do
+    let(:scss) { <<-SCSS }
+      @function grid-width($n) {
+        @return $n * 2;
+      }
+
+      @function grid-height( $n ) {
+        @return $n * 2;
+      }
+    SCSS
+
+    it { should report_lint line: 5, count: 2 }
+  end
+
   context 'when the number of spaces has been explicitly set' do
     let(:linter_config) { { 'spaces' => 1 } }
 
@@ -259,5 +433,25 @@
 
       it { should_not report_lint }
     end
+
+    context 'when a 0-argument function has nothing between parens' do
+      let(:scss) { <<-SCSS }
+        p {
+          property: unique-id();
+        }
+      SCSS
+
+      it { should report_lint line: 2 }
+    end
+
+    context 'when a 0-argument function has one space between parens' do
+      let(:scss) { <<-SCSS }
+        p {
+          property: unique-id( );
+        }
+      SCSS
+
+      it { should_not report_lint }
+    end
   end
 end