From 68bac9f3151014a8ebbc0d0ee3b163838f80770b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 30 Aug 2026 21:59:52 -0500 Subject: [PATCH] Harden the log call scanner and add unit tests --- script/ci-custom.py | 54 +++++++++++++++------ tests/script/test_ci_custom.py | 86 ++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 15 deletions(-) create mode 100644 tests/script/test_ci_custom.py diff --git a/script/ci-custom.py b/script/ci-custom.py index b781167dc4..1ae4f3d5f5 100755 --- a/script/ci-custom.py +++ b/script/ci-custom.py @@ -974,15 +974,23 @@ def lint_no_std_bind(fname, match): LOG_CALL_START_RE = re.compile(r"ESP_LOG\w+\s*\(") -# A C++ string or char literal, escapes included, so ; ( ) ? : inside one are never seen. -CPP_LITERAL_RE = r'"(?:[^"\\]|\\.)*"|\'(?:[^\'\\]|\\.)*\'' -LOG_CALL_TOKEN_RE = re.compile(CPP_LITERAL_RE + r"|[()]", re.DOTALL) -# Alt 2 matches a ? or : whose next non-space char opens a literal; that literal is a ternary branch. -LOG_TERNARY_LITERAL_RE = re.compile(CPP_LITERAL_RE + r'|[?:]\s*(?=")', re.DOTALL) +# Comments, string literals and single char literals are consumed whole so ; ( ) ? : inside them +# are never seen. A char literal is exactly one (escaped) char so a digit separator like 1'000'000 +# cannot open one. +CPP_SKIP_RE = r'//[^\n]*|/\*.*?\*/|"(?:[^"\\]|\\.)*"|\'(?:[^\'\\\n]|\\.)\'' +LOG_CALL_TOKEN_RE = re.compile(CPP_SKIP_RE + r"|[()]", re.DOTALL) +# The last alternative matches a ? or : whose next non-space char opens a literal, a ternary branch. +LOG_TERNARY_LITERAL_RE = re.compile(CPP_SKIP_RE + r'|[?:]\s*(?=")', re.DOTALL) -def _iter_log_calls(content: str) -> Iterator[tuple[int, str]]: - """Yield (start, text) for every ESP_LOG*(...) call, text running to the matching close paren.""" +def _line_col(content: str, pos: int) -> tuple[int, int]: + """1-based line and column of an offset in content.""" + return content.count("\n", 0, pos) + 1, pos - content.rfind("\n", 0, pos) + + +def _iter_log_calls(content: str) -> Iterator[tuple[int, str | None]]: + """Yield (start, text) for every ESP_LOG*(...) call, text running to the matching close paren. + text is None when no matching paren exists so callers can report the call instead of skipping it.""" for head in LOG_CALL_START_RE.finditer(content): depth = 1 for tok in LOG_CALL_TOKEN_RE.finditer(content, head.end()): @@ -993,6 +1001,17 @@ def _iter_log_calls(content: str) -> Iterator[tuple[int, str]]: if depth == 0: yield head.start(), content[head.start() : tok.end()] break + else: + yield head.start(), None + + +def _unbalanced_log_call_error(content: str, pos: int) -> tuple[int, int, str]: + lineno, col = _line_col(content, pos) + return ( + lineno, + col, + "ESP_LOG call has no matching closing parenthesis, so it cannot be checked.", + ) LOG_BAD_CONTINUATION_RE = re.compile(r'\\n(?:[^ \\"\r\n\t]|"\s*\n\s*"[^ \\])') @@ -1003,14 +1022,15 @@ LOG_PERCENT_S_CONTINUATION_RE = re.compile(r'\\n(?:%s|"\s*\n\s*"%s)') def lint_log_multiline_continuation(fname, content): errs = [] for log_start, log_text in _iter_log_calls(content): + if log_text is None: + errs.append(_unbalanced_log_call_error(content, log_start)) + continue for bad_match in LOG_BAD_CONTINUATION_RE.finditer(log_text): # %s may expand to a whitespace prefix at runtime, skip those if LOG_PERCENT_S_CONTINUATION_RE.match(log_text, bad_match.start()): continue # Calculate line number from position in full content - abs_pos = log_start + bad_match.start() - lineno = content.count("\n", 0, abs_pos) + 1 - col = abs_pos - content.rfind("\n", 0, abs_pos) + lineno, col = _line_col(content, log_start + bad_match.start()) errs.append( ( lineno, @@ -1081,12 +1101,16 @@ def lint_log_no_bare_literal_ternary( ) -> list[tuple[int, int, str]]: errs = [] for log_start, log_text in _iter_log_calls(content): + if log_text is None: + errs.append(_unbalanced_log_call_error(content, log_start)) + continue + # A NOLINT anywhere on the lines the call spans silences every branch in it + first_line = content.rfind("\n", 0, log_start) + 1 + last_line = content.find("\n", log_start + len(log_text)) + if "NOLINT" in content[first_line : last_line if last_line != -1 else None]: + continue for offset, literal in _find_ternary_literals(log_text): - abs_pos = log_start + offset - if "NOLINT" in content[abs_pos:].partition("\n")[0]: - continue - lineno = content.count("\n", 0, abs_pos) + 1 - col = abs_pos - content.rfind("\n", 0, abs_pos) + lineno, col = _line_col(content, log_start + offset) errs.append( ( lineno, diff --git a/tests/script/test_ci_custom.py b/tests/script/test_ci_custom.py new file mode 100644 index 0000000000..e75f6a9aba --- /dev/null +++ b/tests/script/test_ci_custom.py @@ -0,0 +1,86 @@ +"""Unit tests for the ESP_LOG call scanner in script/ci-custom.py.""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +import sys + +import pytest + +SCRIPT_DIR = (Path(__file__).parent / ".." / ".." / "script").resolve() +sys.path.insert(0, str(SCRIPT_DIR)) + +_spec = importlib.util.spec_from_file_location("ci_custom", SCRIPT_DIR / "ci-custom.py") +ci_custom = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(ci_custom) + + +def _calls(content: str) -> list[str | None]: + return [text for _, text in ci_custom._iter_log_calls(content)] + + +def _ternary_errors(content: str) -> list[tuple[int, int]]: + errs = ci_custom.lint_log_no_bare_literal_ternary(Path("x.cpp"), content) + return [(line, col) for line, col, _ in errs] + + +@pytest.mark.parametrize( + "content", + [ + 'ESP_LOGD(TAG, "a ) b ( c; d")', + 'ESP_LOGD(TAG, "quote \\" inside")', + "ESP_LOGD(TAG, \"%s\", format_hex_pretty(x, '-', false).c_str())", + "ESP_LOGD(TAG, \"%c%c\", '(', ')')", + "ESP_LOGD(TAG, \"%d\", 1'000'000)", + 'ESP_LOGD(TAG, // it\'s a comment with ) and (\n "x")', + 'ESP_LOGD(TAG, /* :) */ "x")', + ], +) +def test_iter_log_calls_spans_whole_call(content: str) -> None: + calls = _calls(content + ";\nint other = (1);") + assert calls == [content] + + +def test_iter_log_calls_reports_unbalanced_call() -> None: + content = 'ESP_LOGD(TAG, "x";\nvoid f();' + assert _calls(content) == [None] + errs = ci_custom.lint_log_no_bare_literal_ternary(Path("x.cpp"), content) + assert len(errs) == 1 + assert errs[0][:2] == (1, 1) + assert "no matching closing parenthesis" in errs[0][2] + + +@pytest.mark.parametrize( + ("content", "expected"), + [ + ('ESP_LOGD(TAG, "%s", x ? "on" : "off");', [(1, 25), (1, 32)]), + ( + 'ESP_LOGD(TAG, "%s", x ? LOG_STR_LITERAL("on") : LOG_STR_LITERAL("off"));', + [], + ), + ('ESP_LOGD(TAG, "%s", x ? LOG_STR_LITERAL("on") : "off");', [(1, 49)]), + ('ESP_LOGD(TAG, "%s", x ? "on" : "");', [(1, 25)]), + ( + 'ESP_LOGD(TAG, "%s",\n x ? "yes"\n : "no");', + [(2, 14), (3, 14)], + ), + ("ESP_LOGD(TAG, \"%c\", x ? '1' : '0');", []), + ('ESP_LOGD(TAG, "a ? b : c %s", x ? "on" : "off");', [(1, 35), (1, 42)]), + ('ESP_LOGD(TAG, "x:" "y %s", p);', []), + ('ESP_LOGD(TAG, "%s", x ? "on" : "off"); // NOLINT', []), + ('ESP_LOGD(TAG, "%s",\n x ? "yes"\n : "no"); // NOLINT', []), + ], +) +def test_ternary_literal_detection( + content: str, expected: list[tuple[int, int]] +) -> None: + assert _ternary_errors(content) == expected + + +def test_ternary_error_message_names_the_literal() -> None: + errs = ci_custom.lint_log_no_bare_literal_ternary( + Path("x.cpp"), 'ESP_LOGD(TAG, "%s", x ? "enabled" : LOG_STR_LITERAL("off"));' + ) + assert len(errs) == 1 + assert 'LOG_STR_LITERAL("enabled")' in errs[0][2]