Page MenuHomePhabricator

[clang-tidy] Adding static analyzer check to list of clang-tidy checks
ClosedPublic

Authored by Nathan-Huckleberry on Jul 9 2019, 4:14 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2019, 4:14 PM

The contents of each check page are identical other than the check name.

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/checks/clang-analyzer-apiModeling.StdCLibraryFunctions.rst
4 ↗(On Diff #208835)

Please adjust length. Same in other files.

7 ↗(On Diff #208835)

Could documentation refer to specific checker? Same in other files.

aaron.ballman added a subscriber: NoQ.

I'm worried that this will continue to drift out of sync with the static analyzer checks unless we find some way to automate it. I wonder if we could write a script to generate these .rst files and somehow fit it into the workflow for adding new static analyzer checks to re-run the script to produce new files (or modified files) as needed? @NoQ and @Szelethus may have ideas.

NoQ added a comment.Jul 10 2019, 9:29 AM

I'm worried that this will continue to drift out of sync with the static analyzer checks unless we find some way to automate it. I wonder if we could write a script to generate these .rst files and somehow fit it into the workflow for adding new static analyzer checks to re-run the script to produce new files (or modified files) as needed? @NoQ and @Szelethus may have ideas.

I totally agree. The list of checkers changes fairly often.

The checkers are listed in a machine-readable tablegen file, include/clang/StaticAnalyzer/Checkers/Checkers.td.

Note that some of the checkers in the list don't emit any warnings (which is fine in the Static Analyzer as checkers can interact with each other and the only purpose of these checkers is to make other checkers be more correct). There's a related set of checkers that are "hidden" (marked as Hidden in Checkers.td) - those checkers that we don't support enabling/disabling manually, so you should probably not list them either. I think currently all or most of the non-warning checkers are also hidden.

Just thinking aloud!

We were tinkering with the idea of a checker creator script similar to what clang-tidy has, that could help on this potentially.

Is generating the rst code with Tblgen a feasable approach? At first glance, it sounds like a nightmare to implement, and I wonder whether the maintainers of sphinx buildbots would need to adjust to that.

Just thinking aloud!

We were tinkering with the idea of a checker creator script similar to what clang-tidy has, that could help on this potentially.

Is generating the rst code with Tblgen a feasable approach? At first glance, it sounds like a nightmare to implement, and I wonder whether the maintainers of sphinx buildbots would need to adjust to that.

clang-tidy doesn't currently rely on tablegen for producing docs like we do for attributes or command line arguments in Clang itself, but I don't think we need that for an initial offering anyway. I'd be happy with a simple python script that we execute as-needed (like we do for AST matchers, for instance) to generate docs that are committed. I figured that python script could execute a tablegen command to offload some of that work. Eventually, we might get to a point where that script can then be run on a server like we do elsewhere.

I'm worried that this will continue to drift out of sync with the static analyzer checks unless we find some way to automate it. I wonder if we could write a script to generate these .rst files and somehow fit it into the workflow for adding new static analyzer checks to re-run the script to produce new files (or modified files) as needed? @NoQ and @Szelethus may have ideas.

I wrote a script to generate these. I can clean it up a bit more and add it.

  • Added script for generation of docs based off Checkers.td
Nathan-Huckleberry marked an inline comment as done.Jul 10 2019, 2:19 PM
Nathan-Huckleberry added inline comments.
clang-tools-extra/docs/clang-tidy/checks/clang-analyzer-apiModeling.StdCLibraryFunctions.rst
7 ↗(On Diff #208835)

The naming convention for links on the static analyzer documentation makes it difficult to properly generate hyperlinks.

May be script should generate documentation during build, so files .rst files are not needed?

Please also insert empty line between header (===) and text.

May be script should generate documentation during build, so files .rst files are not needed?

I'd personally weakly advise against that, to be honest;
that would not be inherently bad though, i think.

Please also insert empty line between header (===) and text.

Is there some docs for these checks on clang analyzer side? (will there be?)
Perhaps these can link to those main docs?

Nathan-Huckleberry added a comment.EditedJul 10 2019, 3:08 PM

May be script should generate documentation during build, so files .rst files are not needed?

I'd personally weakly advise against that, to be honest;
that would not be inherently bad though, i think.

Please also insert empty line between header (===) and text.

Is there some docs for these checks on clang analyzer side? (will there be?)
Perhaps these can link to those main docs?

Docs exist for these checks on the analyzer side, they're just all on the same page and hyperlinking to them individually is difficult because the links have a nonstandard naming convention. Currently I'm just linking to that page.

Docs exist for these checks on the analyzer side, they're just all on the same page and hyperlinking to them individually is difficult because the links have a nonstandard naming convention. Currently I'm just linking to that page.

I think it'll be reasonable to standardize Static Analyzer documentation links first.

Docs exist for these checks on the analyzer side, they're just all on the same page and hyperlinking to them individually is difficult because the links have a nonstandard naming convention. Currently I'm just linking to that page.

I think it'll be reasonable to standardize Static Analyzer documentation links first.

Just put up a patch for that: https://reviews.llvm.org/D64543

aaron.ballman added inline comments.Jul 11 2019, 8:38 AM
clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py
2–3 ↗(On Diff #209066)

Add full stops to the end of the sentences.

6 ↗(On Diff #209066)

Will this work with the monorepo layout?

12 ↗(On Diff #209066)

path -> the path
correct -> the correct

Add a full stop to the end of the comment. (Please check the other comments in the file as well.)

73 ↗(On Diff #209066)

Is there a way we can automatically link directly to the proper anchor instead of the general checker page?

  • Added script for generation of docs based off Checkers.td
  • Updated to match newly standardized anchor urls
  • Fix style on comments in script
aaron.ballman added inline comments.Jul 11 2019, 1:47 PM
clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py
67 ↗(On Diff #209260)

For our other aliased checks, we usually add:

.. meta::
   :http-equiv=refresh: 5;URL=whatever.html

so that the redirect is automatic. I think we should probably retain that here as well, WDYT?

  • Add auto redirect and remove alpha checkers
  • Added script for generation of docs based off Checkers.td
  • Updated to match newly standardized anchor urls
  • Add auto redirect and remove alpha checkers
  • Forgot to fix list.rst

I don't see obvious red flags strictly regarding the analyzer!

I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).

I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).

I'm fairly certain it wouldn't. If I don't forget it, I'll take a look when I get home ;)

I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).

This works in the git monorepo layout as that's what I'm using. Not sure if it works with the svn in-tree directory layout.

I think this looks reasonable to me, though I am still not certain if the relative path in the python script will work with both the svn in-tree directory layout as well as the git monorepo layout (which I'm far less familiar with).

This works in the git monorepo layout as that's what I'm using. Not sure if it works with the svn in-tree directory layout.

I use svn in-tree, so I tried it out and it does not seem to work for me.

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
  File "gen-static-analyzer-docs.py", line 120, in <module>
    main()
  File "gen-static-analyzer-docs.py", line 109, in main
    checkers = get_checkers()
  File "gen-static-analyzer-docs.py", line 23, in get_checkers
    result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python --version
Python 2.7.16

I use svn in-tree, so I tried it out and it does not seem to work for me.

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
  File "gen-static-analyzer-docs.py", line 120, in <module>
    main()
  File "gen-static-analyzer-docs.py", line 109, in main
    checkers = get_checkers()
  File "gen-static-analyzer-docs.py", line 23, in get_checkers
    result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python --version
Python 2.7.16

Via: http://llvmweekly.org/issue/289

Tom Stellard reminds us that LLVM's subversion is scheduled to be retired by Oct 21 2019, and enourages everyone to move their workflow to GitHub as soon as possible. See the GitHub migration status for more info.

@aaron.ballman should we really be spending time supporting a workflow that is deprecated and due for removal?

I use svn in-tree, so I tried it out and it does not seem to work for me.

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
  File "gen-static-analyzer-docs.py", line 120, in <module>
    main()
  File "gen-static-analyzer-docs.py", line 109, in main
    checkers = get_checkers()
  File "gen-static-analyzer-docs.py", line 23, in get_checkers
    result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python --version
Python 2.7.16

Via: http://llvmweekly.org/issue/289

Tom Stellard reminds us that LLVM's subversion is scheduled to be retired by Oct 21 2019, and enourages everyone to move their workflow to GitHub as soon as possible. See the GitHub migration status for more info.

@aaron.ballman should we really be spending time supporting a workflow that is deprecated and due for removal?

Yes -- svn is still an official way to work with our repositories and it must continue to work until we retire it.

  • Make filepath optionally user specified
aaron.ballman added inline comments.Fri, Jul 26, 10:25 AM
clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py
113 ↗(On Diff #211958)

I'm not super enthusiastic about this approach because the default is known to be wrong for supported ways of building Clang. It does give us some way forward, but getting the default wrong for some number of our developers is unfortunate.

Why not test the path you list there (which should be good for monorepo) and if that isn't found, test ../../../../../include/clang/StaticAnalyzer/Checkers/ (which should be good for in-tree builds)? This way the arguments only need to be supplied if you have a truly odd directory layout that we can't otherwise guess at.

  • Add in-tree as possible default location

When I run the script locally, I still get issues with subprocess.run().

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
  File "gen-static-analyzer-docs.py", line 148, in <module>
    main()
  File "gen-static-analyzer-docs.py", line 137, in main
    checkers = get_checkers(file_path)
  File "gen-static-analyzer-docs.py", line 22, in get_checkers
    result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'
clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py
120–123 ↗(On Diff #211967)

Swap the order of these and use an elif so we don't set the path and then overwrite it?

  • Order swap and elif

This seems reasonable to me except for the fact that the script doesn't run when I try it locally.

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py
Traceback (most recent call last):
  File "gen-static-analyzer-docs.py", line 148, in <module>
    main()
  File "gen-static-analyzer-docs.py", line 137, in main
    checkers = get_checkers(file_path)
  File "gen-static-analyzer-docs.py", line 22, in get_checkers
    result = subprocess.run(["llvm-tblgen", "--dump-json", "-I",
AttributeError: 'module' object has no attribute 'run'

c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python --version
Python 2.7.16

I don't think that API is available in Python 2.7, which is our current min required version of Python. That's the last outstanding item for the review that I can see.

  • Support python2
This revision is now accepted and ready to land.Thu, Aug 1, 1:13 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Aug 2, 10:20 AM