Merge pull request #2274 from rrosenblum/fix_unneeded_percent_q

Do not register an offense for UnneededPercentQ when %q or %Q occurs in an interpolated string

lib/rubocop/cop/style/unneeded_percent_q.rb

@@ -7,51 +7,58 @@ module Style
       class UnneededPercentQ < Cop
         MSG = 'Use `%s` only for strings that contain both single quotes and ' \
               'double quotes%s.'
+        DYNAMIC_MSG = ', or for dynamic strings that contain double quotes'
+        SINGLE_QUOTE = "'".freeze
+        QUOTE = '"'.freeze
+        EMPTY = ''.freeze
+        PERCENT_Q = '%q'.freeze
+        PERCENT_CAPITAL_Q = '%Q'.freeze
 
         def on_dstr(node)
-          # Using %Q to avoid escaping inner " is ok.
-          check(node) unless node.loc.expression.source =~ /"/
+          check(node)
         end
 
         def on_str(node)
+          # Interpolated strings that contain more than just interpolation
+          # will call `on_dstr` for the entire string and `on_str` for the
+          # non interpolated portion of the string
+          return unless string_literal?(node)
           check(node)
         end
 
-        # We process regexp nodes because the inner str nodes can cause
-        # confusion in on_str if they start with %( or %Q(.
-        def on_regexp(node)
-          string_parts = node.children.select { |child| child.type == :str }
-          string_parts.each { |s| ignore_node(s) }
-        end
-
         private
 
         def check(node)
-          if node.loc.respond_to?(:heredoc_body)
-            ignore_node(node)
-            return
-          end
-          return if ignored_node?(node) || part_of_ignored_node?(node)
           src = node.loc.expression.source
-          return unless src.start_with?('%q') || src.start_with?('%Q')
-          return if src =~ /'/ && src =~ /"/
+          return unless start_with_percent_q_variant?(src)
+          return if src.include?(SINGLE_QUOTE) && src.include?(QUOTE)
           return if src =~ StringHelp::ESCAPED_CHAR_REGEXP
 
-          extra = if src.start_with?('%Q')
-                    ', or for dynamic strings that contain double quotes'
+          extra = if src.start_with?(PERCENT_CAPITAL_Q)
+                    DYNAMIC_MSG
                   else
-                    ''
+                    EMPTY
                   end
           add_offense(node, :expression, format(MSG, src[0, 2], extra))
         end
 
         def autocorrect(node)
-          delimiter = node.loc.expression.source =~ /^%Q[^"]+$|'/ ? '"' : "'"
+          delimiter =
+            node.loc.expression.source =~ /^%Q[^"]+$|'/ ? QUOTE : SINGLE_QUOTE
           lambda do |corrector|
             corrector.replace(node.loc.begin, delimiter)
             corrector.replace(node.loc.end, delimiter)
           end
         end
+
+        def string_literal?(node)
+          node.loc.respond_to?(:begin) && node.loc.respond_to?(:end) &&
+            node.loc.begin && node.loc.end
+        end
+
+        def start_with_percent_q_variant?(string)
+          string.start_with?(PERCENT_Q) || string.start_with?(PERCENT_CAPITAL_Q)
+        end
       end
     end
   end

spec/rubocop/cop/style/unneeded_percent_q_spec.rb

@@ -6,114 +6,136 @@
   subject(:cop) { described_class.new }
 
   context 'with %q strings' do
-    let(:source) do
-      <<-END.strip_indent
-        %q('hi') # line 1
-        %q("hi")
-        %q(hi)
-        %q('"hi"')
-        %q('hi\\t') # line 5
-      END
-    end
-    let(:corrected) do
-      <<-END.strip_indent
-        "'hi'" # line 1
-        '"hi"'
-        'hi'
-        %q('"hi"')
-        %q('hi\\t') # line 5
-      END
-    end
-    before { inspect_source(cop, source) }
-
     it 'registers an offense for only single quotes' do
-      expect(cop.offenses.map(&:line)).to include(1)
+      inspect_source(cop, "%q('hi')")
+
       expect(cop.messages).to eq(['Use `%q` only for strings that contain ' \
-                                  'both single quotes and double quotes.'] * 3)
+                                  'both single quotes and double quotes.'])
     end
 
     it 'registers an offense for only double quotes' do
-      expect(cop.offenses.map(&:line)).to include(2)
+      inspect_source(cop, '%q("hi")')
+
+      expect(cop.messages).to eq(['Use `%q` only for strings that contain ' \
+                                  'both single quotes and double quotes.'])
     end
 
     it 'registers an offense for no quotes' do
-      expect(cop.offenses.map(&:line)).to include(3)
+      inspect_source(cop, '%q(hi)')
+
+      expect(cop.messages).to eq(['Use `%q` only for strings that contain ' \
+                                  'both single quotes and double quotes.'])
     end
 
     it 'accepts a string with single quotes and double quotes' do
-      expect(cop.offenses.map(&:line)).not_to include(4)
+      inspect_source(cop, %q(%q('"hi"')))
+
+      expect(cop.messages).to be_empty
     end
 
     it 'accepts a string with a tab character' do
-      expect(cop.offenses.map(&:line)).not_to include(5)
-    end
+      inspect_source(cop, "%q('hi\\t')")
 
-    it 'auto-corrects' do
-      new_source = autocorrect_source(cop, source)
-      expect(new_source).to eq(corrected)
+      expect(cop.messages).to be_empty
     end
-  end
 
