Properly parse selectors in css2sass.

Closes #281
Closes #278

doc-src/SASS_CHANGELOG.md

@@ -3,6 +3,10 @@
 * Table of contents
 {:toc}
 
+## 3.1.16 (Unreleased)
+
+* Fix some bugs in `sass-convert` selector parsing when converting from CSS.
+
 ## 3.1.15
 
 * Support extending multiple comma-separated selectors (e.g. `@extend .foo, .bar`).

lib/sass/css.rb

@@ -75,14 +75,29 @@ def check_encoding!
     # @return [Tree::Node] The root node of the parsed tree
     def build_tree
       root = Sass::SCSS::CssParser.new(@template, @options[:filename]).parse
+      parse_selectors    root
       expand_commas      root
+      nest_seqs          root
       parent_ref_rules   root
-      remove_parent_refs root
       flatten_rules      root
       fold_commas        root
+      dump_selectors     root
       root
     end
 
+    # Parse all the selectors in the document and assign them to
+    # {Sass::Tree::RuleNode#parsed_rules}.
+    #
+    # @param root [Tree::Node] The parent node
+    def parse_selectors(root)
+      root.children.each do |child|
+        next parse_selectors(child) if child.is_a?(Tree::DirectiveNode)
+        next unless child.is_a?(Tree::RuleNode)
+        parser = Sass::SCSS::CssParser.new(child.rule.first, child.filename, child.line)
+        child.parsed_rules = parser.parse_selector
+      end
+    end
+
     # Transform
     #
     #     foo, bar, baz
@@ -100,36 +115,22 @@ def build_tree
     # @param root [Tree::Node] The parent node
     def expand_commas(root)
       root.children.map! do |child|
-        unless child.is_a?(Tree::RuleNode) && child.rule.first.include?(',')
+        # child.parsed_rules.members.size > 1 iff the rule contains a comma
+        unless child.is_a?(Tree::RuleNode) && child.parsed_rules.members.size > 1
           expand_commas(child) if child.is_a?(Tree::DirectiveNode)
           next child
         end
-        child.rule.first.split(',').map do |rule|
-          next if rule.strip.empty?
-          node = Tree::RuleNode.new([rule.strip])
+        child.parsed_rules.members.map do |seq|
+          node = Tree::RuleNode.new([])
+          node.parsed_rules = make_cseq(seq)
           node.children = child.children
           node
         end
       end
       root.children.flatten!
     end
 
-    # Make rules use parent refs so that
-    #
-    #     foo
-    #       color: green
-    #     foo.bar
-    #       color: blue
-    #
-    # becomes
-    #
-    #     foo
-    #       color: green
-    #       &.bar
-    #         color: blue
-    #
-    # This has the side effect of nesting rules,
-    # so that
+    # Make rules use nesting so that
     #
     #     foo
     #       color: green
@@ -142,28 +143,31 @@ def expand_commas(root)
     #
     #     foo
     #       color: green
-    #       & bar
+    #       bar
     #         color: red
-    #       & baz
+    #       baz
     #         color: blue
     #
     # @param root [Tree::Node] The parent node
-    def parent_ref_rules(root)
+    def nest_seqs(root)
       current_rule = nil
       root.children.map! do |child|
         unless child.is_a?(Tree::RuleNode)
-          parent_ref_rules(child) if child.is_a?(Tree::DirectiveNode)
+          nest_seqs(child) if child.is_a?(Tree::DirectiveNode)
           next child
         end
 
