Macro Code Coverage

[Blacksmoke16]

One aspect of code coverage that is not currently handled via kcov and such, is that of compile time code coverage for macros. Being able to know if there are unused code paths within conditional macro logic would be really helpful. Mainly given there wouldn't be an error until it's hit if there was a bug/typo within that branch. It would be great if Crystal could generate some sort of simple coverage file for this to make use of the same tooling. E.g. something along the lines of https://docs.codecov.com/docs/codecov-custom-coverage-format.

Related: #1157

[Blacksmoke16]

I started on an prototype implementation of this and have something that seems like it's working quite well. However, the exact design of this feature is still slightly up in the air. A primary use case of this feature will be, for me at least, to run against spec files to ensure the test coverage I have against my macro logic is sufficient and not missing any paths.

Unlike the crystal tool unreachable command, which works fine because all runtime code paths would be identified by it. However, when it comes to macros, due to their nature, a similar tool for macro coverage would only identify the happy paths of the flow. Any macro logic that raises a compile time error, as I do via https://forum.crystal-lang.org/t/testing-compile-time-errors/272/5, would be missed.

Because of this we have a few options on how to best enable the generation of the coverage report:

• Just make it a tool like unreachable
  • Pro: Fits in well with the rest of the tools
  • Con: Running the specs AND generating the report would each require a separate command. I.e. the code has to be compiled twice
  • Pro: Able to be used on any Crystal source file
• Add a new global --macro-code-coverage-report option
  • Pro: Allows running and generating of the report in the same compilation
  • Pro: Can be used on any Crystal source file
  • Con: Adds yet another option that won't be heavily used
• Use some sort of ENV var denoting a directory to save the report(s) to
  • Pro: Allows the report to be generated globally for multiple Crystal commands (run/build)
  • Pro: Would natively be inherited within Process.run to handle generating those sub-report files as well
  • Con: Would be a somewhat new pattern compared to the other options
• Combine with unreachable tool
  • Pro: Able to kill two birds with one stone
  • Con: Needs more vetting as we don't want to jump the gun

I'm currently leaning towards the ENV var, just because it seems to fit my use case the best. But def interested in hearing other's use cases, reasoning for another option, or entirely new options.

[straight-shoota]

An important characteristic of macro code is that it's not only determined by other code, but macros are often used to respond to compiler flags. Depending on the build target or explicit configuration flags, you get different control flow in macros.
I can't tell exactly how important that is for coverage testing because you could also just build programs with these flags and test the behaviour. The coverage tool is probably more important for really complex macro logic which I suppose tends to be mostly determined by code (macro calls, annotations) and less by external flags.

In order to fully test a piece of macro functionality that responds to flags, you may need to run it with a bunch of different flag combinations. For that use case it should be possible to easily run only macro coverage for little overhead (i.e. without codegen). That may hint towards a separate tool (crystal tool macro-coverage?), but I'm not sure.

[Blacksmoke16]

The coverage tool is probably more important for really complex macro logic which I suppose tends to be mostly determined by code (macro calls, annotations) and less by external flags.

This is pretty much my use case yea. I don't really have any logic based on compiler flags (that is embedded in the macro logic at least). The logic that is behind flags is tested by running the normal specs on different OSs.

For that use case it should be possible to easily run only macro coverage for little overhead (i.e. without codegen)

For this, would it be easy enough to just pass --no-codegen with the ENV var to a crystal run/build and get the same outcome? It also would be fairly easy to support more than one of these options. E.g. both the tool and ENV var support to best support both of these use cases.

[Blacksmoke16]

Another con of the ENV var is there wouldn't be a good way to control what gets included in the report. I.e. the --include and --exclude options of the unreachable command. Maybe it would be enough to hard-code it to src/ and spec/ tho?

Also will need a way to handle terminal nodes, like raise "". Probably check if we're in this "mode", swallow the error, and gracefully exit?

[Blacksmoke16]

Okay after playing with this more, I have another possible solution for the entry point.

I realized that my concern with it being a tool may not be entirely true. Running the tool itself does essentially what Process.run was doing. I.e. building the code w/o codegen. So instead of doubling the CI time, you could replace Process.run with the tool and everything should "just work?"

However to fully support this, the tool would need (working initial thoughts):

• An optional flag to enable codegen
  • Some of my tests enable codegen to run additional runtime assertions based on the state setup by the macro logic. I now realize this is itself a major slowdown in CI, with 5 specs taking 25s on my machine with codegen, while its only 6s without. So I'll have to see if there's a way to not depend upon this so this flag would be un-needed.
    • EDIT2: Yea, think I came up with a way to handle most of this at compile time. Isn't as clean but it'll do the job. Not yet sure if it removes the need for this option, but we shall see.