-  context 'with %Q strings' do
-    let(:source) do
-      <<-END.strip_indent
-        %Q(hi) # line 1
-        %Q("hi")
-        %Q(hi\#{4})
-        %Q('"hi"')
-        %Q("\\thi")
-        %Q("hi\#{4}")
-        /%Q?/ # line 7
-      END
+    it 'accepts regular expressions starting with %q' do
+      inspect_source(cop, '/%q?/')
+
+      expect(cop.messages).to be_empty
     end
-    let(:corrected) do
-      <<-END.strip_indent
-        "hi" # line 1
-        '"hi"'
-        "hi\#{4}"
-        %Q('"hi"')
-        %Q("\\thi")
-        %Q("hi\#{4}")
-        /%Q?/ # line 7
-      END
+
+    context 'auto-correct' do
+      it 'registers an offense for only single quotes' do
+        new_source = autocorrect_source(cop, "%q('hi')")
+
+        expect(new_source).to eq(%q("'hi'"))
+      end
+
+      it 'registers an offense for only double quotes' do
+        new_source = autocorrect_source(cop, '%q("hi")')
+
+        expect(new_source).to eq(%q('"hi"'))
+      end
+
+      it 'registers an offense for no quotes' do
+        new_source = autocorrect_source(cop, '%q(hi)')
+
+        expect(new_source).to eq("'hi'")
+      end
     end
-    before { inspect_source(cop, source) }
+  end
 
+  context 'with %Q strings' do
     it 'registers an offense for static string without quotes' do
-      expect(cop.offenses.map(&:line)).to include(1)
+      inspect_source(cop, '%Q(hi)')
+
       expect(cop.messages).to eq(['Use `%Q` only for strings that contain ' \
                                   'both single quotes and double quotes, or ' \
                                   'for dynamic strings that contain double ' \
-                                  'quotes.'] * 3)
+                                  'quotes.'])
     end
 
     it 'registers an offense for static string with only double quotes' do
-      expect(cop.offenses.map(&:line)).to include(2)
+      inspect_source(cop, '%Q("hi")')
+
+      expect(cop.messages).to eq(['Use `%Q` only for strings that contain ' \
+                                  'both single quotes and double quotes, or ' \
+                                  'for dynamic strings that contain double ' \
+                                  'quotes.'])
     end
 
     it 'registers an offense for dynamic string without quotes' do
-      expect(cop.offenses.map(&:line)).to include(3)
+      inspect_source(cop, "%Q(hi\#{4})")
+
+      expect(cop.messages).to eq(['Use `%Q` only for strings that contain ' \
+                                  'both single quotes and double quotes, or ' \
+                                  'for dynamic strings that contain double ' \
+                                  'quotes.'])
     end
 
     it 'accepts a string with single quotes and double quotes' do
-      expect(cop.offenses.map(&:line)).not_to include(4)
+      inspect_source(cop, %q(%Q('"hi"')))
+
+      expect(cop.messages).to be_empty
     end
 
     it 'accepts a string with double quotes and tab character' do
-      expect(cop.offenses.map(&:line)).not_to include(5)
+      inspect_source(cop, '%Q("\\thi")')
+
+      expect(cop.messages).to be_empty
     end
 
     it 'accepts a dynamic %Q string with double quotes' do
-      expect(cop.offenses.map(&:line)).not_to include(6)
+      inspect_source(cop, '%Q("hi\#{4}")')
+
+      expect(cop.messages).to be_empty
     end
 
     it 'accepts regular expressions starting with %Q' do
-      expect(cop.offenses.map(&:line)).not_to include(7)
+      inspect_source(cop, '/%Q?/')
+
+      expect(cop.messages).to be_empty
     end
 
-    it 'auto-corrects' do
-      new_source = autocorrect_source(cop, source)
-      expect(new_source).to eq(corrected)
+    context 'auto-correct' do
+      it 'corrects a static string without quotes' do
+        new_source = autocorrect_source(cop, '%Q(hi)')
+
+        expect(new_source).to eq('"hi"')
+      end
+
+      it 'corrects a static string with only double quotes' do
+        new_source = autocorrect_source(cop, '%Q("hi")')
+
+        expect(new_source).to eq(%q('"hi"'))
+      end
+
+      it 'corrects a dynamic string without quotes' do
+        new_source = autocorrect_source(cop, "%Q(hi\#{4})")
+
+        expect(new_source).to eq(%("hi\#{4}"))
+      end
     end
   end
 
@@ -124,4 +146,32 @@
                          'END'])
     expect(cop.offenses).to be_empty
   end
+
+  it 'accepts %q at the beginning of a double quoted string ' \
+     'with interpolation' do
+    inspect_source(cop, "\"%q(a)\#{b}\"")
+
+    expect(cop.messages).to be_empty
+  end
+
+  it 'accepts %Q at the beginning of a double quoted string ' \
+     'with interpolation' do
+    inspect_source(cop, "\"%Q(a)\#{b}\"")
+
+    expect(cop.messages).to be_empty
+  end
+
+  it 'accepts %q at the beginning of a section of a double quoted string ' \
+     'with interpolation' do
+    inspect_source(cop, %("%\#{b}%q(a)"))
+
+    expect(cop.messages).to be_empty
+  end
+
+  it 'accepts %Q at the beginning of a section of a double quoted string ' \
+     'with interpolation' do
+    inspect_source(cop, %("%\#{b}%Q(a)"))
+
+    expect(cop.messages).to be_empty
+  end
 end