Support merging doc comments of macro generated items

[Blacksmoke16]

Say you have a macro that defines something that is displayed in the generated API docs, a method for example:

macro gen_method(name)
  # Comment added via macro
  def {{name.id}}
  end
end

# Comment on macro call
gen_method foo

In this example the doc comments on the method definition and macro invocation are mutually exclusive; with the docs added within the macro itself taking precedence:

This is ultimately due to this logic:

crystal/src/compiler/crystal/semantic/semantic_visitor.cr

Line 382 in 123de81

node.doc ||= @doc

I propose that there should be a way to support both, such that you can have macro generated docs, but then provide additional documentation when calling said macro.

A naive approach could be just do something like node.doc = "#{@doc}\n\n#{node.doc}".lstrip. A more robust approach could be the introduction of a new doc directive that will gsub the invocation docs where ever that token is located in the macro definition docs. We can't reuse :inherit: due to that probably causing conflicts if the method generated via the macro happens to override the same method in a parent type.

[straight-shoota]

I fear this might be adding too much complexity while offering a not very flexible solution.

Whould it be possible to apply the doc comment concatenation inside the macro?

macro gen_method(name, *, comment)
  # {{ comment.id }}
  #
  # Comment added via macro
  def {{name.id}}
  end
end

gen_method foo, comment: "Comment on macro call"

If we had access to a macro's caller context, the call site could potentially look exactly as it does in your example.

[Blacksmoke16]

Whould it be possible to apply the doc comment concatenation inside the macro?

It would be a possible workaround yea, just wouldn't be all that natural from the end users perspective tho. Especially when the comment can have newlines, code fences, etc.

If we had access to a macro's caller context, the call site could potentially look exactly as it does in your example.

Even if we had this, wouldn't it be blocked by the outcome of #7273? Which last I read thru was mostly 👎.

[straight-shoota]

I think this could serve as a reasonable use case for #7273. The negative comments in that issue revolved around the presented use case. And the fact that docs are only available in docs mode which wouldn't be an issue here because it's explicitly used for docs mode.
So I don't see any issue with having ASTNode#docs and only populate it in docs mode (the default would be an empty string).

[Blacksmoke16]

The macro's caller context would be something new as well yea? Were you thinking that would be a new meta var like @caller that is a Call of the call that invoked the macro yea? Is that something that would be specific to macros, or could work on normal methods too?

[straight-shoota]

Yeah, something like that. I wasn't thinking about a specific implementation though, just the general concept.
It would be limited to macros because defs are in general instantiated only once and cannot be traced back to specific callers at compile time. The equivalent at runtime is the caller method which is based on the stack trace.

[Blacksmoke16]

Okay, I got something working via:

diff --git a/src/compiler/crystal/macros/interpreter.cr b/src/compiler/crystal/macros/interpreter.cr
index d9f5473ec..5b53f6067 100644
--- a/src/compiler/crystal/macros/interpreter.cr
+++ b/src/compiler/crystal/macros/interpreter.cr
@@ -69,7 +69,7 @@ module Crystal
         vars[macro_block_arg.name] = call_block || Nop.new
       end
 
-      new(program, scope, path_lookup, a_macro.location, vars, call.block, a_def, in_macro)
+      new(program, scope, path_lookup, a_macro.location, vars, call.block, a_def, in_macro, call)
     end
 
     record MacroVarKey, name : String, exps : Array(ASTNode)?
@@ -77,7 +77,7 @@ module Crystal
     def initialize(@program : Program,
                    @scope : Type, @path_lookup : Type, @location : Location?,
                    @vars = {} of String => ASTNode, @block : Block? = nil, @def : Def? = nil,
-                   @in_macro = false)
+                   @in_macro = false, @call : Call? = nil)
       @str = IO::Memory.new(512) # Can't be String::Builder because of `{{debug}}`
       @last = Nop.new
     end
@@ -540,6 +540,8 @@ module Crystal
         @last = TypeNode.new(@program)
       when "@def"
         @last = @def || NilLiteral.new
+      when "@caller"
+        @last = @call || NilLiteral.new
       else
         node.raise "unknown macro instance var: '#{node.name}'"
       end
diff --git a/src/compiler/crystal/macros/methods.cr b/src/compiler/crystal/macros/methods.cr
index 90c52fa60..f27824e4f 100644
--- a/src/compiler/crystal/macros/methods.cr
+++ b/src/compiler/crystal/macros/methods.cr
@@ -400,6 +400,8 @@ module Crystal
         interpret_check_args { symbolize }
       when "class_name"
         interpret_check_args { class_name }
+      when "docs"
+        interpret_check_args { StringLiteral.new(self.doc || "") }
       when "raise"
         macro_raise self, args, interpreter, Crystal::MacroRaiseException
       when "warning"

With the test code:

macro def_method(name)
  # {{ @caller.docs.id }}
  #
  # I am a macro comment
  def {{name.id}} : String
    "foo"
  end
end

# Macro call comment
def_method test

# I am a def comment
def foo
  {{ @def.docs }}
end

But I'm starting to see some issues/limitations with this approach, as it stands at least.

1. If you don't provide a caller doc comment, then the generated method will have no summary due to the two empty new lines. This can be resolved by calling #strip on the string before parsing it for the summary line. Maybe we should be doing this regardless of this issue

2. Having more than one line in the call comment results in:

Code in test.cr:15:3

 15 | def_method test
      ^
Called macro defined in test.cr:1:1

 1 | macro def_method(name)

Which expanded to:

 > 1 |   # Macro call comment:
 > 2 | ```
         ^
Error: unexpected token: "DELIMITER_START"

Probably because each line isn't going to have the # applied to the start of lines other than the first one.

Was thinking we could prepend the character to all lines after the first? Should be fine given even if you did like {{ @caller.docs }} in a method body or something it's going to be empty and method bodies are pointless when generating docs anyway. Also would be fine if you did {{ @caller.docs }} and passed it as an arg to another macro.

Something like:

interpret_check_args do
  doc_lines = (self.doc || "").lines

  doc_lines.each_with_index do |line, idx|
    # Assume the first line will always inherently have a `#` prefix when expanded
    next if idx.zero?

    doc_lines[idx] = "# #{line}"
  end

  StringLiteral.new doc_lines.join '\n'
end

We could also require the extra lines to be escaped directly in the call docs:

# Macro call comment:
# # ```
# # puts "hello!"
# # ```
def_method test

but that isn't a great UX.

[straight-shoota]

self.doc.gsub("\n", "\n# ") should do the same transformation much simpler.
I would argue this should probably happen in the calling macro code though. {{ @caller.docs.gsub(/\n/, "\n# ").id }}

[Blacksmoke16]

Ah nice. Even if the calling macro is going to need to do this pretty much every time? Could maybe add a named argument to docs such that you could do {{ @caller.docs(comment: true).id }} to make that a bit simpler.

[Blacksmoke16]

This was resolved via a combination of #14048 and #14050.

[crysbot]

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/next-level-documentation/6564/1