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

[Vici37]

This is a very subtle bug discovered during trying to introduce :nodoc: filtering into the cr-source-typer tool as initially suggested here. My apologies for the long winded PR description; this consumed about 2 days of my time to track down and I want to document it for posterity.

Semi Minimal Reproduction

After constructing a Program and Parser, add the .wants_docs = true configuration to both of them. Then have the Parser parse test.cr below and then run full semantic on the resulting parsed node. This is left as an exercise for the reader (this is a decent example within the compiler itself)

my_record.cr:

record MyRecord,
    # This is a comment on a field of a record
    hello : String

test.cr:

require "./my_record"

puts MyRecord.new("world").hello

Resulting exception:

Unhandled exception: In test.cr:3:28

 3 | puts MyRecord.new("world").hello
                                ^----
Error: undefined method 'hello' for MyRecord  

This error goes away if any of the following happen:

1. The single line comment in my_record.cr is removed
2. The wants_docs is set to false
3. If a struct is used instead of the record macro
4. MyRecord is defined in test.cr itself instead of being required (I didn't figure out why)

Following the Macro Expansions

Trial and error eventually got me to the macro evaluation as the culprit (prior thoughts were on the lexer / parser being overly aggressive somehow, as those were the only places where wants_docs, docs_enabled, or the doc type variable were really being used). It turns out if a type node has a doc variable set on it, then macro {{node}} will expand that part of it first as a comment, followed by a newline (here for the doc writing in macro expansion, here for the source of the emit_doc property). This resulted in the below macro expansion(s).

struct MyRecord
    getter # This is a comment on a field of a record
hello : String

... [ snipped other method generation ] ...
end

The followup macro expansion of the getter macro would result in nothing being generated (empty *properties list). The dangling hello : String is technically valid but represents a private (protected?) field of the MyRecord struct now. Adding parens on the getter macro generation now captures the hello : String as part of that *properties list in the getter macro and now generates the def for it as the TypeDeclaration it is.

How did you find this?

The above minimal reproduction represents the src/compiler/crystal/interpreter/interpreter.cr -> CallFrame record. Running the cr-source-typer tool with full semantic on the prelude node with wants_docs found that and started complaining about a lack of real_frame_index method being defined.

Why doesn't this show up during doc generation?

Running crystal docs only runs the top_level_semantic on the AST, which doesn't open up methods, follow calls, and then discover methods don't exist. The impact instead is that these methods generated by the getter macro wouldn't be generated and the resulting docs wouldn't include them 🤷

[Vici37]

Only line 78 needed the parenthesis; I added them to the other methods for consistency.

[straight-shoota]

Great catch! 🚀

We should probably add a spec. I'm not sure what's the best way to do that... Maybe an adaptation of assert_expand with wants_doc= true?
It should go into record_spec.cr.

I'm also wondering if there's anything we can do about this problem more generally 🤔
But I suppose not? Macro expansions can't know whether they are in a location where doc comments are allowed or would break semantics.
We might want to comb through macro usage in stdlib and see if there are similar issues with other expansions. And document this somewhere.

[straight-shoota]

Btw. it would be nice if we could retain the actual doc comment so that This is a comment on a field of a record would actually show up in the API docs for MyRecord#hello.
I think this might be possible by expanding {{ property.doc_comment }} before the getter call... It won't have any effect if there's no doc comment.

[Vici37]

I'll see if I can get a spec to cover it, or update an existing one by applying a comment to one already being tested 👍

Yeah, with macros it's tricky. Part of the problem I think is that {{property}} will always include the doc comment already, if it's present, which makes wanting to write out, say, an Assign a bit annoying because there's no way (without reconstructing the string yourself) to have it output without the doc comment. Since the doc_comment isn't present the vast majority of places (unless running with crystal docs), maybe it would be better to not have {{property}} include the doc comment by default, and leave that to the macro writer to explicitly update to their own macro expansion?

I think the fix would be to include the doc_comment within the getter macro itself, since it would need to be attached to the underlying def, not the type declaration that comes first. And then the record macro won't have to care about that doc property. I'll experiment a bit.

[Vici37]

Regarding the documentation for doc_comment as defined here, it looks like the first line is not prefixed with a leading, # and there's still an expectation of writing # {{node.doc_comment}}? (With additional lines properly getting prefixed with # on their own). I want to make sure that's the expectation as I try things out :)

[Vici37]

Got a spec written that fails without the parenthesis, passes with them. Needed to go through semantic as assert_expand looks to not be macro related.

Unfortunately I couldn't get the doc_comment to fully work - they show up in the expansion of the record macro, but not in the subsequent expansion of the getter macro Apparently I can't keep track of my experiments and adding the doc_comment does have the docs show up in crystal docs correctly 🤷 However adding the doc_comment part led to test failures in my GH build here, so not including that in this PR.

I'll create another issue regarding the lack of doc_comments surviving the record macro for documentation purposes, but this PR unblocks a semantic exception being thrown, at least :)

[Vici37]

Given I've reverted all of my changes and CI is still claiming something's broken, I'm going to assume I was unlucky in Pull Request Roulette. I'll close this PR and create a new one