-        first, rest = child.rule.first.scan(/\A(&?(?: .|[^ ])[^.#: \[]*)([.#: \[].*)?\Z/m).first
+        seq = first_seq(child)
+        seq.members.reject! {|sseq| sseq == "\n"}
+        first, rest = seq.members.first, seq.members[1..-1]
 
-        if current_rule.nil? || current_rule.rule.first != first
-          current_rule = Tree::RuleNode.new([first])
+        if current_rule.nil? || first_sseq(current_rule) != first
+          current_rule = Tree::RuleNode.new([])
+          current_rule.parsed_rules = make_seq(first)
         end
 
-        if rest
-          child.rule = ["&" + rest]
+        unless rest.empty?
+          child.parsed_rules = make_seq(*rest)
           current_rule << child
         else
           current_rule.children += child.children
@@ -174,32 +178,57 @@ def parent_ref_rules(root)
       root.children.compact!
       root.children.uniq!
 
-      root.children.each { |v| parent_ref_rules(v) }
+      root.children.each {|v| nest_seqs(v)}
     end
 
-    # Remove useless parent refs so that
+    # Make rules use parent refs so that
     #
     #     foo
-    #       & bar
-    #         color: blue
+    #       color: green
+    #     foo.bar
+    #       color: blue
     #
     # becomes
     #
     #     foo
-    #       bar
+    #       color: green
+    #       &.bar
     #         color: blue
     #
     # @param root [Tree::Node] The parent node
-    def remove_parent_refs(root)
-      root.children.each do |child|
-        case child
-        when Tree::RuleNode
-          child.rule.first.gsub! /^& +/, ''
-          remove_parent_refs child
-        when Tree::DirectiveNode
-          remove_parent_refs child
+    def parent_ref_rules(root)
+      current_rule = nil
+      root.children.map! do |child|
+        unless child.is_a?(Tree::RuleNode)
+          parent_ref_rules(child) if child.is_a?(Tree::DirectiveNode)
+          next child
+        end
+
+        sseq = first_sseq(child)
+        next child unless sseq.is_a?(Sass::Selector::SimpleSequence)
+
+        firsts, rest = [sseq.members.first], sseq.members[1..-1]
+        firsts.push rest.shift if firsts.first.is_a?(Sass::Selector::Parent)
+
+        if current_rule.nil? || first_sseq(current_rule).members != firsts
+          current_rule = Tree::RuleNode.new([])
+          current_rule.parsed_rules = make_sseq(*firsts)
+        end
+
+        unless rest.empty?
+          rest.unshift Sass::Selector::Parent.new
+          child.parsed_rules = make_sseq(*rest)
+          current_rule << child
+        else
+          current_rule.children += child.children
         end
+
+        current_rule
       end
+      root.children.compact!
+      root.children.uniq!
+
+      root.children.each {|v| parent_ref_rules(v)}
     end
 
     # Flatten rules so that
@@ -236,18 +265,18 @@ def flatten_rules(root)
       end
     end
 
-    # Flattens a single rule
+    # Flattens a single rule.
     #
     # @param rule [Tree::RuleNode] The candidate for flattening
     # @see #flatten_rules
     def flatten_rule(rule)
       while rule.children.size == 1 && rule.children.first.is_a?(Tree::RuleNode)
         child = rule.children.first
 
-        if child.rule.first[0] == ?&
-          rule.rule = [child.rule.first.gsub(/^&/, rule.rule.first)]
+        if first_simple_sel(child).is_a?(Sass::Selector::Parent)
+          rule.parsed_rules = child.parsed_rules.resolve_parent_refs(rule.parsed_rules)
         else
-          rule.rule = ["#{rule.rule.first} #{child.rule.first}"]
+          rule.parsed_rules = make_seq(first_sseq(rule), *first_seq(child).members)
         end
 
         rule.children = child.children
@@ -280,7 +309,7 @@ def fold_commas(root)
         end
 
         if prev_rule && prev_rule.children == child.children
-          prev_rule.rule.first << ", #{child.rule.first}"
+          prev_rule.parsed_rules.members << first_seq(child)
           next nil
         end
 
@@ -290,5 +319,72 @@ def fold_commas(root)
       end
       root.children.compact!
     end
+
+    # Dump all the parsed {Sass::Tree::RuleNode} selectors to strings.
+    #
+    # @param root [Tree::Node] The parent node
+    def dump_selectors(root)
+      root.children.each do |child|
+        next dump_selectors(child) if child.is_a?(Tree::DirectiveNode)
+        next unless child.is_a?(Tree::RuleNode)
+        child.rule = child.parsed_rules.to_s
+        dump_selectors(child)
+      end
+    end
+
+    # Create a {Sass::Selector::CommaSequence}.
+    #
+    # @param seqs [Array<Sass::Selector::Sequence>]
+    # @return [Sass::Selector::CommaSequence]
+    def make_cseq(*seqs)
+      Sass::Selector::CommaSequence.new(seqs)
+    end
+
+    # Create a {Sass::Selector::CommaSequence} containing only a single
+    # {Sass::Selector::Sequence}.
+    #
+    # @param sseqs [Array<Sass::Selector::Sequence, String>]
+    # @return [Sass::Selector::CommaSequence]
+    def make_seq(*sseqs)
+      make_cseq(Sass::Selector::Sequence.new(sseqs))
+    end
+
+    # Create a {Sass::Selector::CommaSequence} containing only a single
+    # {Sass::Selector::Sequence} which in turn contains only a single
+    # {Sass::Selector::SimpleSequence}.
+    #
+    # @param sseqs [Array<Sass::Selector::Sequence, String>]
+    # @return [Sass::Selector::CommaSequence]
+    def make_sseq(*sseqs)
+      make_seq(Sass::Selector::SimpleSequence.new(sseqs))
+    end
+
+    # Return the first {Sass::Selector::Sequence} in a {Sass::Tree::RuleNode}.
+    #
+    # @param rule [Sass::Tree::RuleNode]
+    # @return [Sass::Selector::Sequence]
+    def first_seq(rule)
+      rule.parsed_rules.members.first
+    end
+
+    # Return the first {Sass::Selector::SimpleSequence} in a
+    # {Sass::Tree::RuleNode}.
+    #
+    # @param rule [Sass::Tree::RuleNode]
+    # @return [Sass::Selector::SimpleSequence, String]
+    def first_sseq(rule)
+      first_seq(rule).members.first
+    end
+
+    # Return the first {Sass::Selector::Simple} in a {Sass::Tree::RuleNode},
+    # unless the rule begins with a combinator.
+    #
+    # @param rule [Sass::Tree::RuleNode]
+    # @return [Sass::Selector::Simple?]
+    def first_simple_sel(rule)
+      sseq = first_sseq(rule)
+      return unless sseq.is_a?(Sass::Selector::SimpleSequence)
+      sseq.members.first
+    end
   end
 end

lib/sass/selector/abstract_sequence.rb

@@ -2,8 +2,9 @@ module Sass
   module Selector
     # The abstract parent class of the various selector sequence classes.
     #
-    # All subclasses should implement a `members` method
-    # that returns an array of object that respond to `#line=` and `#filename=`.
+    # All subclasses should implement a `members` method that returns an array
+    # of object that respond to `#line=` and `#filename=`, as well as a `to_a`
+    # method that returns an array of strings and script nodes.
     class AbstractSequence
       # The line of the Sass template on which this selector was declared.
       #
@@ -57,6 +58,14 @@ def eql?(other)
         other.class == self.class && other.hash == self.hash && _eql?(other)
       end
       alias_method :==, :eql?
+
+      # Converts the selector into a string. This is the standard selector
+      # string, along with any SassScript interpolation that may exist.
+      #
+      # @return [String]
+      def to_s
+        to_a.map {|e| e.is_a?(Sass::Script::Node) ? "\#{#{e.to_sass}}" : e}.join
+      end
     end
   end
 end

lib/sass/selector/simple.rb

@@ -33,6 +33,12 @@ def inspect
         to_a.map {|e| e.is_a?(Sass::Script::Node) ? "\#{#{e.to_sass}}" : e}.join
       end
 
+      # @see \{#inspect}
+      # @return [String]
+      def to_s
+        inspect
+      end
+
       # Returns a hash code for this selector object.
       #
       # By default, this is based on the value of \{#to\_a},

test/sass/css2sass_test.rb

@@ -277,6 +277,30 @@ def test_double_comma
 CSS
   end
 
+  def test_selector_splitting
+    assert_equal(<<SASS, css2sass(<<CSS))
+.foo >
+  .bar
+    a: b
+  .baz
+    c: d
+SASS
+.foo>.bar {a: b}
+.foo>.baz {c: d}
+CSS
+
+    assert_equal(<<SASS, css2sass(<<CSS))
+.foo
+  &::bar
+    a: b
+  &::baz
+    c: d
+SASS
+.foo::bar {a: b}
+.foo::baz {c: d}
+CSS
+  end
+
   # Error reporting
 
   def test_error_reporting