diff --git a/Makefile b/Makefile index 6df930724..b7699e5e1 100644 --- a/Makefile +++ b/Makefile @@ -172,6 +172,7 @@ help: ' coccicheck run kernel Coccinelle report rules' \ ' cppcheck analyze fresh kernel and userspace build contexts' \ ' checkpatch check C source/header style, including KUnit' \ + ' scs_lint check SCST code style rules (doc/scs.md)' \ ' DIFFBR=commit compare with one commit; select relevant inputs' \ ' lint-test test shared analyzer tooling (no modules)' \ ' all make all' \ @@ -286,6 +287,13 @@ checkpatch: export SCST_LINT_KDIR := $(KDIR) checkpatch: @$(PYTHON) -B scripts/style/checkpatch.py +.PHONY: scs_lint scs-lint-test +scs_lint: + @scripts/scs_lint/run_scs_lint.sh + +scs-lint-test: + @uv run --no-project scripts/scs_lint/test_scs_lint.py + .PHONY: cppcheck lint-userspace cppcheck: export SCST_LINT_KDIR := $(KDIR) cppcheck: export SCST_LINT_CC := $(CC) diff --git a/doc/scs.md b/doc/scs.md new file mode 100644 index 000000000..27448f10b --- /dev/null +++ b/doc/scs.md @@ -0,0 +1,30 @@ +# SCST code style + +SCST follows the Linux kernel coding style and [CONTRIBUTING.md](../CONTRIBUTING.md). +This document records additional project layout rules. Rule identifiers are +stable and appear in diagnostics from `make scs_lint`. + +## SCS.1: KUnit visibility annotations + +Place `SCST_KUNIT_VISIBLE`, `SCST_KUNIT_VISIBLE_INLINE`, or direct +`VISIBLE_IF_KUNIT` on a line of its own before a function declaration or +definition. Do not put a return type, function name or other attribute on that +line. Keep the existing signature and other attributes on the following lines. + +```c +SCST_KUNIT_VISIBLE +void scst_example(struct scst_cmd *cmd) +{ +} +``` + +This follows engine's OCS.7.5 layout rule. It does not change visibility, +inlining, linkage or the behavior of KUnit helpers. + +`scs_lint` checks all preprocessor branches in the selected project C sources +and headers. Comments, string literals and preprocessor directives (including +macro definitions) are outside this rule. There are no per-file waivers. + +Run `make scs_lint` for all tracked project sources, +`make scs_lint DIFFBR=` for changed files, or +`scripts/scs_lint/run_scs_lint.sh path/to/file.c` for explicit files. diff --git a/scripts/README.md b/scripts/README.md index 7feb7946a..ceeb084c6 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -16,6 +16,7 @@ direct callers of a relocated script must use its new directory below. | `coccinelle/` | Kernel semantic-patch report rules (`make coccicheck`). | | `cppcheck/` | Native analysis using captured compiler commands (`make cppcheck`). | | `style/` | Whole-file Checkpatch analysis (`make checkpatch`, `scripts/checkpatch_scan`). | +| `scs_lint/` | SCST code style rules (`make scs_lint`); see [SCS](../doc/scs.md). | | `tests/` | Isolated regression fixtures for the shared analyzer helpers (`make lint-test`). | The `checkpatch`, `checkpatch_commits` and `checkpatch_diff` helpers retain @@ -80,7 +81,7 @@ python3 -B scripts/analyzer_policy.py validate ``` The Python commands also accept `--format=json` and a policy name: `common`, -`sparse`, `smatch`, `coccicheck`, `cppcheck` or `checkpatch`. +`sparse`, `smatch`, `coccicheck`, `cppcheck`, `checkpatch` or `scs_lint`. Use `make analyzer-policy ANALYZER=smatch` to see the effective input scope and all diagnostic exceptions. A file inventory does not establish analysis or build coverage. @@ -252,6 +253,21 @@ or tool failures return 2. Make maps both failures to 2. Complete output and coverage are in `.cache/lint/checkpatch.{log,json}`. `scripts/checkpatch_scan` uses the same policy and exit codes; legacy patch/commit helpers remain separate. +## SCST code style + +`make scs_lint` checks [SCS.1](../doc/scs.md#scs1-kunit-visibility-annotations): +KUnit visibility annotations belong on their own line. It uses the common +source policy and checks every preprocessor branch, including KUnit sources. +There are no additional exclusions or diagnostic exceptions. + +Use `make scs_lint DIFFBR=` for changed files or +`scripts/scs_lint/run_scs_lint.sh path/to/file.c` for explicit inputs. +The wrapper needs `uv` and Python 3.11+; uv supplies the pinned Tree-sitter +dependencies. No kernel build is needed. Reports are saved in +`.cache/lint/scs_lint.{log,json}`. Findings return 1; incomplete analysis +returns 2 (Make maps both to 2). Empty input is a skip. +`make scs-lint-test` runs the isolated style-rule and CLI fixtures. + ## Analyzer CI The [workflow](../.github/workflows/analyzers.yml) runs on pushes to `master` diff --git a/scripts/analyzer_policy.py b/scripts/analyzer_policy.py index b94eda292..6eb974c87 100644 --- a/scripts/analyzer_policy.py +++ b/scripts/analyzer_policy.py @@ -24,8 +24,8 @@ KBUILD_SCHEMA = { "kernel_exceptions": [{"header": str, "message": str, "reason": str}], } BASIC_SCHEMA = {"version": int, "notes": [str]} -POLICIES = ("common", "sparse", "smatch", "coccicheck", "cppcheck", "checkpatch") -BASIC_POLICIES = ("coccicheck", "cppcheck", "checkpatch") +POLICIES = ("common", "sparse", "smatch", "coccicheck", "cppcheck", "checkpatch", "scs_lint") +BASIC_POLICIES = ("coccicheck", "cppcheck", "checkpatch", "scs_lint") def relative_path(value: str, location: str): diff --git a/scripts/policy/scs_lint.json b/scripts/policy/scs_lint.json new file mode 100644 index 000000000..1cc303b10 --- /dev/null +++ b/scripts/policy/scs_lint.json @@ -0,0 +1,9 @@ +{ + "version": 1, + "notes": [ + "SCS.1: KUnit visibility annotations must occupy their own source line.", + "Check all preprocessor branches in common-policy C sources and headers, including KUnit.", + "Comments, literals and directive contents are outside the layout rule.", + "No additional input exclusions or diagnostic exceptions." + ] +} diff --git a/scripts/scs_lint/run_scs_lint.sh b/scripts/scs_lint/run_scs_lint.sh new file mode 100755 index 000000000..5b4d29664 --- /dev/null +++ b/scripts/scs_lint/run_scs_lint.sh @@ -0,0 +1,11 @@ +#!/usr/bin/env bash +set -euo pipefail + +HERE="$(cd "$(dirname "$0")" && pwd)" + +if ! command -v uv >/dev/null; then + echo "scs_lint: uv is required to run the pinned Python dependencies" >&2 + exit 2 +fi + +exec uv run --no-project "$HERE/scs_lint.py" "$@" diff --git a/scripts/scs_lint/scs_lint.py b/scripts/scs_lint/scs_lint.py new file mode 100644 index 000000000..88ae8add9 --- /dev/null +++ b/scripts/scs_lint/scs_lint.py @@ -0,0 +1,129 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.11" +# dependencies = [ +# "tree-sitter==0.25.2", +# "tree-sitter-c==0.24.1", +# ] +# /// + +"""Check mechanically enforceable SCST code style rules in C sources.""" + +import argparse +import fcntl +import json +import os +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + +import tree_sitter as ts +import tree_sitter_c + +sys.dont_write_bytecode = True +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +from lint_scope import select_sources # noqa: E402 +from lint_common import entrypoint, log_path, show_log # noqa: E402 + +REPOSITORY = Path(__file__).resolve().parents[2] +VISIBILITY = (b"SCST_KUNIT_VISIBLE", b"SCST_KUNIT_VISIBLE_INLINE", b"VISIBLE_IF_KUNIT") +CONDITIONALS = {"preproc_if", "preproc_ifdef", "preproc_elif", "preproc_elifdef", "preproc_else"} + + +@dataclass(frozen=True) +class Violation: + line: int + column: int + rule: str + message: str + + +def code_nodes(root: ts.Node): + """Visit code in every conditional branch, excluding directive contents.""" + stack = [root] + while stack: + node = stack.pop() + hidden = [] + if node.type.startswith("preproc_"): + if node.type not in CONDITIONALS: + continue + hidden = [node.child_by_field_name("condition"), node.child_by_field_name("name")] + yield node + stack.extend(child for child in reversed(node.children) if child not in hidden) + + +def check_source(source: bytes, parser: ts.Parser) -> list[Violation]: + if not any(annotation in source for annotation in VISIBILITY): + return [] + + code = bytearray(source) + annotations = [] + for node in code_nodes(parser.parse(source).root_node): + if node.type == "comment": + # Mask comments without moving the source offsets used for diagnostics. + code[node.start_byte:node.end_byte] = b" " * (node.end_byte - node.start_byte) + elif node.type in {"identifier", "type_identifier"} and node.text in VISIBILITY: + annotations.append(node) + + violations = [] + for node in annotations: + start = source.rfind(b"\n", 0, node.start_byte) + 1 + end = source.find(b"\n", node.end_byte) + if end == -1: + end = len(source) + if code[start:end].strip() != node.text: + row, column = node.start_point + violations.append(Violation( + row + 1, column + 1, "SCS.1", + f"place {node.text.decode()} on a line of its own")) + return violations + + +def main(argv: list[str] | None = None) -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("files", nargs="*", type=Path, metavar="FILE", + help="C source/header files; default: tracked project C sources and headers") + ap.add_argument("--base", default=os.environ.get("DIFFBR") or None) + args = ap.parse_args(argv) + + with log_path(REPOSITORY, "scs_lint.lock", truncate=False).open("a") as lock: + fcntl.flock(lock, fcntl.LOCK_EX) + log = log_path(REPOSITORY, "scs_lint.log") + report = {"tool": "scs_lint", "outcome": "incomplete", "exit_code": 2, + "remaining": 0, "selected_files": [], "checked_files": []} + show_log(log) + try: + paths = select_sources(REPOSITORY, args.files, args.base, "scs_lint") + report["selected_files"] = [path.relative_to(REPOSITORY).as_posix() for path in paths] + parser = ts.Parser(ts.Language(tree_sitter_c.language())) + with log.open("a") as output: + for path in paths: + for violation in check_source(path.read_bytes(), parser): + message = (f"{path}:{violation.line}:{violation.column}: error: " + f"{violation.rule}: {violation.message}") + print(message) + output.write(message + "\n") + report["remaining"] += 1 + report["checked_files"].append(path.relative_to(REPOSITORY).as_posix()) + status = int(bool(report["remaining"])) + report.update(outcome=("findings" if status else "clean") if paths else "skipped", + exit_code=status) + summary = f"{report['remaining']} violation(s), {len(paths)} file(s) checked" + if not paths: + summary += "; no eligible inputs, skipped" + print(summary, file=sys.stderr) + output.write(summary + "\n") + return status + except (OSError, ValueError, subprocess.CalledProcessError) as exc: + message = f"scs_lint: {exc}" + print(message, file=sys.stderr) + with log.open("a") as output: + output.write(message + "\n") + return 2 + finally: + log.with_suffix(".json").write_text(json.dumps(report, indent=2) + "\n") + + +if __name__ == "__main__": + sys.exit(entrypoint(main)) diff --git a/scripts/scs_lint/test_scs_lint.py b/scripts/scs_lint/test_scs_lint.py new file mode 100644 index 000000000..cf6b4f078 --- /dev/null +++ b/scripts/scs_lint/test_scs_lint.py @@ -0,0 +1,216 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.11" +# dependencies = [ +# "tree-sitter==0.25.2", +# "tree-sitter-c==0.24.1", +# ] +# /// + +import contextlib +import io +import json +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +import tree_sitter as ts +import tree_sitter_c +from scs_lint import check_source, main + + +class KUnitVisibilityTest(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.parser = ts.Parser(ts.Language(tree_sitter_c.language())) + + def violations(self, src: bytes) -> list: + return check_source(src, self.parser) + + def test_rejects_shared_type_signature_and_attribute_lines(self): + for declaration in ( + b"SCST_KUNIT_VISIBLE void\nfn(void)", + b"SCST_KUNIT_VISIBLE void fn(void)", + b"SCST_KUNIT_VISIBLE __printf(1, 2)\nvoid fn(const char *fmt, ...)", + b"__maybe_unused SCST_KUNIT_VISIBLE\nvoid fn(void)"): + with self.subTest(declaration=declaration): + errors = self.violations(declaration + b"\n{\n}\n") + self.assertEqual(len(errors), 1) + self.assertEqual(errors[0].line, 1) + self.assertEqual(errors[0].rule, "SCS.1") + + def test_accepts_standalone_definition_and_prototype(self): + src = (b"SCST_KUNIT_VISIBLE\nvoid fn(void) {}\n" + b"SCST_KUNIT_VISIBLE\nvoid other(void);\n") + self.assertEqual(self.violations(src), []) + + def test_checks_inline_wrapper_and_direct_kernel_annotation(self): + for annotation in (b"SCST_KUNIT_VISIBLE_INLINE", b"VISIBLE_IF_KUNIT"): + self.assertEqual(self.violations(annotation + b"\nvoid fn(void);\n"), []) + self.assertEqual(len(self.violations(annotation + b" void fn(void);\n")), 1) + + def test_does_not_enforce_attribute_layout(self): + src = (b"SCST_KUNIT_VISIBLE\n" + b"__printf(1, 2) void fn(const char *fmt, ...);\n") + self.assertEqual(self.violations(src), []) + + def test_ignores_comments_literals_and_macro_definitions(self): + src = (b"/* SCST_KUNIT_VISIBLE void fn(void); */\n" + b"// SCST_KUNIT_VISIBLE void fn(void);\n" + b'const char *text = "SCST_KUNIT_VISIBLE void";\n' + b"#define SCST_KUNIT_VISIBLE static\n" + b"#define VISIBILITY SCST_KUNIT_VISIBLE\n") + self.assertEqual(self.violations(src), []) + + def test_allows_comments_beside_macro_without_hiding_code(self): + src = (b"/* before */ SCST_KUNIT_VISIBLE /* after */\nvoid fn(void);\n" + b"SCST_KUNIT_VISIBLE // after\nvoid other(void);\n" + b"SCST_KUNIT_VISIBLE /* note */ void bad(void);\n") + self.assertEqual([error.line for error in self.violations(src)], [5]) + + def test_handles_last_line_without_newline(self): + errors = self.violations(b"SCST_KUNIT_VISIBLE void fn(void);") + self.assertEqual([(error.line, error.column) for error in errors], [(1, 1)]) + + def test_checks_all_preprocessor_branches(self): + src = (b"#if 0\n" + b"SCST_KUNIT_VISIBLE void inactive(void);\n" + b"#else\nSCST_KUNIT_VISIBLE void active(void);\n#endif\n") + self.assertEqual([error.line for error in self.violations(src)], [2, 4]) + + def test_ignores_multiline_directives_and_macro_bodies(self): + src = (b"#define DECLARE() \\\n" + b"SCST_KUNIT_VISIBLE void fn(void);\n" + b"#if defined(SCST_KUNIT_VISIBLE) && \\\n" + b" SCST_KUNIT_VISIBLE\n" + b"SCST_KUNIT_VISIBLE void bad(void);\n" + b"#elif defined(SCST_KUNIT_VISIBLE)\n" + b"SCST_KUNIT_VISIBLE\nvoid good(void);\n" + b"#endif\n" + b"#undef SCST_KUNIT_VISIBLE\n") + self.assertEqual([error.line for error in self.violations(src)], [5]) + + def test_keeps_positions_after_multiline_comment(self): + src = (b"/* SCST_KUNIT_VISIBLE\ncomment */\n" + b" SCST_KUNIT_VISIBLE void bad(void);\n") + self.assertEqual([(error.line, error.column) for error in self.violations(src)], [(3, 5)]) + + +class CommandTest(unittest.TestCase): + def setUp(self): + self.temporary = tempfile.TemporaryDirectory(prefix="scs-lint-test-") + self.addCleanup(self.temporary.cleanup) + self.root = Path(self.temporary.name) + environment = mock.patch.dict("os.environ", {"DIFFBR": ""}) + environment.start() + self.addCleanup(environment.stop) + + def source(self, name, text): + path = self.root / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + return path + + def run_lint(self, files, base=None): + out = io.StringIO() + err = io.StringIO() + with (contextlib.redirect_stdout(out), contextlib.redirect_stderr(err), + mock.patch("scs_lint.REPOSITORY", self.root)): + status = main([str(path) for path in files] + (["--base", base] if base else [])) + return status, out.getvalue(), err.getvalue() + + def report(self): + return json.loads((self.root / ".cache/lint/scs_lint.json").read_text()) + + def test_reports_bad_file_and_checks_other_files(self): + bad = self.source("scst/bad.c", "SCST_KUNIT_VISIBLE void bad(void);\n") + good = self.source("scst/good.h", "SCST_KUNIT_VISIBLE\nvoid good(void);\n") + status, out, err = self.run_lint([good, bad]) + self.assertEqual(status, 1) + self.assertIn(f"{bad}:1:1: error: SCS.1:", out) + self.assertNotIn(str(good), out) + self.assertIn("1 violation(s), 2 file(s) checked", err) + self.assertEqual(self.report()["outcome"], "findings") + self.assertEqual(self.report()["checked_files"], ["scst/bad.c", "scst/good.h"]) + self.assertEqual(self.report()["exit_code"], 1) + + def test_accepts_explicit_untracked_file_and_deduplicates(self): + path = self.source("scst/new.c", "SCST_KUNIT_VISIBLE\nvoid good(void);\n") + status, out, err = self.run_lint([path, path]) + self.assertEqual((status, out), (0, "")) + self.assertIn("0 violation(s), 1 file(s) checked", err) + + def test_explicit_outside_scope_is_an_error(self): + path = self.source("scripts/example.c", "SCST_KUNIT_VISIBLE void bad(void);\n") + status, out, err = self.run_lint([path]) + self.assertEqual(status, 2) + self.assertIn("not a project C source/header", err) + + def test_default_skips_working_tree_deletions(self): + subprocess.run(["git", "init", "-q", str(self.root)], check=True) + source = self.source("scst/deleted.c", "int deleted(void);\n") + subprocess.run(["git", "-C", str(self.root), "add", str(source)], check=True) + source.unlink() + status, out, err = self.run_lint([]) + self.assertEqual((status, out), (0, "")) + self.assertIn("0 file(s) checked", err) + self.assertEqual(self.report()["outcome"], "skipped") + + def test_changed_files_only_and_no_source_changes_skip(self): + subprocess.run(["git", "init", "-q", str(self.root)], check=True) + changed = self.source("scst/changed.c", "SCST_KUNIT_VISIBLE\nvoid good(void);\n") + unchanged = self.source("scst/unchanged.h", "SCST_KUNIT_VISIBLE void bad(void);\n") + subprocess.run(["git", "-C", str(self.root), "add", "."], check=True) + subprocess.run(["git", "-C", str(self.root), "-c", "user.name=Fixture", + "-c", "user.email=fixture@example.com", "commit", "-qm", "Fixture"], + check=True) + self.assertEqual(self.run_lint([], base="HEAD")[0], 0) + self.assertEqual(self.report()["outcome"], "skipped") + changed.write_text("SCST_KUNIT_VISIBLE void changed(void);\n") + status, out, _ = self.run_lint([], base="HEAD") + self.assertEqual(status, 1) + self.assertNotIn(str(unchanged), out) + self.assertEqual(self.report()["checked_files"], ["scst/changed.c"]) + + def test_unreadable_file_preserves_earlier_findings_but_fails_analysis(self): + first = self.source("scst/a.c", "SCST_KUNIT_VISIBLE void bad(void);\n") + second = self.source("scst/z.c", "void other(void);\n") + read_bytes = Path.read_bytes + + def read(path): + if path == second: + raise PermissionError("cannot read z.c") + return read_bytes(path) + + with mock.patch.object(Path, "read_bytes", read): + status, out, err = self.run_lint([first, second]) + self.assertEqual(status, 2) + self.assertIn("SCS.1", out) + self.assertIn("cannot read z.c", err) + self.assertEqual(self.report()["outcome"], "incomplete") + self.assertEqual(self.report()["checked_files"], ["scst/a.c"]) + self.assertEqual(self.report()["remaining"], 1) + log = (self.root / ".cache/lint/scs_lint.log").read_text() + self.assertIn("SCS.1", log) + self.assertIn("cannot read z.c", log) + + def test_invalid_input_is_not_a_clean_result(self): + unsupported = self.source("example.txt", "void good(void);\n") + for path in (self.root / "missing.c", unsupported, self.root): + with self.subTest(path=path): + status, out, err = self.run_lint([path]) + self.assertEqual((status, out), (2, "")) + self.assertIn("not a project C source/header", err) + self.assertNotIn("0 violation(s)", err) + + def test_unavailable_default_repository_is_an_error(self): + self.root = self.root / "missing" + status, out, err = self.run_lint([]) + self.assertEqual((status, out), (2, "")) + self.assertIn("scs_lint:", err) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/tests/test_analyzer_policy.py b/scripts/tests/test_analyzer_policy.py index fddc82725..2614ec5da 100644 --- a/scripts/tests/test_analyzer_policy.py +++ b/scripts/tests/test_analyzer_policy.py @@ -157,7 +157,7 @@ class PolicyCliTests(unittest.TestCase): source = analyzer_policy.POLICY_PATH.parent.parent for name in ("analyzer_policy.py", "lint_scope.py", "policy/common.json", "policy/sparse.json", "policy/smatch.json", "policy/coccicheck.json", - "policy/cppcheck.json", "policy/checkpatch.json"): + "policy/cppcheck.json", "policy/checkpatch.json", "policy/scs_lint.json"): shutil.copyfile(source / name, self.scripts / name) def run_cli(self, *args): @@ -233,7 +233,7 @@ class PolicyCliTests(unittest.TestCase): def test_basic_analyzers_inherit_common_scope_without_exceptions(self): common = json.loads((self.scripts / "policy/common.json").read_text()) - for tool in ("coccicheck", "cppcheck", "checkpatch"): + for tool in ("coccicheck", "cppcheck", "checkpatch", "scs_lint"): with self.subTest(tool=tool): result = self.run_cli("show", tool, "--format=json") self.assertEqual(result.returncode, 0, result.stderr)