[Bug Fix] Allow wants_docs=true to be used with full semantic processing

[Vici37]

This is another attempt at addressing this bug from #16074. Previous PR was suffering from perpetual build failures that looked unrelated to my changes, so trying again! Sorry for the PR spam! 😅

[Vici37]

Hm, nope, same build problems already :/ Whelp, I'm confused. The lint error makes sense...

Is it the call to semantic within the spec that's triggering the error? Everything else is passing for me locally.

[Vici37]

Ah dang, it's the require "../spec_helper" that's triggering it. I am, in fact, able to reproduce it if I actually read and run the command from the wasm build 🤦

bin/crystal build spec/wasm32_std_spec.cr -o wasm32_std_spec.wasm --target wasm32-wasi -Dwithout_iconv -Dwithout_openssl

[Vici37]

Not sure if it's the best spot, but it was somewhere testing macros and already included the spec_helper.cr file that was previously causing problems 🤷

[ysbaddaden]

Yeah, only the compiler specs are meant to include spec/spec_helper. It basically requires the compiler, and we don't want to inject it into std specs (and apparently it doesn't work).

Now, it's not a codegen issue but a semantic issue, so it should be under spec/compiler/semantic. It's related to docs, so maybe spec/compiler/semantic/doc_spec.cr?

[ysbaddaden]

I toyed with the record macro, trying to inject the comment before the getter to try and document the method instead of wrapping it. Something broke as soon as I used a multiline comment. For example:

record Foo,
  # this is a multiline
  # comment
  name : String?

Would be expanded as:

        getter(# This is a multiline
# comment
name : String | ::Nil)

And semantics with wants_docs: true will fail with unknown token: '#' (Crystal::SyntaxException). With wants_docs: false compilation succeeds with or without the change.

There's no TypeDeclaration#doc= method to clear the docs, but we can manually inline the type declaration. For example (with the comment moved before the getter):

diff --git a/spec/compiler/semantic/doc_spec.cr b/spec/compiler/semantic/doc_spec.cr
index af14b85a7..22ede5a48 100644
--- a/spec/compiler/semantic/doc_spec.cr
+++ b/spec/compiler/semantic/doc_spec.cr
@@ -675,4 +675,21 @@ describe "Semantic: doc" do
       a_def.doc.should eq("Some description")
     end
   end
+
+  it "expands record macro with comments (#16074)" do
+    result = semantic <<-CRYSTAL, wants_doc: true
+      require "macros"
+      require "object/properties"
+
+      record Foo,
+        # This is a multiline
+        # comment
+        name : String?
+
+      Foo.new("test").name
+    CRYSTAL
+
+    a_def = result.program.types["Foo"].lookup_defs("name").first
+    a_def.doc.should eq("This is a multiline\ncomment")
+  end
 end
diff --git a/src/macros.cr b/src/macros.cr
index c01a5d35e..a168f9c80 100644
--- a/src/macros.cr
+++ b/src/macros.cr
@@ -75,7 +75,8 @@ macro record(__name name, *properties, **kwargs)
       {% if property.is_a?(Assign) %}
         getter {{property.target.id}}
       {% elsif property.is_a?(TypeDeclaration) %}
-        getter {{property}}
+        {% unless property.doc == "" %}# {{property.doc.gsub(/\n/, "\n# ").id}}{% end %}
+        getter {{property.var.id}} : {{property.type}} {% if property.value %} = {{property.value}} {% end %}
       {% else %}
         getter :{{property.id}}
       {% end %}

[ysbaddaden]

This triggers another bug because property.value is evaluated in the record macro, so if the value is false or nil then my patch won't expand the = false or = nil default values.

Weirdly property.value isn't evaluated in the getter macro 😕

Here's the fixed patch:

diff --git a/src/macros.cr b/src/macros.cr
index c01a5d35e..59b6c57f8 100644
--- a/src/macros.cr
+++ b/src/macros.cr
@@ -75,7 +75,8 @@ macro record(__name name, *properties, **kwargs)
       {% if property.is_a?(Assign) %}
         getter {{property.target.id}}
       {% elsif property.is_a?(TypeDeclaration) %}
-        getter {{property}}
+        {% unless property.doc == "" %}# {{property.doc.gsub(/\n/, "\n# ").id}}{% end %}
+        getter {{property.var.id}} : {{property.type}}{% if !property.value.nil? || property.value.stringify == "nil" %} = {{property.value}}{% end %}
       {% else %}
         getter :{{property.id}}
       {% end %}

It shall also add some specs to spec/std/record_spec.cr to test different default values.

[straight-shoota]

@Vici37 Are you still happy to work on this and address the review comments?

[crysbot]

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

https://forum.crystal-lang.org/t/error-cant-find-file-sanitize/8739/5