From abec67a365ea80faaea4579d0f9a35676eb516e1 Mon Sep 17 00:00:00 2001 From: dayfine Date: Thu, 29 Oct 2026 21:57:11 -0700 Subject: [PATCH 1/4] Code health: default the base to the merge-base with upstream Comparing with a stale fork main reported upstream changes as the branch's own. Without --base-ref, use git merge-base HEAD upstream/main (origin/main when there is no upstream remote) and print the base used. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AKYzhrMnPK61XHWEch4a6E --- devtools/code_health/README.md | 7 ++++++- devtools/code_health/__main__.py | 12 ++++++++---- devtools/code_health/git_files.py | 14 ++++++++++++++ devtools/code_health/tests/test_cli.py | 22 +++++++++++++++++++++- 4 files changed, 49 insertions(+), 6 deletions(-) diff --git a/devtools/code_health/README.md b/devtools/code_health/README.md index 08dd20c1c3..d58512a8ad 100644 --- a/devtools/code_health/README.md +++ b/devtools/code_health/README.md @@ -6,9 +6,14 @@ From the checkout root, create a dedicated environment (Python 3.8+): python3 -m venv .venv-code-health .venv-code-health/bin/python -m pip install -r devtools/code_health/requirements.txt .venv-code-health/bin/python -m unittest discover -s devtools/code_health/tests -v -.venv-code-health/bin/python -m devtools.code_health --base-ref origin/main +.venv-code-health/bin/python -m devtools.code_health ``` +Without `--base-ref`, the base is `git merge-base HEAD upstream/main`, or +`origin/main` when there is no `upstream` remote, and the command prints the +base it used. A stale fork `main` therefore cannot report upstream changes +as this branch's. CI passes the PR base explicitly. + Only added/modified/renamed Git-tracked `.h .hpp .c .cpp .tpp` files under `Common/` and `SerialPrograms/` are analyzed. Directory components starting with `3rdParty` are excluded. Unchanged and deleted files are not read. diff --git a/devtools/code_health/__main__.py b/devtools/code_health/__main__.py index cecf7003ec..7df15ed1fe 100644 --- a/devtools/code_health/__main__.py +++ b/devtools/code_health/__main__.py @@ -1,15 +1,18 @@ -"""Lint changed C++: python -m devtools.code_health --base-ref origin/main.""" +"""Lint changed C++: python -m devtools.code_health [--base-ref REV].""" import argparse import subprocess import sys from pathlib import Path -from .git_files import changed_files, read_revision +from .git_files import changed_files, default_base, read_revision from .metrics import measure from .policy import evaluate -def run(root, base_ref): +def run(root, base_ref=None): + if base_ref is None: + base_ref, how = default_base(root) + print(f'Base: {base_ref} ({how}).') changes = changed_files(root, base_ref) failures = [] advisory = 0 @@ -29,7 +32,8 @@ def run(root, base_ref): def main(): parser = argparse.ArgumentParser(description=__doc__) parser.add_argument('--root', type=Path, default=Path.cwd()) - parser.add_argument('--base-ref', default='origin/main', help='Compare changed files with this revision') + parser.add_argument('--base-ref', help='Compare changed files with this revision (default: ' + 'merge-base of HEAD and upstream/main, else origin/main)') args = parser.parse_args() try: return run(args.root.resolve(), args.base_ref) diff --git a/devtools/code_health/git_files.py b/devtools/code_health/git_files.py index 09ca00a3fc..89615e908e 100644 --- a/devtools/code_health/git_files.py +++ b/devtools/code_health/git_files.py @@ -31,5 +31,19 @@ def changed_files(root, base_ref): return changes +UPSTREAMS = ('upstream/main', 'origin/main') + + +def default_base(root): + """(merge-base of HEAD with upstream/main, else origin/main; description).""" + for ref in UPSTREAMS: + exists = subprocess.run(['git', '-C', str(root), 'rev-parse', '--verify', '-q', ref + '^{commit}'], + capture_output=True) + if exists.returncode == 0: + base = git(root, 'merge-base', 'HEAD', ref).decode().strip() + return base, f'merge-base of HEAD and {ref}' + raise ValueError('No --base-ref given and neither upstream/main nor origin/main exists') + + def read_revision(root, revision, path): return git(root, 'show', revision + ':' + path).decode('utf-8-sig') diff --git a/devtools/code_health/tests/test_cli.py b/devtools/code_health/tests/test_cli.py index 781ab13ee6..085bfde16f 100644 --- a/devtools/code_health/tests/test_cli.py +++ b/devtools/code_health/tests/test_cli.py @@ -5,7 +5,7 @@ import unittest from pathlib import Path -from devtools.code_health.git_files import included +from devtools.code_health.git_files import default_base, included REPO = Path(__file__).resolve().parents[3] @@ -110,5 +110,25 @@ def test_cpp_extension_and_directory_scope(self): self.assertFalse(included(path), path) + def test_default_base_is_merge_base_with_upstream_then_origin(self): + self.source(3) + self.commit() + fork_point = self.git('rev-parse', 'HEAD').decode().strip() + with self.assertRaises(ValueError): + default_base(self.root) + self.git('update-ref', 'refs/remotes/origin/main', fork_point) + self.source(60) + self.commit() + self.assertEqual((fork_point, 'merge-base of HEAD and origin/main'), default_base(self.root)) + # A stale fork main: upstream moved on from an older commit; prefer its merge-base. + self.git('update-ref', 'refs/remotes/origin/main', 'HEAD') + self.git('update-ref', 'refs/remotes/upstream/main', fork_point) + self.assertEqual((fork_point, 'merge-base of HEAD and upstream/main'), default_base(self.root)) + result = subprocess.run([sys.executable, '-m', 'devtools.code_health', '--root', str(self.root)], + cwd=REPO, capture_output=True, text=True) + self.assertEqual(1, result.returncode, result.stderr) + self.assertIn(f'Base: {fork_point} (merge-base of HEAD and upstream/main).', result.stdout) + + if __name__ == '__main__': unittest.main() From 744ee1fdcce95babc495eec0f9d69398f04d94a3 Mon Sep 17 00:00:00 2001 From: dayfine Date: Thu, 29 Oct 2026 21:57:40 -0700 Subject: [PATCH 2/4] Code health: check the shape of new and changed classes Count public methods (limit 7, advisory above 5), public non-static data in classes with mutating methods, and single-method interfaces, with a lexical member scan over lizard's tokens. Classes are compared with the base like functions, so only new violations and growth fail. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AKYzhrMnPK61XHWEch4a6E --- devtools/code_health/README.md | 9 + devtools/code_health/__main__.py | 16 +- devtools/code_health/classes.py | 316 +++++++++++++++++++++ devtools/code_health/config.py | 17 ++ devtools/code_health/policy.py | 25 +- devtools/code_health/tests/test_classes.py | 101 +++++++ devtools/code_health/tests/test_cli.py | 16 ++ 7 files changed, 494 insertions(+), 6 deletions(-) create mode 100644 devtools/code_health/classes.py create mode 100644 devtools/code_health/config.py create mode 100644 devtools/code_health/tests/test_classes.py diff --git a/devtools/code_health/README.md b/devtools/code_health/README.md index d58512a8ad..855f377c95 100644 --- a/devtools/code_health/README.md +++ b/devtools/code_health/README.md @@ -26,6 +26,9 @@ are included. Missing base revisions and analysis errors fail the command. | Function length | 50 physical lines, signature through closing brace; 25 recommended | | Nesting | 4 lexical brace levels inside the function body | | Parameters | 7 | +| Public methods per class/struct | 7 (excluding ctors, dtors, operators, deleted and Qt signals); 6-7 advisory | +| Public non-static data in a class with mutating methods | 0 (exempt: plain structs, value types with only const/static methods, `*Option`/`*Cell` fields, test files) | +| Single-method interface (one pure virtual + optional virtual dtor, no bases) | not allowed: prefer a callable (`MoveOnlyFunction` / `std::function`) | New files/functions must meet these limits. In modified files, compare each metric with its value at the base revision: existing violations may remain or @@ -47,6 +50,12 @@ ignores enclosing namespace/class braces, comments and string literals. This is lexical analysis, not a C++ compiler or clang-tidy replacement. Macro-generated functions and advanced syntax may require parser follow-up. CCN is not enforced. +Class shapes come from a lexical member scan over lizard's tokens +(`classes.py`), keyed by qualified class name and compared with the base like +functions. libclang would need a compilation database with Qt include paths +to parse these headers; the scan only has to tell access sections, member +functions and data apart. Classes split across `#if` branches are skipped. + ## Build and native C++ tests in one command The existing `.github/workflows/cpp-ci-serial-programs-base.yml` already runs diff --git a/devtools/code_health/__main__.py b/devtools/code_health/__main__.py index 7df15ed1fe..45daddfea3 100644 --- a/devtools/code_health/__main__.py +++ b/devtools/code_health/__main__.py @@ -4,9 +4,14 @@ import sys from pathlib import Path +from .classes import measure_classes from .git_files import changed_files, default_base, read_revision from .metrics import measure -from .policy import evaluate +from .policy import advisories, evaluate + + +def analyze(path, source): + return measure(path, source) + measure_classes(path, source) def run(root, base_ref=None): @@ -14,15 +19,18 @@ def run(root, base_ref=None): base_ref, how = default_base(root) print(f'Base: {base_ref} ({how}).') changes = changed_files(root, base_ref) - failures = [] + failures, notes = [], [] advisory = 0 for current, original in changes: - metrics = measure(current, (root / current).read_text(encoding='utf-8-sig')) - previous = measure(current, read_revision(root, base_ref, original)) if original else [] + metrics = analyze(current, (root / current).read_text(encoding='utf-8-sig')) + previous = analyze(current, read_revision(root, base_ref, original)) if original else [] failures.extend(evaluate(metrics, previous)) + notes.extend(advisories(metrics, previous)) advisory += sum(m.check == 'file_lines' and 500 < m.value <= 1000 for m in metrics) print(f'Scanned {len(changes)} changed C++ files against {base_ref}.') print(f'Advisory: {advisory} changed files exceed 500 lines; prefer functions <=25 lines.') + for note in notes: + print('Advisory: ' + note) for failure in failures: print(failure, file=sys.stderr) print(f'Code health: {len(failures)} failures.') diff --git a/devtools/code_health/classes.py b/devtools/code_health/classes.py new file mode 100644 index 0000000000..504958e0f8 --- /dev/null +++ b/devtools/code_health/classes.py @@ -0,0 +1,316 @@ +"""Class/struct shape checks from a lexical scan (no compiler or include paths needed). + +Members are recognized from lizard's C++ tokens: access sections, declarations ended +by ';' and inline bodies. Unbalanced braces from #if branches make a class unmeasured +rather than failing the run. +""" +import re +from dataclasses import dataclass + +from lizard_languages import CLikeReader + +from .config import CLASS_LIMITS +from .metrics import Metric + + +MACRO = re.compile(r'^[A-Z][A-Z0-9_]*$') +IDENTIFIER = re.compile(r'^[A-Za-z_]\w*$') +ACCESS = {'public', 'private', 'protected'} +SIGNALS = {'signals', 'Q_SIGNALS'} +SLOTS = {'slots', 'Q_SLOTS'} +TYPE_KEYWORDS = {'class', 'struct', 'union'} +# Words that may precede '(' in a declaration without naming a function. +NOT_NAMES = {'decltype', 'alignas', 'alignof', 'sizeof', 'noexcept', 'throw', 'requires', + '__attribute__', '__declspec', 'void', 'int', 'bool', 'char', 'auto'} +SKIP_DECLARATIONS = {'friend', 'using', 'typedef', 'static_assert', 'enum'} + + +@dataclass(frozen=True) +class Member: + access: str + kind: str # method, ctor, dtor, operator, data, type + pure: bool = False + deleted: bool = False + virtual: bool = False + static: bool = False + const: bool = False # a const (or static) member function: does not mutate the object + option: bool = False # a config option or table cell field (type named *Option or *Cell) + + +@dataclass(frozen=True) +class ClassInfo: + name: str + line: int + has_bases: bool + members: tuple + + +def tokens(source): + result, line = [], 1 + for text in CLikeReader.generate_tokens(source): + if not (text.isspace() or text.startswith(('//', '/*', '#'))): + result.append((text, line)) + line += text.count('\n') + return result + + +def brace_pairs(toks): + pairs, stack = {}, [] + for index, (text, _) in enumerate(toks): + if text == '{': + stack.append(index) + elif text == '}' and stack: + pairs[stack.pop()] = index + return pairs + + +def skip_group(toks, index, open_text, close_text): + """Index just past the group opened at index; len(toks) if unbalanced.""" + depth = 0 + for k in range(index, len(toks)): + if toks[k][0] == open_text: + depth += 1 + elif toks[k][0] == close_text: + depth -= 1 + if depth == 0: + return k + 1 + return len(toks) + + +def class_head(toks, index): + """For a class/struct/union keyword, return (name, has_bases, '{' index) or None.""" + if index > 0 and toks[index - 1][0] in ('enum', '<', ',', 'friend', '(', 'typename'): + return None + angle, bases, names, k = 0, False, [], index + 1 + while k < len(toks): + text = toks[k][0] + if text == '{' and angle == 0: + break + if text == '(' and k > 0 and (toks[k - 1][0] in NOT_NAMES or MACRO.match(toks[k - 1][0])): + k = skip_group(toks, k, '(', ')') + continue + if text == '[': + k = skip_group(toks, k, '[', ']') + continue + if text in (';', '(', ')', '=', '}', '{') and angle == 0: + return None + if text == '<': + angle += 1 + elif text == '>': + angle -= 1 + if angle < 0: + return None + elif text == ',' and angle == 0 and not bases: + return None + elif text == ':' and angle == 0: + bases = True + elif not bases and angle == 0 and IDENTIFIER.match(text) and text != 'final': + names.append(text) + k += 1 + else: + return None + if len(names) > 1: + names = [n for n in names if not MACRO.match(n)] or names + return (names[-1] if names else None), bases, k + + +def strip_template(decl): + if decl and decl[0] == 'template' and len(decl) > 1 and decl[1] == '<': + depth = 0 + for k in range(1, len(decl)): + depth += {'<': 1, '>': -1}.get(decl[k], 0) + if depth == 0: + return decl[k + 1:] + return decl + + +def call_paren(decl): + """Index of the '(' that opens a function's parameters, or None for data.""" + angle = 0 + for k, text in enumerate(decl): + if text == 'operator': + return decl.index('(', k + 2) if '(' in decl[k + 2:] else None + if text == '=' and angle == 0: + return None + if text == '<' and k > 0 and IDENTIFIER.match(decl[k - 1]): + angle += 1 + elif text == '>' and angle > 0: + angle -= 1 + elif text == '(' and angle == 0 and k > 0: + if decl[k - 1] in NOT_NAMES or not IDENTIFIER.match(decl[k - 1]): + return None + if k + 1 < len(decl) and decl[k + 1] in ('*', '&', '^'): + return None + return k + return None + + +def is_option(decl): + """Option and cell fields are the framework's reflected settings, public by convention.""" + angle, words = 0, [] + for text in decl: + if text in ('=', '{}', ':', '['): + break + angle += {'<': 1, '>': -1}.get(text, 0) + if angle == 0 and IDENTIFIER.match(text): + words.append(text) + return any(w.endswith(('Option', 'Options', 'Cell')) for w in words[:-1]) + + +def _close(tail): + depth = 0 + for k, text in enumerate(tail): + depth += {'(': 1, ')': -1}.get(text, 0) + if depth == 0: + return k + 1 + return len(tail) + + +def classify(decl, access, class_name): + decl = strip_template(decl) + if not decl or decl[0] in SKIP_DECLARATIONS: + return None + if decl[0] in TYPE_KEYWORDS: + declarator = '{}' in decl and IDENTIFIER.match(decl[-1] if decl[-1] != '{}' else '') + return Member(access, 'data') if declarator else Member(access, 'type') + paren = call_paren(decl) + if paren is None: + return Member(access, 'data', static='static' in decl, option=is_option(decl)) + name = decl[paren - 1] + if 'operator' in decl[:paren]: + kind = 'operator' + elif paren >= 2 and decl[paren - 2] == '~': + kind = 'dtor' + elif name == class_name: + kind = 'ctor' + else: + kind = 'method' + tail = decl[paren:] + return Member(access, kind, + pure=len(tail) >= 2 and tail[-2:] == ['=', '0'], + deleted='delete' in tail and '=' in tail, + virtual='virtual' in decl or 'override' in tail, + static='static' in decl[:paren], + const='static' in decl[:paren] or 'const' in tail[_close(tail):]) + + +def members(toks, start, end, pairs, keyword, class_name): + access = 'private' if keyword == 'class' else 'public' + found, decl, paren_depth, k = [], [], 0, start + while k < end: + text = toks[k][0] + following = toks[k + 1][0] if k + 1 < end else '' + if not decl: + if text in ACCESS and following == ':': + access, k = text, k + 2 + continue + if text in ACCESS and following in SLOTS: + access, k = text, k + 3 + continue + if text in SIGNALS and following == ':': + access, k = 'signals', k + 2 + continue + if text == ';' or (MACRO.match(text) and following != '::' and text != class_name): + k = skip_group(toks, k + 1, '(', ')') if following == '(' else k + 1 + continue + if text == '(': + paren_depth += 1 + elif text == ')': + paren_depth -= 1 + if text == '{': + close = pairs.get(k) + if close is None or close > end: + return None + is_function = paren_depth == 0 and call_paren(strip_template(decl)) is not None + in_initializers = is_function and ':' in decl and decl[-1] not in (')', '{}') + if is_function and not in_initializers and decl[-1] != '=': + member = classify(decl, access, class_name) + if member: + found.append(member) + decl, k = [], close + 1 + if k < end and toks[k][0] == ';': + k += 1 + continue + decl.append('{}') + k = close + 1 + continue + if text == ';' and paren_depth == 0: + member = classify(decl, access, class_name) + if member: + found.append(member) + decl, k = [], k + 1 + continue + decl.append(text) + k += 1 + return found + + +def scope_label(toks, index, heads): + """Name a '{' scope for qualified class names: classes and namespaces only.""" + if index in heads: + return heads[index][0] + names = [] + for k in range(index - 1, -1, -1): + text = toks[k][0] + if text == 'namespace': + return '::'.join(reversed(names)) or None + if not (IDENTIFIER.match(text) or text == '::'): + return None + if text != '::': + names.append(text) + return None + + +def find_classes(source): + toks = tokens(source) + pairs = brace_pairs(toks) + heads = {} + for index, (text, line) in enumerate(toks): + if text in TYPE_KEYWORDS: + head = class_head(toks, index) + if head and head[0] and head[2] in pairs: + heads[head[2]] = (head[0], head[1], line, text) + result, stack = [], [] + for index, (text, _) in enumerate(toks): + if text == '{': + label = scope_label(toks, index, heads) + if index in heads: + name, bases, line, keyword = heads[index] + body = members(toks, index + 1, pairs[index], pairs, keyword, name) + if body is not None: + qualified = '::'.join([s for s in stack if s] + [name]) + result.append(ClassInfo(qualified, line, bases, tuple(body))) + stack.append(label) + elif text == '}' and stack: + stack.pop() + return result + + +def is_single_method_interface(info): + methods = [m for m in info.members if m.kind == 'method'] + others = [m for m in info.members + if m.kind != 'method' and not (m.kind == 'dtor' and m.virtual)] + return not info.has_bases and len(methods) == 1 and methods[0].pure and not others + + +def is_test_file(path): + return '/Tests/' in path or path.endswith(('_Tests.cpp', '_Test.cpp', 'Tests.cpp')) + + +def measure_classes(path, source): + """Public data is allowed in value types (only const methods) and in test fakes.""" + result = [] + for info in find_classes(source.replace('\r\n', '\n').replace('\r', '\n')): + methods = [m for m in info.members if m.kind == 'method' and not m.deleted] + public_methods = sum(m.access == 'public' for m in methods) + mutators = [m for m in methods if not m.const] + public_data = sum(m.access == 'public' and m.kind == 'data' and not (m.static or m.option) + for m in info.members) if mutators and not is_test_file(path) else 0 + values = ( + ('public_methods', public_methods), + ('public_data', public_data), + ('single_method_interface', int(is_single_method_interface(info))), + ) + for check, value in values: + result.append(Metric(path, info.name, check, value, CLASS_LIMITS[check], info.line)) + return result diff --git a/devtools/code_health/config.py b/devtools/code_health/config.py new file mode 100644 index 0000000000..3159f272ad --- /dev/null +++ b/devtools/code_health/config.py @@ -0,0 +1,17 @@ +"""Limits for class shapes and the repository layout used by the layering check.""" + +CLASS_LIMITS = { + 'public_methods': 7, + 'public_data': 0, + 'single_method_interface': 0, +} + +# Advisory floors: new or changed values above these (and within the limit) are reported. +ADVISORY = {'public_methods': 5} + +HINTS = { + 'public_methods': 'split the class or move helpers into free functions', + 'public_data': 'make data private, or split it into a plain struct without methods', + 'single_method_interface': 'prefer a callable (MoveOnlyFunction / std::function) ' + 'over a single-method interface', +} diff --git a/devtools/code_health/policy.py b/devtools/code_health/policy.py index b78e852fdf..2189b03393 100644 --- a/devtools/code_health/policy.py +++ b/devtools/code_health/policy.py @@ -1,14 +1,35 @@ """Compare changed C++ with its base revision; no stored debt registry.""" +from .config import ADVISORY, HINTS -def evaluate(metrics, previous): +def base_values(previous): old = {} for metric in previous: old[metric.key] = max(old.get(metric.key, 0), metric.value) + return old + + +def evaluate(metrics, previous): + old = base_values(previous) failures = [] for metric in metrics: ceiling = max(metric.limit, old.get(metric.key, metric.limit)) if metric.value > ceiling: failures.append(f'{metric.path}:{metric.line}: {metric.symbol or ""}: ' - f'{metric.check}={metric.value}, limit={metric.limit}, allowed {ceiling}') + f'{metric.check}={metric.value}, limit={metric.limit}, allowed {ceiling}' + + (f'; {HINTS[metric.check]}' if metric.check in HINTS else '')) return failures + + +def advisories(metrics, previous): + """New or changed values above an advisory floor that still pass.""" + old = base_values(previous) + notes = [] + for metric in metrics: + floor = ADVISORY.get(metric.check) + if floor is None or old.get(metric.key) == metric.value: + continue + if floor < metric.value <= max(metric.limit, old.get(metric.key, metric.limit)): + notes.append(f'{metric.path}:{metric.line}: {metric.symbol or ""}: ' + f'{metric.check}={metric.value} (advisory above {floor}, limit {metric.limit})') + return notes diff --git a/devtools/code_health/tests/test_classes.py b/devtools/code_health/tests/test_classes.py new file mode 100644 index 0000000000..adf4e12ac4 --- /dev/null +++ b/devtools/code_health/tests/test_classes.py @@ -0,0 +1,101 @@ +import unittest + +from devtools.code_health.classes import find_classes, measure_classes +from devtools.code_health.metrics import Metric +from devtools.code_health.policy import advisories, evaluate + + +def values(source, symbol=None): + result = {} + for metric in measure_classes('Common/x.h', source): + if symbol is None or metric.symbol == symbol: + result[metric.check] = metric.value + return result + + +def methods(count, extra=''): + return 'class A{\npublic:\n' + extra + ''.join(f' void m{i}();\n' for i in range(count)) + '};\n' + + +class ClassShapeTests(unittest.TestCase): + def test_single_method_interface(self): + for source in ('struct L{ virtual void on(int x) = 0; };', + 'class L{ public: virtual ~L() = default; virtual void on() const = 0; };', + 'class L{\npublic:\n virtual ~L(){}\n virtual void on() = 0;\n};'): + with self.subTest(source=source): + self.assertEqual(1, values(source)['single_method_interface']) + for source in ('struct L{ virtual void on() = 0; virtual void off() = 0; };', + 'struct L : Base{ virtual void on() = 0; };', + 'struct L{ virtual void on(); };', + 'struct L{ virtual void on() = 0; int state; };'): + with self.subTest(source=source): + self.assertEqual(0, values(source)['single_method_interface']) + + def test_public_method_count_excludes_special_members(self): + extra = (' A();\n A(const A&) = delete;\n ~A();\n A& operator=(A&&);\n' + ' operator bool() const;\n void gone() = delete;\nprivate:\n void hidden();\npublic:\n') + self.assertEqual(7, values(methods(7, extra))['public_methods']) + qt = 'class W : public QWidget{\n Q_OBJECT\npublic:\n void a();\nsignals:\n void b();\npublic slots:\n void c();\n};' + self.assertEqual(2, values(qt)['public_methods']) + + def test_public_method_limit_and_advisory(self): + for count, failing, advisory in ((5, False, False), (6, False, True), (7, False, True), (8, True, False)): + with self.subTest(count=count): + metrics = measure_classes('Common/x.h', methods(count)) + self.assertEqual(failing, bool(evaluate(metrics, []))) + self.assertEqual(advisory, bool(advisories(metrics, []))) + + def test_existing_large_class_may_change_without_growing(self): + before = measure_classes('Common/x.h', methods(9)) + self.assertEqual([], evaluate(measure_classes('Common/x.h', methods(9, ' A();\n')), before)) + self.assertEqual([], advisories(measure_classes('Common/x.h', methods(9)), before)) + self.assertTrue(evaluate(measure_classes('Common/x.h', methods(10)), before)) + + def test_public_data_only_fails_with_methods(self): + self.assertEqual(0, values('struct P{ int a; std::string b = "{"; };')['public_data']) + self.assertEqual(1, values('struct P{ int a; void set(int x); };')['public_data']) + self.assertEqual(0, values('class P{ int a; public: void set(int x); };')['public_data']) + exempt = ('struct P{ void f(); static int count; static constexpr int K = 3;\n' + ' BooleanCheckBoxOption ENABLE; EnumDropdownCell mode; Q q; };') + self.assertEqual(1, values(exempt)['public_data']) + self.assertEqual(1, values('struct P{ void f(); std::function callback; };')['public_data']) + + def test_value_types_and_test_fakes_may_expose_data(self): + value = ('struct TilePos{ int x = 0; int y = 0; TilePos next(Direction d) const;\n' + ' static TilePos origin(); bool operator==(const TilePos&) const = default; };') + self.assertEqual(0, values(value)['public_data']) + fake = 'struct Fake{ std::vector calls; void hold(int x){ calls.push_back(x); } };' + self.assertEqual(1, values(fake)['public_data']) + tests = measure_classes('SerialPrograms/Source/Pokemon/X_Tests.cpp', fake) + self.assertEqual([0], [m.value for m in tests if m.check == 'public_data']) + + def test_constructor_initializers_and_inline_bodies(self): + source = ('struct A{\n A(int a) : m_a(a), m_b{2} { if (a){ m_a = 1; } }\n' + ' void f(int x = {}) { for (;;){} }\n void (*fp)(int);\n' + 'private:\n int m_a; int m_b;\n};') + self.assertEqual({'public_methods': 1, 'public_data': 1, 'single_method_interface': 0}, + values(source)) + + def test_qualified_nested_names_and_skipped_declarations(self): + source = ('namespace PA{ namespace Game{\ntemplate void g();\n' + 'class X; enum class E{ A, B }; struct Y{ struct Z{ int a; }; friend class X; };\n}}') + self.assertEqual(['PA::Game::Y', 'PA::Game::Y::Z'], [c.name for c in find_classes(source)]) + + def test_crlf_and_unbalanced_preprocessor_branches_do_not_raise(self): + source = '#if X\nclass A : public B{\n#else\nclass A{\n#endif\npublic:\n void f();\n};\n' + measure_classes('Common/x.h', source) + lf = methods(8) + self.assertEqual(measure_classes('Common/x.h', lf), + measure_classes('Common/x.h', lf.replace('\n', '\r\n'))) + + def test_failure_suggests_a_callable(self): + failures = evaluate(measure_classes('Common/x.h', 'struct L{ virtual void on() = 0; };'), []) + self.assertEqual(1, len(failures)) + self.assertIn('MoveOnlyFunction', failures[0]) + + def test_metrics_are_plain_values(self): + self.assertIsInstance(measure_classes('Common/x.h', methods(1))[0], Metric) + + +if __name__ == '__main__': + unittest.main() diff --git a/devtools/code_health/tests/test_cli.py b/devtools/code_health/tests/test_cli.py index 085bfde16f..401c0c7105 100644 --- a/devtools/code_health/tests/test_cli.py +++ b/devtools/code_health/tests/test_cli.py @@ -130,5 +130,21 @@ def test_default_base_is_merge_base_with_upstream_then_origin(self): self.assertIn(f'Base: {fork_point} (merge-base of HEAD and upstream/main).', result.stdout) + def test_class_shape_applies_to_new_classes_only(self): + interface = 'struct Listener{ virtual void on_event() = 0; };\n' + (self.root / 'Common/old.h').write_text(interface) + self.commit() + with (self.root / 'Common/old.h').open('a') as file: + file.write('int helper();\n') + self.git('add', '.') + self.assertEqual(0, self.cli().returncode) + (self.root / 'Common/new.h').write_text(interface.replace('Listener', 'Fresh')) + self.git('add', '.') + result = self.cli() + self.assertEqual(1, result.returncode, result.stderr) + self.assertIn('Fresh: single_method_interface=1', result.stderr) + self.assertIn('std::function', result.stderr) + + if __name__ == '__main__': unittest.main() From 9e7270e87c90a7ab798c52e97af0a1f580b806d8 Mon Sep 17 00:00:00 2001 From: dayfine Date: Thu, 29 Oct 2026 21:58:09 -0700 Subject: [PATCH 3/4] Code health: keep game modules out of shared code Fail when a change adds an #include of Pokemon/, PokemonBDSP/ or VideoGames/ to Common, CommonFramework or CommonTools. config.py lists game and platform top-level directories; a test keeps the list complete. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AKYzhrMnPK61XHWEch4a6E --- devtools/code_health/README.md | 10 +++++ devtools/code_health/__main__.py | 8 +++- devtools/code_health/config.py | 12 ++++++ devtools/code_health/layering.py | 42 ++++++++++++++++++ devtools/code_health/tests/test_cli.py | 15 +++++++ devtools/code_health/tests/test_layering.py | 48 +++++++++++++++++++++ 6 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 devtools/code_health/layering.py create mode 100644 devtools/code_health/tests/test_layering.py diff --git a/devtools/code_health/README.md b/devtools/code_health/README.md index 855f377c95..b08a59dda1 100644 --- a/devtools/code_health/README.md +++ b/devtools/code_health/README.md @@ -56,6 +56,16 @@ functions. libclang would need a compilation database with Qt include paths to parse these headers; the scan only has to tell access sections, member functions and data apart. Classes split across `#if` branches are skipped. +## Layering + +Files under `Common/`, `SerialPrograms/Source/CommonFramework/` and +`SerialPrograms/Source/CommonTools/` may not add an `#include` of a game +module: `Pokemon/`, `PokemonBDSP/` or `VideoGames/` under +`SerialPrograms/Source`. Consoles, controllers and frameworks (including +`NintendoSwitch/`) are platform modules games build on, not games; `config.py` +lists both groups and a test fails when a new top-level directory is in neither. +Includes already present at the base are not reported. + ## Build and native C++ tests in one command The existing `.github/workflows/cpp-ci-serial-programs-base.yml` already runs diff --git a/devtools/code_health/__main__.py b/devtools/code_health/__main__.py index 45daddfea3..db571c5623 100644 --- a/devtools/code_health/__main__.py +++ b/devtools/code_health/__main__.py @@ -4,6 +4,7 @@ import sys from pathlib import Path +from . import layering from .classes import measure_classes from .git_files import changed_files, default_base, read_revision from .metrics import measure @@ -22,8 +23,11 @@ def run(root, base_ref=None): failures, notes = [], [] advisory = 0 for current, original in changes: - metrics = analyze(current, (root / current).read_text(encoding='utf-8-sig')) - previous = analyze(current, read_revision(root, base_ref, original)) if original else [] + source = (root / current).read_text(encoding='utf-8-sig') + previous_source = read_revision(root, base_ref, original) if original else None + metrics = analyze(current, source) + previous = analyze(current, previous_source) if original else [] + failures.extend(layering.check(current, source, previous_source)) failures.extend(evaluate(metrics, previous)) notes.extend(advisories(metrics, previous)) advisory += sum(m.check == 'file_lines' and 500 < m.value <= 1000 for m in metrics) diff --git a/devtools/code_health/config.py b/devtools/code_health/config.py index 3159f272ad..b1fa3b012c 100644 --- a/devtools/code_health/config.py +++ b/devtools/code_health/config.py @@ -15,3 +15,15 @@ 'single_method_interface': 'prefer a callable (MoveOnlyFunction / std::function) ' 'over a single-method interface', } + +# Layering: shared code may not include game modules. Directory names are relative to +# SOURCE_ROOT, which is also the include root; Common/ lives at the repository root. +SOURCE_ROOT = 'SerialPrograms/Source/' +SHARED_DIRS = ('Common/', SOURCE_ROOT + 'CommonFramework/', SOURCE_ROOT + 'CommonTools/') +# Game-specific programs, inference and data. +GAME_MODULES = ('Pokemon', 'PokemonBDSP', 'VideoGames') +# Frameworks, consoles, controllers and tools that games build on. NintendoSwitch is +# the console layer; shared code including it is not a game dependency. +PLATFORM_MODULES = ('CommandLine', 'CommonFramework', 'CommonTools', 'ComputerPrograms', + 'ControllerInput', 'Controllers', 'GameConsole', 'Integrations', + 'Kernels', 'ML', 'NintendoSwitch', 'Tests') diff --git a/devtools/code_health/layering.py b/devtools/code_health/layering.py new file mode 100644 index 0000000000..45a2f4c91f --- /dev/null +++ b/devtools/code_health/layering.py @@ -0,0 +1,42 @@ +"""Shared code (Common, CommonFramework, CommonTools) may not include game modules.""" +import posixpath +import re + +from .config import GAME_MODULES, SHARED_DIRS, SOURCE_ROOT + + +INCLUDE = re.compile(r'^[ \t]*#[ \t]*include[ \t]*[<"]([^>"]+)[>"]', re.M) + + +def game_module(path, include): + """The game module an include resolves to, trying the include root then the file's dir.""" + candidates = (include, posixpath.normpath(posixpath.join(posixpath.dirname(path), include))) + for candidate in candidates: + candidate = candidate.lstrip('./') if candidate.startswith('../') else candidate + if candidate.startswith(SOURCE_ROOT): + candidate = candidate[len(SOURCE_ROOT):] + top = candidate.split('/', 1) + if len(top) == 2 and top[0] in GAME_MODULES: + return top[0] + return None + + +def game_includes(path, source): + if not path.startswith(SHARED_DIRS) or source is None: + return {} + source = source.replace('\r\n', '\n') + found = {} + for match in INCLUDE.finditer(source): + module = game_module(path, match.group(1)) + if module: + found.setdefault(match.group(1), (source.count('\n', 0, match.start()) + 1, module)) + return found + + +def check(path, source, previous_source): + """Fail includes of game modules that this change adds to shared code.""" + before = game_includes(path, previous_source) + return [f'{path}:{line}: layering: shared code includes game module {module} ("{include}"); ' + 'move the dependency into the game or invert it through an interface/callback' + for include, (line, module) in sorted(game_includes(path, source).items()) + if include not in before] diff --git a/devtools/code_health/tests/test_cli.py b/devtools/code_health/tests/test_cli.py index 401c0c7105..46e1131bb5 100644 --- a/devtools/code_health/tests/test_cli.py +++ b/devtools/code_health/tests/test_cli.py @@ -146,5 +146,20 @@ def test_class_shape_applies_to_new_classes_only(self): self.assertIn('std::function', result.stderr) + def test_shared_code_may_not_add_game_includes(self): + shared = self.root / 'SerialPrograms/Source/CommonTools/Tool.cpp' + shared.parent.mkdir(parents=True) + shared.write_text('#include "Pokemon/Pokemon_Strings.h"\n') + self.commit() + shared.write_text('#include "Pokemon/Pokemon_Strings.h"\n#include "NintendoSwitch/X.h"\n') + self.git('add', '.') + self.assertEqual(0, self.cli().returncode) + shared.write_text('#include "VideoGames/PokemonSV/PokemonSV_Settings.h"\n') + self.git('add', '.') + result = self.cli() + self.assertEqual(1, result.returncode, result.stderr) + self.assertIn('Tool.cpp:1: layering', result.stderr) + + if __name__ == '__main__': unittest.main() diff --git a/devtools/code_health/tests/test_layering.py b/devtools/code_health/tests/test_layering.py new file mode 100644 index 0000000000..937f691df0 --- /dev/null +++ b/devtools/code_health/tests/test_layering.py @@ -0,0 +1,48 @@ +import unittest +from pathlib import Path + +from devtools.code_health.config import GAME_MODULES, PLATFORM_MODULES, SOURCE_ROOT +from devtools.code_health.layering import check + + +REPO = Path(__file__).resolve().parents[3] +SHARED = SOURCE_ROOT + 'CommonTools/Async/Example.cpp' + + +class LayeringTests(unittest.TestCase): + def test_added_game_includes_fail_in_shared_code(self): + for include in ('Pokemon/Inference/Pokemon_NameReader.h', + 'VideoGames/PokemonSV/PokemonSV_Settings.h', + 'PokemonBDSP/PokemonBDSP_Settings.h', + '../../VideoGames/PokemonFRLG/PokemonFRLG_Settings.h'): + with self.subTest(include=include): + failures = check(SHARED, f'#include "{include}"\n', '') + self.assertEqual(1, len(failures)) + self.assertIn(f'{SHARED}:1: layering', failures[0]) + + def test_platform_includes_and_game_code_pass(self): + self.assertEqual([], check(SHARED, '#include "NintendoSwitch/Controllers/X.h"\n' + '#include "Controllers/Controller.h"\n#include \n', '')) + game = SOURCE_ROOT + 'VideoGames/PokemonSV/Programs/X.cpp' + self.assertEqual([], check(game, '#include "Pokemon/Pokemon_Strings.h"\n', '')) + + def test_existing_and_commented_includes_pass(self): + existing = '#include "Pokemon/Inference/Pokemon_NameReader.h"\r\n' + self.assertEqual([], check(SHARED, existing + 'int x;\r\n', existing)) + self.assertEqual([], check(SHARED, '//#include "Pokemon/X.h"\n', '')) + + def test_root_common_and_relative_paths(self): + source = '#include "../SerialPrograms/Source/VideoGames/PokemonLA/X.h"\n' + self.assertEqual(1, len(check('Common/Cpp/X.cpp', source, None))) + + def test_every_top_level_source_directory_is_classified(self): + root = REPO / SOURCE_ROOT + if not root.is_dir(): + self.skipTest('SerialPrograms/Source is not checked out') + directories = {p.name for p in root.iterdir() if p.is_dir()} + self.assertEqual(set(), directories - set(GAME_MODULES) - set(PLATFORM_MODULES)) + self.assertEqual(set(), set(GAME_MODULES) & set(PLATFORM_MODULES)) + + +if __name__ == '__main__': + unittest.main() From 48afe71e8caf53a301bc9839d509606bf95288ad Mon Sep 17 00:00:00 2001 From: dayfine Date: Thu, 29 Oct 2026 21:59:27 -0700 Subject: [PATCH 4/4] Code health: quiet class checks on legacy classes and const fields Runs over origin/main~20..main reported legacy classes far over the public-method limit as advisory, and a public const type field as mutable data. Advise only for 6-7 methods and exempt const fields. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AKYzhrMnPK61XHWEch4a6E --- devtools/code_health/README.md | 2 +- devtools/code_health/classes.py | 9 ++++++--- devtools/code_health/policy.py | 4 ++-- devtools/code_health/tests/test_classes.py | 3 +++ 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/devtools/code_health/README.md b/devtools/code_health/README.md index b08a59dda1..96064f1cbb 100644 --- a/devtools/code_health/README.md +++ b/devtools/code_health/README.md @@ -27,7 +27,7 @@ are included. Missing base revisions and analysis errors fail the command. | Nesting | 4 lexical brace levels inside the function body | | Parameters | 7 | | Public methods per class/struct | 7 (excluding ctors, dtors, operators, deleted and Qt signals); 6-7 advisory | -| Public non-static data in a class with mutating methods | 0 (exempt: plain structs, value types with only const/static methods, `*Option`/`*Cell` fields, test files) | +| Public non-static data in a class with mutating methods | 0 (exempt: plain structs, value types with only const/static methods, const fields, `*Option`/`*Cell` fields, test files) | | Single-method interface (one pure virtual + optional virtual dtor, no bases) | not allowed: prefer a callable (`MoveOnlyFunction` / `std::function`) | New files/functions must meet these limits. In modified files, compare each diff --git a/devtools/code_health/classes.py b/devtools/code_health/classes.py index 504958e0f8..d84622d691 100644 --- a/devtools/code_health/classes.py +++ b/devtools/code_health/classes.py @@ -33,7 +33,7 @@ class Member: deleted: bool = False virtual: bool = False static: bool = False - const: bool = False # a const (or static) member function: does not mutate the object + const: bool = False # const/static method, or const data: cannot mutate the object option: bool = False # a config option or table cell field (type named *Option or *Cell) @@ -175,7 +175,10 @@ def classify(decl, access, class_name): return Member(access, 'data') if declarator else Member(access, 'type') paren = call_paren(decl) if paren is None: - return Member(access, 'data', static='static' in decl, option=is_option(decl)) + # A const value (not a pointer to const) cannot be modified through the public field. + read_only = 'const' in decl and '*' not in decl and '&' not in decl + return Member(access, 'data', static='static' in decl, option=is_option(decl), + const=read_only) name = decl[paren - 1] if 'operator' in decl[:paren]: kind = 'operator' @@ -304,7 +307,7 @@ def measure_classes(path, source): methods = [m for m in info.members if m.kind == 'method' and not m.deleted] public_methods = sum(m.access == 'public' for m in methods) mutators = [m for m in methods if not m.const] - public_data = sum(m.access == 'public' and m.kind == 'data' and not (m.static or m.option) + public_data = sum(m.access == 'public' and m.kind == 'data' and not (m.static or m.option or m.const) for m in info.members) if mutators and not is_test_file(path) else 0 values = ( ('public_methods', public_methods), diff --git a/devtools/code_health/policy.py b/devtools/code_health/policy.py index 2189b03393..942de09074 100644 --- a/devtools/code_health/policy.py +++ b/devtools/code_health/policy.py @@ -22,14 +22,14 @@ def evaluate(metrics, previous): def advisories(metrics, previous): - """New or changed values above an advisory floor that still pass.""" + """New or changed values between an advisory floor and the limit; legacy excess is not repeated.""" old = base_values(previous) notes = [] for metric in metrics: floor = ADVISORY.get(metric.check) if floor is None or old.get(metric.key) == metric.value: continue - if floor < metric.value <= max(metric.limit, old.get(metric.key, metric.limit)): + if floor < metric.value <= metric.limit: notes.append(f'{metric.path}:{metric.line}: {metric.symbol or ""}: ' f'{metric.check}={metric.value} (advisory above {floor}, limit {metric.limit})') return notes diff --git a/devtools/code_health/tests/test_classes.py b/devtools/code_health/tests/test_classes.py index adf4e12ac4..e88c09ea5f 100644 --- a/devtools/code_health/tests/test_classes.py +++ b/devtools/code_health/tests/test_classes.py @@ -49,6 +49,7 @@ def test_existing_large_class_may_change_without_growing(self): before = measure_classes('Common/x.h', methods(9)) self.assertEqual([], evaluate(measure_classes('Common/x.h', methods(9, ' A();\n')), before)) self.assertEqual([], advisories(measure_classes('Common/x.h', methods(9)), before)) + self.assertEqual([], advisories(measure_classes('Common/x.h', methods(8)), before)) self.assertTrue(evaluate(measure_classes('Common/x.h', methods(10)), before)) def test_public_data_only_fails_with_methods(self): @@ -59,6 +60,8 @@ def test_public_data_only_fails_with_methods(self): ' BooleanCheckBoxOption ENABLE; EnumDropdownCell mode; Q q; };') self.assertEqual(1, values(exempt)['public_data']) self.assertEqual(1, values('struct P{ void f(); std::function callback; };')['public_data']) + read_only = 'class D{ public: const Type type; const char* name; virtual void run(); };' + self.assertEqual(1, values(read_only)['public_data']) def test_value_types_and_test_fakes_may_expose_data(self): value = ('struct TilePos{ int x = 0; int y = 0; TilePos next(Direction d) const;\n'