diff --git a/.github/workflows/drift-guard.yml b/.github/workflows/drift-guard.yml index 71ef64059..1d23102e7 100644 --- a/.github/workflows/drift-guard.yml +++ b/.github/workflows/drift-guard.yml @@ -8,8 +8,14 @@ name: Drift guard # which is the same shape of defect one level down: a control that parses, looks # applied, and measures nothing. # -# Sections below: the spec <-> code gates and the NOLINT-directive gate, the -# ladder rung-list gate, then the scenario-coverage gate. +# And one for the class below that again: a character that is not there to be +# read at all (the raw-bidi-control scan, #628). Same family -- a file that +# looks correct to every reader while saying something else -- which is why it +# rides prose-lint alongside the NOLINT scan rather than living elsewhere. +# +# Sections below: the spec <-> code gates, the NOLINT-directive gate and the +# raw-bidi-control gate, the ladder rung-list gate, then the scenario-coverage +# gate. # # Two independent gates for the "spec <-> code drift" class of bug (a # docs/spec/*.md file stating a mechanical fact -- an enum cardinality, a @@ -202,6 +208,45 @@ jobs: - name: Check every NOLINTNEXTLINE annotates a line of code run: bash scripts/check_nolint_directives.sh + # ── Raw bidi control characters in first-party files (#628) ─────────── + # A bidirectional control renders as nothing and occupies no column, so a + # diff containing one looks exactly like a diff that does not. In a + # string literal that is a silently wrong assertion (#610's subject: nine + # lines of src/qt/forms/tests/tst_i18n.qml whose expected values held raw + # U+200E / U+200F / U+061C). In a comment it is source that stops saying + # what it means -- #628 reproduced that on itself, typing the six + # characters of a U+061C escape through a JSON-payload tool that + # decoded it before the file was written, twice, in two files. This + # very comment made it a fourth time, and the gate below caught it + # before the branch was pushed. Between them lies the trojan-source + # shape, where an override or isolate makes a line render as a different + # program from the one that compiles. + # + # Review is not a control for this class, and that is the argument for a + # gate rather than a habit: #610 itself asked for this lint and deferred + # it, and the PR that closed #610 was reviewed by people who could not + # have seen a raw control had one survived, because the diff renders them + # as nothing. + # + # A fast, dependency-free text scan over every tracked file that compiles + # nothing, so it rides this job for the same reason the NOLINT-directive, + # CI-clang-pin and Catch2-name scans above do. + # + # This one ships already green -- 0 raw controls across 1244 tracked + # files -- which makes the self-test the whole of its evidence rather + # than a formality. "0 occurrences found" is exactly what a broken + # detector prints, so the checker probes its own detector against every + # declared codepoint before opening a file, treats a scan of zero files + # as a failure, and the self-test below holds its own independent list of + # the twelve and requires each to be found in tests/lint/bidi_controls/ -- + # which is what notices a codepoint being deleted from the checker's + # table, something the checker's own probe cannot see. + - name: Self-test the raw-bidi-control checker + run: bash scripts/test_check_bidi_controls.sh + + - name: Check no first-party file carries a raw bidi control + run: python3 scripts/check_bidi_controls.py + # ── Ladder rung list <-> the CI filters that are supposed to track it ── # examples/rungs.txt is the ladder's single authoritative rung list, and # almost every consumer now derives from it at run time. Three cannot -- diff --git a/scripts/check_bidi_controls.py b/scripts/check_bidi_controls.py new file mode 100755 index 000000000..4913a6c33 --- /dev/null +++ b/scripts/check_bidi_controls.py @@ -0,0 +1,357 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +"""Usage: python3 scripts/check_bidi_controls.py [PATH...] + +Fails if any first-party file carries a *raw* Unicode bidirectional control +character. The escaped spelling (`\\u200E`, `\\u{200E}`, `‎` -- whatever +the file's own language provides) is always accepted; only the literal +codepoint is rejected. + +With no PATH, scans every tracked file in the repository (`git ls-files`), +minus this gate's own fixtures. With PATHs, walks them on disk -- which is how +scripts/test_check_bidi_controls.sh reaches the fixtures, and how a caller +scopes the scan to one subtree. + +## Why this gate exists (morph#628, morph#642, morph#610) + +A bidi control is invisible. It renders as nothing, occupies no column, and +survives copy-paste, so a diff containing one looks exactly like a diff not +containing one. Every consequence of that is silent: + + - In a *string literal* it is a silently wrong test. morph#610's subject was + nine assertion lines in src/qt/forms/tests/tst_i18n.qml whose expected + values contained raw U+200E / U+200F / U+061C; a wrong one there asserts + something nobody can read. + - In a *comment* it is source that stops saying what it means. morph#628 + reproduced exactly this: typing the six characters `\\u061C` through a + JSON-payload tool decoded the escape before the file was written, so a + single invisible U+061C landed in include/morph/render/locale_format.hpp + and in tests/test_render_locale_format.cpp. + - Between the two lies the "trojan source" shape: an override or isolate + that reorders how a line *renders* relative to how it *compiles*, so the + reviewer and the compiler read different programs. + +Comments are therefore in scope, not excluded. morph#610's original sketch said +"outside comments"; morph#628's own reproduction was inside comments, and the +trojan-source shape lives in comments by construction. + +Review is not a control for this class. The PR that closed morph#610 -- the +ticket about this exact hazard -- was itself reviewed, and the review could not +have seen a raw control had one survived, because the diff renders them as +nothing. That is the argument for a lint rather than a habit. + +## What is rejected + +Twelve codepoints, in three classes, which is morph#610's list: + + U+061C ARABIC LETTER MARK + U+200E, U+200F LEFT-TO-RIGHT / RIGHT-TO-LEFT MARK + U+202A .. U+202E the embeddings, overrides and their terminator + U+2066 .. U+2069 the isolates and their terminator + +The marks are the ones this repository has actually hit; the embeddings, +overrides and isolates are the sharper hazard, because those are the ones that +reorder rendered text rather than merely nudging a neighbouring run. + +## The two vacuity traps, and how each is closed + +A scan that reports "0 raw controls" is satisfied by a broken pattern exactly +as well as by a clean tree, and the tree *is* clean today (0 occurrences across +1244 tracked files at f7c231df), so this gate ships already green. It would therefore be +worth nothing unless both ways of going blind are closed: + + 1. **The detector drifts away from the declared set.** Before scanning + anything, the checker feeds itself a probe holding each declared + codepoint, one at a time, and requires every one to come back flagged. A + `detect()` rewritten as a range with the wrong bound, or narrowed to the + two marks this repository happens to have hit, fails here on every run, + with no fixture involved and on a clean tree. + 2. **The file set stops matching.** Scanning zero files is an error, not a + pass -- that is what a wrong path argument, a `git ls-files` that failed, + or a prune rule that swallowed the tree all look like from outside. + +What that probe cannot see is the declared set itself *shrinking*: delete an +entry from BIDI_CONTROLS and the probe stops asking about it, because both +sides derive from the same table. Closing that is the job of +scripts/test_check_bidi_controls.sh, which holds its own independent list of +the twelve, drives the checker over the fixtures in tests/lint/bidi_controls/, +and requires every codepoint on *its* list to be named in the output. It also +pins that a directory with no scannable files is rejected rather than called +clean, that each diagnostic names its own file, that no diagnostic contains a +raw control, and that both EXEMPT hygiene rules fire. + +## The diagnostic must be readable + +A gate against invisible characters must not print them. Every offending line +is echoed with each control replaced by a visible `` marker, so the +failure message says where the character is instead of reproducing the exact +problem it is reporting. The self-test pins this: the diagnostic itself must +contain no raw control. + +## Exemptions + +EXEMPT below maps a repository-relative path to a written reason. The reason is +the payload: an exemption without one is the hand-maintained record this gate +replaces. + +Two rules keep it from becoming stale, copied from +scripts/check_workflow_option_coverage.py's EXEMPT and +scripts/check_workflow_job_banners.py's UNBANNERED: an entry for a file that +does not exist is an error, and an entry for a file that has no raw controls is +an error. An exemption has to be necessary to be allowed to stay. + +The grain is the file, not the line. That is deliberate: a file that genuinely +needs a raw control -- one proving a parser accepts a literal, say -- is a file +whose whole subject is that character, and an exemption for it should be +argued once in prose rather than sprinkled per line where it would drift back +into being invisible. + +Reads text only; compiles and runs nothing. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +# Files allowed to carry raw bidi controls, and why. +# +# Empty, and that is the measured state of the tree rather than an aspiration: +# `git ls-files` lists 1242 files at c55ea5b7 and none of them contains any of +# the twelve codepoints below. Adding an entry needs an argument a reviewer can +# check; removing one is always allowed. +EXEMPT: dict[str, str] = {} + +# The twelve rejected codepoints, written out one by one. The detector below is +# built from this list, and so is the probe that proves the detector works -- +# but the probe tests each codepoint *individually*, so a mistake that narrows +# the set is caught even though both derive from here. +BIDI_CONTROLS: dict[int, str] = { + 0x061C: "ARABIC LETTER MARK", + 0x200E: "LEFT-TO-RIGHT MARK", + 0x200F: "RIGHT-TO-LEFT MARK", + 0x202A: "LEFT-TO-RIGHT EMBEDDING", + 0x202B: "RIGHT-TO-LEFT EMBEDDING", + 0x202C: "POP DIRECTIONAL FORMATTING", + 0x202D: "LEFT-TO-RIGHT OVERRIDE", + 0x202E: "RIGHT-TO-LEFT OVERRIDE", + 0x2066: "LEFT-TO-RIGHT ISOLATE", + 0x2067: "RIGHT-TO-LEFT ISOLATE", + 0x2068: "FIRST STRONG ISOLATE", + 0x2069: "POP DIRECTIONAL ISOLATE", +} + +# This gate's own fixtures hold raw controls on purpose, so the repository-wide +# run must not see them -- exactly as scripts/check_nolint_directives.sh prunes +# tests/lint for its own inert directives. Naming a path inside the fixture +# tree still scans it, which is how the self-test reaches them. +FIXTURE_ROOT = "tests/lint/bidi_controls" + +# Directories a disk walk never descends into. Only needed in PATH mode; the +# default mode asks git, which already knows. +PRUNE_DIRS = {".git", "_deps", "__pycache__", "node_modules", ".venv"} + + +def detect(text: str) -> set[int]: + """Every rejected codepoint present in `text`. The one detector both the + scan and the self-probe go through, so the probe cannot pass against a + different rule from the one the tree is judged by.""" + return {ord(ch) for ch in text if ord(ch) in BIDI_CONTROLS} + + +def self_probe() -> list[str]: + """Assert the detector still sees each of the twelve codepoints, one at a + time. This is what stops a clean tree and a broken detector from producing + the same green.""" + missed = [ + f"U+{cp:04X} {name} is no longer detected" + for cp, name in sorted(BIDI_CONTROLS.items()) + if detect(chr(cp)) != {cp} + ] + return missed + + +def render(line: str) -> str: + """The line with every control replaced by a visible marker, so a message + about invisible characters is itself readable.""" + return "".join( + f"" if ord(ch) in BIDI_CONTROLS else ch for ch in line + ) + + +def tracked_files(root: Path) -> list[Path]: + """Every tracked file, which is the repository's own answer to 'what is + first-party here' -- derived rather than a list this file would have to + keep in step.""" + out = subprocess.run( + ["git", "-C", str(root), "ls-files", "-z"], + capture_output=True, + text=True, + check=True, + ).stdout + return [root / name for name in out.split("\0") if name] + + +def walked_files(path: Path) -> list[Path]: + if path.is_file(): + return [path] + found: list[Path] = [] + for child in sorted(path.rglob("*")): + if any(part in PRUNE_DIRS for part in child.parts): + continue + if child.is_file() and not child.is_symlink(): + found.append(child) + return found + + +def scan(path: Path) -> tuple[list[tuple[int, int, int, str]], bool]: + """Occurrences as (line, column, codepoint, line text), plus whether the + file was read as text at all. + + A file holding a NUL byte is git's own definition of binary, and a byte + sequence that happens to spell a control there is not source anybody reads. + Decoding with `errors="replace"` is safe for the rest: a replacement + character is never one of the twelve.""" + data = path.read_bytes() + if b"\0" in data: + return [], False + text = data.decode("utf-8", errors="replace") + hits: list[tuple[int, int, int, str]] = [] + for lineno, line in enumerate(text.splitlines(), start=1): + for col, ch in enumerate(line, start=1): + if ord(ch) in BIDI_CONTROLS: + hits.append((lineno, col, ord(ch), line)) + return hits, True + + +def main(argv: list[str]) -> int: + root = Path.cwd() + errors: list[str] = [] + + missed = self_probe() + if missed: + for miss in missed: + print(f"::error::{miss}", file=sys.stderr) + print( + "\nThe raw-bidi-control detector no longer recognises every codepoint " + "it claims to. A scan run with it would report a clean tree whether " + "or not the tree is clean, which is the one outcome this gate must " + "never produce. Fix BIDI_CONTROLS / detect() before trusting any " + "result from this script.", + file=sys.stderr, + ) + return 1 + + if argv: + candidates: list[Path] = [] + for arg in argv: + path = Path(arg) + if not path.exists(): + print(f"error: no such path: {arg}", file=sys.stderr) + return 1 + candidates.extend(walked_files(path)) + else: + try: + candidates = tracked_files(root) + except subprocess.CalledProcessError as exc: + print( + f"error: `git ls-files` failed in {root}: {exc.stderr.strip()}\n" + f" Without it this gate does not know which files are " + f"first-party, and must not report success.", + file=sys.stderr, + ) + return 1 + candidates = [ + p + for p in candidates + if not p.relative_to(root).as_posix().startswith(f"{FIXTURE_ROOT}/") + ] + + scanned = 0 + binary = 0 + offenders: dict[str, list[tuple[int, int, int, str]]] = {} + found_any: set[str] = set() + + for path in candidates: + try: + rel = path.relative_to(root).as_posix() + except ValueError: + rel = path.as_posix() + hits, is_text = scan(path) + if not is_text: + binary += 1 + continue + scanned += 1 + if hits: + found_any.add(rel) + if rel not in EXEMPT: + offenders[rel] = hits + + if scanned == 0: + target = " ".join(argv) if argv else str(root) + print( + f"error: no text files found under: {target}\n" + f" This gate has nothing to check and must not report success. " + f"Either the scan's file set or the path it was given has stopped " + f"matching the tree.", + file=sys.stderr, + ) + return 1 + + for rel, hits in sorted(offenders.items()): + for lineno, col, cp, line in hits: + errors.append( + f"{rel}:{lineno}:{col}: raw U+{cp:04X} {BIDI_CONTROLS[cp]}\n" + f" {lineno} | {render(line)}" + ) + + for rel, reason in sorted(EXEMPT.items()): + if not (root / rel).exists(): + errors.append( + f"{rel} is exempted by this checker (reason: {reason}) but no such " + f"file exists. Delete the exemption: it is a stale record of a " + f"file that has moved or gone." + ) + elif rel not in found_any: + errors.append( + f"{rel} is exempted by this checker (reason: {reason}) but contains " + f"no raw bidi control. Delete the exemption -- one that is not " + f"doing anything is cover for the next one that is wrong." + ) + + if errors: + for err in errors: + print(f"::error::{err}", file=sys.stderr) + print( + f""" +{len(errors)} problem(s). Scanned {scanned} text file(s), skipped {binary} binary. + +A raw bidi control is invisible: it renders as nothing and a diff containing +one looks exactly like a diff that does not. In a string literal that is a +silently wrong assertion (morph#610); in a comment it is source that stops +saying what it means (morph#628); an override or isolate can make a line render +as a different program from the one that compiles. + +Write the escape your file's language provides instead -- `\\u200E` in C++, QML +and JSON, `\\u{{200E}}` where braces are accepted, `‎` in markup. The +escaped form is what every one of the thirteen surviving occurrences in +src/qt/forms/tests/tst_i18n.qml uses. + +If a raw control is genuinely necessary, add the file to EXEMPT in +scripts/check_bidi_controls.py with a reason a reviewer can check.""", + file=sys.stderr, + ) + return 1 + + exemptions = f", {len(EXEMPT)} exemption(s)" if EXEMPT else "" + print( + f"bidi-control lint OK: {scanned} text file(s) scanned " + f"({binary} binary skipped), {len(BIDI_CONTROLS)} codepoint(s) searched " + f"for, 0 raw occurrence(s){exemptions}." + ) + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/test_check_bidi_controls.sh b/scripts/test_check_bidi_controls.sh new file mode 100755 index 000000000..7ad84cdfb --- /dev/null +++ b/scripts/test_check_bidi_controls.sh @@ -0,0 +1,254 @@ +#!/usr/bin/env bash +# Usage: bash scripts/test_check_bidi_controls.sh +# +# Self-test for scripts/check_bidi_controls.py, the gate that keeps raw Unicode +# bidirectional control characters out of first-party files (morph#628). +# +# This gate needs its own test more than most, for a reason its own subject +# makes sharp. The tree is already clean -- 0 raw controls across 1244 tracked +# files -- so the gate passes on day one whether or not it can detect anything +# at all, and the thing it would be failing to detect is by definition +# invisible. "0 occurrences found" is exactly what a broken detector prints. +# +# So the cases below are not about the tree. They are about whether the gate +# would still be a gate on the day something arrives: +# +# - Every declared codepoint must be found in a fixture that contains it. +# This list is written out HERE, independently of BIDI_CONTROLS in the +# checker: the checker's own start-up probe iterates that same table, so it +# cannot notice an entry being deleted from it. This case can. +# - Each invalid fixture must be rejected on its own and must name its own +# file, so a rejection for some unrelated reason does not read as detection. +# - No diagnostic may contain a raw control. A gate against invisible +# characters that prints them is reporting the defect by committing it. +# - A directory with no scannable files must be rejected, not called clean. +# - Both EXEMPT hygiene rules must fire, and an exemption must actually work. +# +# Mutations are applied one at a time to a scratch copy: applied together, a +# single detection would mask every other. +set -euo pipefail + +readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly checker="scripts/check_bidi_controls.py" +readonly fixtures="tests/lint/bidi_controls" + +# The twelve codepoints this gate claims to reject, restated independently of +# the checker. If the checker's table loses one, the coverage case below fails +# even though the checker's own probe is content. +readonly -a declared=( + 061C 200E 200F + 202A 202B 202C 202D 202E + 2066 2067 2068 2069 +) + +failures=0 + +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +# ── the real tree must pass ───────────────────────────────────────────────── +# Not evidence on its own -- that is this whole file's premise -- but a +# regression here means the gate cannot be landed at all. +if output="$( cd "$repo_root" && python3 "$checker" 2>&1 )"; then + note "the repository passes: ${output}" +else + fail "the gate rejects the repository as it stands:" + printf '%s\n' "$output" >&2 +fi + +# ── valid fixtures must be accepted ───────────────────────────────────────── +# Including the one whose prose NAMES the codepoints in ASCII, which must not +# be flagged: a gate that rejected it would make its own documentation +# unwritable, the same allowance check_nolint_directives.sh makes for the +# sentence naming NOLINTNEXTLINE. +if output="$( cd "$repo_root" && python3 "$checker" "${fixtures}/valid" 2>&1 )"; then + note "valid fixtures accepted" +else + fail "valid fixtures were rejected -- escaped spellings or ASCII prose about \ +these characters are being read as occurrences:" + printf '%s\n' "$output" >&2 +fi + +# ── every invalid fixture directory must be rejected, on its own ──────────── +shopt -s nullglob +invalid_dirs=("${repo_root}/${fixtures}"/invalid/*/) +shopt -u nullglob + +if [ "${#invalid_dirs[@]}" -eq 0 ]; then + fail "no fixtures found under ${fixtures}/invalid -- this self-test would pass vacuously" +fi + +for dir in "${invalid_dirs[@]}"; do + name="$(basename "$dir")" + rel="${fixtures}/invalid/${name}" + if output="$( cd "$repo_root" && python3 "$checker" "$rel" 2>&1 )"; then + fail "invalid fixture ${name} was accepted; the checker no longer detects it" + continue + fi + # A nonzero exit is not enough: the checker's other failure paths are "no + # text files found" and the start-up probe, and a fixture whose files went + # missing would take the first -- so the fixture would still look + # "rejected" while nothing was scanned. Require the occurrence diagnostic, + # and require it to name a file under this fixture. + if ! grep -q 'raw U+' <<<"$output"; then + fail "invalid fixture ${name} was rejected, but not as a raw bidi control \ +-- the checker failed for some other reason (a vacuous scan, most likely):" + printf '%s\n' "$output" >&2 + elif ! grep -qF "${rel}/" <<<"$output"; then + fail "invalid fixture ${name} was rejected without naming any file under \ +${rel}; the diagnostic does not point at the offender:" + printf '%s\n' "$output" >&2 + else + note "invalid fixture ${name} rejected" + fi +done + +# ── every declared codepoint must actually be found ───────────────────────── +# The case that decides whether the declared set still means anything. Run the +# checker over the whole invalid corpus and require each of the twelve above to +# be named. Delete a codepoint from BIDI_CONTROLS and this is what goes red. +all_output="$( cd "$repo_root" && python3 "$checker" "${fixtures}/invalid" 2>&1 || true )" +missing=() +for cp in "${declared[@]}"; do + if ! grep -qF "raw U+${cp} " <<<"$all_output"; then + missing+=("U+${cp}") + fi +done +if [ "${#missing[@]}" -ne 0 ]; then + fail "the fixtures contain these codepoints but the checker reported none of \ +them: ${missing[*]}. Either the checker's declared set has shrunk or the \ +fixture holding them has been edited away." +else + note "all ${#declared[@]} declared codepoints found in the invalid fixtures" +fi + +# ── the diagnostic must itself be readable ────────────────────────────────── +# A gate against invisible characters must not print them: a message a reader +# cannot see is the defect being reported, one level up. Every control in an +# echoed line is replaced by a visible marker. +if grep -qP '[\x{061C}\x{200E}\x{200F}\x{202A}-\x{202E}\x{2066}-\x{2069}]' <<<"$all_output"; then + fail "the checker's own diagnostics contain raw bidi controls; the failure \ +message reproduces the defect it reports" +else + note "no diagnostic contains a raw bidi control" +fi + +# ── a directory with no scannable files must be rejected ──────────────────── +# "Found nothing" is the one outcome this gate must never call clean: it is +# what a wrong path, a swallowed prune rule or a dead file walk all look like. +empty_dir="${scratch}/empty" +mkdir -p "$empty_dir" +if ( cd "$repo_root" && python3 "$checker" "$empty_dir" >/dev/null 2>&1 ); then + fail "a directory containing no files was accepted; the gate can pass vacuously" +else + note "directory with no scannable files rejected" +fi + +# ── mutation cases against a scratch copy ─────────────────────────────────── +# `sample/` holds one clean file and one carrying a raw U+200E, so an exemption +# has something to be right about and something to be wrong about. +readonly pristine="${scratch}/pristine" +mkdir -p "${pristine}/scripts" "${pristine}/sample" +cp "${repo_root}/${checker}" "${pristine}/scripts/" +printf 'constexpr const char* kSign = "\\u200E+";\n' > "${pristine}/sample/clean.cpp" +python3 -c 'import pathlib,sys; pathlib.Path(sys.argv[1]).write_text("constexpr const char* kSign = \"\u200E+\";\n", encoding="utf-8")' \ + "${pristine}/sample/dirty.cpp" + +make_tree() { + rm -rf "${scratch}/case" + mkdir -p "${scratch}/case" + cp -R "${pristine}/." "${scratch}/case" +} + +# `sed -i` is not portable between GNU and BSD sed; edit through a temp file. +edit() { + local file="$1"; shift + sed "$@" "$file" > "${file}.new" + mv "${file}.new" "$file" +} + +expect_caught() { + local description="$1" mutator="$2" expected="$3" output + make_tree + if ! ( cd "${scratch}/case" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "${scratch}/case" && python3 "$checker" sample 2>&1 )"; then + fail "NOT caught: ${description} -- the gate passed a tree it should reject" + printf '%s\n' "$output" >&2 + return + fi + if grep -qF "$expected" <<<"$output"; then + note "caught: ${description}" + else + fail "caught for the WRONG reason: ${description} -- no diagnostic \ +containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +expect_accepted() { + local description="$1" mutator="$2" output + make_tree + if ! ( cd "${scratch}/case" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "${scratch}/case" && python3 "$checker" sample 2>&1 )"; then + note "accepted: ${description}" + else + fail "FALSE POSITIVE: ${description} -- the gate rejected a tree it \ +should accept:" + printf '%s\n' "$output" >&2 + fi +} + +# The baseline the exemption cases are measured against: unexempted, the raw +# U+200E in sample/dirty.cpp is reported and the escaped one in +# sample/clean.cpp is not. +expect_caught "an unexempted file carrying a raw U+200E" \ + "true" \ + "sample/dirty.cpp:1:32: raw U+200E" + +# The mechanism has to work, or every case below tests an exemption that could +# not be used. Exempting the file that is actually dirty clears the tree. +expect_accepted "an exemption for the file that carries the control" \ + "edit ${checker} -e 's|^EXEMPT: dict\[str, str\] = {}|EXEMPT: dict[str, str] = {\"sample/dirty.cpp\": \"a reason\"}|'" + +# Hygiene rule one: an exemption for a file with nothing to exempt. This is the +# entry that outlives its cause and quietly covers whatever lands in that file +# next. +expect_caught "an exemption for a file with no raw controls" \ + "edit ${checker} -e 's|^EXEMPT: dict\[str, str\] = {}|EXEMPT: dict[str, str] = {\"sample/clean.cpp\": \"a reason\"}|'" \ + "contains no raw bidi control" + +# Hygiene rule two: an exemption for a file that does not exist. +expect_caught "an exemption naming a file that does not exist" \ + "edit ${checker} -e 's|^EXEMPT: dict\[str, str\] = {}|EXEMPT: dict[str, str] = {\"sample/gone.cpp\": \"a reason\"}|'" \ + "no such file exists" + +# The start-up probe: a detector narrowed to the two marks this repository has +# actually hit still passes over a clean tree, and would pass over the whole +# repository today. The probe is what makes that impossible -- and note it +# fires here even though sample/ is a tree the gate would otherwise reject for +# an ordinary reason, because it runs before any file is opened. +expect_caught "the detector narrowed to a subset of the declared codepoints" \ + "edit ${checker} -e 's|^ return {ord(ch) for ch in text if ord(ch) in BIDI_CONTROLS}| return {ord(ch) for ch in text if ord(ch) == 0x200E}|'" \ + "is no longer detected" + +# The same failure written the other way round: a detector that finds nothing +# at all, which is the shape a mistyped range or an emptied table produces. +expect_caught "the detector disabled outright" \ + "edit ${checker} -e 's|^ return {ord(ch) for ch in text if ord(ch) in BIDI_CONTROLS}| return set()|'" \ + "is no longer detected" + +if [ "$failures" -ne 0 ]; then + printf '\n%d case(s) failed.\n' "$failures" >&2 + exit 1 +fi + +printf '\nall cases passed.\n' diff --git a/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp b/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp new file mode 100644 index 000000000..5533a236e --- /dev/null +++ b/tests/lint/bidi_controls/invalid/arabic_letter_mark/alm_in_a_comment.cpp @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// morph#628's own reproduction, byte for byte: a JSON-payload tool decoded +// the escape before the file was written, so the comment below carries a +// single raw U+061C where six characters were typed. +// +// `inline` is load-bearing, not style: a namespace-scope `constexpr` +// has internal linkage, and clang-tidy-diff analyses every line of a +// new file, so an unused one is reported as +// clang-diagnostic-unused-const-variable and fails the job. Nothing +// here is compiled, but the gate that reads it does not know that. + +// Before morph#591 this edge emitted "؜-1050.25" -- the sign is invisible. + +inline constexpr const char* kSign = "؜-"; diff --git a/tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml b/tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml new file mode 100644 index 000000000..b953f9186 --- /dev/null +++ b/tests/lint/bidi_controls/invalid/directional_marks/marks_in_literals.qml @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// morph#642's shape: assertion literals carrying raw U+200E / U+200F while +// their neighbours in the same file use the escape. + +import QtQuick + +Item { + property string azIR: "‎+‎" + property string ckbIQ: "‏+" +} diff --git a/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp b/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp new file mode 100644 index 000000000..863fb9215 --- /dev/null +++ b/tests/lint/bidi_controls/invalid/embeddings_and_overrides/reordering.cpp @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The embeddings, their overrides and their terminator. These are the +// trojan-source characters: they change how the line renders relative to +// how it compiles, so the reviewer and the compiler read different +// programs. +// +// `inline` keeps clang-diagnostic-unused-const-variable off a fixture +// nothing compiles; see invalid/arabic_letter_mark/. + +inline constexpr const char* kEmbed = "‪a‬"; +inline constexpr const char* kEmbedRtl = "‫a‬"; +// LRE with no terminating PDF: misc-misleading-bidirectional reads the +// string's *content* and is right about it. This gate reads the source +// *bytes*, which is why the fixture has to keep them. +// NOLINTNEXTLINE(misc-misleading-bidirectional) +inline constexpr const char* kOverride = "‭a‮b"; diff --git a/tests/lint/bidi_controls/invalid/isolates/isolates.hpp b/tests/lint/bidi_controls/invalid/isolates/isolates.hpp new file mode 100644 index 000000000..54486ad1c --- /dev/null +++ b/tests/lint/bidi_controls/invalid/isolates/isolates.hpp @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The isolates and their terminator -- the modern spelling of the same +// reordering hazard, and the class morph#628's triage called the sharper +// one. + +#pragma once + +constexpr const char* kLri = "⁦x⁩"; +constexpr const char* kRli = "⁧x⁩"; +constexpr const char* kFsi = "⁨x⁩"; diff --git a/tests/lint/bidi_controls/valid/escaped_literals.cpp b/tests/lint/bidi_controls/valid/escaped_literals.cpp new file mode 100644 index 000000000..4baf551bd --- /dev/null +++ b/tests/lint/bidi_controls/valid/escaped_literals.cpp @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The accepted spelling. Every bidi control here is an escape, so the bytes on +// disk are ASCII and a reviewer reads exactly what the compiler reads. This is +// what src/qt/forms/tests/tst_i18n.qml does for all thirteen of its +// occurrences, and what morph#610 converted nine assertion lines to. +// +// Two things keep clang-tidy-diff green over a file every line of which +// is a changed line. `inline` gives each constant external linkage, so +// clang-diagnostic-unused-const-variable does not fire on a fixture +// nothing compiles. And misc-misleading-bidirectional still fires on two +// of the escapes below -- an escape produces the same bytes in the +// compiled string, so that check judges the string's content while this +// gate judges the source bytes. The two are complementary, and the +// per-line suppressions record where they disagree. + +inline constexpr const char* kArabicLetterMark = "\u061C"; +inline constexpr const char* kLeftToRightMark = "\u200E"; +inline constexpr const char* kRightToLeftMark = "\u200F"; +// RLO with no terminating PDF, and FSI with no terminating PDI: the +// content check reads the expanded literal, which is exactly the point +// of writing them as escapes. +// NOLINTNEXTLINE(misc-misleading-bidirectional) +inline constexpr const char* kRightToLeftOverride = "\u202E"; +// NOLINTNEXTLINE(misc-misleading-bidirectional) +inline constexpr const char* kFirstStrongIsolate = "\u2068"; +inline constexpr const char* kPopDirectionalIsolate = "\u2069"; + +// The brace form too, which is how a u8 literal often spells it. +inline constexpr const char* kBraced = "\u{200E}+\u{200E}"; diff --git a/tests/lint/bidi_controls/valid/no_controls_at_all.qml b/tests/lint/bidi_controls/valid/no_controls_at_all.qml new file mode 100644 index 000000000..91530bb90 --- /dev/null +++ b/tests/lint/bidi_controls/valid/no_controls_at_all.qml @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// An ordinary file with nothing to say about bidi at all. The valid set must +// contain one, or "accepted" would only ever mean "accepted a file that +// mentions the subject". + +import QtQuick + +Item { + property string sign: "+" +} diff --git a/tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp b/tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp new file mode 100644 index 000000000..8a0aa662d --- /dev/null +++ b/tests/lint/bidi_controls/valid/prose_naming_the_codepoints.hpp @@ -0,0 +1,14 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Prose that NAMES the characters must not be flagged, or this gate would make +// its own documentation unwritable -- the same allowance +// scripts/check_nolint_directives.sh makes for the sentence in +// include/morph/detail/fixed_string.hpp that names NOLINTNEXTLINE. +// +// A raw U+200E LEFT-TO-RIGHT MARK in a string literal is a silently wrong +// assertion; U+202E RIGHT-TO-LEFT OVERRIDE and U+2066..U+2069, the isolates, +// are the ones that reorder rendered text. Write them as \u200E, \u202E and +// so on. U+061C, the ARABIC LETTER MARK, is the one morph#628 typed by +// accident. + +#pragma once