diff --git a/script/ci-custom.py b/script/ci-custom.py index 1ae4f3d5f5..e3d826cb38 100755 --- a/script/ci-custom.py +++ b/script/ci-custom.py @@ -974,13 +974,21 @@ def lint_no_std_bind(fname, match): LOG_CALL_START_RE = re.compile(r"ESP_LOG\w+\s*\(") -# 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]|\\.)\'' +# Comments, raw/plain 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_COMMENT_RE = r"//[^\n]*|/\*.*?\*/" +CPP_SKIP_RE = ( + CPP_COMMENT_RE + r'|R"([^(\s]*)\(.*?\)\1"|"(?:[^"\\]|\\.)*"|\'(?:[^\'\\\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) +# The last alternative matches a ? or : followed (after spaces or comments) by an opening quote, +# i.e. a string literal used as a ternary branch. +LOG_TERNARY_LITERAL_RE = re.compile( + CPP_SKIP_RE + r"|[?:](?:\s|" + CPP_COMMENT_RE + r')*(?=")', re.DOTALL +) +# A bare NOLINT; a clang-tidy NOLINT(check-name) is aimed at a different tool. +NOLINT_RE = re.compile(r"\bNOLINT\b(?!\()") def _line_col(content: str, pos: int) -> tuple[int, int]: @@ -1090,8 +1098,9 @@ LOG_LITERAL_LINT_EXCLUDE = [ "esphome/components/usb_host/*", "esphome/components/zigbee/*", "esphome/components/lvgl/*", - # Test fixtures - not production embedded code + # Test fixtures and host only unit tests - not production embedded code "tests/integration/fixtures/*", + "tests/components/*", ] @@ -1102,12 +1111,13 @@ def lint_log_no_bare_literal_ternary( 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 + continue # reported by lint_log_multiline_continuation, which sees every file # 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]: + if NOLINT_RE.search( + content[first_line : last_line if last_line != -1 else None] + ): continue for offset, literal in _find_ternary_literals(log_text): lineno, col = _line_col(content, log_start + offset) diff --git a/tests/script/test_ci_custom.py b/tests/script/test_ci_custom.py index e75f6a9aba..10baaf37cb 100644 --- a/tests/script/test_ci_custom.py +++ b/tests/script/test_ci_custom.py @@ -35,6 +35,8 @@ def _ternary_errors(content: str) -> list[tuple[int, int]]: "ESP_LOGD(TAG, \"%d\", 1'000'000)", 'ESP_LOGD(TAG, // it\'s a comment with ) and (\n "x")', 'ESP_LOGD(TAG, /* :) */ "x")', + 'ESP_LOGD(TAG, "%s", R"(say "hi" :) )")', + 'ESP_LOGD(TAG, "%s", R"x(a)"b)x")', ], ) def test_iter_log_calls_spans_whole_call(content: str) -> None: @@ -42,13 +44,44 @@ def test_iter_log_calls_spans_whole_call(content: str) -> None: assert calls == [content] -def test_iter_log_calls_reports_unbalanced_call() -> None: +def test_iter_log_calls_reports_unbalanced_call_once() -> 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) + errs = ci_custom.lint_log_multiline_continuation(Path("x.cpp"), content) assert len(errs) == 1 assert errs[0][:2] == (1, 1) assert "no matching closing parenthesis" in errs[0][2] + assert _ternary_errors(content) == [] + + +@pytest.mark.parametrize( + ("content", "expected"), + [ + # A ; inside the format string no longer cuts the call short + ('ESP_LOGD(TAG, "a; b\\nc %s", x);', [(1, 20)]), + # A \n%s continuation is exempt since %s may expand to leading whitespace + ('ESP_LOGD(TAG, "a\\n%s", x);', []), + ('ESP_LOGD(TAG, "a\\n b");', []), + ], +) +def test_multiline_continuation_detection( + content: str, expected: list[tuple[int, int]] +) -> None: + errs = ci_custom.lint_log_multiline_continuation(Path("x.cpp"), content) + assert [(line, col) for line, col, _ in errs] == expected + + +def test_exclusion_list_only_names_components_without_esp8266_tests() -> None: + root = Path(__file__).parent / ".." / ".." + for pattern in ci_custom.LOG_LITERAL_LINT_EXCLUDE: + if not pattern.startswith("esphome/components/"): + continue + prefix = pattern.removeprefix("esphome/components/").split("/")[0] + for comp in (root / "esphome" / "components").glob(prefix): + test = root / "tests" / "components" / comp.name / "test.esp8266-ard.yaml" + assert not test.exists(), ( + f"{comp.name} builds for ESP8266, drop {pattern!r}" + ) @pytest.mark.parametrize( @@ -70,6 +103,11 @@ def test_iter_log_calls_reports_unbalanced_call() -> None: ('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', []), + ('ESP_LOGD(TAG, "%s", x ? /* c */ "on" : "off");', [(1, 33), (1, 40)]), + ( + 'ESP_LOGD(TAG, "%s",\n x ? "on" // NOLINT(some-clang-check)\n : "off");', + [(2, 14), (3, 14)], + ), ], ) def test_ternary_literal_detection(