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.Jul 26 2019, 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.Aug 1 2019, 1:13 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2019, 10:20 AM

It seems like the list got pretty outdated by the time.
Do you think we should really refer to the clang-analyzer checks in this list? What about simply referring to the clangsa docs instead?
I fear, this will be a constant problem in the future.

That being said, a specific grep-like test would suffice to ensure it's always updated, but I would be surprised as a clang static analyzer developer that I need to run the clang-tools-extra tests besides the clang-analysis ones.
It would cause build bot breakages all the time - and just as many revert commits by choosing that direction.

WDYT? @aaron.ballman @njames93 @whisperity

It seems like the list got pretty outdated by the time.
Do you think we should really refer to the clang-analyzer checks in this list? What about simply referring to the clangsa docs instead?
I fear, this will be a constant problem in the future.

Yeah, I was worried this would get out of sycn and it really did not take long for that to happen in practice. I think there's utility in listing the checks in clang-tidy's documentation instead of pointing users to the SA documentation page. Users should have one website to go to where they can see what clang-tidy checks are available to them.

That being said, a specific grep-like test would suffice to ensure it's always updated, but I would be surprised as a clang static analyzer developer that I need to run the clang-tools-extra tests besides the clang-analysis ones.
It would cause build bot breakages all the time - and just as many revert commits by choosing that direction.

WDYT? @aaron.ballman @njames93 @whisperity

So long as pre-commit CI runs the test, I think this might be a reasonable path forward. You may forget to run the tests locally, but you should still get some failure indication before we land a patch. That should also curtail the churn from having to revert. WDYT?

I was a bit confused as well about the ClangSA support in clang-tidy. What's the current status? Is clang-tidy running *all* checks from StaticAnalyzer?

I was a bit confused as well about the ClangSA support in clang-tidy. What's the current status? Is clang-tidy running *all* checks from StaticAnalyzer?

If your package is built with CSA enabled, and you don't want to run Cross Translation Unit analysis mode, then yeah. After all, both Clang-Tidy and Clang Static Analyser are just "FrontendAction" libraries under the hood.


Yeah, I was worried this would get out of sycn and it really did not take long for that to happen in practice. I think there's utility in listing the checks in clang-tidy's documentation instead of pointing users to the SA documentation page. Users should have one website to go to where they can see what clang-tidy checks are available to them.

Does this still apply given that the quality and formatting match of the documentation website increased over the past one or two releases? The old website if I see correctly no longer lists the checkers (and the previous format was also increasingly outdated and bad experience...), and we have the RST-based new website.

Come to think of it, maybe some RST hacks could automate "pulling in" the list from the (new) CSA website to the Tidy list, as long as maybe they are in a separate category (heading) in the Tidy list?
Although I'm unsure how much being /tools/clang/ and /tools/clang/tools/extra/ mess this thing up.
In case of Python, for example, there is a well-understood way of registering the documentation site of third-party packages, and your documentation generator can automatically generate the cross-reference that links to the other, from your doc's point-of-view, external website. But that involves a considerable amount of "magic".
As far as I know, Sphinx is written in Python, so one could write Python code that runs during the documentation generation (at least the RST part, I'm not sure about the man or infopages or groff stuff!) that can do "magic".


What we COULD do, however, is get rid of the individual checker documentation files (which all just contain machine generated redirects, at least according to this diff...) and have ALL the cross-referencing links (hand-written, autogenerated by our integration, or autogenerated by RST magic) point to not a page living inside Tidy's scope that just does a redirect, but to the appropriate heading in the (new) CSA doc suite?

(Then again, irrespective of what magic we will use, the non-trivial grouping structure on the new CSA docs site can become a problem...)

Yeah, I was worried this would get out of sycn and it really did not take long for that to happen in practice. I think there's utility in listing the checks in clang-tidy's documentation instead of pointing users to the SA documentation page. Users should have one website to go to where they can see what clang-tidy checks are available to them.

Does this still apply given that the quality and formatting match of the documentation website increased over the past one or two releases? The old website if I see correctly no longer lists the checkers (and the previous format was also increasingly outdated and bad experience...), and we have the RST-based new website.

I think that clang-tidy users should be able to view the clang-tidy docs and see what checks are available to them (including static analyzer checks). Similarly, I think CSA users should be able to view the CSA docs and see what checks are available to them (including the clang-tidy checks that I think we're now exposing or were exploring exposing).

Come to think of it, maybe some RST hacks could automate "pulling in" the list from the (new) CSA website to the Tidy list, as long as maybe they are in a separate category (heading) in the Tidy list?
Although I'm unsure how much being /tools/clang/ and /tools/clang/tools/extra/ mess this thing up.
In case of Python, for example, there is a well-understood way of registering the documentation site of third-party packages, and your documentation generator can automatically generate the cross-reference that links to the other, from your doc's point-of-view, external website. But that involves a considerable amount of "magic".
As far as I know, Sphinx is written in Python, so one could write Python code that runs during the documentation generation (at least the RST part, I'm not sure about the man or infopages or groff stuff!) that can do "magic".

That's a neat idea! So long as it doesn't become a maintenance burden for some reason, I think it could possibly be a viable path forward.


What we COULD do, however, is get rid of the individual checker documentation files (which all just contain machine generated redirects, at least according to this diff...) and have ALL the cross-referencing links (hand-written, autogenerated by our integration, or autogenerated by RST magic) point to not a page living inside Tidy's scope that just does a redirect, but to the appropriate heading in the (new) CSA doc suite?

(Then again, irrespective of what magic we will use, the non-trivial grouping structure on the new CSA docs site can become a problem...)

If I understand correctly, that would then get rid of the concrete list of checks from list.rst, wouldn't it? Otherwise, we're still going to get out of date trivially as new CSA checks are added but people forget to update list.rst. (Or did I misunderstand?)