diff --git a/AGENTS.md b/AGENTS.md index 06fdd488d2..dc5457c54d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -293,9 +293,9 @@ Both app-level (`Core::Settings`) and session-level (`Main::SessionSettings`) us ## Coding Style -**Do NOT write useless comments in code:** +**Comments are rationed:** -This is important! Do not write single-line comments that describe what the next line does - they are bloat. Comments are allowed ONLY to describe complex algorithms in detail, when the explanation requires at least 4-5 lines. Self-documenting code with clear variable and function names is preferred. +A comment is one line; two or three only when the block opens with `// WHY:`. A commit may add two comment lines plus one such exception, and a trailing comment is a line too; only `} // namespace X` closers and `#endif // X` labels are free. Say why, never what. Hooks enforce it, and `Telegram/SourceFiles/test/` is exempt. Do not remove existing comments just to satisfy this rule. Preserve comments unless your change makes them incorrect or truly obsolete; when moving or refactoring code, move the useful comment with it. Inline comments that label positional arguments for generated or schema-driven APIs (for example TL/MTP constructors) are useful because the field names are not visible in the call itself. @@ -310,12 +310,14 @@ if (user->isPremium()) { auto name = user->name(); if (user->isPremium()) { -// ACCEPTABLE - complex algorithm explanation (4+ lines): -// The algorithm works by first collecting all visible messages -// in the viewport, then calculating their intersection with -// the clip rectangle. Messages are grouped by date headers, -// and we need to account for sticky headers that may overlap -// with the first message in each group. +// ACCEPTABLE - one line, and it carries a reason: +_limit = kDefaultLimit; // the server rejects anything larger + +// ACCEPTABLE - the exception: opens with WHY, three lines at most, +// once per commit. +// WHY: the server sends the id before the peer exists, so the row is +// created empty and filled on the next update, or the list flickers +// on every reconnect. ``` **Style and formatting rules** are in `REVIEW.md` — see that file for empty-line-before-closing-brace, operator placement in multi-line expressions, if-with-initializer, and other mechanical style rules. diff --git a/tools/rules/hook.py b/tools/rules/hook.py index eb6ae5ca09..287b36c948 100644 --- a/tools/rules/hook.py +++ b/tools/rules/hook.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Refuse a function .clang-tidy bans: as an edit hook, or --staged.""" +"""Refuse what the house rules ban: as an edit hook, or --staged.""" import json import os import re @@ -9,6 +9,12 @@ import sys HERE = os.path.dirname(os.path.abspath(__file__)) ROOT = os.path.dirname(os.path.dirname(HERE)) SOURCES = (".cpp", ".h", ".mm", ".cxx") +LICENSE = "This file is part of " +HARNESS = "/SourceFiles/test/" +MARKER = "// WHY:" +MARKED_LINES = 3 +PLAIN_LINES = 2 +MARKED_BLOCKS = 1 def banned(): @@ -17,6 +23,87 @@ def banned(): return re.findall(r"\^::(\w+)\$,([^,;]+),", stream.read()) +def hollow(match): + """The literal's quotes around filler, so offsets stay put.""" + quoted = match.group() + return quoted[0] + "_" * (len(quoted) - 2) + quoted[-1] + + +def blanked(line): + line = re.sub(r'"(?:[^"\\]|\\.)*"', hollow, line) + return re.sub(r"'(?:[^'\\]|\\.)*'", hollow, line) + + +def without_comments(text): + text = "\n".join(blanked(line) for line in text.split("\n")) + text = re.sub(r"//[^\n]*", "", text) + return re.sub(r"/\*.*?\*/", "", text, flags=re.S) + + +def comment_start(line): + """Offset of the first comment marker outside literals, or -1.""" + body = blanked(line) + found = [at for at in (body.find("//"), body.find("/*")) if at >= 0] + return min(found) if found else -1 + + +def comment_text(line): + """What a moved comment keeps: its text, not the code beside it.""" + at = comment_start(line) + return line[at:].strip() if at >= 0 else line.strip() + + +def classify(line, inside): + """("full" | "tail" | None, inside a /* */ block after this line).""" + body = blanked(line) + if inside: + return "full", "*/" not in body + at = comment_start(line) + if at < 0: + return None, False + opens = body[at:].startswith("/*") and "*/" not in body[at + 2:] + code = body[:at].strip() + if code == "}" or code.startswith("#"): + return None, opens # a closer or directive may carry its label + return ("tail" if code else "full"), opens + + +def blocks(numbered): + """Comment blocks as (line number, lines): runs of whole-line + comments, and every trailing comment on its own.""" + out, run, first, previous, inside = [], [], 0, None, False + for number, line in numbered: + gap = previous is not None and number != previous + 1 + was = inside and not gap + kind, inside = classify(line, was) + if run and (gap or kind != "full"): + out.append((first, run)) + run = [] + if kind == "full" or (kind == "tail" and inside): + first = first if run else number + run.append(line) + if was and not inside: + out.append((first, run)) # a closed /* */ ends its block + run = [] + elif kind == "tail": + out.append((number, [line])) + previous = number + if run: + out.append((first, run)) + return out + + +def fault(block): + """Why this comment block breaks the rule, or None.""" + if any(LICENSE in line for line in block): + return None + if block[0].strip().startswith(MARKER): + return (f"a {MARKER} block runs {MARKED_LINES} lines at most" + if len(block) > MARKED_LINES else None) + return (f"a comment is one line; up to {MARKED_LINES} only when it opens" + f" with {MARKER}" if len(block) > 1 else None) + + def written(payload): """The text this call put into the file, across all the edit shapes.""" tool = payload.get("tool_input") or {} @@ -26,40 +113,89 @@ def written(payload): return "\n".join(parts) -def code_only(text): - """The same text with comments and string literals taken out.""" - text = re.sub(r"//[^\n]*", "", text) - text = re.sub(r"/\*.*?\*/", "", text, flags=re.S) - return re.sub(r'"(?:[^"\\]|\\.)*"', '""', text) +def replaced(payload, path): + """What was there before, so moved code is not read as freshly written.""" + tool = payload.get("tool_input") or {} + parts = [tool.get("old_string") or ""] + parts += [edit.get("old_string") or "" + for edit in tool.get("edits") or []] + if tool.get("content"): + # Relative to the file, so a submodule answers for its own files. + parts.append(subprocess.run( + ["git", "show", "HEAD:./" + os.path.basename(path)], + cwd=os.path.dirname(path) or ".", capture_output=True, + text=True).stdout) + return "\n".join(parts) -def complain(path, found, where): +def check_edit(payload, path): + text = written(payload) + found = [(name, into) for name, into in banned() + if re.search(r"\b" + name + r"\s*\(", without_comments(text))] for name, into in found: - print(f"{where}{name} is banned in this repository -- write {into}" - " instead.", file=sys.stderr) - print(f"The rule lives in .clang-tidy. Fix {os.path.basename(path)} now," - " before anything else.", file=sys.stderr) + print(f"{name} is banned in this repository -- write {into} instead.", + file=sys.stderr) + was = replaced(payload, path) + for _, block in ([] if HARNESS in path + else blocks(list(enumerate(text.split("\n"))))): + why = fault(block) + if why and "\n".join(block) not in was: + found.append(True) + print(f"{len(block)}-line comment: {why}.", file=sys.stderr) + if not found: + return 0 + print(f"The rules live in .clang-tidy and AGENTS.md. Fix" + f" {os.path.basename(path)} now, before anything else.", + file=sys.stderr) + return 2 def staged(): - """Every banned name added by the staged change, with its line.""" - diff = subprocess.run(["git", "diff", "--cached", "-U0"], cwd=ROOT, + # No cwd: the hook answers for the repository it was invoked in. + diff = subprocess.run(["git", "diff", "--cached", "-U0", "-M"], capture_output=True, text=True).stdout - path, number, bad = "", 0, [] + path, number, added, dropped, bad = "", 0, {}, set(), 0 for line in diff.split("\n"): if line.startswith("+++ b/"): path, number = line[6:], 0 elif line.startswith("@@"): number = int(re.search(r"\+(\d+)", line).group(1)) - 1 + elif line.startswith("-") and not line.startswith("---"): + dropped.add(comment_text(line[1:])) elif line.startswith("+") and not line.startswith("+++"): number += 1 - if not path.endswith(SOURCES): + if path.endswith(SOURCES): + added.setdefault(path, []).append((number, line[1:])) + plain, marked = 0, 0 + for path, lines in added.items(): + for name, into in banned(): + for number, line in lines: + if re.search(r"\b" + name + r"\s*\(", without_comments(line)): + print(f"{path}:{number}: {name} is banned -- write" + f" {into} instead.", file=sys.stderr) + bad += 1 + for number, block in ([] if HARNESS in "/" + path + else blocks(lines)): + if any(LICENSE in one for one in block): continue - found = [(name, into) for name, into in banned() - if re.search(r"\b" + name + r"\s*\(", code_only(line[1:]))] - if found: - complain(path, found, f"{path}:{number}: ") - bad += found + if all(comment_text(one) in dropped for one in block): + continue + why = fault(block) + if why: + print(f"{path}:{number}: {why}.", file=sys.stderr) + bad += 1 + elif block[0].strip().startswith(MARKER): + marked += 1 + else: + plain += len(block) + if plain > PLAIN_LINES: + print(f"this commit adds {plain} comment lines; {PLAIN_LINES} is the" + " budget, and one marked block on top of it.", file=sys.stderr) + bad += 1 + if marked > MARKED_BLOCKS: + print(f"this commit takes {marked} {MARKER} exceptions;" + f" {MARKED_BLOCKS} is the budget.", file=sys.stderr) + bad += 1 return 1 if bad else 0 @@ -72,15 +208,7 @@ def main(): return 0 path = ((payload.get("tool_response") or {}).get("filePath") or (payload.get("tool_input") or {}).get("file_path") or "") - if not path.endswith(SOURCES): - return 0 - text = code_only(written(payload)) - found = [(name, into) for name, into in banned() - if re.search(r"\b" + name + r"\s*\(", text)] - if not found: - return 0 - complain(path, found, "") - return 2 + return check_edit(payload, path) if path.endswith(SOURCES) else 0 if __name__ == "__main__":