• An optional flag to control where the report is written on the filesystem, defaulting to STDOUT.
  • A lot of my macro logic is testing the error states, and because I handled exceptions by just swallowing it, that would now imply there wasn't an error so the test would fail. A challenge related to this is currently the tool is just outputting the report JSON to STDOUT. This might conflict with other output, such as the exception message/trace itself. So would make sense to optionally have this written to a file directly as to not interfere with the actual logic.
  • EDIT: Maybe it would be enough to send the report thru a diff location compared to the other output. E.g. STDERR or something? 🤷
• Support --stdin-filename as to not require creating a real file
  • Should be pretty trivial, as other commands already support this.
• TBD

Does this sound reasonable? Otherwise, where is everyone leaning in regards how to expose this feature?

[Blacksmoke16]

annotation Test; end

@[Test(id: 10)]
record Foo

module MyModule
  macro included
    macro finished
      {% verbatim do %}
        {%
          pp Foo.annotation(Test)[:id] * 10
        %}
      {% end %}
    end
  end
end

class Container
  include MyModule
end

Container.new

It seems we do indeed need to handle VirtualFile to account for macros expanded via hooks. However, in regards to the pp Foo.annotation(Test)[:id] * 10 node:

pp! node,
  f.source,
  location,
  location.macro_location,
  f.macro.location,
  location.expanded_location

node                       # => pp((Foo.annotation(Test))[:id] * 10)
f.source                   # => "    macro finished\n" +
"      \n" +
"        {% pp((Foo.annotation(Test))[:id] * 10) %}\n" +
"      \n" +
"    end\n" +
"  "
location                   # => expanded macro: included:3:12
location.macro_location    # => /home/george/dev/git/crystal/cov.cr:7:3
f.macro.location           # => /home/george/dev/git/crystal/cov.cr:7:3
location.expanded_location # => /home/george/dev/git/crystal/cov.cr:19:3

There doesn't seem to be a direct way to have it point to the actual location of the node, which would be line 11 in this context :/. Line 3 is correct in relation to f.source, but that doesn't really help me here as I need the real file + line number to report...

Any ideas, or is this something that needs to be added to the compiler?

[Blacksmoke16]

So as an update on my progress, the current state of things is master...Blacksmoke16:crystal:macro-code-coverage. It's very hacky, prob less than ideal code at the moment, but all specs pass and it serves its purpose as a prototype.

I was able to handle:

macro finished
  {% verbatim do %}
    {% 10 * 10 %}

    {%
      20 * 20
      30 * 30
    %}
  {% end %}
end

By adding location.macro_location.line_number + location.line_number + (is_single_line ? 0 : 1). Where is_single_line is deduced by making checking if that particular nodes's line is surrounded by {% %}, otherwise it spans multiple lines.

However, one of the only contexts it currently doesn't handle is:

macro finished
  {% verbatim do %}
    {%
      10 * 10

      # Foo
      10 * 20
    %}
  {% end %}
end

Because the empty new line and # Foo comment line between 10 * 10 and 10 * 20 are seemingly stripped, so have no way to account for them. Because of this the coverage reports says {4 => 1, 5 => 1} instead of {4 => 1, 7 => 1}.

So would love to know if anyone has any ideas on how to handle this.

[Blacksmoke16]

Only hacky idea I could think of so far is like having a way to:

1. Retain extra new lines when parsing
2. Replace comments with newlines

From here, this should make the lines match what's in the source, which should allow me to calculate proper real line numbers. Not really sure how to go about this tho...

[Blacksmoke16]

I realized that we have access to the original filename in the source. So it's possible to open that file, read the lines, and find the one that includes the node's source in it. I'm not sure how robust this is if more than one line happens to include the same string representation of another node later one, it would always use the first.

This does suggest that it might be possible to get something working tho, possibly might be able to include this in the VirtualFile node itself or something to make it more performant and to scope the lines down a bit to make my existing logic a bit more robust. I'll have to play around this more...

[Blacksmoke16]

So I pushed up some more commits to my test branch: master...Blacksmoke16:crystal:macro-code-coverage. It's very hacky but seems to be working most of the time for macro hooks and multi-line {% ... %} expressions.

The main problem now is when when doing the check of line.includes?(node.to_s), line could be klass.raise "oh noes" but node.to_s is klass.raise("oh noes") so it doesn't match.

IDK if there's a good way around that other than more holistically providing some better way to get at the real line number natively. But maybe it's "good enough" to just require people to use () or something for now if they want accurate coverage?

EDIT: Maybe one thing that would help me understand this is say you have the following code:

macro finished
  {% verbatim do %}
    {%
      10 * 10

      # Foo

      10 * 20
    %}
  {% end %}
end

The macro interpreter can access each of the 4 numbers as NumerLiteral nodes. But the location of these is like expanded macro: finished:2:13. However if I update the parser to do like NumberLiteral.new(@token.value.to_s, @token.number_kind).at(@token.location) that is able to capture the source location of that node. However, I think because #parse_macro_source re-parses the body in context of a VirtualFile, the original location is essentially overridden?

Would it be possible to add like ASTNode#source_location and have it still accessible off the node even after being re-parsed? If yes, I think this would solve a lot of the hack/issues with the POC.