diff --git a/.github/workflows/test-linting.yml b/.github/workflows/test-linting.yml index f212dd9d15e1..0f07d0a61940 100644 --- a/.github/workflows/test-linting.yml +++ b/.github/workflows/test-linting.yml @@ -73,6 +73,12 @@ jobs: run: | uv run --no-sync python scripts/ruff_strict_gate.py --base "$BASE_SHA" + - name: Check type-discipline budget (casts / type guards, delta vs base) + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + uv run --no-sync python scripts/type_discipline_gate.py --base "$BASE_SHA" + - name: Print OpenAI version run: | uv run --no-sync python -c "import openai; print(f'OpenAI version: {openai.__version__}')" diff --git a/ruff-strict-budget.json b/ruff-strict-budget.json index 6363b72353f9..16fe0a004e73 100644 --- a/ruff-strict-budget.json +++ b/ruff-strict-budget.json @@ -8,5 +8,5 @@ "PLR0913": { "baseline": 1813, "slack": 3 }, "PLW0603": { "baseline": 183, "slack": 3 }, "RUF012": { "baseline": 158, "slack": 3 }, - "TID251": { "baseline": 2404, "slack": 10 } + "TID251": { "baseline": 2662, "slack": 10 } } diff --git a/ruff-strict.toml b/ruff-strict.toml index 03145255ebf5..fb68b60d4133 100644 --- a/ruff-strict.toml +++ b/ruff-strict.toml @@ -17,4 +17,16 @@ max-args = 5 "typing.Dict".msg = "Frozen dataclass / NamedTuple / ReadOnly TypedDict; create a Mapping alias with concrete value types if truly dynamic." "typing.Set".msg = "frozenset[X] or AbstractSet[X]." "typing.MutableSequence".msg = "Sequence[X]." -"typing.MutableMapping".msg = "See typing.Dict." \ No newline at end of file +"typing.MutableMapping".msg = "See typing.Dict." +# Unchecked casts: cast() lies to the type checker with no runtime guarantee. +# Validate into a concrete frozen type at the boundary (msgspec/pydantic) instead. +# Per-call-site coverage lives in check_type_discipline.py (LIT006); this freezes +# new cast imports. Suppress (with a reason) via `# noqa: TID251 # `. +"typing.cast".msg = "No unchecked casts: validate into a frozen dataclass/NamedTuple/ReadOnly TypedDict at the boundary (msgspec/pydantic)." +"typing_extensions.cast".msg = "Same as typing.cast." +# Unverified narrowing predicates: the checker never validates the guard body, so a +# wrong guard silently corrupts types. Banned outright (there are none today). +"typing.TypeGuard".msg = "Unverified narrowing. Parse into a concrete type, or use isinstance for a runtime-checked narrowing." +"typing_extensions.TypeGuard".msg = "Same as typing.TypeGuard." +"typing.TypeIs".msg = "Unverified narrowing (the body is trusted). Parse into a concrete type instead." +"typing_extensions.TypeIs".msg = "Same as typing.TypeIs." \ No newline at end of file diff --git a/scripts/check_type_discipline.py b/scripts/check_type_discipline.py new file mode 100644 index 000000000000..0949fceed5f9 --- /dev/null +++ b/scripts/check_type_discipline.py @@ -0,0 +1,334 @@ +#!/usr/bin/env python3 +"""Type-discipline checker: the rules ruff can't enforce. + +Rules +----- +LIT001 Coarse builtin annotation (dict/list/set, bare or parameterized) at an + interface: function parameters, return types, or class-body attributes. + Locals are intentionally not checked. + Suppress with `# coarse-ok: ` on the offending line. +LIT002 *args/**kwargs without an annotation. (Defense-in-depth over ANN002/003; + Unpack[...], P.args/P.kwargs, or a concrete type all pass.) +LIT003 noqa suppression without rule codes or without a reason. + Required shape: `# noqa: TID251 # ` +LIT004 type/pyright/mypy ignore without bracketed codes or without a reason. + Required shape: `# pyright: ignore[reportArgumentType] # ` +LIT005 A `# coarse-ok` / `# cast-ok` / `# guard-ok` suppression without a reason. +LIT006 `cast(...)` call. typing.cast is an unchecked assertion (the moral equivalent + of TypeScript's `as`); it lies to the type checker with zero runtime guarantee. + Validate into a concrete frozen type at the boundary instead. + Suppress with `# cast-ok: ` on the call's first line. +LIT007 `TypeGuard[...]` / `TypeIs[...]` annotation. The narrowing predicate's body is + never verified by the checker, so a wrong guard silently corrupts types. + Prefer parsing into a concrete type. Suppress with `# guard-ok: `. + +Usage +----- + python check_type_discipline.py litellm/ tests/ + python check_type_discipline.py --changed-only file1.py file2.py + +Exit code 1 if any violation is found. Stdlib only. +""" + +from __future__ import annotations + +import ast +import re +import sys +import tokenize +from dataclasses import dataclass +from pathlib import Path +from collections.abc import Iterable, Iterator, Mapping, Sequence +from typing import NamedTuple + +BANNED_BUILTINS = frozenset({"dict", "list", "set"}) +UNSAFE_GUARDS = frozenset({"TypeGuard", "TypeIs"}) +MIN_REASON_LEN = 3 + +NOQA_RE = re.compile( + r"#\s*noqa" + r"(?P:\s*(?P[A-Z]+[0-9]+(?:\s*,\s*[A-Z]+[0-9]+)*))?" + r"(?P.*)", + re.IGNORECASE, +) +IGNORE_RE = re.compile( + r"#\s*(?:type|pyright|mypy):\s*ignore(?P\[[^\]]*\])?(?P.*)" +) +COARSE_OK_RE = re.compile(r"#\s*coarse-ok(?::\s*(?P.*))?") +CAST_OK_RE = re.compile(r"#\s*cast-ok(?::\s*(?P.*))?") +GUARD_OK_RE = re.compile(r"#\s*guard-ok(?::\s*(?P.*))?") + +# Suppression tokens that must each carry a reason (LIT005). +OK_SUPPRESSIONS: tuple[tuple[str, re.Pattern[str]], ...] = ( + ("coarse-ok", COARSE_OK_RE), + ("cast-ok", CAST_OK_RE), + ("guard-ok", GUARD_OK_RE), +) # coarse-ok: + + +class Violation(NamedTuple): + path: Path + line: int + code: str + message: str + + def render(self) -> str: + return f"{self.path}:{self.line}: {self.code} {self.message}" + + +@dataclass(frozen=True, slots=True) +class Comments: + """Per-line comment text, plus the lines carrying each valid `*-ok` suppression.""" + + # Mapping, not dict: keys are line numbers, genuinely dynamic. Read-only downstream. + by_line: Mapping[int, str] + coarse_ok_lines: frozenset[int] + cast_ok_lines: frozenset[int] + guard_ok_lines: frozenset[int] + + +# --------------------------------------------------------------------------- # +# Comment scanning (LIT003 / LIT004 / LIT005) +# --------------------------------------------------------------------------- # + + +def _reason_of(rest: str) -> str: + return rest.strip().lstrip("#-").strip() + + +def _valid_ok(regex: re.Pattern[str], text: str) -> bool: + """True iff `text` carries this suppression with a reason of usable length.""" + m = regex.search(text) + return bool(m) and len((m.group("reason") or "").strip()) >= MIN_REASON_LEN + + +def _comment_violations(path: Path, line_no: int, text: str) -> Iterator[Violation]: + """Pure: all LIT003/004/005 findings for one comment.""" + for token, regex in OK_SUPPRESSIONS: + m = regex.search(text) + if m and len((m.group("reason") or "").strip()) < MIN_REASON_LEN: + yield Violation(path, line_no, "LIT005", f"{token} requires a reason: `# {token}: `") + + m = NOQA_RE.search(text) + if m: + if not m.group("codes"): + yield Violation(path, line_no, "LIT003", "noqa requires rule codes: `# noqa: XXX123 # `") + elif len(_reason_of(m.group("rest"))) < MIN_REASON_LEN: + yield Violation(path, line_no, "LIT003", "noqa requires a reason: `# noqa: XXX123 # `") + + m = IGNORE_RE.search(text) + if m: + codes = m.group("codes") + if not codes or codes == "[]": + yield Violation(path, line_no, "LIT004", + "ignore requires codes: `# pyright: ignore[ruleName] # `") + elif len(_reason_of(m.group("rest"))) < MIN_REASON_LEN: + yield Violation(path, line_no, "LIT004", + "ignore requires a reason: `# pyright: ignore[ruleName] # `") + + +def scan_comments(path: Path, source: str) -> tuple[Comments, tuple[Violation, ...]]: + try: + tokens = tokenize.generate_tokens(iter(source.splitlines(keepends=True)).__next__) + comment_toks = tuple((t.start[0], t.string) for t in tokens if t.type == tokenize.COMMENT) + except tokenize.TokenError: + return Comments({}, frozenset(), frozenset(), frozenset()), () + + def _lines_with(regex: re.Pattern[str]) -> frozenset[int]: + return frozenset(line for line, text in comment_toks if _valid_ok(regex, text)) + + return ( + Comments( + by_line={line: text for line, text in comment_toks}, + coarse_ok_lines=_lines_with(COARSE_OK_RE), + cast_ok_lines=_lines_with(CAST_OK_RE), + guard_ok_lines=_lines_with(GUARD_OK_RE), + ), + tuple(v for line, text in comment_toks for v in _comment_violations(path, line, text)), + ) + + +# --------------------------------------------------------------------------- # + + +def banned_names_in(annotation: ast.expr) -> Iterator[str]: + """Yield banned builtin names anywhere inside an annotation expression. + + Handles nesting (Optional[dict[str, str]], tuple[list[int], ...]) and + string forward references. + """ + for node in ast.walk(annotation): + if isinstance(node, ast.Name) and node.id in BANNED_BUILTINS: + yield node.id + elif isinstance(node, ast.Constant) and isinstance(node.value, str): + try: + inner = ast.parse(node.value, mode="eval").body + except SyntaxError: + continue + yield from banned_names_in(inner) + + +def _coarse(path: Path, line: int, name: str, where: str) -> Violation: + return Violation( + path, line, "LIT001", + f"coarse `{name}` annotation in {where}; use a frozen dataclass, " + f"NamedTuple, ReadOnly TypedDict, tuple[X, ...], frozenset[X], or Sequence[X] " + f"(suppress: `# coarse-ok: `)", + ) + + +def _annotation_violations( + path: Path, annotation: ast.expr | None, line: int, where: str, ok_lines: frozenset[int] +) -> Iterator[Violation]: + if annotation is None or line in ok_lines: + return + yield from (_coarse(path, line, name, where) for name in banned_names_in(annotation)) + + +def _function_violations( + path: Path, node: ast.FunctionDef | ast.AsyncFunctionDef, ok_lines: frozenset[int] +) -> Iterator[Violation]: + args = node.args + for arg in (*args.posonlyargs, *args.args, *args.kwonlyargs): + yield from _annotation_violations( + path, arg.annotation, arg.lineno, f"parameter `{arg.arg}` of `{node.name}`", ok_lines + ) + + for star, label in ((args.vararg, "*args"), (args.kwarg, "**kwargs")): + if star is None: + continue + if star.annotation is None: + if star.lineno not in ok_lines: + yield Violation( + path, star.lineno, "LIT002", + f"unannotated `{label}` in `{node.name}`; annotate with " + f"Unpack[SomeTypedDict], P.args/P.kwargs, or a concrete type", + ) + else: + yield from _annotation_violations( + path, star.annotation, star.lineno, f"`{label}` of `{node.name}`", ok_lines + ) + + if node.returns is not None: + yield from _annotation_violations( + path, node.returns, node.returns.lineno, f"return type of `{node.name}`", ok_lines + ) + + +def _class_violations(path: Path, node: ast.ClassDef, ok_lines: frozenset[int]) -> Iterator[Violation]: + for stmt in node.body: + if isinstance(stmt, ast.AnnAssign): + target = stmt.target.id if isinstance(stmt.target, ast.Name) else "" + yield from _annotation_violations( + path, stmt.annotation, stmt.lineno, + f"attribute `{target}` of class `{node.name}`", ok_lines, + ) + + +def iter_interface_violations(path: Path, tree: ast.AST, comments: Comments) -> Iterator[Violation]: + ok = comments.coarse_ok_lines + for node in ast.walk(tree): + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + yield from _function_violations(path, node, ok) + elif isinstance(node, ast.ClassDef): + yield from _class_violations(path, node, ok) + + +# --------------------------------------------------------------------------- # +# Unchecked casts (LIT006) and unverified narrowing predicates (LIT007) +# --------------------------------------------------------------------------- # + + +def _is_cast_call(node: ast.Call) -> bool: + """`cast(...)` or `typing.cast(...)`, however the name was imported/aliased. + + Name-based like BANNED_BUILTINS: a stray method called `.cast()` is a rare + false positive, suppressible with `# cast-ok: `. + """ + func = node.func + return (isinstance(func, ast.Name) and func.id == "cast") or ( + isinstance(func, ast.Attribute) and func.attr == "cast" + ) + + +def iter_cast_violations(path: Path, tree: ast.AST, comments: Comments) -> Iterator[Violation]: + for node in ast.walk(tree): + if isinstance(node, ast.Call) and _is_cast_call(node) and node.lineno not in comments.cast_ok_lines: + yield Violation( + path, node.lineno, "LIT006", + "cast() is an unchecked assertion (the type checker takes it on faith); " + "validate into a frozen dataclass/NamedTuple/ReadOnly TypedDict at the " + "boundary instead (suppress: `# cast-ok: `)", + ) + + +def iter_guard_violations(path: Path, tree: ast.AST, comments: Comments) -> Iterator[Violation]: + # The `from typing import TypeGuard` line is an ast.alias, not a Name/Attribute, + # so only *uses* (e.g. `-> TypeGuard[int]`) are flagged here; ruff bans the import. + for node in ast.walk(tree): + if not isinstance(node, (ast.Name, ast.Attribute)): + continue + name = node.id if isinstance(node, ast.Name) else node.attr + if name in UNSAFE_GUARDS and node.lineno not in comments.guard_ok_lines: + yield Violation( + path, node.lineno, "LIT007", + f"`{name}` narrowing predicate: the checker never verifies the body, so a " + f"wrong guard silently corrupts types; parse into a concrete type instead " + f"(suppress: `# guard-ok: `)", + ) + + +# --------------------------------------------------------------------------- # +# Driver +# --------------------------------------------------------------------------- # + + +def check_file(path: Path) -> tuple[Violation, ...]: + try: + source = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError) as exc: + return (Violation(path, 0, "LIT000", f"could not read file: {exc}"),) + + comments, violations = scan_comments(path, source) + + try: + tree = ast.parse(source, filename=str(path)) + except SyntaxError as exc: + return (*violations, Violation(path, exc.lineno or 0, "LIT000", f"syntax error: {exc.msg}")) + + return ( + *violations, + *iter_interface_violations(path, tree, comments), + *iter_cast_violations(path, tree, comments), + *iter_guard_violations(path, tree, comments), + ) + + +def collect_paths(raw: Iterable[str]) -> Iterator[Path]: + for item in raw: + p = Path(item) + if p.is_dir(): + yield from sorted(p.rglob("*.py")) + elif p.suffix == ".py": + yield p + + +def main(argv: Sequence[str]) -> int: + paths = [a for a in argv if not a.startswith("-")] + if not paths: + print("usage: check_type_discipline.py ...", file=sys.stderr) + return 2 + + violations = sorted(v for path in collect_paths(paths) for v in check_file(path)) + for v in violations: + print(v.render()) + + if violations: + print(f"\n{len(violations)} violation(s).", file=sys.stderr) + return 1 + return 0 + + +if __name__ == "__main__": + raise SystemExit(main(sys.argv[1:])) + \ No newline at end of file diff --git a/scripts/type_discipline_gate.py b/scripts/type_discipline_gate.py new file mode 100644 index 000000000000..847bdcaafd15 --- /dev/null +++ b/scripts/type_discipline_gate.py @@ -0,0 +1,171 @@ +#!/usr/bin/env python3 +"""Total-count gate for the LIT* rules in scripts/check_type_discipline.py. + +Sibling of scripts/ruff_strict_gate.py. Each rule listed in +type-discipline-budget.json has a hard ceiling (baseline + slack). The gate counts +each rule across the whole `litellm` tree and fails when a rule is both over its +ceiling and higher than the base it merges into, so a change is blamed for the +violations it adds, never for drift that already exists in the base. + +Rules not present in the budget are ignored, so the checker can emit LIT001-005 +without gating them here; today only LIT006 (cast) and LIT007 (TypeGuard/TypeIs) +are budgeted. Re-baseline with `--update` to ratchet a ceiling down. +""" + +import argparse +import json +import re +import shutil +import subprocess +import sys +import tempfile +from collections import Counter +from pathlib import Path +from typing import NamedTuple + +REPO_ROOT = Path(__file__).resolve().parent.parent +CHECKER = REPO_ROOT / "scripts" / "check_type_discipline.py" +BUDGET_PATH = REPO_ROOT / "type-discipline-budget.json" +TARGET = "litellm" +DEFAULT_BASE = "origin/litellm_internal_staging" + +_HUNK = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") +_LINE = re.compile(r"^(?P.+?):(?P\d+): (?PLIT\d+) ") + + +class Violation(NamedTuple): + file: str + line: int + code: str + + +class Breach(NamedTuple): + rule: str + total: int + cap: int + added: int + + +def _run(cmd: list, cwd: Path = REPO_ROOT) -> str: + proc = subprocess.run(cmd, cwd=cwd, capture_output=True, text=True) + if proc.returncode not in (0, 1): + sys.stderr.write(proc.stderr) + raise SystemExit(f"{cmd[0]} exited {proc.returncode}") + return proc.stdout + + +def _check(root: Path, checker: Path) -> list: + # Resolve root first: on macOS tempfile dirs (/var/...) resolve to /private/var/..., + # and the checker prints already-resolved absolute paths, so relative_to would fail. + root = root.resolve() + out = _run([sys.executable, str(checker), str(root / TARGET)], cwd=root) + found = [] + for line in out.splitlines(): + m = _LINE.match(line) + if m is None: + continue + name = Path(m.group("file")) + full = name if name.is_absolute() else root / name + rel = full.resolve().relative_to(root).as_posix() + found.append(Violation(rel, int(m.group("line")), m.group("code"))) + return found + + +def head_violations() -> list: + return _check(REPO_ROOT, CHECKER) + + +def count_by_rule(violations: list) -> dict: + return dict(Counter(v.code for v in violations)) + + +def base_counts(ref: str) -> dict: + parent = Path(tempfile.mkdtemp(prefix="lit_base_")) + worktree = parent / "wt" + try: + _run(["git", "worktree", "add", "--detach", str(worktree), ref]) + # Measure the base with the *current* rule logic, not whatever shipped at base. + (worktree / "scripts").mkdir(parents=True, exist_ok=True) + checker = worktree / "scripts" / "check_type_discipline.py" + shutil.copy(CHECKER, checker) + return count_by_rule(_check(worktree, checker)) + finally: + _run(["git", "worktree", "remove", "--force", str(worktree)]) + shutil.rmtree(parent, ignore_errors=True) + + +def evaluate(head: dict, base: dict, budget: dict) -> list: + breaches = [] + for rule, spec in budget.items(): + cap = spec["baseline"] + spec["slack"] + total = head.get(rule, 0) + if total > cap and total > base.get(rule, 0): + breaches.append(Breach(rule, total, cap, total - base.get(rule, 0))) + return sorted(breaches) + + +def parse_changed_lines(diff_text: str) -> dict: + changed: dict = {} + path = None + for line in diff_text.splitlines(): + if line.startswith("+++ b/"): + path = line[6:] + elif path and (match := _HUNK.match(line)): + start = int(match.group(1)) + count = int(match.group(2)) if match.group(2) is not None else 1 + changed.setdefault(path, set()).update(range(start, start + count)) + return changed + + +def introduced(violations: list, changed: dict) -> list: + return [v for v in violations if v.line in changed.get(v.file, set())] + + +def cmd_check(base: str) -> None: + budget = json.loads(BUDGET_PATH.read_text()) + head = head_violations() + base_point = _run(["git", "merge-base", base, "HEAD"]).strip() or base + breaches = evaluate(count_by_rule(head), base_counts(base_point), budget) + if not breaches: + print(f"OK: every LIT rule is within its codebase ceiling (base {base})") + return + new = introduced( + head, + parse_changed_lines( + _run(["git", "diff", base_point, "--unified=0", "--no-color", "--", TARGET]) + ), + ) + print(f"FAIL: LIT-rule totals exceed their ceiling (base {base}):") + for breach in breaches: + print( + f" {breach.rule}: total {breach.total} over cap {breach.cap} (this change added {breach.added})" + ) + for violation in sorted(v for v in new if v.code == breach.rule): + print(f" {violation.file}:{violation.line}") + print( + "Remove the new violations, justify each with `# cast-ok: ` / " + "`# guard-ok: `, or remove an equal number elsewhere; the ceiling is " + "baseline + slack in type-discipline-budget.json." + ) + raise SystemExit(1) + + +def cmd_update() -> None: + budget = json.loads(BUDGET_PATH.read_text()) + head = count_by_rule(head_violations()) + for rule in budget: + budget[rule]["baseline"] = head.get(rule, 0) + BUDGET_PATH.write_text(json.dumps(budget, indent=2, sort_keys=True) + "\n") + print("Re-captured per-rule baselines from the current tree") + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--base", default=DEFAULT_BASE) + parser.add_argument("--update", action="store_true") + args = parser.parse_args() + cmd_update() if args.update else cmd_check(args.base) + + +if __name__ == "__main__": + main() diff --git a/type-discipline-budget.json b/type-discipline-budget.json new file mode 100644 index 000000000000..2437f47d3d7a --- /dev/null +++ b/type-discipline-budget.json @@ -0,0 +1,10 @@ +{ + "LIT006": { + "baseline": 1013, + "slack": 10 + }, + "LIT007": { + "baseline": 0, + "slack": 0 + } +}