Spec checker tool

[farazdagi]

What type of PR is this?

Feature

What does this PR do? Why is it needed?

• This tool allows to download specs documentation, parse Python code blocks from it, and then use parsed blocks to compare with documentation pseudo code we are using when implementing specs functions. Otherwise, it is a simple CLI utility relying on AST parsing.
• The delta of this PR is big, due to inclusion of parsed reference specs (see the rationale for this below).
• This PR implements the separate CLI tool (and not the static analyzer), because go_tool_library doesn't support embededsrcs attribute (which go_library does support), and our tool relies on go:embed. Once rules_go are updated, we can turn this tool into static analyzer, easily.
• Multiple definitions with the same function names are possible (for example you can have def func_name(): defined for both phase1 and phase0). As long as one of the definitions matches our comments, we consider comments to be up to date.

Which issues(s) does this PR fix?

Fixes #5717

How to update referenced specs?

bazel run //tools/specs-checker download -- --dir=$PWD/tools/specs-checker/data

This will pull the files defined in specDirs (see main.go), parse them (extract Python code snippets, discarding any other text), and save them to the folder from which bazel run //tools/specs-checker check will be able to embed (using go:embed):

Note: see included README for more details.

How to run tests?

bazel run //tools/specs-checker check -- --dir $PWD/beacon-chain
bazel run //tools/specs-checker check -- --dir $PWD/validator
bazel run //tools/specs-checker check -- --dir $PWD/shared

Or, to check the whole project:

bazel run //tools/specs-checker check -- --dir $PWD

How do results of the check look like?

Why save parsed reference definitions, why not discard them after the check?
There are several reasons for doing that:

• I found it particularly useful when downloading updates, since reference definitions are versioned, on git diff I immediately see what files have changed, to better understand the scope of updates I need to do on our comments.
• When used as static analyzer, re-downloading from GitHub on every build attempt is not the best idea, so having those results cached comes handy. I could have implemented more sophisticated caching scheme, where tool will check the update time of the spec files and re-download only if older than a given period, but I consider it as a bit of an overkill for the tool that simple. So, it is up to us to decide how often do we want to pull updated specs from the upstream.
• Finally, it is way easier to debug, as you have all the reference definitions saved within our project. As a side-kick: when updating the comments (if it is out of date), you can just grep the saved reference docs, and do not have to fish through specs repo site in order to find the up to date version of a snippet.

[codecov]

Codecov Report

Merging #8722 (8cfa92a) into develop (169cd78) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #8722      +/-   ##
===========================================
- Coverage    60.54%   60.54%   -0.01%     
===========================================
  Files          519      519              
  Lines        36271    36320      +49     
===========================================
+ Hits         21961    21989      +28     
- Misses       11146    11161      +15     
- Partials      3164     3170       +6     

[prestonvanloon]

This is amazing. Is there any issue filed with rules_go to support embed in go tool library?

[prestonvanloon]

Do you want to check that all refDefs match perfectly or just any of them match perfectly?

[farazdagi]

I loop through all versions of a given definition (say, in altair we have the function with the same name but different implementation, I want to make sure that both comments in altair-related code and phase0 related code are checked).

So, at the beginning of loop that checks that all definition's lines match our comment, the matchesPerfectly flag is set to true. Line checking loop breaks when non-matching line is found, the matchesPerfectly flag is set to false. So, a single matching definition is enough for flag to survive, and us be sure that our comment matches at least one known version of spec's function.

NB: Will search rules go for an issue, and if none exists will create my own -- somehow missed that part, thanks for pointing that out.

[farazdagi]

This is amazing. Is there any issue filed with rules_go to support embed in go tool library?

• I've created an issue: go: go_tool_library support for the //go:embed directive bazelbuild/rules_go#2860
• Btw, while looking whether the issue has already been filed, I found an alternative way to implement embedding (go_embed_data is mentioned in go: support go:embed directives in 1.16 bazelbuild/rules_go#2775). So, it seems we can implement the checker as CI static code analyzer with go_embed_data, do you think it is worth implementing it right now, or should we wait for update on rules_go side?

[farazdagi]

Turning back to draft, will implement as static code analyzer (using workaround with go_embed_data).

[farazdagi]

It doesn't seem that I can use go_embed_data target within go_tool_library, whenever I do so, I get circular dependency error:

Let's merge it as is -- and once patch is available, will update the implementation to become static analyzer.

[prestonvanloon]

Nice job. This is great!