Fix crystal tool expand for private definitions (#3998)

* Fix `crystal tool expand` for private definitions * Refactor `crystal tool expand` - Remove `process_toplevel`, and enhance `process_type` instead - Use `each_value` instead of `values.each`
crystal-lang · Feb 9, 2017 · 2ddfe52 · 2ddfe52
1 parent 4743f5b
commit 2ddfe52
Show file tree

Hide file tree

Showing 3 changed files with 111 additions and 44 deletions.
diff --git a/spec/compiler/crystal/tools/expand_spec.cr b/spec/compiler/crystal/tools/expand_spec.cr
@@ -340,7 +340,7 @@ describe "expand" do
     assert_expand_simple code, original: "foo", expanded: ":Foo\n:foo\n"
   end
 
-  %w(module class struct lib enum).each do |keyword|
+  %w(module class struct enum lib).each do |keyword|
     it "expands macro expression inside #{keyword}" do
       code = <<-CODE
       #{keyword} Foo
@@ -350,6 +350,33 @@ describe "expand" do
 
       assert_expand_simple code, original: %({{ "Foo = 1".id }}), expanded: "Foo = 1"
     end
+
+    it "expands macro expression inside private #{keyword}" do
+      code = <<-CODE
+      private #{keyword} Foo
+        ‸{{ "Foo = 1".id }}
+      end
+      CODE
+
+      assert_expand_simple code, original: %({{ "Foo = 1".id }}), expanded: "Foo = 1"
+    end
+
+    unless keyword == "lib"
+      it "expands macro expression inside def of private #{keyword}" do
+        code = <<-CODE
+        private #{keyword} Foo
+          Foo = 1
+          def self.foo
+            {{ :‸foo }}
+          end
+        end
+
+        Foo.foo
+        CODE
+
+        assert_expand_simple code, original: "{{ :foo }}", expanded: ":foo"
+      end
+    end
   end
 
   %w(struct union).each do |keyword|
@@ -364,42 +391,88 @@ describe "expand" do
 
       assert_expand_simple code, original: %({{ "Foo = 1".id }}), expanded: "Foo = 1"
     end
+
+    it "expands macro expression inside C #{keyword} of private lib" do
+      code = <<-CODE
+      private lib Foo
+        #{keyword} Foo
+          ‸{{ "Foo = 1".id }}
+        end
+      end
+      CODE
+
+      assert_expand_simple code, original: %({{ "Foo = 1".id }}), expanded: "Foo = 1"
+    end
   end
 
-  it "expands macro expression inside def" do
-    code = <<-CODE
-    def foo(x : T) forall T
-      ‸{{ T }}
+  ["", "private "].each do |prefix|
+    it "expands macro expression inside #{prefix}def" do
+      code = <<-CODE
+      #{prefix}def foo(x : T) forall T
+        ‸{{ T }}
+      end
+
+      foo 1
+      foo "bar"
+      CODE
+
+      assert_expand code, [
+        ["{{ T }}", "Int32"],
+        ["{{ T }}", "String"],
+      ]
     end
 
-    foo 1
-    foo "bar"
-    CODE
+    it "expands macro expression inside def of #{prefix}module" do
+      code = <<-CODE
+      #{prefix}module Foo(T)
+        def self.foo
+          {{ ‸T }}
+        end
+      end
 
-    assert_expand code, [
-      ["{{ T }}", "Int32"],
-      ["{{ T }}", "String"],
-    ]
-  end
+      Foo(Int32).foo
+      Foo(String).foo
+      Foo(1).foo
+      CODE
 
-  it "expands macro expression inside def of module" do
-    code = <<-CODE
-    module Foo(T)
-      def self.foo
-        {{ ‸T }}
+      assert_expand code, [
+        ["{{ T }}", "Int32"],
+        ["{{ T }}", "String"],
+        ["{{ T }}", "1"],
+      ]
+    end
+
+    it "expands macro expression inside def of nested #{prefix}module" do
+      code = <<-CODE
+      #{prefix}module Foo
+        #{prefix}module Bar(T)
+          def self.foo
+            {{ ‸T }}
+          end
+        end
+
+        Bar(Int32).foo
+        Bar(String).foo
+        Bar(1).foo
       end
+      CODE
+
+      assert_expand code, [
+        ["{{ T }}", "Int32"],
+        ["{{ T }}", "String"],
+        ["{{ T }}", "1"],
+      ]
     end
+  end
 
-    Foo(Int32).foo
-    Foo(String).foo
-    Foo(1).foo
+  it "expands macro expression inside fun" do
+    code = <<-CODE
+    fun foo
+      {{ :foo‸ }}
+    end
     CODE
 
-    assert_expand code, [
-      ["{{ T }}", "Int32"],
-      ["{{ T }}", "String"],
-      ["{{ T }}", "1"],
-    ]
+    assert_expand_simple code, original: "{{ :foo }}", expanded: ":foo"
   end
 
   it "doesn't expand macro expression" do
@@ -436,7 +509,7 @@ describe "expand" do
     ‸foo
     CODE
 
-    assert_expand_fail code, "no expansion found: foo is not macro"
+    assert_expand_fail code, "no expansion found: foo may not be a macro"
   end
 
   it "expands macro with doc" do
@@ -448,7 +521,7 @@ describe "expand" do
       end
       # symbol of {{ x }}
       def {{ x }}_sym
-        :{{ x }}
+        {{ x.symbolize }}
       end
     end
 

diff --git a/src/compiler/crystal/syntax/parser.cr b/src/compiler/crystal/syntax/parser.cr
@@ -4541,11 +4541,12 @@ module Crystal
 
     def parse_visibility_modifier(modifier)
       doc = @token.doc
+      location = @token.location
 
       next_token_skip_space
       exp = parse_op_assign
 
-      modifier = VisibilityModifier.new(modifier, exp)
+      modifier = VisibilityModifier.new(modifier, exp).at(location).at_end(exp)
       modifier.doc = doc
       exp.doc = doc
       modifier

diff --git a/src/compiler/crystal/tools/expand.cr b/src/compiler/crystal/tools/expand.cr
@@ -77,20 +77,18 @@ module Crystal
     end
 
     def process_type(type)
-      if type.is_a?(NamedType)
-        type.types?.try &.values.each do |inner_type|
+      return unless type
+
+      if type.is_a?(NamedType) || type.is_a?(Program) || type.is_a?(FileModule)
+        type.types?.try &.each_value do |inner_type|
           process_type(inner_type)
         end
       end
 
       process_type type.metaclass if type.metaclass != type
 
       if type.is_a?(DefInstanceContainer)
-        type.def_instances.values.try do |typed_defs|
-          typed_defs.each do |typed_def|
-            typed_def.accept(self)
-          end
-        end
+        type.def_instances.each_value &.accept(self)
       end
 
       if type.is_a?(GenericType)
@@ -102,13 +100,8 @@ module Crystal
 
     def process(result : Compiler::Result)
       @in_defs = true
-      result.program.def_instances.each_value do |typed_def|
-        typed_def.accept(self)
-      end
-
-      result.program.types?.try &.values.each do |type|
-        process_type type
-      end
+      process_type result.program
+      process_type result.program.file_module?(@target_location.original_filename)
       @in_defs = false
 
       result.node.accept(self)
@@ -136,7 +129,7 @@ module Crystal
           if node.expanded
             @found_nodes << node
           else
-            @message = "no expansion found: #{node} is not macro"
+            @message = "no expansion found: #{node} may not be a macro"
           end
           false
         else