Skip to content

tests: check: ast-grep: add static analysis rules#545

Open
pzmarzly wants to merge 17 commits into
facebook:mainfrom
pzmarzly:push-muppwpwwukvo
Open

tests: check: ast-grep: add static analysis rules#545
pzmarzly wants to merge 17 commits into
facebook:mainfrom
pzmarzly:push-muppwpwwukvo

Conversation

@pzmarzly
Copy link
Copy Markdown
Contributor

If https://ast-grep.github.io/ is installed (e.g. via pip install ast-grep-cli), we can use it to enforce many of our coding standards. See #510. Previous attempt in #502.

Each rule is added in separate commit, so it's best to review this PR commit-by-commit.

Rules with existing violations are added as disabled. We should reenable them one by one later. Each reenablement will create a large PR, that may create merge conflicts for everyone.

Test

ast-grep scan --config tests/check/ast-grep/sgconfig.yml src/bfcli/ src/libbpfilter/
https://gist.github.com/pzmarzly/7dfba8ca061a942a271b14c6320ea13b

ast-grep scan --config tests/check/ast-grep/sgconfig.yml src/bfcli/ src/libbpfilter/ --off=assert-pointer-params --off=doxygen-prefer-backticks --off=doxygen-public-functions --off=no-direct-free --off=single-line-comment-style (current invocation in CMakeLists.txt):
[empty result, i.e. all OK]

List of rules

Status Name Description
disabled assert-pointer-params Functions with pointer parameters must assert() them at entry.
enabled bf-prefix-public-functions Non-static functions must use the bf_/bfc_ prefix (or _bf_/_bfc_ when static).
enabled double-pointer-output Constructor bf_*_new() must take a TYPE ** output parameter for ownership transfer.
disabled doxygen-prefer-backticks Prefer `name` over @p name, @ref name, @c name in Doxygen comments.
disabled doxygen-public-functions Public function declarations in headers must have a preceding /** @brief ... */ comment.
enabled enum-bf-prefix Named enums must use bf_/bfc_ prefix.
enabled enum-sentinel-format Enums must end with a _BF_*_MAX / _BFC_*_MAX sentinel (with listed exceptions for bitmasks and kernel ABI mirrors).
enabled fd-init-minus-one Variables declared with _cleanup_close_ must be initialized to -1 to avoid closing fd 0 on error paths.
enabled free-function-pattern bf_*_free() / _bf_*_free() must return void and accept a double-pointer parameter.
enabled negative-errno-returns Error returns must be negated (-ENOMEM, not ENOMEM).
disabled no-direct-free Disallows raw free(); use freep() or a typed bf_*_free() cleanup function instead (with exempt callers).
enabled no-fprintf-stderr Use logging macros (bf_err, bf_warn, …) instead of fprintf(stderr, …) / vfprintf(stderr, …).
enabled no-ifndef-guards Headers must use #pragma once, not #ifndef/#define include guards.
disabled single-line-comment-style Single-line comments must use //, reserving /* */ for multi-line (SPDX excluded).
enabled spdx-license-header Every source file must contain an SPDX-License-Identifier comment.
enabled struct-bf-prefix Struct definitions (with a body) must use the bf_/bfc_ prefix; external type references are ignored.

@pzmarzly pzmarzly requested a review from qdeslandes as a code owner May 15, 2026 13:32
@meta-cla meta-cla Bot added the cla signed label May 15, 2026
@pzmarzly
Copy link
Copy Markdown
Contributor Author

@claude review

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 15, 2026

Claude review of PR #545 (6c8bf90)

Suggestions

  • bf_freep not covered by no-direct-free exemptiontests/check/ast-grep/rules/no-direct-free.yml:26 — The exemption regex won't match bf_freep (which legitimately wraps free()), causing a false positive when the rule is enabled
  • Exclusion list sync comment is misleadingtests/check/ast-grep/rules/assert-pointer-params.yml:35 — Comment says "keep in sync with bf-prefix-public-functions.yml" but the lists intentionally differ; rephrase or align
  • No version pin for ast-grep-cli.github/fedora-42.Dockerfile:40 — All Dockerfiles install ast-grep-cli without a version pin, risking CI breakage on upstream changes

Nits

  • Parenthesized errno returns escape detectiontests/check/ast-grep/rules/negative-errno-returns.yml:12return (ENOMEM); won't be caught due to missing stopBy: end
  • Redundant .gitkeep filestests/check/ast-grep/rules/.gitkeep — Both rules/ and rule-tests/ directories now have real content

Workflow run

Comment thread tests/check/ast-grep/rules/no-direct-free.yml Outdated
Comment thread tests/check/ast-grep/rules/assert-pointer-params.yml
sed \
xxd && \
dnf clean all -y

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: suggestion: ast-grep-cli is installed without a version pin (pipx install --global ast-grep-cli). A breaking change in ast-grep's rule syntax or --off flag behavior could cause CI failures across all branches simultaneously. Consider pinning to a specific version (e.g. pipx install --global ast-grep-cli==0.38.0) and updating periodically via a dedicated PR. Same applies to all other Dockerfiles (Fedora via pipx, Ubuntu via pip).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, it'd be nice to see failures introduced in new version immediately, not "whenever we remember to bump hardcoded versions". I think fetching the latest version should be fine.

kind: return_statement
has:
kind: identifier
regex: "^E[A-Z0-9]{2,}$"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude: nit: The rule uses has: without stopBy: end, so return (ENOMEM); (parenthesized) would escape detection because the direct child of return_statement is parenthesized_expression, not identifier. This doesn't occur in the current codebase but is a latent gap. Adding stopBy: end to the has: clause would make it robust against this pattern.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very niche case, ignoring.

@pzmarzly pzmarzly force-pushed the push-muppwpwwukvo branch from 6c8bf90 to 0677b44 Compare May 15, 2026 14:34
@pzmarzly pzmarzly force-pushed the push-muppwpwwukvo branch from 0677b44 to 3aa5c03 Compare May 15, 2026 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant