From d6aecb1b911c661dc3e548050e6bf84d40a04f2d Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 16:11:00 +0100 Subject: [PATCH 01/29] Cover all five override directives in the apple_support guard. _has_apple_support_override() only matched single_version_override -- archive_override, git_override, local_path_override, and multiple_version_override all silently returned False. Same gap flagged by review on the equivalent toolchains_llvm guard in #423. Added the remaining four directives to the alternation and a test for each; confirmed each new test fails against the pre-fix pattern (standalone repro) before adding the fix. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 42 +++++++++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 1a8d243b..e6e71c50 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -22,13 +22,18 @@ def _repo_root(start: Path | None = None) -> Path: def _has_apple_support_override(module_bazel: str) -> bool: - """True if any single_version_override targets apple_support. + """True if any override pins apple_support to a specific commit/version set. Matches each override call's full argument body regardless of argument - order or whether module_name shares a line with the opening "(", instead - of assuming module_name is the first line after "single_version_override(". + order or formatting, rather than assuming module_name is the first line + after the opening "(", and covers all five Bazel module override + directives, not just single_version_override. """ - for call in re.finditer(r"single_version_override\(\s*([^)]*)\)", module_bazel): + for call in re.finditer( + r"(?:archive_override|git_override|local_path_override" + r"|multiple_version_override|single_version_override)\(\s*([^)]*)\)", + module_bazel, + ): if re.search(r'module_name\s*=\s*"apple_support"', call.group(1)): return True return False @@ -95,6 +100,35 @@ def test_has_apple_support_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_archive_override(self) -> None: + sample = """ +archive_override( + module_name = "apple_support", + urls = ["https://example.com/apple_support.tar.gz"], +) +""" + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_local_path_override(self) -> None: + sample = """ +local_path_override( + module_name = "apple_support", + path = "../apple_support", +) +""" + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_multiple_version_override( + self, + ) -> None: + sample = """ +multiple_version_override( + module_name = "apple_support", + versions = ["1.24.2", "2.10.1"], +) +""" + self.assertTrue(_has_apple_support_override(sample)) + if __name__ == "__main__": unittest.main() From 2c9575656a6bac24e5e9fd2e9ad18285509b957c Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 16:41:58 +0100 Subject: [PATCH 02/29] Parse bazel_dep's apple_support version independently of argument order. Same gap review caught on the toolchains_llvm guard in #423: the inline regex only matched "name" immediately followed by "version", so reordered or multiline bazel_dep(...) arguments would fail the guard even with the dependency correctly pinned. Extracted _apple_support_bazel_dep_version(), parsing the whole call body and finding both fields independently, matching _rules_cc_bazel_dep_version() in ci_windows_cppopts_test.py. Confirmed the new reordered-args test fails against the old inline pattern (standalone repro) before the fix. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index e6e71c50..e9c483e8 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -21,6 +21,16 @@ def _repo_root(start: Path | None = None) -> Path: raise AssertionError("could not locate repository root from test file path") +def _apple_support_bazel_dep_version(module_bazel: str) -> str: + for match in re.finditer(r"bazel_dep\(\s*([^)]*)\)", module_bazel): + body = match.group(1) + name = re.search(r'name\s*=\s*"apple_support"', body) + version = re.search(r'version\s*=\s*"([^"]+)"', body) + if name and version: + return version.group(1) + raise AssertionError('expected bazel_dep(... name = "apple_support" ...)') + + def _has_apple_support_override(module_bazel: str) -> bool: """True if any override pins apple_support to a specific commit/version set. @@ -48,12 +58,9 @@ def test_module_pins_apple_support_past_xcode27_crosstool_fixes(self) -> None: this project used to carry. """ module = (_repo_root() / "MODULE.bazel").read_text(encoding="utf-8") - dep = re.search( - r'bazel_dep\(\s*name\s*=\s*"apple_support"\s*,\s*version\s*=\s*"([^"]+)"', - module, + version = tuple( + int(p) for p in _apple_support_bazel_dep_version(module).split(".") ) - self.assertIsNotNone(dep, "expected a direct apple_support bazel_dep") - version = tuple(int(p) for p in dep.group(1).split(".")) self.assertGreaterEqual( version, (2, 8, 4), @@ -75,6 +82,12 @@ def test_apple_support_patches_were_removed(self) -> None: "libtool nodiscard patch is obsolete once apple_support >= 2.8.4 is pinned", ) + def test_apple_support_bazel_dep_version_tolerates_reordered_args( + self, + ) -> None: + sample = 'bazel_dep(version = "2.10.1", name = "apple_support")' + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' From d9589ac6ddb32fb5b24492e6b2520028364c60a2 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 17:18:35 +0100 Subject: [PATCH 03/29] Use a balanced-paren/string/comment-aware scanner for call bodies. Same bug review found on the toolchains_llvm guard in #423: [^)]*)\) stops at the first ")" even inside a string, comment, or nested call, so a leading comment like "# fixes Xcode 27 crosstool warnings (see docs)" before the real args would truncate the match. _iter_call_bodies() replaces both regexes with a character scanner tracking string/comment state and paren depth. Added a test with a ")" in a leading comment for each helper; confirmed both fail against the old regex (standalone repro) before the fix. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 78 +++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 8 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index e9c483e8..cadae183 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -21,9 +21,48 @@ def _repo_root(start: Path | None = None) -> Path: raise AssertionError("could not locate repository root from test file path") +def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: + """Yield the argument-list text of each call to any name in call_names. + + A plain `[^)]*)` regex stops at the first ")" even when it occurs inside + a string literal, a "#" comment, or a nested call (e.g. select(...)), + truncating the body before later args like module_name/version. This + scans character-by-character instead, tracking string/comment state and + paren depth so the body always spans to its real matching ")". + """ + pattern = re.compile(r"(?:%s)\(" % "|".join(re.escape(n) for n in call_names)) + bodies = [] + for start_match in pattern.finditer(text): + i = start_match.end() + depth = 1 + in_string: str | None = None + in_comment = False + body_start = i + while i < len(text) and depth > 0: + ch = text[i] + if in_comment: + if ch == "\n": + in_comment = False + elif in_string: + if ch == "\\": + i += 1 + elif ch == in_string: + in_string = None + elif ch == "#": + in_comment = True + elif ch in "\"'": + in_string = ch + elif ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + i += 1 + bodies.append(text[body_start : i - 1]) + return bodies + + def _apple_support_bazel_dep_version(module_bazel: str) -> str: - for match in re.finditer(r"bazel_dep\(\s*([^)]*)\)", module_bazel): - body = match.group(1) + for body in _iter_call_bodies(("bazel_dep",), module_bazel): name = re.search(r'name\s*=\s*"apple_support"', body) version = re.search(r'version\s*=\s*"([^"]+)"', body) if name and version: @@ -39,12 +78,15 @@ def _has_apple_support_override(module_bazel: str) -> bool: after the opening "(", and covers all five Bazel module override directives, not just single_version_override. """ - for call in re.finditer( - r"(?:archive_override|git_override|local_path_override" - r"|multiple_version_override|single_version_override)\(\s*([^)]*)\)", - module_bazel, - ): - if re.search(r'module_name\s*=\s*"apple_support"', call.group(1)): + override_names = ( + "archive_override", + "git_override", + "local_path_override", + "multiple_version_override", + "single_version_override", + ) + for body in _iter_call_bodies(override_names, module_bazel): + if re.search(r'module_name\s*=\s*"apple_support"', body): return True return False @@ -88,6 +130,17 @@ def test_apple_support_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "2.10.1", name = "apple_support")' self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_survives_an_earlier_paren( + self, + ) -> None: + """A ")" inside a leading comment must not truncate the call body.""" + sample = """bazel_dep( + # fixes Xcode 27 crosstool warnings (see docs) + version = "2.10.1", + name = "apple_support", +)""" + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' @@ -104,6 +157,15 @@ def test_has_apple_support_override_detects_reordered_args(self) -> None: """ self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_survives_an_earlier_paren(self) -> None: + """A ")" inside a leading comment must not truncate the call body.""" + sample = """single_version_override( + # pin past the Xcode 27 fixes (see docs) + module_name = "apple_support", + version = "1.24.2", +)""" + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_other_modules(self) -> None: sample = """ single_version_override( From a8ffd415c7aca9e5aa281edd13477f53dfa18f22 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 17:20:39 +0100 Subject: [PATCH 04/29] Address PR #424 review: add missing git_override test, fix stale docstring. - git_override was the one new directive in the #421 follow-up without its own dedicated test, even though the alternation already matched it correctly -- added test_has_apple_support_override_detects_git_override so a future regression in that branch would be caught. - _has_apple_support_override()'s docstring said it detects a pin "to a specific commit/version set," which doesn't describe local_path_override (selects a local directory, not a commit or version). Reworded to describe the actual predicate: any of the five override directives targeting apple_support. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index cadae183..09422d1a 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -71,12 +71,13 @@ def _apple_support_bazel_dep_version(module_bazel: str) -> str: def _has_apple_support_override(module_bazel: str) -> bool: - """True if any override pins apple_support to a specific commit/version set. + """True if any Bazel module override directive targets apple_support. - Matches each override call's full argument body regardless of argument - order or formatting, rather than assuming module_name is the first line - after the opening "(", and covers all five Bazel module override - directives, not just single_version_override. + Covers all five override directives (archive_override, git_override, + local_path_override, multiple_version_override, single_version_override) + — not just single_version_override — and matches each call's full + argument body regardless of argument order or formatting, rather than + assuming module_name is the first line after the opening "(". """ override_names = ( "archive_override", @@ -181,6 +182,16 @@ def test_has_apple_support_override_detects_archive_override(self) -> None: module_name = "apple_support", urls = ["https://example.com/apple_support.tar.gz"], ) +""" + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_git_override(self) -> None: + sample = """ +git_override( + module_name = "apple_support", + remote = "https://github.com/bazelbuild/apple_support", + commit = "abc123", +) """ self.assertTrue(_has_apple_support_override(sample)) From 8c8e2b00d0193e569a1fda9b6eeceea5fc29b237 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 17:38:35 +0100 Subject: [PATCH 05/29] Ignore commented-out MODULE.bazel calls in the call-body scanner. Same bug fixed on the toolchains_llvm guard in #423: _iter_call_bodies() located each call with a plain regex search for "name(", which matches that text inside a "#" comment too -- a commented-out "bazel_dep(... version = \"9.9.9\")" before the real dependency made the version guard read the dead value, and a commented-out override made _has_apple_support_override return a false positive. Rewritten as a single linear scan that tracks string/comment state continuously. Confirmed both failure modes against the old scanner (standalone repro) before the fix, and added a dedicated commented-out-call test for each helper. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 97 +++++++++++++++++++++++------- 1 file changed, 75 insertions(+), 22 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 09422d1a..f97f10e8 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -22,39 +22,77 @@ def _repo_root(start: Path | None = None) -> Path: def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: - """Yield the argument-list text of each call to any name in call_names. + """Yield the argument-list text of each top-level call to call_names. - A plain `[^)]*)` regex stops at the first ")" even when it occurs inside - a string literal, a "#" comment, or a nested call (e.g. select(...)), - truncating the body before later args like module_name/version. This - scans character-by-character instead, tracking string/comment state and - paren depth so the body always spans to its real matching ")". + A single linear scan tracks string-literal and "#" comment state across + the whole text, so a call name is only recognized outside both — a + commented-out "bazel_dep(...)" line is correctly ignored, rather than a + plain regex search finding the name text inside the comment and reading + its (ignored-by-Bazel) args as if they were live. The same running state + continues through each matched call's body, so a ")" inside a string, + comment, or nested call (e.g. select(...)) never ends the body early — + it always spans to its real matching ")". """ - pattern = re.compile(r"(?:%s)\(" % "|".join(re.escape(n) for n in call_names)) - bodies = [] - for start_match in pattern.finditer(text): - i = start_match.end() + sorted_names = sorted(call_names, key=len, reverse=True) + bodies: list[str] = [] + i = 0 + n = len(text) + in_string: str | None = None + in_comment = False + while i < n: + ch = text[i] + if in_comment: + if ch == "\n": + in_comment = False + i += 1 + continue + if in_string: + if ch == "\\": + i += 2 + continue + if ch == in_string: + in_string = None + i += 1 + continue + if ch == "#": + in_comment = True + i += 1 + continue + if ch in "\"'": + in_string = ch + i += 1 + continue + matched_name = next( + ( + name + for name in sorted_names + if text.startswith(name, i) and text[i + len(name) : i + len(name) + 1] == "(" + ), + None, + ) + if matched_name is None: + i += 1 + continue + i += len(matched_name) + 1 depth = 1 - in_string: str | None = None - in_comment = False body_start = i - while i < len(text) and depth > 0: - ch = text[i] + while i < n and depth > 0: + bch = text[i] if in_comment: - if ch == "\n": + if bch == "\n": in_comment = False elif in_string: - if ch == "\\": + if bch == "\\": i += 1 - elif ch == in_string: + elif bch == in_string: in_string = None - elif ch == "#": + elif bch == "#": in_comment = True - elif ch in "\"'": - in_string = ch - elif ch == "(": + elif bch in "\"'": + in_string = bch + elif bch == "(": depth += 1 - elif ch == ")": + elif bch == ")": depth -= 1 i += 1 bodies.append(text[body_start : i - 1]) @@ -142,6 +180,14 @@ def test_apple_support_bazel_dep_version_survives_an_earlier_paren( )""" self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_ignores_commented_out_call( + self, + ) -> None: + sample = """# bazel_dep(name = "apple_support", version = "9.9.9") +bazel_dep(name = "apple_support", version = "2.10.1") +""" + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' @@ -167,6 +213,13 @@ def test_has_apple_support_override_survives_an_earlier_paren(self) -> None: )""" self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_commented_out_call(self) -> None: + sample = ( + '# archive_override(module_name = "apple_support", ' + 'strip_prefix = "x")\n' + ) + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_other_modules(self) -> None: sample = """ single_version_override( From a718733f0f115a0bf9abdf94bd7f60a5b8cbb28d Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 18:12:33 +0100 Subject: [PATCH 06/29] Tolerate whitespace before "(" and mask comments out of call bodies. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps in _iter_call_bodies(), confirmed with standalone adversarial inputs before fixing: - "archive_override (module_name = ...)" (space before the parenthesis, valid Starlark) matched zero calls, so the guard missed it entirely — a false negative. - The body text it returned still contained comment characters verbatim, so a commented-out "module_name = \"apple_support\"" inside a different module's override made it look like an apple_support override (false positive), and a commented-out stale version before the real one could win the regex search (version guard reads the wrong value). The name/paren scan now skips whitespace between the call name and "(", and the body-building loop drops "#" comment characters from the output (string contents are still kept verbatim) instead of just tracking comment state to avoid breaking paren balance. Added a regression test for each. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 98 ++++++++++++++++++++++++------ 1 file changed, 78 insertions(+), 20 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index f97f10e8..c7f42614 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -28,10 +28,14 @@ def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: the whole text, so a call name is only recognized outside both — a commented-out "bazel_dep(...)" line is correctly ignored, rather than a plain regex search finding the name text inside the comment and reading - its (ignored-by-Bazel) args as if they were live. The same running state - continues through each matched call's body, so a ")" inside a string, - comment, or nested call (e.g. select(...)) never ends the body early — - it always spans to its real matching ")". + its (ignored-by-Bazel) args as if they were live. Whitespace between the + name and "(" is tolerated ("archive_override (..." is valid Starlark). + The same running state continues through each matched call's body: a + ")" inside a string, comment, or nested call (e.g. select(...)) never + ends the body early, and "#" comment text is dropped from the returned + body (string contents are kept verbatim) so a commented-out assignment + inside the call's own args can't be mistaken by a caller's regex for a + live one. """ sorted_names = sorted(call_names, key=len, reverse=True) bodies: list[str] = [] @@ -62,40 +66,63 @@ def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: in_string = ch i += 1 continue - matched_name = next( - ( - name - for name in sorted_names - if text.startswith(name, i) and text[i + len(name) : i + len(name) + 1] == "(" - ), - None, - ) + matched_name = None + paren_index = None + for name in sorted_names: + if text.startswith(name, i): + j = i + len(name) + while j < n and text[j] in " \t\r\n": + j += 1 + if j < n and text[j] == "(": + matched_name = name + paren_index = j + break if matched_name is None: i += 1 continue - i += len(matched_name) + 1 + i = paren_index + 1 depth = 1 - body_start = i + body_chars: list[str] = [] while i < n and depth > 0: bch = text[i] if in_comment: if bch == "\n": in_comment = False - elif in_string: + i += 1 + continue + if in_string: + body_chars.append(bch) if bch == "\\": i += 1 + if i < n: + body_chars.append(text[i]) elif bch == in_string: in_string = None - elif bch == "#": + i += 1 + continue + if bch == "#": in_comment = True - elif bch in "\"'": + i += 1 + continue + if bch in "\"'": in_string = bch - elif bch == "(": + body_chars.append(bch) + i += 1 + continue + if bch == "(": depth += 1 - elif bch == ")": + body_chars.append(bch) + i += 1 + continue + if bch == ")": depth -= 1 + if depth > 0: + body_chars.append(bch) + i += 1 + continue + body_chars.append(bch) i += 1 - bodies.append(text[body_start : i - 1]) + bodies.append("".join(body_chars)) return bodies @@ -188,6 +215,17 @@ def test_apple_support_bazel_dep_version_ignores_commented_out_call( """ self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_ignores_commented_out_version( + self, + ) -> None: + """A stale version in a comment inside the call must not win.""" + sample = """bazel_dep( + name = "apple_support", + # version = "9.9.9" (stale) + version = "2.10.1", +)""" + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' @@ -229,6 +267,26 @@ def test_has_apple_support_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_commented_module_name( + self, + ) -> None: + """A comment mentioning apple_support in a rules_cc override is not + + itself an apple_support override. + """ + sample = """single_version_override( + # module_name = "apple_support" (old pin, no longer used) + module_name = "rules_cc", + version = "0.2.26", +)""" + self.assertFalse(_has_apple_support_override(sample)) + + def test_has_apple_support_override_tolerates_whitespace_before_paren( + self, + ) -> None: + sample = 'archive_override (module_name = "apple_support", strip_prefix = "x")' + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_archive_override(self) -> None: sample = """ archive_override( From d1e88a87fa3d60f26ce99aaf788920422a5c6c31 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 18:22:37 +0100 Subject: [PATCH 07/29] Parse real keyword=value tokens instead of regex-searching masked body text. Mirrors the fix just applied to the identical toolchains_llvm guard in #423: masking "#" comments out of the returned body still left a gap -- a regex search over that body text also matches an unrelated argument's string value that happens to contain assignment-shaped text, e.g. a bazel_dep for rules_cc with patch_cmds = ['historically name = "apple_support", version = "9.9.9"'] would be misread as the apple_support dependency, and the same shape of string value fools the override check. _iter_call_bodies() is replaced by _iter_call_kwargs(), which parses actual "identifier = value" pairs at the call's top-level parenthesis depth only, with the identifier-matcher never offered a character while scanning inside a string or comment. _apple_support_bazel_dep_version() and _has_apple_support_override() now check dict lookups instead of regexes. Added a dedicated test for the string-value case on each helper. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 151 ++++++++++++++++++++--------- 1 file changed, 103 insertions(+), 48 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index c7f42614..815cb692 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -21,24 +21,38 @@ def _repo_root(start: Path | None = None) -> Path: raise AssertionError("could not locate repository root from test file path") -def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: - """Yield the argument-list text of each top-level call to call_names. +_IDENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") + + +def _skip_ws(text: str, i: int, n: int) -> int: + while i < n and text[i] in " \t\r\n": + i += 1 + return i + + +def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, str]]: + """Yield the {keyword: string value} args of each top-level call. A single linear scan tracks string-literal and "#" comment state across - the whole text, so a call name is only recognized outside both — a - commented-out "bazel_dep(...)" line is correctly ignored, rather than a - plain regex search finding the name text inside the comment and reading - its (ignored-by-Bazel) args as if they were live. Whitespace between the - name and "(" is tolerated ("archive_override (..." is valid Starlark). - The same running state continues through each matched call's body: a - ")" inside a string, comment, or nested call (e.g. select(...)) never - ends the body early, and "#" comment text is dropped from the returned - body (string contents are kept verbatim) so a commented-out assignment - inside the call's own args can't be mistaken by a caller's regex for a - live one. + the whole text, so a call name — and each "identifier = value" pair + inside its body — is only recognized outside both. This matters beyond + just truncation: a plain regex search over body *text* (even with + comments stripped) still matches an unrelated argument's *string value* + that happens to contain assignment-shaped text, e.g. + `patch_cmds = ['historically name = "apple_support", ...']` — a + bazel_dep for a different module would be misread as the apple_support + one. Parsing real "identifier = value" pairs only at the call's + top-level parenthesis depth, with identifier-matching paused for every + character inside a string or comment, makes that impossible: nothing + inside a string literal is ever offered to the identifier matcher. + Only simple `identifier = "string"` / `identifier = 'string'` + assignments are captured (list/nested-call values are skipped over for + paren balancing but not parsed), which is everything every caller here + needs. Whitespace between a call name and "(" is tolerated + ("archive_override (..." is valid Starlark). """ sorted_names = sorted(call_names, key=len, reverse=True) - bodies: list[str] = [] + results: list[dict[str, str]] = [] i = 0 n = len(text) in_string: str | None = None @@ -70,9 +84,7 @@ def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: paren_index = None for name in sorted_names: if text.startswith(name, i): - j = i + len(name) - while j < n and text[j] in " \t\r\n": - j += 1 + j = _skip_ws(text, i + len(name), n) if j < n and text[j] == "(": matched_name = name paren_index = j @@ -82,56 +94,70 @@ def _iter_call_bodies(call_names: tuple[str, ...], text: str) -> list[str]: continue i = paren_index + 1 depth = 1 - body_chars: list[str] = [] + kwargs: dict[str, str] = {} while i < n and depth > 0: - bch = text[i] + ch = text[i] if in_comment: - if bch == "\n": + if ch == "\n": in_comment = False i += 1 continue if in_string: - body_chars.append(bch) - if bch == "\\": - i += 1 - if i < n: - body_chars.append(text[i]) - elif bch == in_string: + if ch == "\\": + i += 2 + continue + if ch == in_string: in_string = None i += 1 continue - if bch == "#": + if ch == "#": in_comment = True i += 1 continue - if bch in "\"'": - in_string = bch - body_chars.append(bch) + if ch in "\"'": + in_string = ch i += 1 continue - if bch == "(": + if ch == "(": depth += 1 - body_chars.append(bch) i += 1 continue - if bch == ")": + if ch == ")": depth -= 1 - if depth > 0: - body_chars.append(bch) i += 1 continue - body_chars.append(bch) + if depth == 1: + m = _IDENT_RE.match(text, i) + if m: + ident = m.group(0) + j = _skip_ws(text, m.end(), n) + if j < n and text[j] == "=" and text[j : j + 2] != "==": + j = _skip_ws(text, j + 1, n) + if j < n and text[j] in "\"'": + quote = text[j] + k = j + 1 + value_chars = [] + while k < n and text[k] != quote: + if text[k] == "\\" and k + 1 < n: + value_chars.append(text[k + 1]) + k += 2 + continue + value_chars.append(text[k]) + k += 1 + kwargs[ident] = "".join(value_chars) + i = k + 1 + continue + i = m.end() + continue i += 1 - bodies.append("".join(body_chars)) - return bodies + results.append(kwargs) + return results def _apple_support_bazel_dep_version(module_bazel: str) -> str: - for body in _iter_call_bodies(("bazel_dep",), module_bazel): - name = re.search(r'name\s*=\s*"apple_support"', body) - version = re.search(r'version\s*=\s*"([^"]+)"', body) - if name and version: - return version.group(1) + for kwargs in _iter_call_kwargs(("bazel_dep",), module_bazel): + if kwargs.get("name") == "apple_support" and "version" in kwargs: + return kwargs["version"] raise AssertionError('expected bazel_dep(... name = "apple_support" ...)') @@ -140,9 +166,9 @@ def _has_apple_support_override(module_bazel: str) -> bool: Covers all five override directives (archive_override, git_override, local_path_override, multiple_version_override, single_version_override) - — not just single_version_override — and matches each call's full - argument body regardless of argument order or formatting, rather than - assuming module_name is the first line after the opening "(". + — not just single_version_override — and matches each call's keyword + args regardless of argument order or formatting, rather than assuming + module_name is the first line after the opening "(". """ override_names = ( "archive_override", @@ -151,8 +177,8 @@ def _has_apple_support_override(module_bazel: str) -> bool: "multiple_version_override", "single_version_override", ) - for body in _iter_call_bodies(override_names, module_bazel): - if re.search(r'module_name\s*=\s*"apple_support"', body): + for kwargs in _iter_call_kwargs(override_names, module_bazel): + if kwargs.get("module_name") == "apple_support": return True return False @@ -226,6 +252,21 @@ def test_apple_support_bazel_dep_version_ignores_commented_out_version( )""" self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_ignores_fake_assignment_in_string( + self, + ) -> None: + """Assignment-shaped text inside an unrelated string value for a + + different module's bazel_dep must not be read as real args. + """ + sample = """bazel_dep( + name = "rules_cc", + version = "0.2.26", + patch_cmds = ['historically name = "apple_support", version = "9.9.9"'], +)""" + with self.assertRaises(AssertionError): + _apple_support_bazel_dep_version(sample) + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' @@ -281,6 +322,20 @@ def test_has_apple_support_override_ignores_commented_module_name( )""" self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_fake_assignment_in_string( + self, + ) -> None: + """Assignment-shaped text inside an unrelated string value for a + + different module's override must not be read as real args. + """ + sample = """single_version_override( + module_name = "rules_cc", + version = "0.2.26", + patch_cmds = ['the module_name = "apple_support" override is legacy'], +)""" + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_tolerates_whitespace_before_paren( self, ) -> None: From 24db5d11d46a1fddefa07b4240cc80e9ba151998 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 19:11:40 +0100 Subject: [PATCH 08/29] Handle triple-quoted strings and identifier boundaries correctly. Mirrors the fix just applied to the identical toolchains_llvm guard in #423, confirmed here with the same standalone repros before fixing: - The scanner toggled string state per single quote character, so a triple-quoted string wasn't kept opaque as one unit; a triple-quoted string with an odd number of embedded quote characters before a fake bazel_dep(...) desynced the toggle and exposed it as real code. - The call-name/keyword matcher had no identifier-boundary check, so "legacy_bazel_dep(...)", "bazel_depfoo(...)", and a qualified "extensions.bazel_dep(...)" were all misread as a real bazel_dep call. _skip_string()/_skip_comment() replace the per-character toggle with single jumps that treat a triple-quoted delimiter as one atomic unit, and _is_identifier_boundary() rejects a name match whose preceding character could continue an identifier or is "." (attribute access). Added a dedicated regression test for each. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 181 ++++++++++++++++++----------- 1 file changed, 116 insertions(+), 65 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 815cb692..6c08eb5e 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -30,22 +30,81 @@ def _skip_ws(text: str, i: int, n: int) -> int: return i +def _skip_comment(text: str, i: int, n: int) -> int: + while i < n and text[i] != "\n": + i += 1 + return i + + +def _skip_string(text: str, i: int, n: int) -> tuple[int, str | None]: + """Return (end_index, value) for the string literal starting at text[i]. + + Handles Starlark triple-quoted strings as a single opaque unit (value + is None — never needed as a captured kwarg value here, and critically, + none of its content — including a quote character that would otherwise + look like the end of a single-quoted string — is scanned character by + character for a fake nested call). Ordinary quoted strings are handled + with backslash escapes (value is the unescaped content). + """ + quote = text[i] + if text[i : i + 3] == quote * 3: + delim = quote * 3 + j = i + 3 + while j < n: + if text[j] == "\\": + j += 2 + continue + if text[j : j + 3] == delim: + return j + 3, None + j += 1 + return n, None + j = i + 1 + chars: list[str] = [] + while j < n: + if text[j] == "\\" and j + 1 < n: + chars.append(text[j + 1]) + j += 2 + continue + if text[j] == quote: + return j + 1, "".join(chars) + chars.append(text[j]) + j += 1 + return n, "".join(chars) + + +def _is_identifier_boundary(text: str, i: int) -> bool: + """True unless the character before i could continue an identifier. + + Rejects matching a call/keyword name in the middle of a longer + identifier (e.g. "legacy_bazel_dep(" or "bazel_depfoo(") and rejects a + qualified/attribute call (e.g. "extensions.bazel_dep(") by also + treating a preceding "." as non-boundary. + """ + if i == 0: + return True + prev = text[i - 1] + return not (prev.isalnum() or prev == "_" or prev == ".") + + def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, str]]: """Yield the {keyword: string value} args of each top-level call. - A single linear scan tracks string-literal and "#" comment state across - the whole text, so a call name — and each "identifier = value" pair - inside its body — is only recognized outside both. This matters beyond - just truncation: a plain regex search over body *text* (even with - comments stripped) still matches an unrelated argument's *string value* - that happens to contain assignment-shaped text, e.g. + A single linear scan recognizes a call name, and each "identifier = + value" pair inside its body, only at a genuine identifier boundary and + only outside strings/comments. This matters beyond just truncation: a + plain regex search over body *text* (even with comments stripped) + still matches an unrelated argument's *string value* that happens to + contain assignment-shaped text, e.g. `patch_cmds = ['historically name = "apple_support", ...']` — a bazel_dep for a different module would be misread as the apple_support one. Parsing real "identifier = value" pairs only at the call's top-level parenthesis depth, with identifier-matching paused for every - character inside a string or comment, makes that impossible: nothing - inside a string literal is ever offered to the identifier matcher. - Only simple `identifier = "string"` / `identifier = 'string'` + character inside a string or comment (string/comment skipping is a + single jump via _skip_string/_skip_comment, not a per-character toggle, + so a triple-quoted string's content — including embedded quote + characters — is never scanned as code), makes that impossible: nothing + inside a string literal or comment is ever offered to the identifier + matcher. Only simple `identifier = "string"` / `identifier = 'string'` assignments are captured (list/nested-call values are skipped over for paren balancing but not parsed), which is everything every caller here needs. Whitespace between a call name and "(" is tolerated @@ -55,40 +114,24 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, results: list[dict[str, str]] = [] i = 0 n = len(text) - in_string: str | None = None - in_comment = False while i < n: ch = text[i] - if in_comment: - if ch == "\n": - in_comment = False - i += 1 - continue - if in_string: - if ch == "\\": - i += 2 - continue - if ch == in_string: - in_string = None - i += 1 - continue if ch == "#": - in_comment = True - i += 1 + i = _skip_comment(text, i, n) continue if ch in "\"'": - in_string = ch - i += 1 + i, _ = _skip_string(text, i, n) continue matched_name = None paren_index = None - for name in sorted_names: - if text.startswith(name, i): - j = _skip_ws(text, i + len(name), n) - if j < n and text[j] == "(": - matched_name = name - paren_index = j - break + if _is_identifier_boundary(text, i): + for name in sorted_names: + if text.startswith(name, i): + j = _skip_ws(text, i + len(name), n) + if j < n and text[j] == "(": + matched_name = name + paren_index = j + break if matched_name is None: i += 1 continue @@ -97,26 +140,11 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, kwargs: dict[str, str] = {} while i < n and depth > 0: ch = text[i] - if in_comment: - if ch == "\n": - in_comment = False - i += 1 - continue - if in_string: - if ch == "\\": - i += 2 - continue - if ch == in_string: - in_string = None - i += 1 - continue if ch == "#": - in_comment = True - i += 1 + i = _skip_comment(text, i, n) continue if ch in "\"'": - in_string = ch - i += 1 + i, _ = _skip_string(text, i, n) continue if ch == "(": depth += 1 @@ -126,7 +154,7 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, depth -= 1 i += 1 continue - if depth == 1: + if depth == 1 and _is_identifier_boundary(text, i): m = _IDENT_RE.match(text, i) if m: ident = m.group(0) @@ -134,18 +162,10 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if j < n and text[j] == "=" and text[j : j + 2] != "==": j = _skip_ws(text, j + 1, n) if j < n and text[j] in "\"'": - quote = text[j] - k = j + 1 - value_chars = [] - while k < n and text[k] != quote: - if text[k] == "\\" and k + 1 < n: - value_chars.append(text[k + 1]) - k += 2 - continue - value_chars.append(text[k]) - k += 1 - kwargs[ident] = "".join(value_chars) - i = k + 1 + end, value = _skip_string(text, j, n) + if value is not None: + kwargs[ident] = value + i = end continue i = m.end() continue @@ -267,6 +287,37 @@ def test_apple_support_bazel_dep_version_ignores_fake_assignment_in_string( with self.assertRaises(AssertionError): _apple_support_bazel_dep_version(sample) + def test_apple_support_bazel_dep_version_ignores_fake_call_in_triple_quote( + self, + ) -> None: + """A fake bazel_dep(...) inside a triple-quoted string -- even one + + with an odd number of embedded quote characters before it, which + would desync a naive per-quote toggle -- must stay opaque. + """ + sample = '''""" +Note: the old pin "mentioned here used +bazel_dep(name = "apple_support", version = "9.9.9") +""" +bazel_dep(name = "apple_support", version = "2.10.1") +''' + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + + def test_apple_support_bazel_dep_version_rejects_identifier_substring( + self, + ) -> None: + """"bazel_dep" appearing inside a longer identifier, or as a + + qualified/attribute call, is not a real bazel_dep call. + """ + for sample in ( + 'legacy_bazel_dep(name = "apple_support", version = "9.9.9")', + 'bazel_depfoo(name = "apple_support", version = "9.9.9")', + 'extensions.bazel_dep(name = "apple_support", version = "9.9.9")', + ): + with self.assertRaises(AssertionError): + _apple_support_bazel_dep_version(sample) + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' From 68367e33549c63bb0c08789734c2b54efd7dd911 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 19:41:49 +0100 Subject: [PATCH 09/29] Capture positional module_name/name arguments, not just keyword form. Bazel's *_override APIs and bazel_dep all accept their first argument (module_name / name) positionally, e.g. archive_override("apple_support", urls = [...]). _iter_call_kwargs() only captured "identifier = value" pairs, so a positional call like that produced no module_name entry and _has_apple_support_override() returned False despite a real override being present. Confirmed with a standalone 7-case repro before fixing (the positional case failed; all 7 pass after). A bare top-level string (not preceded by "identifier =") is now captured under the reserved _POSITIONAL_KEY. "Top-level" now also tracks [...]/{...} nesting depth, not just parens, so a string that's merely an *element* of a list value (e.g. urls = ["apple_support"]) is correctly never mistaken for the call's own positional argument. Both consumer functions now check the keyword name first, falling back to the positional one. Added a regression test for the positional case and the list-element false-positive case on each helper. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 85 ++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 6c08eb5e..ea7b2216 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -22,6 +22,7 @@ def _repo_root(start: Path | None = None) -> Path: _IDENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +_POSITIONAL_KEY = "__positional__" def _skip_ws(text: str, i: int, n: int) -> int: @@ -109,6 +110,14 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, paren balancing but not parsed), which is everything every caller here needs. Whitespace between a call name and "(" is tolerated ("archive_override (..." is valid Starlark). + + Bazel's `*_override`/`bazel_dep` APIs all accept their first argument + (module_name / name) positionally — a bare top-level string before any + "identifier =" is stored under `_POSITIONAL_KEY`, letting callers treat + it as that first parameter. "Top-level" also tracks `[...]`/`{...}` + nesting (not just parens), so a string that's an *element* of a list + value (e.g. inside `urls = [...]`) is never mistaken for a positional + call argument. """ sorted_names = sorted(call_names, key=len, reverse=True) results: list[dict[str, str]] = [] @@ -137,13 +146,22 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, continue i = paren_index + 1 depth = 1 + bracket_depth = 0 + brace_depth = 0 kwargs: dict[str, str] = {} while i < n and depth > 0: ch = text[i] if ch == "#": i = _skip_comment(text, i, n) continue + at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 if ch in "\"'": + if at_top and _POSITIONAL_KEY not in kwargs: + end, value = _skip_string(text, i, n) + if value is not None: + kwargs[_POSITIONAL_KEY] = value + i = end + continue i, _ = _skip_string(text, i, n) continue if ch == "(": @@ -154,7 +172,23 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, depth -= 1 i += 1 continue - if depth == 1 and _is_identifier_boundary(text, i): + if ch == "[": + bracket_depth += 1 + i += 1 + continue + if ch == "]": + bracket_depth -= 1 + i += 1 + continue + if ch == "{": + brace_depth += 1 + i += 1 + continue + if ch == "}": + brace_depth -= 1 + i += 1 + continue + if at_top and _is_identifier_boundary(text, i): m = _IDENT_RE.match(text, i) if m: ident = m.group(0) @@ -176,7 +210,8 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, def _apple_support_bazel_dep_version(module_bazel: str) -> str: for kwargs in _iter_call_kwargs(("bazel_dep",), module_bazel): - if kwargs.get("name") == "apple_support" and "version" in kwargs: + name = kwargs.get("name") or kwargs.get(_POSITIONAL_KEY) + if name == "apple_support" and "version" in kwargs: return kwargs["version"] raise AssertionError('expected bazel_dep(... name = "apple_support" ...)') @@ -187,8 +222,9 @@ def _has_apple_support_override(module_bazel: str) -> bool: Covers all five override directives (archive_override, git_override, local_path_override, multiple_version_override, single_version_override) — not just single_version_override — and matches each call's keyword - args regardless of argument order or formatting, rather than assuming - module_name is the first line after the opening "(". + or first-positional module_name argument regardless of argument order + or formatting, rather than assuming module_name is the first line + after the opening "(". """ override_names = ( "archive_override", @@ -198,7 +234,8 @@ def _has_apple_support_override(module_bazel: str) -> bool: "single_version_override", ) for kwargs in _iter_call_kwargs(override_names, module_bazel): - if kwargs.get("module_name") == "apple_support": + module_name = kwargs.get("module_name") or kwargs.get(_POSITIONAL_KEY) + if module_name == "apple_support": return True return False @@ -242,6 +279,16 @@ def test_apple_support_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "2.10.1", name = "apple_support")' self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_accepts_positional_name( + self, + ) -> None: + """bazel_dep's name is positional too: bazel_dep("apple_support", + + version = ...) is valid Starlark. + """ + sample = 'bazel_dep("apple_support", version = "2.10.1")' + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_survives_an_earlier_paren( self, ) -> None: @@ -324,6 +371,34 @@ def test_has_apple_support_override_detects_same_line_args(self) -> None: ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_positional_module_name( + self, + ) -> None: + """The override APIs accept module_name as their first positional + + argument; archive_override("apple_support", ...) is as real an + override as the keyword form. + """ + sample = ( + 'archive_override("apple_support", ' + 'urls = ["https://example.com/apple_support.tar.gz"])' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_ignores_positional_string_in_list( + self, + ) -> None: + """A bare string that is an *element* of a list value (not the + + call's own first positional argument) must not be read as + module_name. + """ + sample = """archive_override( + module_name = "rules_cc", + urls = ["apple_support"], +)""" + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_reordered_args(self) -> None: sample = """ single_version_override( From fa49355b6d95d8027ae84181b12816049d1a61db Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 20:04:26 +0100 Subject: [PATCH 10/29] Extract triple-quoted string values instead of discarding them. _skip_string() unconditionally returned None for triple-quoted literals, assuming they'd never be used as a captured kwarg value -- but a valid single_version_override(module_name = \"\"\"apple_support\"\"\", ...) is real (if unusual) Starlark, and _has_apple_support_override() silently returned False for it. Confirmed with a standalone repro before fixing. _skip_string() now collects the content between a triple-quote delimiter the same way it already does for an ordinary quoted string, while still matching the closing delimiter as the 3-character sequence (never a single quote), so the opacity property from the earlier fix -- an embedded quote never gets scanned as code -- is unaffected. Removed the now-dead "if value is not None" checks in _iter_call_kwargs() since the function no longer returns None. Added a dedicated regression test for the exact triple-quoted-module-name example. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 47 ++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index ea7b2216..11f54a93 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -37,30 +37,36 @@ def _skip_comment(text: str, i: int, n: int) -> int: return i -def _skip_string(text: str, i: int, n: int) -> tuple[int, str | None]: +def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: """Return (end_index, value) for the string literal starting at text[i]. - Handles Starlark triple-quoted strings as a single opaque unit (value - is None — never needed as a captured kwarg value here, and critically, - none of its content — including a quote character that would otherwise - look like the end of a single-quoted string — is scanned character by - character for a fake nested call). Ordinary quoted strings are handled - with backslash escapes (value is the unescaped content). + Handles Starlark triple-quoted strings as a single opaque unit — the + closing delimiter is matched as the 3-character sequence, never a + single quote character, so none of the content in between (including + an embedded quote that would otherwise look like the end of an + ordinary string) is ever scanned character by character for a fake + nested call — while still extracting its value just like an ordinary + quoted string, so a (valid, if unusual) triple-quoted keyword value is + captured rather than silently dropped. Both forms handle backslash + escapes (value is the unescaped content). """ quote = text[i] if text[i : i + 3] == quote * 3: delim = quote * 3 j = i + 3 + chars: list[str] = [] while j < n: - if text[j] == "\\": + if text[j] == "\\" and j + 1 < n: + chars.append(text[j + 1]) j += 2 continue if text[j : j + 3] == delim: - return j + 3, None + return j + 3, "".join(chars) + chars.append(text[j]) j += 1 - return n, None + return n, "".join(chars) j = i + 1 - chars: list[str] = [] + chars = [] while j < n: if text[j] == "\\" and j + 1 < n: chars.append(text[j + 1]) @@ -158,8 +164,7 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if ch in "\"'": if at_top and _POSITIONAL_KEY not in kwargs: end, value = _skip_string(text, i, n) - if value is not None: - kwargs[_POSITIONAL_KEY] = value + kwargs[_POSITIONAL_KEY] = value i = end continue i, _ = _skip_string(text, i, n) @@ -197,8 +202,7 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, j = _skip_ws(text, j + 1, n) if j < n and text[j] in "\"'": end, value = _skip_string(text, j, n) - if value is not None: - kwargs[ident] = value + kwargs[ident] = value i = end continue i = m.end() @@ -385,6 +389,19 @@ def test_has_apple_support_override_detects_positional_module_name( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_triple_quoted_module_name( + self, + ) -> None: + """A triple-quoted keyword value is valid Starlark and must still + + be extracted, not silently treated as absent. + """ + sample = ( + 'single_version_override(module_name = """apple_support""", ' + 'version = "1.24.2")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_positional_string_in_list( self, ) -> None: From f59da4db35d440aa8904b30a934403e198e9f3f8 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 21:26:18 +0100 Subject: [PATCH 11/29] Decode Starlark backslash escapes instead of stripping the backslash. _skip_string() just discarded each backslash and kept the next character literally, which isn't what the escape actually means: "\x61pple_support" (a valid Starlark hex escape for "apple_support") decoded to the wrong string "x61pple_support", so an override written with an escaped module_name bypassed the guard entirely. Confirmed with a standalone repro before fixing. _decode_escape() now handles Starlark's simple single-character escapes (\n, \t, \\, \', \", ...), up to 3 octal digits, and exactly two hex digits after \x, used by both the triple-quoted and ordinary-string branches of _skip_string(). An unrecognized escape is left as a literal backslash + next character rather than erroring, since this is a test helper comparing against known plain-ASCII identifiers, not a production Starlark interpreter. Added a dedicated regression test for the exact hex-escape example from the review. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 73 +++++++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 6 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 11f54a93..6d69abce 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -37,6 +37,54 @@ def _skip_comment(text: str, i: int, n: int) -> int: return i +_SIMPLE_ESCAPES = { + "\\": "\\", + "'": "'", + '"': '"', + "a": "\a", + "b": "\b", + "f": "\f", + "n": "\n", + "r": "\r", + "t": "\t", + "v": "\v", +} +_HEX_DIGITS = "0123456789abcdefABCDEF" +_OCTAL_DIGITS = "01234567" + + +def _decode_escape(text: str, j: int, n: int) -> tuple[int, str]: + """Decode one backslash escape starting at text[j] == "\\". + + Returns (new_j, decoded_text) for Starlark's string-literal escapes: + the simple single-character escapes (\\n, \\t, \\\\, ...), up to 3 + octal digits, and exactly two hex digits after \\x. Discarding the + backslash and keeping the next character literally — the previous + behavior here — decodes "\\x61pple_support" (a valid Starlark escape + for "apple_support") to the wrong string "x61pple_support", letting an + escaped module_name bypass the override guard. An escape this doesn't + recognize is left as a literal backslash + next character. + """ + if j + 1 >= n: + return j + 1, "\\" + c = text[j + 1] + if c in _SIMPLE_ESCAPES: + return j + 2, _SIMPLE_ESCAPES[c] + if c == "x": + hex_digits = text[j + 2 : j + 4] + if len(hex_digits) == 2 and all(d in _HEX_DIGITS for d in hex_digits): + return j + 4, chr(int(hex_digits, 16)) + return j + 2, "x" + if c in _OCTAL_DIGITS: + k = j + 1 + digits = "" + while k < n and len(digits) < 3 and text[k] in _OCTAL_DIGITS: + digits += text[k] + k += 1 + return k, chr(int(digits, 8)) + return j + 2, "\\" + c + + def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: """Return (end_index, value) for the string literal starting at text[i]. @@ -47,8 +95,8 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: ordinary string) is ever scanned character by character for a fake nested call — while still extracting its value just like an ordinary quoted string, so a (valid, if unusual) triple-quoted keyword value is - captured rather than silently dropped. Both forms handle backslash - escapes (value is the unescaped content). + captured rather than silently dropped. Both forms decode backslash + escapes via _decode_escape() (value is the unescaped content). """ quote = text[i] if text[i : i + 3] == quote * 3: @@ -57,8 +105,8 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: chars: list[str] = [] while j < n: if text[j] == "\\" and j + 1 < n: - chars.append(text[j + 1]) - j += 2 + j, decoded = _decode_escape(text, j, n) + chars.append(decoded) continue if text[j : j + 3] == delim: return j + 3, "".join(chars) @@ -69,8 +117,8 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: chars = [] while j < n: if text[j] == "\\" and j + 1 < n: - chars.append(text[j + 1]) - j += 2 + j, decoded = _decode_escape(text, j, n) + chars.append(decoded) continue if text[j] == quote: return j + 1, "".join(chars) @@ -402,6 +450,19 @@ def test_has_apple_support_override_detects_triple_quoted_module_name( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_decodes_hex_escape_in_module_name( + self, + ) -> None: + """\\x61 is a valid Starlark hex escape for "a"; the decoded value + + must still be compared, not the raw escape text. + """ + sample = ( + 'single_version_override(module_name = "\\x61pple_support", ' + 'version = "1.24.2")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_positional_string_in_list( self, ) -> None: From 539a86cffe83dae34311a10f67abd81733807fc6 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 00:02:38 +0100 Subject: [PATCH 12/29] Decode Starlark \u/\U Unicode escapes in module_name values. _decode_escape() only handled \x hex and octal escapes; \uXXXX (4 hex digits) and \UXXXXXXXX (8 hex digits), both valid Starlark Unicode escapes per the Starlark spec, fell through to the "unrecognized escape" branch and were left as literal backslash + "u"/"U" text. A module_name written as "apple_support" therefore decoded to the wrong string and bypassed _has_apple_support_override(). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 48 ++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 6d69abce..fe91c8c9 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -58,12 +58,15 @@ def _decode_escape(text: str, j: int, n: int) -> tuple[int, str]: Returns (new_j, decoded_text) for Starlark's string-literal escapes: the simple single-character escapes (\\n, \\t, \\\\, ...), up to 3 - octal digits, and exactly two hex digits after \\x. Discarding the - backslash and keeping the next character literally — the previous - behavior here — decodes "\\x61pple_support" (a valid Starlark escape - for "apple_support") to the wrong string "x61pple_support", letting an - escaped module_name bypass the override guard. An escape this doesn't - recognize is left as a literal backslash + next character. + octal digits, exactly two hex digits after \\x, and the \\uXXXX / + \\UXXXXXXXX Unicode code point forms (exactly 4 / 8 hex digits). + Discarding the backslash and keeping the next character literally — + the previous behavior here — decodes "\\x61pple_support" (a valid + Starlark escape for "apple_support") to the wrong string + "x61pple_support", letting an escaped module_name bypass the override + guard; "\\u0061pple_support" was the same bug for Unicode escapes. An + escape this doesn't recognize is left as a literal backslash + next + character. """ if j + 1 >= n: return j + 1, "\\" @@ -75,6 +78,12 @@ def _decode_escape(text: str, j: int, n: int) -> tuple[int, str]: if len(hex_digits) == 2 and all(d in _HEX_DIGITS for d in hex_digits): return j + 4, chr(int(hex_digits, 16)) return j + 2, "x" + if c in ("u", "U"): + width = 4 if c == "u" else 8 + hex_digits = text[j + 2 : j + 2 + width] + if len(hex_digits) == width and all(d in _HEX_DIGITS for d in hex_digits): + return j + 2 + width, chr(int(hex_digits, 16)) + return j + 2, c if c in _OCTAL_DIGITS: k = j + 1 digits = "" @@ -463,6 +472,33 @@ def test_has_apple_support_override_decodes_hex_escape_in_module_name( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_decodes_unicode_escape_in_module_name( + self, + ) -> None: + """\\u0061 is a valid Starlark 4-hex-digit Unicode escape for "a"; + + the decoded value must still be compared, not the raw escape text. + """ + sample = ( + 'single_version_override(module_name = "\\u0061pple_support", ' + 'version = "1.24.2")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_decodes_big_unicode_escape_in_module_name( + self, + ) -> None: + """\\U00000061 is a valid Starlark 8-hex-digit Unicode escape for + + "a"; the decoded value must still be compared, not the raw escape + text. + """ + sample = ( + 'single_version_override(module_name = "\\U00000061pple_support", ' + 'version = "1.24.2")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_positional_string_in_list( self, ) -> None: From 975ebeef01c1cd0b03fa5eed9826dbcbb07a992b Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 11:45:43 +0100 Subject: [PATCH 13/29] Fail closed on a non-literal module_name in override detection. _iter_call_kwargs() silently dropped a module_name/name argument whose value was a variable reference or a string concatenation (only the first literal fragment of a concatenation was captured, and a bare variable reference was dropped entirely), so _has_apple_support_override() read it the same as "not apple_support" and missed a real override whose target is computed rather than written as a literal string. Added an _UNRESOLVED sentinel: a non-literal keyword value, a bare identifier used as the first positional argument, and a literal immediately followed by "+" are all marked unresolved instead of silently absent, and the guard now fails closed (treats unresolved the same as a match) rather than assuming it's safe. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 98 +++++++++++++++++++++++++++--- 1 file changed, 91 insertions(+), 7 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index fe91c8c9..36d46a55 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -23,6 +23,13 @@ def _repo_root(start: Path | None = None) -> Path: _IDENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") _POSITIONAL_KEY = "__positional__" +# Sentinel for a module_name/name argument whose value is a variable +# reference or a string concatenation rather than a single literal, so the +# true value can't be read back from the source text. Treated as a possible +# match by every caller here (fail closed) rather than silently absent, +# since the latter would let a real override on the guarded module slip +# through whenever its target is computed instead of written as a literal. +_UNRESOLVED = object() def _skip_ws(text: str, i: int, n: int) -> int: @@ -136,6 +143,18 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: return n, "".join(chars) +def _followed_by_concat(text: str, i: int, n: int) -> bool: + """True if the next non-whitespace character at/after i is "+". + + A string literal immediately followed by "+" is only part of a larger + concatenated expression ("a" + b, or "a" + "b") — the literal just + captured is not the argument's whole value, so the caller must not + treat it as the resolved value. + """ + j = _skip_ws(text, i, n) + return j < n and text[j] == "+" + + def _is_identifier_boundary(text: str, i: int) -> bool: """True unless the character before i could continue an identifier. @@ -221,7 +240,9 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if ch in "\"'": if at_top and _POSITIONAL_KEY not in kwargs: end, value = _skip_string(text, i, n) - kwargs[_POSITIONAL_KEY] = value + kwargs[_POSITIONAL_KEY] = ( + _UNRESOLVED if _followed_by_concat(text, end, n) else value + ) i = end continue i, _ = _skip_string(text, i, n) @@ -256,12 +277,33 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, ident = m.group(0) j = _skip_ws(text, m.end(), n) if j < n and text[j] == "=" and text[j : j + 2] != "==": - j = _skip_ws(text, j + 1, n) - if j < n and text[j] in "\"'": - end, value = _skip_string(text, j, n) - kwargs[ident] = value + rhs_start = _skip_ws(text, j + 1, n) + if rhs_start < n and text[rhs_start] in "\"'": + end, value = _skip_string(text, rhs_start, n) + kwargs[ident] = ( + _UNRESOLVED + if _followed_by_concat(text, end, n) + else value + ) i = end continue + # A non-literal right-hand side (a variable + # reference, a function call, ...) can't be read + # back from the source text. Fail closed: mark it + # unresolved rather than leaving it absent, which + # would read identically to the argument never + # having been given at all. + kwargs[ident] = _UNRESOLVED + i = rhs_start + continue + # Not "ident = ...": a bare identifier is a variable + # used as a value. If it's the first value-shaped + # token in the call, it's being passed as the first + # positional argument; fail closed the same way as a + # non-literal keyword value, rather than silently + # dropping it. + if _POSITIONAL_KEY not in kwargs: + kwargs[_POSITIONAL_KEY] = _UNRESOLVED i = m.end() continue i += 1 @@ -285,7 +327,12 @@ def _has_apple_support_override(module_bazel: str) -> bool: — not just single_version_override — and matches each call's keyword or first-positional module_name argument regardless of argument order or formatting, rather than assuming module_name is the first line - after the opening "(". + after the opening "(". Fails closed (returns True) for an override + whose module_name/name is a variable reference or a string + concatenation — _UNRESOLVED — since the real text that will resolve + to at runtime can't be read back from the source, and treating that + the same as "not apple_support" would let a real override on a + computed target slip past this guard undetected. """ override_names = ( "archive_override", @@ -296,7 +343,7 @@ def _has_apple_support_override(module_bazel: str) -> bool: ) for kwargs in _iter_call_kwargs(override_names, module_bazel): module_name = kwargs.get("module_name") or kwargs.get(_POSITIONAL_KEY) - if module_name == "apple_support": + if module_name == "apple_support" or module_name is _UNRESOLVED: return True return False @@ -499,6 +546,43 @@ def test_has_apple_support_override_decodes_big_unicode_escape_in_module_name( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_keyword_variable_module_name( + self, + ) -> None: + """module_name set to a variable reference can't be read back from + + the source text; the guard must fail closed (treat it as a + possible apple_support override) rather than silently treating + the override as targeting some other module. + """ + sample = 'archive_override(module_name = target, strip_prefix = "x")' + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_fails_closed_on_positional_variable_module_name( + self, + ) -> None: + """Same as the keyword case, but module_name is passed positionally + + as a variable reference. + """ + sample = 'archive_override(target, strip_prefix = "x")' + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_fails_closed_on_concatenated_module_name( + self, + ) -> None: + """module_name assembled via string concatenation has no single + + literal value in the source text; only the first fragment + ("apple_") would be captured by a naive literal-only parser, which + must not be mistaken for the whole, real value. + """ + sample = ( + 'archive_override(module_name = "apple_" + "support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_positional_string_in_list( self, ) -> None: From 3a2036243e287a3635ec10d6497dc73b25fb91e6 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 21:33:01 +0100 Subject: [PATCH 14/29] Fail closed for any incomplete literal, not just one followed by "+". MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _followed_by_concat() only recognized a directly adjacent "+", so module_name = "xapple_support"[1:] (slicing), "apple_%s" % "support" (percent formatting), or a concatenation whose "+" follows an inline comment all still read as the bare literal fragment and could bypass _has_apple_support_override() despite resolving to apple_support. Replaced it with _is_complete_literal(): a literal is only treated as the argument's whole, resolved value when the next token (skipping whitespace and comments) is "," or ")" — the argument/call terminator. Anything else means it's a fragment of a larger expression, so it's marked unresolved (fail closed) regardless of what the operator is. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 80 +++++++++++++++++++++++++----- 1 file changed, 67 insertions(+), 13 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 36d46a55..3e9e2a4a 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -143,16 +143,28 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: return n, "".join(chars) -def _followed_by_concat(text: str, i: int, n: int) -> bool: - """True if the next non-whitespace character at/after i is "+". - - A string literal immediately followed by "+" is only part of a larger - concatenated expression ("a" + b, or "a" + "b") — the literal just - captured is not the argument's whole value, so the caller must not - treat it as the resolved value. +def _is_complete_literal(text: str, i: int, n: int) -> bool: + """True if the string literal ending at i is a whole call argument. + + Skips whitespace and "#" comments (repeatedly, so a comment followed + by more whitespace/another comment is all consumed) after the + literal. The literal is the argument's complete, resolved value only + if what follows is "," (ending this argument) or ")" (ending the + call) — anything else, including "+" (concatenation, even across an + intervening comment), "[" (slicing), "%" (string formatting), "." + (a method call), or any other operator, means the literal is merely a + fragment of a larger expression whose true value can't be read back + from the source text. """ - j = _skip_ws(text, i, n) - return j < n and text[j] == "+" + j = i + while True: + skipped = _skip_ws(text, j, n) + if skipped < n and text[skipped] == "#": + skipped = _skip_comment(text, skipped, n) + if skipped == j: + break + j = skipped + return j < n and text[j] in ",)" def _is_identifier_boundary(text: str, i: int) -> bool: @@ -241,7 +253,7 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if at_top and _POSITIONAL_KEY not in kwargs: end, value = _skip_string(text, i, n) kwargs[_POSITIONAL_KEY] = ( - _UNRESOLVED if _followed_by_concat(text, end, n) else value + value if _is_complete_literal(text, end, n) else _UNRESOLVED ) i = end continue @@ -281,9 +293,9 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if rhs_start < n and text[rhs_start] in "\"'": end, value = _skip_string(text, rhs_start, n) kwargs[ident] = ( - _UNRESOLVED - if _followed_by_concat(text, end, n) - else value + value + if _is_complete_literal(text, end, n) + else _UNRESOLVED ) i = end continue @@ -583,6 +595,48 @@ def test_has_apple_support_override_fails_closed_on_concatenated_module_name( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_concat_split_by_comment( + self, + ) -> None: + """A comment between the first fragment and the "+" must not hide + + the concatenation: the literal still isn't the whole argument. + """ + sample = """archive_override( + module_name = "apple_" # fixes a typo below + + "support", + strip_prefix = "x", +)""" + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_fails_closed_on_sliced_module_name( + self, + ) -> None: + """A slice expression is a literal followed by "[...]", not a + + complete argument by itself; the literal alone must not be + mistaken for the slice's actual result. + """ + sample = ( + 'archive_override(module_name = "xapple_support"[1:], ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_fails_closed_on_percent_formatted_module_name( + self, + ) -> None: + """A "%"-formatted literal is a template, not the resolved value; + + "apple_%s" % "support" resolves to "apple_support" but the + literal "apple_%s" alone must not be mistaken for it. + """ + sample = ( + 'archive_override(module_name = "apple_%s" % "support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_positional_string_in_list( self, ) -> None: From 2423efe751cfec29e2a3a5efad0b8f90e0a3a675 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 21:38:34 +0100 Subject: [PATCH 15/29] Fail closed on a parenthesized positional module_name expression. Mirrors the same fix on #423: a top-level "(" that isn't preceded by "ident =" opens a parenthesized positional expression (e.g. the first argument written as ("apple_" + "support")) rather than a literal or a bare variable reference. _iter_call_kwargs() descended straight into it without ever recording _POSITIONAL_KEY, so _has_apple_support_override() read the module_name as absent and missed the override, contradicting the fail-closed behavior added for other non-literal forms. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 3e9e2a4a..e334fa9d 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -260,6 +260,14 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, i, _ = _skip_string(text, i, n) continue if ch == "(": + # A "(" at the top level, not preceded by "ident =", opens + # a parenthesized positional expression (e.g. the first + # argument written as ("a" + "b")) rather than a literal + # or a bare variable reference; mark it unresolved before + # descending into it, the same as any other non-literal + # positional value. + if at_top and _POSITIONAL_KEY not in kwargs: + kwargs[_POSITIONAL_KEY] = _UNRESOLVED depth += 1 i += 1 continue @@ -580,6 +588,19 @@ def test_has_apple_support_override_fails_closed_on_positional_variable_module_n sample = 'archive_override(target, strip_prefix = "x")' self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_parenthesized_positional_module_name( + self, + ) -> None: + """A parenthesized expression passed as the first positional + + argument (e.g. ("apple_" + "support")) can't be read back as a + literal from the source text either; it must fail closed the + same as any other non-literal positional value, not be silently + skipped while descending into the parens. + """ + sample = 'archive_override(("apple_" + "support"), strip_prefix = "x")' + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_concatenated_module_name( self, ) -> None: From 614a2f40ea02aa442f664837077bb44eb6af4a87 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 19:17:21 +0100 Subject: [PATCH 16/29] Fail closed on list/dict positional module_name expressions. Mirrors the same fix on #423: a top-level list or dict expression opening the first positional argument (e.g. ["apple_support"][0]) was descended into without ever recording _POSITIONAL_KEY, so module_name read as absent and _has_apple_support_override() missed the override. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 35 ++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index e334fa9d..ea797eab 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -276,6 +276,12 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, i += 1 continue if ch == "[": + # Same reasoning as the "(" branch above: a top-level "[" + # not preceded by "ident =" opens a list expression (e.g. + # the first argument written as ["a"][0]) used as the + # first positional argument. + if at_top and _POSITIONAL_KEY not in kwargs: + kwargs[_POSITIONAL_KEY] = _UNRESOLVED bracket_depth += 1 i += 1 continue @@ -284,6 +290,10 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, i += 1 continue if ch == "{": + # Same reasoning again, for a dict/set expression opening + # the first positional argument. + if at_top and _POSITIONAL_KEY not in kwargs: + kwargs[_POSITIONAL_KEY] = _UNRESOLVED brace_depth += 1 i += 1 continue @@ -601,6 +611,31 @@ def test_has_apple_support_override_fails_closed_on_parenthesized_positional_mod sample = 'archive_override(("apple_" + "support"), strip_prefix = "x")' self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_list_positional_module_name( + self, + ) -> None: + """A list expression passed as the first positional argument (e.g. + + ["apple_support"][0]) can't be read back as a literal from the + source text either; it must fail closed, not be silently skipped + while descending into the list. + """ + sample = 'archive_override(["apple_support"][0], strip_prefix = "x")' + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_fails_closed_on_dict_positional_module_name( + self, + ) -> None: + """Same as the list case, but for a dict expression opening the + + first positional argument. + """ + sample = ( + 'archive_override({"apple_support": 1}.keys()[0], ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_concatenated_module_name( self, ) -> None: From 7d162ae798c02b24a3ca75c1058e762e68e61d2c Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 19:29:56 +0100 Subject: [PATCH 17/29] Skip comments, not just whitespace, between "=" and a keyword's RHS. The RHS lookahead after "ident =" only skipped whitespace, so a comment on its own line between "=" and the literal (valid Starlark formatting) made the RHS look non-literal: module_name = # note\n "rules_cc" was marked _UNRESOLVED and falsely tripped _has_apple_support_override() for an unrelated module's override, and the same shape on bazel_dep's version returned the sentinel instead of the actual version string. Factored the comment-skipping loop already in _is_complete_literal() out into _skip_ws_and_comments(), and used it for the RHS lookahead too. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 67 ++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index ea797eab..a43f6f47 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -143,18 +143,13 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: return n, "".join(chars) -def _is_complete_literal(text: str, i: int, n: int) -> bool: - """True if the string literal ending at i is a whole call argument. +def _skip_ws_and_comments(text: str, i: int, n: int) -> int: + """Skip whitespace and "#" comments, repeatedly, starting at i. - Skips whitespace and "#" comments (repeatedly, so a comment followed - by more whitespace/another comment is all consumed) after the - literal. The literal is the argument's complete, resolved value only - if what follows is "," (ending this argument) or ")" (ending the - call) — anything else, including "+" (concatenation, even across an - intervening comment), "[" (slicing), "%" (string formatting), "." - (a method call), or any other operator, means the literal is merely a - fragment of a larger expression whose true value can't be read back - from the source text. + A single _skip_ws() stops at the first "#"; this loops so a comment + followed by more whitespace (its trailing newline, a blank line, or + another comment) is all consumed, landing on the next real token + regardless of how it's laid out across lines. """ j = i while True: @@ -164,6 +159,21 @@ def _is_complete_literal(text: str, i: int, n: int) -> bool: if skipped == j: break j = skipped + return j + + +def _is_complete_literal(text: str, i: int, n: int) -> bool: + """True if the string literal ending at i is a whole call argument. + + Skips whitespace and comments after the literal. The literal is the + argument's complete, resolved value only if what follows is "," + (ending this argument) or ")" (ending the call) — anything else, + including "+" (concatenation, even across an intervening comment), + "[" (slicing), "%" (string formatting), "." (a method call), or any + other operator, means the literal is merely a fragment of a larger + expression whose true value can't be read back from the source text. + """ + j = _skip_ws_and_comments(text, i, n) return j < n and text[j] in ",)" @@ -307,7 +317,7 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, ident = m.group(0) j = _skip_ws(text, m.end(), n) if j < n and text[j] == "=" and text[j : j + 2] != "==": - rhs_start = _skip_ws(text, j + 1, n) + rhs_start = _skip_ws_and_comments(text, j + 1, n) if rhs_start < n and text[rhs_start] in "\"'": end, value = _skip_string(text, rhs_start, n) kwargs[ident] = ( @@ -457,6 +467,22 @@ def test_apple_support_bazel_dep_version_ignores_commented_out_version( )""" self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_skips_comment_after_equals( + self, + ) -> None: + """A comment between "=" and the literal on the next line is valid + + Starlark formatting; it must be skipped, not mistaken for a + non-literal RHS that leaves the version unresolved. + """ + sample = """bazel_dep( + name = "apple_support", + version = + # pinned past the Xcode 27 fixes + "2.10.1", +)""" + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_ignores_fake_assignment_in_string( self, ) -> None: @@ -742,6 +768,23 @@ def test_has_apple_support_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_skips_comment_between_equals_and_value( + self, + ) -> None: + """A comment between "=" and the literal on the next line is valid + + Starlark formatting; it must be skipped, not mistaken for a + non-literal RHS that falsely trips the guard for an override on + some other module. + """ + sample = """archive_override( + module_name = + # pin past Xcode 27 fixes + "rules_cc", + strip_prefix = "x", +)""" + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_commented_module_name( self, ) -> None: From 43f561d83b2d1218055218ac46c417bbb16ef560 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 21:12:21 +0100 Subject: [PATCH 18/29] Recognize a parenthesized-callable-indirection call form. The outer call-name scan only recognized a bare "name(" call, so a valid Starlark call through a parenthesized callable reference -- "(archive_override)(module_name = \"apple_support\", ...)", equivalent to the direct form since grouping parens around any expression are legal and the result can be called immediately -- didn't match textually and let the override bypass _has_apple_support_override(). Added _match_parenthesized_call() to recognize "(" name ")(" as an alternate way to find the same call, wired into the outer scan alongside the existing direct-call match. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 58 ++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index a43f6f47..a2543644 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -191,6 +191,32 @@ def _is_identifier_boundary(text: str, i: int) -> bool: return not (prev.isalnum() or prev == "_" or prev == ".") +def _match_parenthesized_call( + text: str, i: int, n: int, sorted_names: list[str] +) -> tuple[str | None, int]: + """Match "(" name ")(" starting at text[i] == "(". + + Grouping parens around any expression are legal Starlark, and the + result can be called immediately, so "(archive_override)(...)" is a + valid call equivalent to "archive_override(...)" — but it doesn't + match a bare "name(" call textually, letting this scanner's direct + matching be bypassed by wrapping the name in parens. Returns + (matched_name, call_paren_index); matched_name is None if this + position isn't that pattern. + """ + j = _skip_ws(text, i + 1, n) + if not _is_identifier_boundary(text, j): + return None, 0 + for name in sorted_names: + if text.startswith(name, j): + k = _skip_ws(text, j + len(name), n) + if k < n and text[k] == ")": + k = _skip_ws(text, k + 1, n) + if k < n and text[k] == "(": + return name, k + return None, 0 + + def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, str]]: """Yield the {keyword: string value} args of each top-level call. @@ -245,6 +271,10 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, matched_name = name paren_index = j break + if matched_name is None and ch == "(": + matched_name, paren_index = _match_parenthesized_call( + text, i, n, sorted_names + ) if matched_name is None: i += 1 continue @@ -828,6 +858,34 @@ def test_has_apple_support_override_detects_archive_override(self) -> None: """ self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_parenthesized_callable_indirection( + self, + ) -> None: + """Grouping parens around any expression are legal Starlark, and + + the result can be called immediately: (archive_override)(...) is + a valid call equivalent to archive_override(...), but doesn't + match a bare "name(" call textually. + """ + sample = ( + '(archive_override)(module_name = "apple_support", ' + 'urls = ["https://example.com/apple_support.tar.gz"])' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_rejects_longer_identifier_in_parens( + self, + ) -> None: + """"(archive_override_foo)(...)" must not be misread as a + + parenthesized reference to archive_override. + """ + sample = ( + '(archive_override_foo)(module_name = "apple_support", ' + 'urls = ["x"])' + ) + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_git_override(self) -> None: sample = """ git_override( From 0e22b4ff35bc170e3d5955afc63010aee1950e12 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 21:19:35 +0100 Subject: [PATCH 19/29] Decode backslash-newline line continuations instead of keeping both. Mirrors the same fix on #423: _decode_escape() had no case for a backslash immediately followed by an actual LF/CRLF line break in the source (as opposed to the two-character "\n" escape for a newline value), so it fell through to the default "leave as literal backslash + next character" branch. A module_name literal split across two source lines by a trailing backslash -- valid Starlark, which drops the continuation entirely -- kept both the backslash and the newline character, producing a value that never equals the real module name and letting the override bypass the fail-closed guard. Added LF and CRLF line-continuation decoding (both collapse to ""). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 43 ++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index a2543644..6fa03be7 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -65,19 +65,27 @@ def _decode_escape(text: str, j: int, n: int) -> tuple[int, str]: Returns (new_j, decoded_text) for Starlark's string-literal escapes: the simple single-character escapes (\\n, \\t, \\\\, ...), up to 3 - octal digits, exactly two hex digits after \\x, and the \\uXXXX / - \\UXXXXXXXX Unicode code point forms (exactly 4 / 8 hex digits). + octal digits, exactly two hex digits after \\x, the \\uXXXX / + \\UXXXXXXXX Unicode code point forms (exactly 4 / 8 hex digits), and + a line continuation — a backslash immediately followed by an actual + LF or CRLF line break in the source, which Starlark drops entirely + (unlike "\\n", the two-character escape for a newline *value*). Discarding the backslash and keeping the next character literally — the previous behavior here — decodes "\\x61pple_support" (a valid Starlark escape for "apple_support") to the wrong string "x61pple_support", letting an escaped module_name bypass the override - guard; "\\u0061pple_support" was the same bug for Unicode escapes. An - escape this doesn't recognize is left as a literal backslash + next - character. + guard; "\\u0061pple_support" was the same bug for Unicode escapes, and + a module_name literal split across two source lines by a trailing + backslash was the same bug for a line continuation. An escape this + doesn't recognize is left as a literal backslash + next character. """ if j + 1 >= n: return j + 1, "\\" c = text[j + 1] + if c == "\r" and j + 2 < n and text[j + 2] == "\n": + return j + 3, "" + if c == "\n": + return j + 2, "" if c in _SIMPLE_ESCAPES: return j + 2, _SIMPLE_ESCAPES[c] if c == "x": @@ -632,6 +640,31 @@ def test_has_apple_support_override_decodes_big_unicode_escape_in_module_name( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_decodes_line_continuation_in_module_name( + self, + ) -> None: + """A backslash immediately followed by an actual newline in the + + source is a line continuation that Starlark drops entirely, not + the two-character "\\n" escape for a newline value; the module + name spans two source lines but still resolves to one literal. + """ + sample = ( + 'archive_override(module_name = "apple_\\\n' + 'support", strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_decodes_crlf_line_continuation_in_module_name( + self, + ) -> None: + """Same as the LF case, but for a CRLF line ending.""" + sample = ( + 'archive_override(module_name = "apple_\\\r\n' + 'support", strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_keyword_variable_module_name( self, ) -> None: From 4468a3148881abafdecb4de00547e3b03cb5351b Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 21:59:31 +0100 Subject: [PATCH 20/29] Recognize raw string literals instead of misreading them as unresolved. module_name = r"rules_cc" (a raw string, valid Starlark) was read as a bare identifier "r" followed by a separate, unrelated quoted string: the keyword-RHS and positional-capture checks only recognized a quote character directly, so the "r"/"R" prefix fell through to the bare-identifier-is-unresolved path added earlier, and the guard incorrectly failed closed (returned True) for an override on a completely unrelated module -- a false-positive CI failure, not a guard bypass, but still wrong. Added _is_raw_string_start()/_skip_raw_string()/_skip_any_string(): raw strings only give "\" special meaning (a literal quote, string continues) while every other backslash, including one before a newline, is kept completely literal -- unlike an ordinary string's _decode_escape(). Wired _skip_any_string() into the two places that decide whether a token is the argument's literal value (keyword RHS, first-positional capture); left the outer call-name scan and bracket- interior skipping on plain _skip_string(), since those only need to correctly skip past a string's content (never read its value) and already do so correctly for every raw string actually seen in MODULE.bazel. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 116 +++++++++++++++++++++++++++-- 1 file changed, 111 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 6fa03be7..8e862749 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -151,6 +151,57 @@ def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: return n, "".join(chars) +def _is_raw_string_start(text: str, i: int, n: int) -> bool: + """True if text[i] starts a raw string literal: "r"/"R" + quote.""" + return i + 1 < n and text[i] in "rR" and text[i + 1] in "\"'" + + +def _skip_raw_string(text: str, i: int, n: int) -> tuple[int, str]: + """Return (end_index, value) for the raw string starting at text[i]. + + text[i] is the delimiter quote itself (the caller has already + consumed the "r"/"R" prefix). Per the Starlark spec, a raw string + processes backslash escapes only enough to let a literal quote + appear inside it: a backslash immediately followed by the delimiter + quote denotes a literal quote character (the string continues, not + ending there) — every other backslash, including one before a + newline, is kept completely literal rather than decoded, unlike an + ordinary string's _decode_escape(). Only single-quote delimiters are + handled (no triple-quoted raw strings), matching the forms actually + seen in MODULE.bazel. + """ + quote = text[i] + j = i + 1 + chars: list[str] = [] + while j < n: + if text[j] == "\\" and j + 1 < n and text[j + 1] == quote: + chars.append(quote) + j += 2 + continue + if text[j] == quote: + return j + 1, "".join(chars) + chars.append(text[j]) + j += 1 + return n, "".join(chars) + + +def _skip_any_string(text: str, i: int, n: int) -> tuple[int, str]: + """Return (end_index, value) for the string literal starting at text[i], + + recognizing an optional "r"/"R" raw-string prefix before dispatching + to _skip_raw_string() or _skip_string(). A caller that needs to + recognize whether a token *is* a string literal (as opposed to + skipping over one already known to start exactly at i) uses this + instead of _skip_string() directly, so a raw literal like + r"rules_cc" is never misread as a bare identifier reference — which + fails closed on this guard just like any other unresolved value, + wrongly blocking a completely unrelated module's override. + """ + if _is_raw_string_start(text, i, n): + return _skip_raw_string(text, i + 1, n) + return _skip_string(text, i, n) + + def _skip_ws_and_comments(text: str, i: int, n: int) -> int: """Skip whitespace and "#" comments, repeatedly, starting at i. @@ -297,15 +348,15 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, i = _skip_comment(text, i, n) continue at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 - if ch in "\"'": + if ch in "\"'" or _is_raw_string_start(text, i, n): if at_top and _POSITIONAL_KEY not in kwargs: - end, value = _skip_string(text, i, n) + end, value = _skip_any_string(text, i, n) kwargs[_POSITIONAL_KEY] = ( value if _is_complete_literal(text, end, n) else _UNRESOLVED ) i = end continue - i, _ = _skip_string(text, i, n) + i, _ = _skip_any_string(text, i, n) continue if ch == "(": # A "(" at the top level, not preceded by "ident =", opens @@ -356,8 +407,11 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, j = _skip_ws(text, m.end(), n) if j < n and text[j] == "=" and text[j : j + 2] != "==": rhs_start = _skip_ws_and_comments(text, j + 1, n) - if rhs_start < n and text[rhs_start] in "\"'": - end, value = _skip_string(text, rhs_start, n) + if rhs_start < n and ( + text[rhs_start] in "\"'" + or _is_raw_string_start(text, rhs_start, n) + ): + end, value = _skip_any_string(text, rhs_start, n) kwargs[ident] = ( value if _is_complete_literal(text, end, n) @@ -831,6 +885,58 @@ def test_has_apple_support_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_raw_string_for_other_module( + self, + ) -> None: + """r"rules_cc" is a valid Starlark raw string literal; it must be + + recognized as the complete, resolved literal "rules_cc" rather + than misread as a non-literal (bare identifier "r" + a separate + quoted string), which would fail closed and incorrectly block a + valid override on a completely unrelated module. + """ + sample = 'archive_override(module_name = r"rules_cc", strip_prefix = "x")' + self.assertFalse(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_raw_string_module_name( + self, + ) -> None: + """A raw string literal is still a real override when it does + + name apple_support. + """ + sample = ( + 'archive_override(module_name = r"apple_support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_raw_string_positional_module_name( + self, + ) -> None: + """A raw string literal is recognized in the first-positional form + + too, not just as a keyword value. + """ + sample = 'archive_override(r"apple_support", strip_prefix = "x")' + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_decodes_escaped_quote_in_raw_string( + self, + ) -> None: + """Starlark raw strings still give backslash-quote special meaning + + (a literal quote, string continues) even though every other + backslash is kept completely literal; r"apple_\\"support" (an + embedded quote) is a different string from "apple_support" and + must not be mistaken for an apple_support override. + """ + sample = ( + 'archive_override(module_name = r"apple_\\"support", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_skips_comment_between_equals_and_value( self, ) -> None: From 0a416ab7d1ce71a8e545bb5a0466b755b2d3f62b Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 22:04:06 +0100 Subject: [PATCH 21/29] Skip comments, not just whitespace, in grouped-call detection. Mirrors the same fix on #423: _match_parenthesized_call() used _skip_ws() around the grouped callable name's tokens, so a comment inside the grouping -- e.g. "(archive_override # note\n)(module_name = \"apple_support\", ...)", which Starlark ignores the comment in and treats identically to the plain "(name)(" form -- made this scanner miss the call and let the override bypass the guard. Switched to _skip_ws_and_comments(). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 8e862749..707549b0 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -259,18 +259,20 @@ def _match_parenthesized_call( result can be called immediately, so "(archive_override)(...)" is a valid call equivalent to "archive_override(...)" — but it doesn't match a bare "name(" call textually, letting this scanner's direct - matching be bypassed by wrapping the name in parens. Returns - (matched_name, call_paren_index); matched_name is None if this - position isn't that pattern. + matching be bypassed by wrapping the name in parens. Comments are + tolerated around every token here too, the same as Starlark itself + ignores them: "(archive_override # note\n)(...)" is just as valid. + Returns (matched_name, call_paren_index); matched_name is None if + this position isn't that pattern. """ - j = _skip_ws(text, i + 1, n) + j = _skip_ws_and_comments(text, i + 1, n) if not _is_identifier_boundary(text, j): return None, 0 for name in sorted_names: if text.startswith(name, j): - k = _skip_ws(text, j + len(name), n) + k = _skip_ws_and_comments(text, j + len(name), n) if k < n and text[k] == ")": - k = _skip_ws(text, k + 1, n) + k = _skip_ws_and_comments(text, k + 1, n) if k < n and text[k] == "(": return name, k return None, 0 @@ -1025,6 +1027,21 @@ def test_has_apple_support_override_rejects_longer_identifier_in_parens( ) self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_parenthesized_call_with_comment( + self, + ) -> None: + """Starlark ignores comments; a comment between the grouped + + callable name and the closing paren -- (archive_override # + note\\n)(...) -- must not hide this call from the same + detection that handles the plain "(name)(" form. + """ + sample = ( + '(archive_override # note\n' + ')(module_name = "apple_support", strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_git_override(self) -> None: sample = """ git_override( From 91b57ff62fb1953d37c8f6f5d3b4e239adf0331e Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Tue, 13 Oct 2026 12:28:54 +0100 Subject: [PATCH 22/29] Handle triple-quote delimiters in raw string literals. _skip_raw_string() only handled a single-character delimiter, so module_name = r"""rules_cc""" stopped at the first lone quote inside the triple-quote run, leaving the value unresolved. The fail-closed guard then incorrectly returned True for an override on a completely unrelated module -- a false-positive CI failure, same class of bug as the earlier single-quote raw-string fix. Extended _skip_raw_string() to match a triple-quote delimiter as the 3-character sequence, the same way _skip_string() already does for ordinary strings, while keeping the raw "\" escape rule (a backslash before a single quote character is still a literal quote that doesn't end the string, even inside a triple-quoted raw literal). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 64 ++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 707549b0..eb487101 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -166,13 +166,29 @@ def _skip_raw_string(text: str, i: int, n: int) -> tuple[int, str]: quote denotes a literal quote character (the string continues, not ending there) — every other backslash, including one before a newline, is kept completely literal rather than decoded, unlike an - ordinary string's _decode_escape(). Only single-quote delimiters are - handled (no triple-quoted raw strings), matching the forms actually - seen in MODULE.bazel. + ordinary string's _decode_escape(). Handles a triple-quote delimiter + the same way _skip_string() does — matched as the 3-character + sequence, so e.g. r\"\"\"rules_cc\"\"\" is a real literal, not merely + skipped until the first lone quote (which would stop one character + too early and leave the rest unparsed as code). """ quote = text[i] + if text[i : i + 3] == quote * 3: + delim = quote * 3 + j = i + 3 + chars: list[str] = [] + while j < n: + if text[j] == "\\" and j + 1 < n and text[j + 1] == quote: + chars.append(quote) + j += 2 + continue + if text[j : j + 3] == delim: + return j + 3, "".join(chars) + chars.append(text[j]) + j += 1 + return n, "".join(chars) j = i + 1 - chars: list[str] = [] + chars = [] while j < n: if text[j] == "\\" and j + 1 < n and text[j + 1] == quote: chars.append(quote) @@ -521,6 +537,17 @@ def test_apple_support_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "2.10.1", name = "apple_support")' self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_accepts_raw_triple_quoted_name( + self, + ) -> None: + """name = r\"\"\"apple_support\"\"\" is a valid Starlark raw + + triple-quoted literal; it must resolve to the complete literal + rather than stopping at the first lone quote. + """ + sample = 'bazel_dep(name = r"""apple_support""", version = "2.10.1")' + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_accepts_positional_name( self, ) -> None: @@ -923,6 +950,35 @@ def test_has_apple_support_override_detects_raw_string_positional_module_name( sample = 'archive_override(r"apple_support", strip_prefix = "x")' self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_raw_triple_quoted_for_other_module( + self, + ) -> None: + """r\"\"\"rules_cc\"\"\" is a valid Starlark raw triple-quoted literal; + + it must resolve to the complete literal "rules_cc" rather than + stopping at the first lone quote (which would leave the value + unresolved and fail closed, incorrectly blocking a valid + override on a completely unrelated module). + """ + sample = ( + 'archive_override(module_name = r"""rules_cc""", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_raw_triple_quoted_module_name( + self, + ) -> None: + """A raw triple-quoted literal is still a real override when it + + does name apple_support. + """ + sample = ( + 'archive_override(module_name = r"""apple_support""", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_decodes_escaped_quote_in_raw_string( self, ) -> None: From f43d0ce2e3f7255e4a1cc4009869b1e1443eab44 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Thu, 15 Oct 2026 09:24:24 +0100 Subject: [PATCH 23/29] Recursively unwrap nested grouped-callable parens. _match_parenthesized_call() only handled exactly one layer of "(" name ")(" grouping, so "((archive_override))(module_name = \"apple_support\", ...)" -- valid Starlark, since grouping parens nest arbitrarily and the result can still be called immediately -- didn't match and let the override bypass the guard. Rewrote it to count an arbitrary number of leading "(" before the identifier and require the same number of matching ")" after it, rather than special-casing exactly one layer on each side. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 87 ++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 16 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index eb487101..ecccab5a 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -269,28 +269,36 @@ def _is_identifier_boundary(text: str, i: int) -> bool: def _match_parenthesized_call( text: str, i: int, n: int, sorted_names: list[str] ) -> tuple[str | None, int]: - """Match "(" name ")(" starting at text[i] == "(". - - Grouping parens around any expression are legal Starlark, and the - result can be called immediately, so "(archive_override)(...)" is a - valid call equivalent to "archive_override(...)" — but it doesn't - match a bare "name(" call textually, letting this scanner's direct - matching be bypassed by wrapping the name in parens. Comments are - tolerated around every token here too, the same as Starlark itself - ignores them: "(archive_override # note\n)(...)" is just as valid. - Returns (matched_name, call_paren_index); matched_name is None if - this position isn't that pattern. + """Match one or more layers of "(" name ")" grouping, starting at + text[i] == "(", followed by the real call's "(". + + Grouping parens around any expression are legal Starlark and nest + arbitrarily, and the result can still be called immediately, so + "((archive_override))(...)" is just as valid a call as the single- + layer "(archive_override)(...)" — but neither matches a bare + "name(" call textually, letting this scanner's direct matching be + bypassed by wrapping the name in one or more layers of parens. + Comments are tolerated around every token here too, the same as + Starlark itself ignores them: "(archive_override # note\n)(...)" is + just as valid. Returns (matched_name, call_paren_index); + matched_name is None if this position isn't that pattern. """ - j = _skip_ws_and_comments(text, i + 1, n) - if not _is_identifier_boundary(text, j): + depth = 0 + j = i + while j < n and text[j] == "(": + depth += 1 + j = _skip_ws_and_comments(text, j + 1, n) + if depth == 0 or not _is_identifier_boundary(text, j): return None, 0 for name in sorted_names: if text.startswith(name, j): k = _skip_ws_and_comments(text, j + len(name), n) - if k < n and text[k] == ")": + closed = 0 + while closed < depth and k < n and text[k] == ")": + closed += 1 k = _skip_ws_and_comments(text, k + 1, n) - if k < n and text[k] == "(": - return name, k + if closed == depth and k < n and text[k] == "(": + return name, k return None, 0 @@ -1098,6 +1106,53 @@ def test_has_apple_support_override_detects_parenthesized_call_with_comment( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_doubly_parenthesized_call( + self, + ) -> None: + """Grouping parens nest arbitrarily in Starlark; two layers -- + + ((archive_override))(...) -- are just as valid a call as one, + and must not bypass the same detection that handles the + single-layer "(name)(" form. + """ + sample = ( + '((archive_override))(module_name = "apple_support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_triply_parenthesized_call( + self, + ) -> None: + """Same as the double-layer case, but with three layers.""" + sample = ( + '(((archive_override)))(module_name = "apple_support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_ignores_doubly_parenthesized_other_module( + self, + ) -> None: + sample = ( + '((archive_override))(module_name = "rules_cc", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_apple_support_override(sample)) + + def test_has_apple_support_override_rejects_longer_identifier_in_nested_parens( + self, + ) -> None: + """"((archive_override_foo))(...)" must not be misread as a + + doubly-parenthesized reference to archive_override. + """ + sample = ( + '((archive_override_foo))(module_name = "apple_support", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_git_override(self) -> None: sample = """ git_override( From a1b4328f89894cefb97370573fc168df904a3e58 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Thu, 15 Oct 2026 09:37:29 +0100 Subject: [PATCH 24/29] Handle backslash parity in raw strings; capture bazel_dep's 2nd positional. Two fixes: 1. _skip_raw_string() decided whether a quote ended the string by looking at only the single preceding character, so an even-length run of backslashes before the closing quote (e.g. r"foo\\") was misread as escaping it, causing the scanner to run past the real end of the string and corrupt the rest of the parse -- for example consuming a later module_name argument into the runaway string, so the override guard failed spuriously on an unrelated module. Rewrote it to count the full backslash run and use its parity, matching real Starlark/Python raw-string semantics (confirmed against CPython's own r-string evaluation): an odd run escapes the quote (string continues, backslashes and quote both kept literally in the value), an even run does not. 2. bazel_dep allows both name and version positionally (bazel_dep("apple_support", "2.10.1")), but the scanner only ever recorded the first positional argument, so a positional version made _apple_support_bazel_dep_version() raise instead of returning it. Replaced the single first-positional slot with an ordered list (_add_positional/_positional), read by index, so any caller can look at any positional argument, not just the first. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 191 +++++++++++++++++++++-------- 1 file changed, 143 insertions(+), 48 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index ecccab5a..515f65a4 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -32,6 +32,24 @@ def _repo_root(start: Path | None = None) -> Path: _UNRESOLVED = object() +def _add_positional(kwargs: dict, value: object) -> None: + """Record a newly-seen top-level positional argument, in call order. + + Bazel's APIs here accept more than one leading parameter + positionally — not just the first (e.g. bazel_dep's name *and* + version) — so every caller needs all of them, by index, not just + whichever arrived first. Appending (instead of the previous "only + the first" guard) keeps them in the order they appeared. + """ + kwargs.setdefault(_POSITIONAL_KEY, []).append(value) + + +def _positional(kwargs: dict, index: int) -> object | None: + """Return the positional argument at index, or None if fewer were given.""" + values = kwargs.get(_POSITIONAL_KEY, []) + return values[index] if index < len(values) else None + + def _skip_ws(text: str, i: int, n: int) -> int: while i < n and text[i] in " \t\r\n": i += 1 @@ -160,17 +178,19 @@ def _skip_raw_string(text: str, i: int, n: int) -> tuple[int, str]: """Return (end_index, value) for the raw string starting at text[i]. text[i] is the delimiter quote itself (the caller has already - consumed the "r"/"R" prefix). Per the Starlark spec, a raw string - processes backslash escapes only enough to let a literal quote - appear inside it: a backslash immediately followed by the delimiter - quote denotes a literal quote character (the string continues, not - ending there) — every other backslash, including one before a - newline, is kept completely literal rather than decoded, unlike an - ordinary string's _decode_escape(). Handles a triple-quote delimiter - the same way _skip_string() does — matched as the 3-character - sequence, so e.g. r\"\"\"rules_cc\"\"\" is a real literal, not merely - skipped until the first lone quote (which would stop one character - too early and leave the rest unparsed as code). + consumed the "r"/"R" prefix). Per the Starlark spec (and matching + real Python raw-string behavior, which it mirrors here), a raw + string gives a backslash exactly one special power: letting a + literal quote appear without ending the string. Whether a quote + that follows a run of backslashes is that kind of escaped quote + depends on the run's *parity* — r"foo\\" (an even, here zero-after- + pairing, run) ends normally with two literal backslashes in the + value, while r"foo\"" (an odd run) does not end there; the quote is + part of the value, and — unlike an ordinary string's + _decode_escape() — the backslash stays in the value too, exactly as + written. Handles a triple-quote delimiter the same way _skip_string() + does — matched as the 3-character sequence once any escaped quote + immediately before it has been accounted for the same way. """ quote = text[i] if text[i : i + 3] == quote * 3: @@ -178,9 +198,18 @@ def _skip_raw_string(text: str, i: int, n: int) -> tuple[int, str]: j = i + 3 chars: list[str] = [] while j < n: - if text[j] == "\\" and j + 1 < n and text[j + 1] == quote: - chars.append(quote) - j += 2 + if text[j] == "\\": + k = j + while k < n and text[k] == "\\": + k += 1 + run = text[j:k] + if k < n and text[k] == quote and len(run) % 2 == 1: + chars.append(run) + chars.append(quote) + j = k + 1 + continue + chars.append(run) + j = k continue if text[j : j + 3] == delim: return j + 3, "".join(chars) @@ -190,9 +219,18 @@ def _skip_raw_string(text: str, i: int, n: int) -> tuple[int, str]: j = i + 1 chars = [] while j < n: - if text[j] == "\\" and j + 1 < n and text[j + 1] == quote: - chars.append(quote) - j += 2 + if text[j] == "\\": + k = j + while k < n and text[k] == "\\": + k += 1 + run = text[j:k] + if k < n and text[k] == quote and len(run) % 2 == 1: + chars.append(run) + chars.append(quote) + j = k + 1 + continue + chars.append(run) + j = k continue if text[j] == quote: return j + 1, "".join(chars) @@ -326,13 +364,15 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, needs. Whitespace between a call name and "(" is tolerated ("archive_override (..." is valid Starlark). - Bazel's `*_override`/`bazel_dep` APIs all accept their first argument - (module_name / name) positionally — a bare top-level string before any - "identifier =" is stored under `_POSITIONAL_KEY`, letting callers treat - it as that first parameter. "Top-level" also tracks `[...]`/`{...}` - nesting (not just parens), so a string that's an *element* of a list - value (e.g. inside `urls = [...]`) is never mistaken for a positional - call argument. + Bazel's `*_override`/`bazel_dep` APIs accept their leading arguments + (module_name / name, and for bazel_dep also version) positionally — + every bare top-level value before any "identifier =" is appended, in + order, to the list under `_POSITIONAL_KEY` (see `_add_positional`), + letting callers read any of them by index (`_positional`), not just + the first. "Top-level" also tracks `[...]`/`{...}` nesting (not just + parens), so a string that's an *element* of a list value (e.g. + inside `urls = [...]`) is never mistaken for a positional call + argument. """ sorted_names = sorted(call_names, key=len, reverse=True) results: list[dict[str, str]] = [] @@ -375,10 +415,11 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, continue at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 if ch in "\"'" or _is_raw_string_start(text, i, n): - if at_top and _POSITIONAL_KEY not in kwargs: + if at_top: end, value = _skip_any_string(text, i, n) - kwargs[_POSITIONAL_KEY] = ( - value if _is_complete_literal(text, end, n) else _UNRESOLVED + _add_positional( + kwargs, + value if _is_complete_literal(text, end, n) else _UNRESOLVED, ) i = end continue @@ -386,13 +427,13 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, continue if ch == "(": # A "(" at the top level, not preceded by "ident =", opens - # a parenthesized positional expression (e.g. the first + # a parenthesized positional expression (e.g. an # argument written as ("a" + "b")) rather than a literal # or a bare variable reference; mark it unresolved before # descending into it, the same as any other non-literal # positional value. - if at_top and _POSITIONAL_KEY not in kwargs: - kwargs[_POSITIONAL_KEY] = _UNRESOLVED + if at_top: + _add_positional(kwargs, _UNRESOLVED) depth += 1 i += 1 continue @@ -403,10 +444,9 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if ch == "[": # Same reasoning as the "(" branch above: a top-level "[" # not preceded by "ident =" opens a list expression (e.g. - # the first argument written as ["a"][0]) used as the - # first positional argument. - if at_top and _POSITIONAL_KEY not in kwargs: - kwargs[_POSITIONAL_KEY] = _UNRESOLVED + # an argument written as ["a"][0]). + if at_top: + _add_positional(kwargs, _UNRESOLVED) bracket_depth += 1 i += 1 continue @@ -416,9 +456,9 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, continue if ch == "{": # Same reasoning again, for a dict/set expression opening - # the first positional argument. - if at_top and _POSITIONAL_KEY not in kwargs: - kwargs[_POSITIONAL_KEY] = _UNRESOLVED + # a positional argument. + if at_top: + _add_positional(kwargs, _UNRESOLVED) brace_depth += 1 i += 1 continue @@ -455,13 +495,10 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, i = rhs_start continue # Not "ident = ...": a bare identifier is a variable - # used as a value. If it's the first value-shaped - # token in the call, it's being passed as the first - # positional argument; fail closed the same way as a - # non-literal keyword value, rather than silently - # dropping it. - if _POSITIONAL_KEY not in kwargs: - kwargs[_POSITIONAL_KEY] = _UNRESOLVED + # used as a value, passed positionally; fail closed + # the same way as a non-literal keyword value, rather + # than silently dropping it. + _add_positional(kwargs, _UNRESOLVED) i = m.end() continue i += 1 @@ -470,10 +507,24 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, def _apple_support_bazel_dep_version(module_bazel: str) -> str: + """Return the pinned apple_support bazel_dep version. + + bazel_dep's first two parameters, name and version, are both + allowed positionally (bazel_dep("apple_support", "2.10.1")), not + just name; _positional(kwargs, 1) reads version there the same way + _positional(kwargs, 0) already reads name, rather than assuming + version is only ever given as a keyword. + """ for kwargs in _iter_call_kwargs(("bazel_dep",), module_bazel): - name = kwargs.get("name") or kwargs.get(_POSITIONAL_KEY) - if name == "apple_support" and "version" in kwargs: - return kwargs["version"] + name = kwargs.get("name") + if name is None: + name = _positional(kwargs, 0) + if name == "apple_support": + version = kwargs.get("version") + if version is None: + version = _positional(kwargs, 1) + if version is not None and version is not _UNRESOLVED: + return version raise AssertionError('expected bazel_dep(... name = "apple_support" ...)') @@ -500,7 +551,9 @@ def _has_apple_support_override(module_bazel: str) -> bool: "single_version_override", ) for kwargs in _iter_call_kwargs(override_names, module_bazel): - module_name = kwargs.get("module_name") or kwargs.get(_POSITIONAL_KEY) + module_name = kwargs.get("module_name") + if module_name is None: + module_name = _positional(kwargs, 0) if module_name == "apple_support" or module_name is _UNRESOLVED: return True return False @@ -566,6 +619,17 @@ def test_apple_support_bazel_dep_version_accepts_positional_name( sample = 'bazel_dep("apple_support", version = "2.10.1")' self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_accepts_positional_version( + self, + ) -> None: + """version is also allowed positionally, as bazel_dep's second + + argument: bazel_dep("apple_support", "2.10.1") is valid + Starlark, not just the keyword form. + """ + sample = 'bazel_dep("apple_support", "2.10.1")' + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_survives_an_earlier_paren( self, ) -> None: @@ -1003,6 +1067,37 @@ def test_has_apple_support_override_decodes_escaped_quote_in_raw_string( ) self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ends_raw_string_at_even_backslash_run( + self, + ) -> None: + """An even run of backslashes immediately before the closing quote + + (here two) pairs up and does not escape it; the raw string ends + normally there, with both backslashes kept literally in the + value, so the argument after it (module_name) is parsed + correctly rather than being swallowed into a runaway string. + """ + sample = ( + 'archive_override(strip_prefix = r"apple_support\\\\", ' + 'module_name = "rules_cc")' + ) + self.assertFalse(_has_apple_support_override(sample)) + + def test_has_apple_support_override_continues_raw_string_at_odd_backslash_run( + self, + ) -> None: + """An odd run of backslashes immediately before a quote (here + + three) still escapes it, same as a single backslash; the string + does not end there and both the backslashes and the quote stay + literally in the value. + """ + sample = ( + 'archive_override(module_name = r"apple_support\\\\\\"x", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_skips_comment_between_equals_and_value( self, ) -> None: From 5fcf08b3629fcad824eb66c4b393c0f21064a6e1 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Thu, 15 Oct 2026 13:08:21 +0100 Subject: [PATCH 25/29] Use the raw-aware string dispatcher in the outer scan; skip comments before "=". Two scanner defects, both reported by Copilot review: - The outer top-level scan (which looks for a recognized call name anywhere in the text) only recognized ordinary string literals by checking for a leading quote character directly, then skipped them with _skip_string(). A raw string like `r"C:\U12345678\emsdk"` outside any recognized call therefore had its "r" prefix treated as an unrelated bare identifier and its quoted body fed to the ordinary escape decoder, which raises ValueError on escape sequences that are only valid because the "r" prefix makes them literal. This crashed the CI guard on valid module syntax containing no override at all. Fixed by using the same _is_raw_string_start()/_skip_any_string() dispatch here that the call-body scan already uses. - A comment between a keyword and "=" (e.g. `module_name # note\n = "emsdk"`) is legal Starlark, but the keyword-matching code used _skip_ws() right after the identifier, which stops at "#". The keyword was therefore misread as a bare positional identifier (a variable reference), which fails closed -- incorrectly reporting an apple_support override for module_name values that actually name a different module, and incorrectly raising AssertionError out of _apple_support_bazel_dep_version when the misread identifier was "name" on a real apple_support bazel_dep. Fixed by using the existing _skip_ws_and_comments() there instead, matching how the code already skips comments between "=" and the right-hand side. Both reproduced against the exact Copilot-supplied examples before the fix, and added as permanent regression tests. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 62 ++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 3 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 515f65a4..73a83db8 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -383,8 +383,8 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if ch == "#": i = _skip_comment(text, i, n) continue - if ch in "\"'": - i, _ = _skip_string(text, i, n) + if ch in "\"'" or _is_raw_string_start(text, i, n): + i, _ = _skip_any_string(text, i, n) continue matched_name = None paren_index = None @@ -470,7 +470,7 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, m = _IDENT_RE.match(text, i) if m: ident = m.group(0) - j = _skip_ws(text, m.end(), n) + j = _skip_ws_and_comments(text, m.end(), n) if j < n and text[j] == "=" and text[j : j + 2] != "==": rhs_start = _skip_ws_and_comments(text, j + 1, n) if rhs_start < n and ( @@ -676,6 +676,24 @@ def test_apple_support_bazel_dep_version_skips_comment_after_equals( )""" self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_skips_comment_between_keyword_and_equals( + self, + ) -> None: + """A comment between the keyword and "=" is valid Starlark + + formatting (same as between "=" and the value); it must be + skipped so the keyword is still recognized as "name", not + misread as a bare positional identifier that leaves the real + name argument unresolved. + """ + sample = """bazel_dep( + name + # pinned past the Xcode 27 fixes + = "apple_support", + version = "2.10.1", +)""" + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + def test_apple_support_bazel_dep_version_ignores_fake_assignment_in_string( self, ) -> None: @@ -1098,6 +1116,25 @@ def test_has_apple_support_override_continues_raw_string_at_odd_backslash_run( ) self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_raw_string_outside_recognized_call( + self, + ) -> None: + """A raw string literal outside any recognized call must be + + skipped using the raw-aware dispatcher, the same as inside a + call body; otherwise its contents are fed to the ordinary + escape decoder, which raises on sequences that are only valid + because the "r" prefix makes them literal (e.g. "\\U12345678" + is not a valid 8-hex-digit Unicode escape, and "\\e" is not a + recognized simple escape either), spuriously failing a guard + that doesn't even apply to this unrelated statement. + """ + sample = ( + 'SDK_PATH = r"C:\\U12345678\\emsdk"\n' + 'archive_override(module_name = "rules_cc", strip_prefix = "x")' + ) + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_skips_comment_between_equals_and_value( self, ) -> None: @@ -1115,6 +1152,25 @@ def test_has_apple_support_override_skips_comment_between_equals_and_value( )""" self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_skips_comment_between_keyword_and_equals( + self, + ) -> None: + """A comment between the keyword and "=" is valid Starlark + + formatting; it must be skipped so "module_name" is still + recognized as that keyword, not misread as a bare positional + identifier -- which would fail closed and falsely trip the + guard even though the real module_name names a different, + unrelated module. + """ + sample = """local_path_override( + module_name + # note + = "emsdk", + path = "../emsdk", +)""" + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_commented_module_name( self, ) -> None: From 2b1efe98e697db0dc65c2565d5f5603d521b9043 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Fri, 16 Oct 2026 10:13:44 +0100 Subject: [PATCH 26/29] Handle line continuations, comments before "(", and nested override calls. Three scanner defects, all reported by Copilot review (one of them previously fixed on #423 but missed here; mirroring it back now): - _skip_ws() only recognized the bare whitespace characters " \t\r\n", not a backslash-newline line continuation, so a valid call like `archive_override\` + newline + `(module_name = "apple_support", ...)` was never recognized -- the backslash right after the call name wasn't whitespace, so the lookup for "(" failed and _has_apple_support_override silently returned False even with a real override present. Fixed by having _skip_ws() treat a backslash immediately followed by LF or CRLF as whitespace too. - The direct call-name match (immediately before the "(" lookup) used _skip_ws() rather than _skip_ws_and_comments(), so a comment between the name and its own "(" -- e.g. `(local_path_override # note\n (module_name = "apple_support", ...))`, a whole call wrapped in redundant grouping parens -- was missed, even though the grouped "(name)(...)" *indirection* form already tolerated such comments. Fixed by extracting the direct-match and grouped-indirection logic into one _match_call_name() helper (replacing the former inline duplicate), used uniformly by both the outer scan and the new nested scan below, so both forms get the same comment handling for free. - The body-scanning loop consumed a whole call's argument list via paren/bracket/brace balancing only, so a recognized call *nested* in an argument value -- e.g. `patches = single_version_override(...) or []` inside another override's call -- was swallowed as inert text instead of being recognized as the real call it is. Fixed by extracting the body-scanning loop into _scan_call_body(), which now also calls _match_call_name() at every position in the body (not just the call's own name before entry); a match there is scanned recursively and its own {keyword: value} dict is appended to the shared results list before resuming the outer call's own scan past the nested call's span. All three reproduced against the exact Copilot-supplied examples before the fix, and added as permanent regression tests. Full //python/... suite green (24/24). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_macos_asan_test.py | 359 +++++++++++++++++++---------- 1 file changed, 238 insertions(+), 121 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 73a83db8..4f10848c 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -51,8 +51,26 @@ def _positional(kwargs: dict, index: int) -> object | None: def _skip_ws(text: str, i: int, n: int) -> int: - while i < n and text[i] in " \t\r\n": - i += 1 + """Skip whitespace, including a backslash-newline line continuation. + + A backslash immediately followed by an LF or CRLF line break is a + line continuation -- dropped entirely, the same rule _decode_escape() + applies inside a string -- so it is whitespace here too; skipping + only the bare characters in " \t\r\n" left the backslash itself + unconsumed, so e.g. "archive_override\\\n(" (the call name and "(" + split across such a continuation) was never recognized as a call. + """ + while i < n: + if text[i] in " \t\r\n": + i += 1 + continue + if text[i] == "\\" and text[i + 1 : i + 2] == "\n": + i += 2 + continue + if text[i] == "\\" and text[i + 1 : i + 3] == "\r\n": + i += 3 + continue + break return i @@ -340,8 +358,162 @@ def _match_parenthesized_call( return None, 0 +def _match_call_name( + text: str, i: int, n: int, sorted_names: list[str] +) -> tuple[str | None, int]: + """Try to match a call "name(...)" at text[i], directly or through + + one or more layers of grouping parens (see _match_parenthesized_call). + Whitespace and comments between the name and its "(" are tolerated + either way. Returns (matched_name, index_of_opening_paren), or + (None, 0) if text[i] isn't the start of a recognized call. + """ + if _is_identifier_boundary(text, i): + for name in sorted_names: + if text.startswith(name, i): + j = _skip_ws_and_comments(text, i + len(name), n) + if j < n and text[j] == "(": + return name, j + if i < n and text[i] == "(": + return _match_parenthesized_call(text, i, n, sorted_names) + return None, 0 + + +def _scan_call_body( + text: str, + i: int, + n: int, + sorted_names: list[str], + results: list[dict[str, str]], +) -> int: + """Scan one call's body, starting right after its opening "(". + + Parses each "identifier = value" pair at the call's own top-level + parenthesis depth (see _iter_call_kwargs for why only there), then + appends the resulting {keyword: value} dict to `results` and returns + the index just past the matching closing ")". + + A recognized call name appearing anywhere in the body -- not just as + this call's own name -- is itself a real call Bazel executes, e.g. + `patches = single_version_override(...) or []`; such a nested call + is recursively scanned the same way (appending its own dict to + `results` too) and its matched span is skipped whole, rather than + being silently swallowed as inert text while this call's argument + list is paren-balanced. + """ + depth = 1 + bracket_depth = 0 + brace_depth = 0 + kwargs: dict[str, str] = {} + while i < n and depth > 0: + ch = text[i] + if ch == "#": + i = _skip_comment(text, i, n) + continue + at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 + if ch in "\"'" or _is_raw_string_start(text, i, n): + if at_top: + end, value = _skip_any_string(text, i, n) + _add_positional( + kwargs, + value if _is_complete_literal(text, end, n) else _UNRESOLVED, + ) + i = end + continue + i, _ = _skip_any_string(text, i, n) + continue + nested_name, nested_paren = _match_call_name(text, i, n, sorted_names) + if nested_name is not None: + if at_top: + _add_positional(kwargs, _UNRESOLVED) + i = _scan_call_body(text, nested_paren + 1, n, sorted_names, results) + continue + if ch == "(": + # A "(" at the top level, not preceded by "ident =" and not + # a recognized call name (handled above), opens a + # parenthesized positional expression (e.g. an argument + # written as ("a" + "b")) rather than a literal or a bare + # variable reference; mark it unresolved before descending + # into it, the same as any other non-literal positional + # value. + if at_top: + _add_positional(kwargs, _UNRESOLVED) + depth += 1 + i += 1 + continue + if ch == ")": + depth -= 1 + i += 1 + continue + if ch == "[": + # Same reasoning as the "(" branch above: a top-level "[" + # not preceded by "ident =" opens a list expression (e.g. + # an argument written as ["a"][0]). + if at_top: + _add_positional(kwargs, _UNRESOLVED) + bracket_depth += 1 + i += 1 + continue + if ch == "]": + bracket_depth -= 1 + i += 1 + continue + if ch == "{": + # Same reasoning again, for a dict/set expression opening + # a positional argument. + if at_top: + _add_positional(kwargs, _UNRESOLVED) + brace_depth += 1 + i += 1 + continue + if ch == "}": + brace_depth -= 1 + i += 1 + continue + if at_top and _is_identifier_boundary(text, i): + m = _IDENT_RE.match(text, i) + if m: + ident = m.group(0) + j = _skip_ws_and_comments(text, m.end(), n) + if j < n and text[j] == "=" and text[j : j + 2] != "==": + rhs_start = _skip_ws_and_comments(text, j + 1, n) + if rhs_start < n and ( + text[rhs_start] in "\"'" + or _is_raw_string_start(text, rhs_start, n) + ): + end, value = _skip_any_string(text, rhs_start, n) + kwargs[ident] = ( + value + if _is_complete_literal(text, end, n) + else _UNRESOLVED + ) + i = end + continue + # A non-literal right-hand side (a variable + # reference, a function call, ...) can't be read + # back from the source text. Fail closed: mark it + # unresolved rather than leaving it absent, which + # would read identically to the argument never + # having been given at all. + kwargs[ident] = _UNRESOLVED + i = rhs_start + continue + # Not "ident = ...": a bare identifier is a variable + # used as a value, passed positionally; fail closed + # the same way as a non-literal keyword value, rather + # than silently dropping it. + _add_positional(kwargs, _UNRESOLVED) + i = m.end() + continue + i += 1 + results.append(kwargs) + return i + + def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, str]]: - """Yield the {keyword: string value} args of each top-level call. + """Yield the {keyword: string value} args of each call, top-level or + + nested in another call's argument value (see _scan_call_body). A single linear scan recognizes a call name, and each "identifier = value" pair inside its body, only at a genuine identifier boundary and @@ -359,10 +531,10 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, characters — is never scanned as code), makes that impossible: nothing inside a string literal or comment is ever offered to the identifier matcher. Only simple `identifier = "string"` / `identifier = 'string'` - assignments are captured (list/nested-call values are skipped over for - paren balancing but not parsed), which is everything every caller here - needs. Whitespace between a call name and "(" is tolerated - ("archive_override (..." is valid Starlark). + assignments are captured (list values are skipped over for paren + balancing but not parsed, aside from a nested recognized call). + Whitespace between a call name and "(" is tolerated ("archive_override + (..." is valid Starlark). Bazel's `*_override`/`bazel_dep` APIs accept their leading arguments (module_name / name, and for bazel_dep also version) positionally — @@ -386,123 +558,11 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if ch in "\"'" or _is_raw_string_start(text, i, n): i, _ = _skip_any_string(text, i, n) continue - matched_name = None - paren_index = None - if _is_identifier_boundary(text, i): - for name in sorted_names: - if text.startswith(name, i): - j = _skip_ws(text, i + len(name), n) - if j < n and text[j] == "(": - matched_name = name - paren_index = j - break - if matched_name is None and ch == "(": - matched_name, paren_index = _match_parenthesized_call( - text, i, n, sorted_names - ) + matched_name, paren_index = _match_call_name(text, i, n, sorted_names) if matched_name is None: i += 1 continue - i = paren_index + 1 - depth = 1 - bracket_depth = 0 - brace_depth = 0 - kwargs: dict[str, str] = {} - while i < n and depth > 0: - ch = text[i] - if ch == "#": - i = _skip_comment(text, i, n) - continue - at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 - if ch in "\"'" or _is_raw_string_start(text, i, n): - if at_top: - end, value = _skip_any_string(text, i, n) - _add_positional( - kwargs, - value if _is_complete_literal(text, end, n) else _UNRESOLVED, - ) - i = end - continue - i, _ = _skip_any_string(text, i, n) - continue - if ch == "(": - # A "(" at the top level, not preceded by "ident =", opens - # a parenthesized positional expression (e.g. an - # argument written as ("a" + "b")) rather than a literal - # or a bare variable reference; mark it unresolved before - # descending into it, the same as any other non-literal - # positional value. - if at_top: - _add_positional(kwargs, _UNRESOLVED) - depth += 1 - i += 1 - continue - if ch == ")": - depth -= 1 - i += 1 - continue - if ch == "[": - # Same reasoning as the "(" branch above: a top-level "[" - # not preceded by "ident =" opens a list expression (e.g. - # an argument written as ["a"][0]). - if at_top: - _add_positional(kwargs, _UNRESOLVED) - bracket_depth += 1 - i += 1 - continue - if ch == "]": - bracket_depth -= 1 - i += 1 - continue - if ch == "{": - # Same reasoning again, for a dict/set expression opening - # a positional argument. - if at_top: - _add_positional(kwargs, _UNRESOLVED) - brace_depth += 1 - i += 1 - continue - if ch == "}": - brace_depth -= 1 - i += 1 - continue - if at_top and _is_identifier_boundary(text, i): - m = _IDENT_RE.match(text, i) - if m: - ident = m.group(0) - j = _skip_ws_and_comments(text, m.end(), n) - if j < n and text[j] == "=" and text[j : j + 2] != "==": - rhs_start = _skip_ws_and_comments(text, j + 1, n) - if rhs_start < n and ( - text[rhs_start] in "\"'" - or _is_raw_string_start(text, rhs_start, n) - ): - end, value = _skip_any_string(text, rhs_start, n) - kwargs[ident] = ( - value - if _is_complete_literal(text, end, n) - else _UNRESOLVED - ) - i = end - continue - # A non-literal right-hand side (a variable - # reference, a function call, ...) can't be read - # back from the source text. Fail closed: mark it - # unresolved rather than leaving it absent, which - # would read identically to the argument never - # having been given at all. - kwargs[ident] = _UNRESOLVED - i = rhs_start - continue - # Not "ident = ...": a bare identifier is a variable - # used as a value, passed positionally; fail closed - # the same way as a non-literal keyword value, rather - # than silently dropping it. - _add_positional(kwargs, _UNRESOLVED) - i = m.end() - continue - i += 1 - results.append(kwargs) + i = _scan_call_body(text, paren_index + 1, n, sorted_names, results) return results @@ -1205,6 +1265,63 @@ def test_has_apple_support_override_tolerates_whitespace_before_paren( sample = 'archive_override (module_name = "apple_support", strip_prefix = "x")' self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_call_after_lf_continuation( + self, + ) -> None: + """A backslash-LF line continuation between the call name and "(" + + is valid Starlark, dropped entirely the same as inside a string; + it must not leave the call unrecognized. + """ + sample = ( + 'archive_override\\\n(module_name = "apple_support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_call_after_crlf_continuation( + self, + ) -> None: + """Same as the LF case, for a backslash-CRLF line continuation.""" + sample = ( + 'archive_override\\\r\n(module_name = "apple_support", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_comment_before_own_paren( + self, + ) -> None: + """A comment between the callable name and its own "(" is legal + + inside a parenthesized expression statement, e.g. + "(local_path_override # note\\n(module_name = ..., ...))"; the + direct call-name match must skip comments, not just whitespace, + there too. + """ + sample = ( + "(local_path_override # note\n" + '(module_name = "apple_support", path = "../apple_support"))' + ) + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_nested_override_call( + self, + ) -> None: + """A recognized call nested inside another call's argument value + + (e.g. patches = single_version_override(...) or []) is still a + real override Bazel executes, not inert text; it must not be + silently swallowed while the outer call's own argument list is + paren-balanced. + """ + sample = ( + 'single_version_override(module_name = "rules_cc", ' + 'patches = single_version_override(module_name = "apple_support", ' + 'version = "1.24.2") or [])' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_archive_override(self) -> None: sample = """ archive_override( From 5725299cc5f22ab9cf69bdff318d034e2f56afc5 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Fri, 16 Oct 2026 14:15:46 +0100 Subject: [PATCH 27/29] Exclude spaced attribute calls; classify each call argument only once. Two scanner defects, both reported by Copilot review: - Attribute calls were excluded only when "." immediately preceded the name, so "ext. bazel_dep(...)" (or a continuation or comment after the ".") was read as a real directive: an earlier qualified bazel_dep could supply the pinned version, and a qualified archive_override tripped the guard. Both scans now consume the attribute name after any "." (_skip_attribute), so the forward scan that already skips strings and comments decides it; a "." inside a comment is never seen. - After a keyword's "=", scanning resumed at the right-hand side and its tokens could be recorded as positional arguments, so `repo_name = "apple_support_" + "9.9.9"` supplied a fake second positional (bazel_dep's version) to a call that pins none. The body scan now tracks top-level "," boundaries and classifies each argument once, by its first token; later tokens are only scanned for nesting and nested calls. Co-Authored-By: Claude Opus 5.5 --- python/tests/ci_macos_asan_test.py | 206 ++++++++++++++++++++--------- 1 file changed, 142 insertions(+), 64 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 4f10848c..99bcff4c 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -358,6 +358,22 @@ def _match_parenthesized_call( return None, 0 +def _skip_attribute(text: str, i: int, n: int) -> int: + """Skip a "." (at text[i]) and the attribute name that follows it. + + Starlark allows whitespace, line continuations, and (inside brackets) + comments between the "." and the name, so "ext. bazel_dep(...)" is + still an attribute call. Checking only the character just before a + name can't see that; consuming the name here, in the same forward + scan that already skips strings and comments, means it is never + offered to the call matcher at all, while a "." inside a comment or + string is never seen. + """ + j = _skip_ws_and_comments(text, i + 1, n) + m = _IDENT_RE.match(text, j) + return m.end() if m else i + 1 + + def _match_call_name( text: str, i: int, n: int, sorted_names: list[str] ) -> tuple[str | None, int]: @@ -388,10 +404,18 @@ def _scan_call_body( ) -> int: """Scan one call's body, starting right after its opening "(". - Parses each "identifier = value" pair at the call's own top-level - parenthesis depth (see _iter_call_kwargs for why only there), then - appends the resulting {keyword: value} dict to `results` and returns - the index just past the matching closing ")". + Parses the call's own arguments (see _iter_call_kwargs for why only + at its top-level depth), then appends the resulting {keyword: value} + dict to `results` and returns the index just past the matching + closing ")". + + Each argument is classified once, by its first token: "identifier + =" makes it a keyword argument, anything else a positional one + (resolved only when the whole argument is a single string literal). + Every later token up to the next top-level "," belongs to that same + argument and is never recorded on its own -- otherwise, e.g., the + "9.9.9" tail of `repo_name = "apple_support_" + "9.9.9"` would be + read back as an extra positional argument (bazel_dep's version). A recognized call name appearing anywhere in the body -- not just as this call's own name -- is itself a real call Bazel executes, e.g. @@ -404,15 +428,25 @@ def _scan_call_body( depth = 1 bracket_depth = 0 brace_depth = 0 + at_arg_start = True kwargs: dict[str, str] = {} while i < n and depth > 0: ch = text[i] if ch == "#": i = _skip_comment(text, i, n) continue + ws_end = _skip_ws(text, i, n) + if ws_end > i: + i = ws_end + continue at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 - if ch in "\"'" or _is_raw_string_start(text, i, n): - if at_top: + if at_top and ch == ",": + at_arg_start = True + i += 1 + continue + if at_top and at_arg_start and ch != ")": + at_arg_start = False + if ch in "\"'" or _is_raw_string_start(text, i, n): end, value = _skip_any_string(text, i, n) _add_positional( kwargs, @@ -420,62 +454,11 @@ def _scan_call_body( ) i = end continue - i, _ = _skip_any_string(text, i, n) - continue - nested_name, nested_paren = _match_call_name(text, i, n, sorted_names) - if nested_name is not None: - if at_top: - _add_positional(kwargs, _UNRESOLVED) - i = _scan_call_body(text, nested_paren + 1, n, sorted_names, results) - continue - if ch == "(": - # A "(" at the top level, not preceded by "ident =" and not - # a recognized call name (handled above), opens a - # parenthesized positional expression (e.g. an argument - # written as ("a" + "b")) rather than a literal or a bare - # variable reference; mark it unresolved before descending - # into it, the same as any other non-literal positional - # value. - if at_top: - _add_positional(kwargs, _UNRESOLVED) - depth += 1 - i += 1 - continue - if ch == ")": - depth -= 1 - i += 1 - continue - if ch == "[": - # Same reasoning as the "(" branch above: a top-level "[" - # not preceded by "ident =" opens a list expression (e.g. - # an argument written as ["a"][0]). - if at_top: - _add_positional(kwargs, _UNRESOLVED) - bracket_depth += 1 - i += 1 - continue - if ch == "]": - bracket_depth -= 1 - i += 1 - continue - if ch == "{": - # Same reasoning again, for a dict/set expression opening - # a positional argument. - if at_top: - _add_positional(kwargs, _UNRESOLVED) - brace_depth += 1 - i += 1 - continue - if ch == "}": - brace_depth -= 1 - i += 1 - continue - if at_top and _is_identifier_boundary(text, i): m = _IDENT_RE.match(text, i) if m: - ident = m.group(0) j = _skip_ws_and_comments(text, m.end(), n) if j < n and text[j] == "=" and text[j : j + 2] != "==": + ident = m.group(0) rhs_start = _skip_ws_and_comments(text, j + 1, n) if rhs_start < n and ( text[rhs_start] in "\"'" @@ -494,15 +477,42 @@ def _scan_call_body( # back from the source text. Fail closed: mark it # unresolved rather than leaving it absent, which # would read identically to the argument never - # having been given at all. + # having been given at all. Its tokens are still + # scanned below, for nesting and nested calls. kwargs[ident] = _UNRESOLVED i = rhs_start continue - # Not "ident = ...": a bare identifier is a variable - # used as a value, passed positionally; fail closed - # the same way as a non-literal keyword value, rather - # than silently dropping it. - _add_positional(kwargs, _UNRESOLVED) + # Any other first token -- a bare variable reference, a + # parenthesized/list/dict expression, a nested call, ... -- + # starts a positional argument whose value can't be read + # back from the source text; fail closed rather than + # dropping it. The token itself is then scanned below. + _add_positional(kwargs, _UNRESOLVED) + if ch in "\"'" or _is_raw_string_start(text, i, n): + i, _ = _skip_any_string(text, i, n) + continue + if ch == ".": + i = _skip_attribute(text, i, n) + continue + nested_name, nested_paren = _match_call_name(text, i, n, sorted_names) + if nested_name is not None: + i = _scan_call_body(text, nested_paren + 1, n, sorted_names, results) + continue + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + elif ch == "[": + bracket_depth += 1 + elif ch == "]": + bracket_depth -= 1 + elif ch == "{": + brace_depth += 1 + elif ch == "}": + brace_depth -= 1 + elif _is_identifier_boundary(text, i): + m = _IDENT_RE.match(text, i) + if m: i = m.end() continue i += 1 @@ -558,6 +568,9 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, if ch in "\"'" or _is_raw_string_start(text, i, n): i, _ = _skip_any_string(text, i, n) continue + if ch == ".": + i = _skip_attribute(text, i, n) + continue matched_name, paren_index = _match_call_name(text, i, n, sorted_names) if matched_name is None: i += 1 @@ -800,6 +813,45 @@ def test_apple_support_bazel_dep_version_rejects_identifier_substring( with self.assertRaises(AssertionError): _apple_support_bazel_dep_version(sample) + def test_apple_support_bazel_dep_version_ignores_spaced_attribute_call( + self, + ) -> None: + """"ext. bazel_dep(...)" is still an attribute call, whatever + + whitespace, line continuation, or comment separates the "." from + the name; only the real top-level bazel_dep's version counts. + """ + for qualifier in ("ext. ", "ext.\\\n", "[ext. # note\n"): + sample = ( + f'{qualifier}bazel_dep(name = "apple_support", version = "9.9.9")\n' + 'bazel_dep(name = "apple_support", version = "1.24.2")' + ) + with self.subTest(qualifier=qualifier): + self.assertEqual(_apple_support_bazel_dep_version(sample), "1.24.2") + + def test_apple_support_bazel_dep_version_accepts_call_after_comment_ending_in_dot( + self, + ) -> None: + """A "." inside a preceding comment is not a qualifier.""" + sample = '# see the docs.\nbazel_dep(name = "apple_support", version = "2.10.1")' + self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") + + def test_apple_support_bazel_dep_version_rejects_versionless_call( + self, + ) -> None: + """Tokens inside a keyword's value are never positional arguments. + + Here no version is pinned at all; the "9.9.9" literal is just the + tail of repo_name's concatenated value and must not be read back + as bazel_dep's second positional (version) argument. + """ + sample = ( + 'bazel_dep(name = "apple_support", dev_dependency = False, ' + 'repo_name = "apple_support_" + "9.9.9")' + ) + with self.assertRaises(AssertionError): + _apple_support_bazel_dep_version(sample) + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' @@ -1322,6 +1374,32 @@ def test_has_apple_support_override_detects_nested_override_call( ) self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_spaced_attribute_call( + self, + ) -> None: + """"ext. archive_override(...)" is an attribute call, not the + + archive_override directive, whatever separates the "." from the + name. + """ + for qualifier in ("ext. ", "ext.\\\n", "[ext. # note\n"): + sample = ( + f'{qualifier}archive_override(module_name = "apple_support", ' + 'urls = ["x"])' + ) + with self.subTest(qualifier=qualifier): + self.assertFalse(_has_apple_support_override(sample)) + + def test_has_apple_support_override_detects_call_after_comment_ending_in_dot( + self, + ) -> None: + """A "." inside a preceding comment is not a qualifier.""" + sample = ( + "# see the docs.\n" + 'archive_override(module_name = "apple_support", urls = ["x"])' + ) + self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_archive_override(self) -> None: sample = """ archive_override( From d935001255ca303b83f82a0e5aa0abcba50c1c5c Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Fri, 16 Oct 2026 15:37:42 +0100 Subject: [PATCH 28/29] Parse MODULE.bazel with Python's ast instead of a hand-written lexer. Starlark's syntax is a subset of Python's, so ast.parse handles strings, comments, continuations and statement boundaries exactly. This fixes the scanner joining "dep_function = bazel_dep" and a following "(...)" tuple into a fictitious call, and replaces ~1000 lines of lexer and its tokenizer-level tests with ~70 lines. Co-Authored-By: Claude Opus 5.5 --- python/tests/ci_macos_asan_test.py | 1237 ++-------------------------- 1 file changed, 84 insertions(+), 1153 deletions(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 99bcff4c..8d8b539f 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -5,7 +5,7 @@ from __future__ import annotations -import re +import ast import unittest from pathlib import Path @@ -21,581 +21,57 @@ def _repo_root(start: Path | None = None) -> Path: raise AssertionError("could not locate repository root from test file path") -_IDENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") -_POSITIONAL_KEY = "__positional__" -# Sentinel for a module_name/name argument whose value is a variable -# reference or a string concatenation rather than a single literal, so the -# true value can't be read back from the source text. Treated as a possible -# match by every caller here (fail closed) rather than silently absent, -# since the latter would let a real override on the guarded module slip -# through whenever its target is computed instead of written as a literal. +# Sentinel for an argument whose value is not a single string literal (a +# variable reference, a concatenation, ...), so it can't be read back from +# the source. Callers treat it as a possible match (fail closed) rather than +# as absent, so an override on a computed target can't slip past the guard. _UNRESOLVED = object() -def _add_positional(kwargs: dict, value: object) -> None: - """Record a newly-seen top-level positional argument, in call order. +def _literal(node: ast.expr) -> object: + """The node's value if it is a string literal, else _UNRESOLVED.""" + if isinstance(node, ast.Constant) and isinstance(node.value, str): + return node.value + return _UNRESOLVED - Bazel's APIs here accept more than one leading parameter - positionally — not just the first (e.g. bazel_dep's name *and* - version) — so every caller needs all of them, by index, not just - whichever arrived first. Appending (instead of the previous "only - the first" guard) keeps them in the order they appeared. - """ - kwargs.setdefault(_POSITIONAL_KEY, []).append(value) - - -def _positional(kwargs: dict, index: int) -> object | None: - """Return the positional argument at index, or None if fewer were given.""" - values = kwargs.get(_POSITIONAL_KEY, []) - return values[index] if index < len(values) else None - - -def _skip_ws(text: str, i: int, n: int) -> int: - """Skip whitespace, including a backslash-newline line continuation. - - A backslash immediately followed by an LF or CRLF line break is a - line continuation -- dropped entirely, the same rule _decode_escape() - applies inside a string -- so it is whitespace here too; skipping - only the bare characters in " \t\r\n" left the backslash itself - unconsumed, so e.g. "archive_override\\\n(" (the call name and "(" - split across such a continuation) was never recognized as a call. - """ - while i < n: - if text[i] in " \t\r\n": - i += 1 - continue - if text[i] == "\\" and text[i + 1 : i + 2] == "\n": - i += 2 - continue - if text[i] == "\\" and text[i + 1 : i + 3] == "\r\n": - i += 3 - continue - break - return i - - -def _skip_comment(text: str, i: int, n: int) -> int: - while i < n and text[i] != "\n": - i += 1 - return i - - -_SIMPLE_ESCAPES = { - "\\": "\\", - "'": "'", - '"': '"', - "a": "\a", - "b": "\b", - "f": "\f", - "n": "\n", - "r": "\r", - "t": "\t", - "v": "\v", -} -_HEX_DIGITS = "0123456789abcdefABCDEF" -_OCTAL_DIGITS = "01234567" - - -def _decode_escape(text: str, j: int, n: int) -> tuple[int, str]: - """Decode one backslash escape starting at text[j] == "\\". - - Returns (new_j, decoded_text) for Starlark's string-literal escapes: - the simple single-character escapes (\\n, \\t, \\\\, ...), up to 3 - octal digits, exactly two hex digits after \\x, the \\uXXXX / - \\UXXXXXXXX Unicode code point forms (exactly 4 / 8 hex digits), and - a line continuation — a backslash immediately followed by an actual - LF or CRLF line break in the source, which Starlark drops entirely - (unlike "\\n", the two-character escape for a newline *value*). - Discarding the backslash and keeping the next character literally — - the previous behavior here — decodes "\\x61pple_support" (a valid - Starlark escape for "apple_support") to the wrong string - "x61pple_support", letting an escaped module_name bypass the override - guard; "\\u0061pple_support" was the same bug for Unicode escapes, and - a module_name literal split across two source lines by a trailing - backslash was the same bug for a line continuation. An escape this - doesn't recognize is left as a literal backslash + next character. - """ - if j + 1 >= n: - return j + 1, "\\" - c = text[j + 1] - if c == "\r" and j + 2 < n and text[j + 2] == "\n": - return j + 3, "" - if c == "\n": - return j + 2, "" - if c in _SIMPLE_ESCAPES: - return j + 2, _SIMPLE_ESCAPES[c] - if c == "x": - hex_digits = text[j + 2 : j + 4] - if len(hex_digits) == 2 and all(d in _HEX_DIGITS for d in hex_digits): - return j + 4, chr(int(hex_digits, 16)) - return j + 2, "x" - if c in ("u", "U"): - width = 4 if c == "u" else 8 - hex_digits = text[j + 2 : j + 2 + width] - if len(hex_digits) == width and all(d in _HEX_DIGITS for d in hex_digits): - return j + 2 + width, chr(int(hex_digits, 16)) - return j + 2, c - if c in _OCTAL_DIGITS: - k = j + 1 - digits = "" - while k < n and len(digits) < 3 and text[k] in _OCTAL_DIGITS: - digits += text[k] - k += 1 - return k, chr(int(digits, 8)) - return j + 2, "\\" + c - - -def _skip_string(text: str, i: int, n: int) -> tuple[int, str]: - """Return (end_index, value) for the string literal starting at text[i]. - - Handles Starlark triple-quoted strings as a single opaque unit — the - closing delimiter is matched as the 3-character sequence, never a - single quote character, so none of the content in between (including - an embedded quote that would otherwise look like the end of an - ordinary string) is ever scanned character by character for a fake - nested call — while still extracting its value just like an ordinary - quoted string, so a (valid, if unusual) triple-quoted keyword value is - captured rather than silently dropped. Both forms decode backslash - escapes via _decode_escape() (value is the unescaped content). - """ - quote = text[i] - if text[i : i + 3] == quote * 3: - delim = quote * 3 - j = i + 3 - chars: list[str] = [] - while j < n: - if text[j] == "\\" and j + 1 < n: - j, decoded = _decode_escape(text, j, n) - chars.append(decoded) - continue - if text[j : j + 3] == delim: - return j + 3, "".join(chars) - chars.append(text[j]) - j += 1 - return n, "".join(chars) - j = i + 1 - chars = [] - while j < n: - if text[j] == "\\" and j + 1 < n: - j, decoded = _decode_escape(text, j, n) - chars.append(decoded) - continue - if text[j] == quote: - return j + 1, "".join(chars) - chars.append(text[j]) - j += 1 - return n, "".join(chars) - - -def _is_raw_string_start(text: str, i: int, n: int) -> bool: - """True if text[i] starts a raw string literal: "r"/"R" + quote.""" - return i + 1 < n and text[i] in "rR" and text[i + 1] in "\"'" - - -def _skip_raw_string(text: str, i: int, n: int) -> tuple[int, str]: - """Return (end_index, value) for the raw string starting at text[i]. - - text[i] is the delimiter quote itself (the caller has already - consumed the "r"/"R" prefix). Per the Starlark spec (and matching - real Python raw-string behavior, which it mirrors here), a raw - string gives a backslash exactly one special power: letting a - literal quote appear without ending the string. Whether a quote - that follows a run of backslashes is that kind of escaped quote - depends on the run's *parity* — r"foo\\" (an even, here zero-after- - pairing, run) ends normally with two literal backslashes in the - value, while r"foo\"" (an odd run) does not end there; the quote is - part of the value, and — unlike an ordinary string's - _decode_escape() — the backslash stays in the value too, exactly as - written. Handles a triple-quote delimiter the same way _skip_string() - does — matched as the 3-character sequence once any escaped quote - immediately before it has been accounted for the same way. - """ - quote = text[i] - if text[i : i + 3] == quote * 3: - delim = quote * 3 - j = i + 3 - chars: list[str] = [] - while j < n: - if text[j] == "\\": - k = j - while k < n and text[k] == "\\": - k += 1 - run = text[j:k] - if k < n and text[k] == quote and len(run) % 2 == 1: - chars.append(run) - chars.append(quote) - j = k + 1 - continue - chars.append(run) - j = k - continue - if text[j : j + 3] == delim: - return j + 3, "".join(chars) - chars.append(text[j]) - j += 1 - return n, "".join(chars) - j = i + 1 - chars = [] - while j < n: - if text[j] == "\\": - k = j - while k < n and text[k] == "\\": - k += 1 - run = text[j:k] - if k < n and text[k] == quote and len(run) % 2 == 1: - chars.append(run) - chars.append(quote) - j = k + 1 - continue - chars.append(run) - j = k - continue - if text[j] == quote: - return j + 1, "".join(chars) - chars.append(text[j]) - j += 1 - return n, "".join(chars) - - -def _skip_any_string(text: str, i: int, n: int) -> tuple[int, str]: - """Return (end_index, value) for the string literal starting at text[i], - - recognizing an optional "r"/"R" raw-string prefix before dispatching - to _skip_raw_string() or _skip_string(). A caller that needs to - recognize whether a token *is* a string literal (as opposed to - skipping over one already known to start exactly at i) uses this - instead of _skip_string() directly, so a raw literal like - r"rules_cc" is never misread as a bare identifier reference — which - fails closed on this guard just like any other unresolved value, - wrongly blocking a completely unrelated module's override. - """ - if _is_raw_string_start(text, i, n): - return _skip_raw_string(text, i + 1, n) - return _skip_string(text, i, n) +def _calls(call_names: tuple[str, ...], module_bazel: str) -> list[ast.Call]: + """Every call, at any nesting depth, of a bare name in call_names. -def _skip_ws_and_comments(text: str, i: int, n: int) -> int: - """Skip whitespace and "#" comments, repeatedly, starting at i. - - A single _skip_ws() stops at the first "#"; this loops so a comment - followed by more whitespace (its trailing newline, a blank line, or - another comment) is all consumed, landing on the next real token - regardless of how it's laid out across lines. - """ - j = i - while True: - skipped = _skip_ws(text, j, n) - if skipped < n and text[skipped] == "#": - skipped = _skip_comment(text, skipped, n) - if skipped == j: - break - j = skipped - return j - - -def _is_complete_literal(text: str, i: int, n: int) -> bool: - """True if the string literal ending at i is a whole call argument. - - Skips whitespace and comments after the literal. The literal is the - argument's complete, resolved value only if what follows is "," - (ending this argument) or ")" (ending the call) — anything else, - including "+" (concatenation, even across an intervening comment), - "[" (slicing), "%" (string formatting), "." (a method call), or any - other operator, means the literal is merely a fragment of a larger - expression whose true value can't be read back from the source text. + MODULE.bazel is Starlark, whose syntax is a subset of Python's, so + Python's own parser handles strings, comments, line continuations and + statement boundaries exactly. Attribute calls such as ext.bazel_dep(...) + have an ast.Attribute callee and are excluded; grouping parens around a + callable, as in (bazel_dep)(...), leave no trace in the tree. """ - j = _skip_ws_and_comments(text, i, n) - return j < n and text[j] in ",)" - + return [ + node + for node in ast.walk(ast.parse(module_bazel)) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Name) + and node.func.id in call_names + ] -def _is_identifier_boundary(text: str, i: int) -> bool: - """True unless the character before i could continue an identifier. - Rejects matching a call/keyword name in the middle of a longer - identifier (e.g. "legacy_bazel_dep(" or "bazel_depfoo(") and rejects a - qualified/attribute call (e.g. "extensions.bazel_dep(") by also - treating a preceding "." as non-boundary. - """ - if i == 0: - return True - prev = text[i - 1] - return not (prev.isalnum() or prev == "_" or prev == ".") - - -def _match_parenthesized_call( - text: str, i: int, n: int, sorted_names: list[str] -) -> tuple[str | None, int]: - """Match one or more layers of "(" name ")" grouping, starting at - text[i] == "(", followed by the real call's "(". - - Grouping parens around any expression are legal Starlark and nest - arbitrarily, and the result can still be called immediately, so - "((archive_override))(...)" is just as valid a call as the single- - layer "(archive_override)(...)" — but neither matches a bare - "name(" call textually, letting this scanner's direct matching be - bypassed by wrapping the name in one or more layers of parens. - Comments are tolerated around every token here too, the same as - Starlark itself ignores them: "(archive_override # note\n)(...)" is - just as valid. Returns (matched_name, call_paren_index); - matched_name is None if this position isn't that pattern. - """ - depth = 0 - j = i - while j < n and text[j] == "(": - depth += 1 - j = _skip_ws_and_comments(text, j + 1, n) - if depth == 0 or not _is_identifier_boundary(text, j): - return None, 0 - for name in sorted_names: - if text.startswith(name, j): - k = _skip_ws_and_comments(text, j + len(name), n) - closed = 0 - while closed < depth and k < n and text[k] == ")": - closed += 1 - k = _skip_ws_and_comments(text, k + 1, n) - if closed == depth and k < n and text[k] == "(": - return name, k - return None, 0 - - -def _skip_attribute(text: str, i: int, n: int) -> int: - """Skip a "." (at text[i]) and the attribute name that follows it. - - Starlark allows whitespace, line continuations, and (inside brackets) - comments between the "." and the name, so "ext. bazel_dep(...)" is - still an attribute call. Checking only the character just before a - name can't see that; consuming the name here, in the same forward - scan that already skips strings and comments, means it is never - offered to the call matcher at all, while a "." inside a comment or - string is never seen. - """ - j = _skip_ws_and_comments(text, i + 1, n) - m = _IDENT_RE.match(text, j) - return m.end() if m else i + 1 - - -def _match_call_name( - text: str, i: int, n: int, sorted_names: list[str] -) -> tuple[str | None, int]: - """Try to match a call "name(...)" at text[i], directly or through - - one or more layers of grouping parens (see _match_parenthesized_call). - Whitespace and comments between the name and its "(" are tolerated - either way. Returns (matched_name, index_of_opening_paren), or - (None, 0) if text[i] isn't the start of a recognized call. - """ - if _is_identifier_boundary(text, i): - for name in sorted_names: - if text.startswith(name, i): - j = _skip_ws_and_comments(text, i + len(name), n) - if j < n and text[j] == "(": - return name, j - if i < n and text[i] == "(": - return _match_parenthesized_call(text, i, n, sorted_names) - return None, 0 - - -def _scan_call_body( - text: str, - i: int, - n: int, - sorted_names: list[str], - results: list[dict[str, str]], -) -> int: - """Scan one call's body, starting right after its opening "(". - - Parses the call's own arguments (see _iter_call_kwargs for why only - at its top-level depth), then appends the resulting {keyword: value} - dict to `results` and returns the index just past the matching - closing ")". - - Each argument is classified once, by its first token: "identifier - =" makes it a keyword argument, anything else a positional one - (resolved only when the whole argument is a single string literal). - Every later token up to the next top-level "," belongs to that same - argument and is never recorded on its own -- otherwise, e.g., the - "9.9.9" tail of `repo_name = "apple_support_" + "9.9.9"` would be - read back as an extra positional argument (bazel_dep's version). - - A recognized call name appearing anywhere in the body -- not just as - this call's own name -- is itself a real call Bazel executes, e.g. - `patches = single_version_override(...) or []`; such a nested call - is recursively scanned the same way (appending its own dict to - `results` too) and its matched span is skipped whole, rather than - being silently swallowed as inert text while this call's argument - list is paren-balanced. - """ - depth = 1 - bracket_depth = 0 - brace_depth = 0 - at_arg_start = True - kwargs: dict[str, str] = {} - while i < n and depth > 0: - ch = text[i] - if ch == "#": - i = _skip_comment(text, i, n) - continue - ws_end = _skip_ws(text, i, n) - if ws_end > i: - i = ws_end - continue - at_top = depth == 1 and bracket_depth == 0 and brace_depth == 0 - if at_top and ch == ",": - at_arg_start = True - i += 1 - continue - if at_top and at_arg_start and ch != ")": - at_arg_start = False - if ch in "\"'" or _is_raw_string_start(text, i, n): - end, value = _skip_any_string(text, i, n) - _add_positional( - kwargs, - value if _is_complete_literal(text, end, n) else _UNRESOLVED, - ) - i = end - continue - m = _IDENT_RE.match(text, i) - if m: - j = _skip_ws_and_comments(text, m.end(), n) - if j < n and text[j] == "=" and text[j : j + 2] != "==": - ident = m.group(0) - rhs_start = _skip_ws_and_comments(text, j + 1, n) - if rhs_start < n and ( - text[rhs_start] in "\"'" - or _is_raw_string_start(text, rhs_start, n) - ): - end, value = _skip_any_string(text, rhs_start, n) - kwargs[ident] = ( - value - if _is_complete_literal(text, end, n) - else _UNRESOLVED - ) - i = end - continue - # A non-literal right-hand side (a variable - # reference, a function call, ...) can't be read - # back from the source text. Fail closed: mark it - # unresolved rather than leaving it absent, which - # would read identically to the argument never - # having been given at all. Its tokens are still - # scanned below, for nesting and nested calls. - kwargs[ident] = _UNRESOLVED - i = rhs_start - continue - # Any other first token -- a bare variable reference, a - # parenthesized/list/dict expression, a nested call, ... -- - # starts a positional argument whose value can't be read - # back from the source text; fail closed rather than - # dropping it. The token itself is then scanned below. - _add_positional(kwargs, _UNRESOLVED) - if ch in "\"'" or _is_raw_string_start(text, i, n): - i, _ = _skip_any_string(text, i, n) - continue - if ch == ".": - i = _skip_attribute(text, i, n) - continue - nested_name, nested_paren = _match_call_name(text, i, n, sorted_names) - if nested_name is not None: - i = _scan_call_body(text, nested_paren + 1, n, sorted_names, results) - continue - if ch == "(": - depth += 1 - elif ch == ")": - depth -= 1 - elif ch == "[": - bracket_depth += 1 - elif ch == "]": - bracket_depth -= 1 - elif ch == "{": - brace_depth += 1 - elif ch == "}": - brace_depth -= 1 - elif _is_identifier_boundary(text, i): - m = _IDENT_RE.match(text, i) - if m: - i = m.end() - continue - i += 1 - results.append(kwargs) - return i - - -def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, str]]: - """Yield the {keyword: string value} args of each call, top-level or - - nested in another call's argument value (see _scan_call_body). - - A single linear scan recognizes a call name, and each "identifier = - value" pair inside its body, only at a genuine identifier boundary and - only outside strings/comments. This matters beyond just truncation: a - plain regex search over body *text* (even with comments stripped) - still matches an unrelated argument's *string value* that happens to - contain assignment-shaped text, e.g. - `patch_cmds = ['historically name = "apple_support", ...']` — a - bazel_dep for a different module would be misread as the apple_support - one. Parsing real "identifier = value" pairs only at the call's - top-level parenthesis depth, with identifier-matching paused for every - character inside a string or comment (string/comment skipping is a - single jump via _skip_string/_skip_comment, not a per-character toggle, - so a triple-quoted string's content — including embedded quote - characters — is never scanned as code), makes that impossible: nothing - inside a string literal or comment is ever offered to the identifier - matcher. Only simple `identifier = "string"` / `identifier = 'string'` - assignments are captured (list values are skipped over for paren - balancing but not parsed, aside from a nested recognized call). - Whitespace between a call name and "(" is tolerated ("archive_override - (..." is valid Starlark). - - Bazel's `*_override`/`bazel_dep` APIs accept their leading arguments - (module_name / name, and for bazel_dep also version) positionally — - every bare top-level value before any "identifier =" is appended, in - order, to the list under `_POSITIONAL_KEY` (see `_add_positional`), - letting callers read any of them by index (`_positional`), not just - the first. "Top-level" also tracks `[...]`/`{...}` nesting (not just - parens), so a string that's an *element* of a list value (e.g. - inside `urls = [...]`) is never mistaken for a positional call - argument. - """ - sorted_names = sorted(call_names, key=len, reverse=True) - results: list[dict[str, str]] = [] - i = 0 - n = len(text) - while i < n: - ch = text[i] - if ch == "#": - i = _skip_comment(text, i, n) - continue - if ch in "\"'" or _is_raw_string_start(text, i, n): - i, _ = _skip_any_string(text, i, n) - continue - if ch == ".": - i = _skip_attribute(text, i, n) - continue - matched_name, paren_index = _match_call_name(text, i, n, sorted_names) - if matched_name is None: - i += 1 - continue - i = _scan_call_body(text, paren_index + 1, n, sorted_names, results) - return results +def _argument(call: ast.Call, keyword: str, position: int) -> object | None: + """The argument given as keyword or at position, or None if absent.""" + for kw in call.keywords: + if kw.arg == keyword: + return _literal(kw.value) + if position < len(call.args): + return _literal(call.args[position]) + return None def _apple_support_bazel_dep_version(module_bazel: str) -> str: """Return the pinned apple_support bazel_dep version. - bazel_dep's first two parameters, name and version, are both - allowed positionally (bazel_dep("apple_support", "2.10.1")), not - just name; _positional(kwargs, 1) reads version there the same way - _positional(kwargs, 0) already reads name, rather than assuming - version is only ever given as a keyword. + bazel_dep's name and version may each be given positionally or by + keyword. """ - for kwargs in _iter_call_kwargs(("bazel_dep",), module_bazel): - name = kwargs.get("name") - if name is None: - name = _positional(kwargs, 0) - if name == "apple_support": - version = kwargs.get("version") - if version is None: - version = _positional(kwargs, 1) + for call in _calls(("bazel_dep",), module_bazel): + if _argument(call, "name", 0) == "apple_support": + version = _argument(call, "version", 1) if version is not None and version is not _UNRESOLVED: return version raise AssertionError('expected bazel_dep(... name = "apple_support" ...)') @@ -604,17 +80,9 @@ def _apple_support_bazel_dep_version(module_bazel: str) -> str: def _has_apple_support_override(module_bazel: str) -> bool: """True if any Bazel module override directive targets apple_support. - Covers all five override directives (archive_override, git_override, - local_path_override, multiple_version_override, single_version_override) - — not just single_version_override — and matches each call's keyword - or first-positional module_name argument regardless of argument order - or formatting, rather than assuming module_name is the first line - after the opening "(". Fails closed (returns True) for an override - whose module_name/name is a variable reference or a string - concatenation — _UNRESOLVED — since the real text that will resolve - to at runtime can't be read back from the source, and treating that - the same as "not apple_support" would let a real override on a - computed target slip past this guard undetected. + Covers all five override directives. Fails closed (returns True) for + an override whose module_name is not a string literal, since its + runtime value can't be read back from the source. """ override_names = ( "archive_override", @@ -623,10 +91,8 @@ def _has_apple_support_override(module_bazel: str) -> bool: "multiple_version_override", "single_version_override", ) - for kwargs in _iter_call_kwargs(override_names, module_bazel): - module_name = kwargs.get("module_name") - if module_name is None: - module_name = _positional(kwargs, 0) + for call in _calls(override_names, module_bazel): + module_name = _argument(call, "module_name", 0) if module_name == "apple_support" or module_name is _UNRESOLVED: return True return False @@ -671,17 +137,6 @@ def test_apple_support_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "2.10.1", name = "apple_support")' self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_accepts_raw_triple_quoted_name( - self, - ) -> None: - """name = r\"\"\"apple_support\"\"\" is a valid Starlark raw - - triple-quoted literal; it must resolve to the complete literal - rather than stopping at the first lone quote. - """ - sample = 'bazel_dep(name = r"""apple_support""", version = "2.10.1")' - self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_accepts_positional_name( self, ) -> None: @@ -703,17 +158,6 @@ def test_apple_support_bazel_dep_version_accepts_positional_version( sample = 'bazel_dep("apple_support", "2.10.1")' self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_survives_an_earlier_paren( - self, - ) -> None: - """A ")" inside a leading comment must not truncate the call body.""" - sample = """bazel_dep( - # fixes Xcode 27 crosstool warnings (see docs) - version = "2.10.1", - name = "apple_support", -)""" - self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_ignores_commented_out_call( self, ) -> None: @@ -722,51 +166,6 @@ def test_apple_support_bazel_dep_version_ignores_commented_out_call( """ self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_ignores_commented_out_version( - self, - ) -> None: - """A stale version in a comment inside the call must not win.""" - sample = """bazel_dep( - name = "apple_support", - # version = "9.9.9" (stale) - version = "2.10.1", -)""" - self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - - def test_apple_support_bazel_dep_version_skips_comment_after_equals( - self, - ) -> None: - """A comment between "=" and the literal on the next line is valid - - Starlark formatting; it must be skipped, not mistaken for a - non-literal RHS that leaves the version unresolved. - """ - sample = """bazel_dep( - name = "apple_support", - version = - # pinned past the Xcode 27 fixes - "2.10.1", -)""" - self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - - def test_apple_support_bazel_dep_version_skips_comment_between_keyword_and_equals( - self, - ) -> None: - """A comment between the keyword and "=" is valid Starlark - - formatting (same as between "=" and the value); it must be - skipped so the keyword is still recognized as "name", not - misread as a bare positional identifier that leaves the real - name argument unresolved. - """ - sample = """bazel_dep( - name - # pinned past the Xcode 27 fixes - = "apple_support", - version = "2.10.1", -)""" - self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_ignores_fake_assignment_in_string( self, ) -> None: @@ -821,21 +220,14 @@ def test_apple_support_bazel_dep_version_ignores_spaced_attribute_call( whitespace, line continuation, or comment separates the "." from the name; only the real top-level bazel_dep's version counts. """ - for qualifier in ("ext. ", "ext.\\\n", "[ext. # note\n"): + for qualifier, close in (("ext. ", ""), ("ext.\\\n", ""), ("[ext. # note\n", "]")): sample = ( - f'{qualifier}bazel_dep(name = "apple_support", version = "9.9.9")\n' + f'{qualifier}bazel_dep(name = "apple_support", version = "9.9.9"){close}\n' 'bazel_dep(name = "apple_support", version = "1.24.2")' ) with self.subTest(qualifier=qualifier): self.assertEqual(_apple_support_bazel_dep_version(sample), "1.24.2") - def test_apple_support_bazel_dep_version_accepts_call_after_comment_ending_in_dot( - self, - ) -> None: - """A "." inside a preceding comment is not a qualifier.""" - sample = '# see the docs.\nbazel_dep(name = "apple_support", version = "2.10.1")' - self.assertEqual(_apple_support_bazel_dep_version(sample), "2.10.1") - def test_apple_support_bazel_dep_version_rejects_versionless_call( self, ) -> None: @@ -852,6 +244,27 @@ def test_apple_support_bazel_dep_version_rejects_versionless_call( with self.assertRaises(AssertionError): _apple_support_bazel_dep_version(sample) + def test_apple_support_bazel_dep_version_keeps_statements_separate( + self, + ) -> None: + """A newline outside brackets ends a statement, so a callable on one + line and a tuple on the next are two statements, not a call. + """ + for callable_expr in ("bazel_dep", "(bazel_dep)"): + sample = ( + f"dep_function = {callable_expr}\n" + '("apple_support", "9.9.9")\n' + 'bazel_dep(name = "apple_support", version = "1.24.2")\n' + ) + with self.subTest(callable_expr=callable_expr): + self.assertEqual(_apple_support_bazel_dep_version(sample), "1.24.2") + + def test_has_apple_support_override_keeps_statements_separate(self) -> None: + for callable_expr in ("archive_override", "(archive_override)"): + sample = f'override = {callable_expr}\n("apple_support", "x")\n' + with self.subTest(callable_expr=callable_expr): + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_detects_same_line_args(self) -> None: sample = ( 'single_version_override(module_name = "apple_support", version = "1.24.2")' @@ -872,200 +285,26 @@ def test_has_apple_support_override_detects_positional_module_name( ) self.assertTrue(_has_apple_support_override(sample)) - def test_has_apple_support_override_detects_triple_quoted_module_name( - self, - ) -> None: - """A triple-quoted keyword value is valid Starlark and must still - - be extracted, not silently treated as absent. - """ - sample = ( - 'single_version_override(module_name = """apple_support""", ' - 'version = "1.24.2")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_decodes_hex_escape_in_module_name( - self, - ) -> None: - """\\x61 is a valid Starlark hex escape for "a"; the decoded value - - must still be compared, not the raw escape text. - """ - sample = ( - 'single_version_override(module_name = "\\x61pple_support", ' - 'version = "1.24.2")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_decodes_unicode_escape_in_module_name( + def test_has_apple_support_override_fails_closed_on_non_literal_module_name( self, ) -> None: - """\\u0061 is a valid Starlark 4-hex-digit Unicode escape for "a"; - - the decoded value must still be compared, not the raw escape text. - """ - sample = ( - 'single_version_override(module_name = "\\u0061pple_support", ' - 'version = "1.24.2")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_decodes_big_unicode_escape_in_module_name( - self, - ) -> None: - """\\U00000061 is a valid Starlark 8-hex-digit Unicode escape for - - "a"; the decoded value must still be compared, not the raw escape - text. - """ - sample = ( - 'single_version_override(module_name = "\\U00000061pple_support", ' - 'version = "1.24.2")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_decodes_line_continuation_in_module_name( - self, - ) -> None: - """A backslash immediately followed by an actual newline in the - - source is a line continuation that Starlark drops entirely, not - the two-character "\\n" escape for a newline value; the module - name spans two source lines but still resolves to one literal. - """ - sample = ( - 'archive_override(module_name = "apple_\\\n' - 'support", strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_decodes_crlf_line_continuation_in_module_name( - self, - ) -> None: - """Same as the LF case, but for a CRLF line ending.""" - sample = ( - 'archive_override(module_name = "apple_\\\r\n' - 'support", strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_keyword_variable_module_name( - self, - ) -> None: - """module_name set to a variable reference can't be read back from - - the source text; the guard must fail closed (treat it as a - possible apple_support override) rather than silently treating - the override as targeting some other module. - """ - sample = 'archive_override(module_name = target, strip_prefix = "x")' - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_positional_variable_module_name( - self, - ) -> None: - """Same as the keyword case, but module_name is passed positionally - - as a variable reference. - """ - sample = 'archive_override(target, strip_prefix = "x")' - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_parenthesized_positional_module_name( - self, - ) -> None: - """A parenthesized expression passed as the first positional - - argument (e.g. ("apple_" + "support")) can't be read back as a - literal from the source text either; it must fail closed the - same as any other non-literal positional value, not be silently - skipped while descending into the parens. + """A module_name that is not a single string literal can't be read + back from the source; the guard must treat it as a possible + apple_support override rather than as some other module. """ - sample = 'archive_override(("apple_" + "support"), strip_prefix = "x")' - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_list_positional_module_name( - self, - ) -> None: - """A list expression passed as the first positional argument (e.g. - - ["apple_support"][0]) can't be read back as a literal from the - source text either; it must fail closed, not be silently skipped - while descending into the list. - """ - sample = 'archive_override(["apple_support"][0], strip_prefix = "x")' - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_dict_positional_module_name( - self, - ) -> None: - """Same as the list case, but for a dict expression opening the - - first positional argument. - """ - sample = ( - 'archive_override({"apple_support": 1}.keys()[0], ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_concatenated_module_name( - self, - ) -> None: - """module_name assembled via string concatenation has no single - - literal value in the source text; only the first fragment - ("apple_") would be captured by a naive literal-only parser, which - must not be mistaken for the whole, real value. - """ - sample = ( - 'archive_override(module_name = "apple_" + "support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_concat_split_by_comment( - self, - ) -> None: - """A comment between the first fragment and the "+" must not hide - - the concatenation: the literal still isn't the whole argument. - """ - sample = """archive_override( - module_name = "apple_" # fixes a typo below - + "support", - strip_prefix = "x", -)""" - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_sliced_module_name( - self, - ) -> None: - """A slice expression is a literal followed by "[...]", not a - - complete argument by itself; the literal alone must not be - mistaken for the slice's actual result. - """ - sample = ( - 'archive_override(module_name = "xapple_support"[1:], ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_fails_closed_on_percent_formatted_module_name( - self, - ) -> None: - """A "%"-formatted literal is a template, not the resolved value; - - "apple_%s" % "support" resolves to "apple_support" but the - literal "apple_%s" alone must not be mistaken for it. - """ - sample = ( - 'archive_override(module_name = "apple_%s" % "support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) + for module_name in ( + "module_name = target", + "target", + '("apple_" + "support")', + '["apple_support"][0]', + '{"apple_support": 1}.keys()[0]', + 'module_name = "apple_" + "support"', + 'module_name = "xapple_support"[1:]', + 'module_name = "apple_%s" % "support"', + ): + sample = f'archive_override({module_name}, strip_prefix = "x")' + with self.subTest(module_name=module_name): + self.assertTrue(_has_apple_support_override(sample)) def test_has_apple_support_override_ignores_positional_string_in_list( self, @@ -1091,15 +330,6 @@ def test_has_apple_support_override_detects_reordered_args(self) -> None: """ self.assertTrue(_has_apple_support_override(sample)) - def test_has_apple_support_override_survives_an_earlier_paren(self) -> None: - """A ")" inside a leading comment must not truncate the call body.""" - sample = """single_version_override( - # pin past the Xcode 27 fixes (see docs) - module_name = "apple_support", - version = "1.24.2", -)""" - self.assertTrue(_has_apple_support_override(sample)) - def test_has_apple_support_override_ignores_commented_out_call(self) -> None: sample = ( '# archive_override(module_name = "apple_support", ' @@ -1116,187 +346,6 @@ def test_has_apple_support_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_apple_support_override(sample)) - def test_has_apple_support_override_ignores_raw_string_for_other_module( - self, - ) -> None: - """r"rules_cc" is a valid Starlark raw string literal; it must be - - recognized as the complete, resolved literal "rules_cc" rather - than misread as a non-literal (bare identifier "r" + a separate - quoted string), which would fail closed and incorrectly block a - valid override on a completely unrelated module. - """ - sample = 'archive_override(module_name = r"rules_cc", strip_prefix = "x")' - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_raw_string_module_name( - self, - ) -> None: - """A raw string literal is still a real override when it does - - name apple_support. - """ - sample = ( - 'archive_override(module_name = r"apple_support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_raw_string_positional_module_name( - self, - ) -> None: - """A raw string literal is recognized in the first-positional form - - too, not just as a keyword value. - """ - sample = 'archive_override(r"apple_support", strip_prefix = "x")' - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_ignores_raw_triple_quoted_for_other_module( - self, - ) -> None: - """r\"\"\"rules_cc\"\"\" is a valid Starlark raw triple-quoted literal; - - it must resolve to the complete literal "rules_cc" rather than - stopping at the first lone quote (which would leave the value - unresolved and fail closed, incorrectly blocking a valid - override on a completely unrelated module). - """ - sample = ( - 'archive_override(module_name = r"""rules_cc""", ' - 'strip_prefix = "x")' - ) - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_raw_triple_quoted_module_name( - self, - ) -> None: - """A raw triple-quoted literal is still a real override when it - - does name apple_support. - """ - sample = ( - 'archive_override(module_name = r"""apple_support""", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_decodes_escaped_quote_in_raw_string( - self, - ) -> None: - """Starlark raw strings still give backslash-quote special meaning - - (a literal quote, string continues) even though every other - backslash is kept completely literal; r"apple_\\"support" (an - embedded quote) is a different string from "apple_support" and - must not be mistaken for an apple_support override. - """ - sample = ( - 'archive_override(module_name = r"apple_\\"support", ' - 'strip_prefix = "x")' - ) - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_ends_raw_string_at_even_backslash_run( - self, - ) -> None: - """An even run of backslashes immediately before the closing quote - - (here two) pairs up and does not escape it; the raw string ends - normally there, with both backslashes kept literally in the - value, so the argument after it (module_name) is parsed - correctly rather than being swallowed into a runaway string. - """ - sample = ( - 'archive_override(strip_prefix = r"apple_support\\\\", ' - 'module_name = "rules_cc")' - ) - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_continues_raw_string_at_odd_backslash_run( - self, - ) -> None: - """An odd run of backslashes immediately before a quote (here - - three) still escapes it, same as a single backslash; the string - does not end there and both the backslashes and the quote stay - literally in the value. - """ - sample = ( - 'archive_override(module_name = r"apple_support\\\\\\"x", ' - 'strip_prefix = "x")' - ) - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_ignores_raw_string_outside_recognized_call( - self, - ) -> None: - """A raw string literal outside any recognized call must be - - skipped using the raw-aware dispatcher, the same as inside a - call body; otherwise its contents are fed to the ordinary - escape decoder, which raises on sequences that are only valid - because the "r" prefix makes them literal (e.g. "\\U12345678" - is not a valid 8-hex-digit Unicode escape, and "\\e" is not a - recognized simple escape either), spuriously failing a guard - that doesn't even apply to this unrelated statement. - """ - sample = ( - 'SDK_PATH = r"C:\\U12345678\\emsdk"\n' - 'archive_override(module_name = "rules_cc", strip_prefix = "x")' - ) - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_skips_comment_between_equals_and_value( - self, - ) -> None: - """A comment between "=" and the literal on the next line is valid - - Starlark formatting; it must be skipped, not mistaken for a - non-literal RHS that falsely trips the guard for an override on - some other module. - """ - sample = """archive_override( - module_name = - # pin past Xcode 27 fixes - "rules_cc", - strip_prefix = "x", -)""" - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_skips_comment_between_keyword_and_equals( - self, - ) -> None: - """A comment between the keyword and "=" is valid Starlark - - formatting; it must be skipped so "module_name" is still - recognized as that keyword, not misread as a bare positional - identifier -- which would fail closed and falsely trip the - guard even though the real module_name names a different, - unrelated module. - """ - sample = """local_path_override( - module_name - # note - = "emsdk", - path = "../emsdk", -)""" - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_ignores_commented_module_name( - self, - ) -> None: - """A comment mentioning apple_support in a rules_cc override is not - - itself an apple_support override. - """ - sample = """single_version_override( - # module_name = "apple_support" (old pin, no longer used) - module_name = "rules_cc", - version = "0.2.26", -)""" - self.assertFalse(_has_apple_support_override(sample)) - def test_has_apple_support_override_ignores_fake_assignment_in_string( self, ) -> None: @@ -1311,52 +360,6 @@ def test_has_apple_support_override_ignores_fake_assignment_in_string( )""" self.assertFalse(_has_apple_support_override(sample)) - def test_has_apple_support_override_tolerates_whitespace_before_paren( - self, - ) -> None: - sample = 'archive_override (module_name = "apple_support", strip_prefix = "x")' - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_call_after_lf_continuation( - self, - ) -> None: - """A backslash-LF line continuation between the call name and "(" - - is valid Starlark, dropped entirely the same as inside a string; - it must not leave the call unrecognized. - """ - sample = ( - 'archive_override\\\n(module_name = "apple_support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_call_after_crlf_continuation( - self, - ) -> None: - """Same as the LF case, for a backslash-CRLF line continuation.""" - sample = ( - 'archive_override\\\r\n(module_name = "apple_support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_comment_before_own_paren( - self, - ) -> None: - """A comment between the callable name and its own "(" is legal - - inside a parenthesized expression statement, e.g. - "(local_path_override # note\\n(module_name = ..., ...))"; the - direct call-name match must skip comments, not just whitespace, - there too. - """ - sample = ( - "(local_path_override # note\n" - '(module_name = "apple_support", path = "../apple_support"))' - ) - self.assertTrue(_has_apple_support_override(sample)) - def test_has_apple_support_override_detects_nested_override_call( self, ) -> None: @@ -1382,24 +385,14 @@ def test_has_apple_support_override_ignores_spaced_attribute_call( archive_override directive, whatever separates the "." from the name. """ - for qualifier in ("ext. ", "ext.\\\n", "[ext. # note\n"): + for qualifier, close in (("ext. ", ""), ("ext.\\\n", ""), ("[ext. # note\n", "]")): sample = ( f'{qualifier}archive_override(module_name = "apple_support", ' - 'urls = ["x"])' + f'urls = ["x"]){close}' ) with self.subTest(qualifier=qualifier): self.assertFalse(_has_apple_support_override(sample)) - def test_has_apple_support_override_detects_call_after_comment_ending_in_dot( - self, - ) -> None: - """A "." inside a preceding comment is not a qualifier.""" - sample = ( - "# see the docs.\n" - 'archive_override(module_name = "apple_support", urls = ["x"])' - ) - self.assertTrue(_has_apple_support_override(sample)) - def test_has_apple_support_override_detects_archive_override(self) -> None: sample = """ archive_override( @@ -1437,68 +430,6 @@ def test_has_apple_support_override_rejects_longer_identifier_in_parens( ) self.assertFalse(_has_apple_support_override(sample)) - def test_has_apple_support_override_detects_parenthesized_call_with_comment( - self, - ) -> None: - """Starlark ignores comments; a comment between the grouped - - callable name and the closing paren -- (archive_override # - note\\n)(...) -- must not hide this call from the same - detection that handles the plain "(name)(" form. - """ - sample = ( - '(archive_override # note\n' - ')(module_name = "apple_support", strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_doubly_parenthesized_call( - self, - ) -> None: - """Grouping parens nest arbitrarily in Starlark; two layers -- - - ((archive_override))(...) -- are just as valid a call as one, - and must not bypass the same detection that handles the - single-layer "(name)(" form. - """ - sample = ( - '((archive_override))(module_name = "apple_support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_detects_triply_parenthesized_call( - self, - ) -> None: - """Same as the double-layer case, but with three layers.""" - sample = ( - '(((archive_override)))(module_name = "apple_support", ' - 'strip_prefix = "x")' - ) - self.assertTrue(_has_apple_support_override(sample)) - - def test_has_apple_support_override_ignores_doubly_parenthesized_other_module( - self, - ) -> None: - sample = ( - '((archive_override))(module_name = "rules_cc", ' - 'strip_prefix = "x")' - ) - self.assertFalse(_has_apple_support_override(sample)) - - def test_has_apple_support_override_rejects_longer_identifier_in_nested_parens( - self, - ) -> None: - """"((archive_override_foo))(...)" must not be misread as a - - doubly-parenthesized reference to archive_override. - """ - sample = ( - '((archive_override_foo))(module_name = "apple_support", ' - 'strip_prefix = "x")' - ) - self.assertFalse(_has_apple_support_override(sample)) - def test_has_apple_support_override_detects_git_override(self) -> None: sample = """ git_override( From 22b30abd6ef5fcdbf3a0460d1b5528fdfb6c94a3 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Fri, 16 Oct 2026 17:56:23 +0100 Subject: [PATCH 29/29] Fail closed when **kwargs could supply an override's module_name. ast records keyword unpacking as a keyword with arg None, so local_path_override(**{"module_name": "apple_support", ...}) read as having no module_name and slipped past the guard. Co-Authored-By: Claude Opus 5.5 --- python/tests/ci_macos_asan_test.py | 38 +++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/python/tests/ci_macos_asan_test.py b/python/tests/ci_macos_asan_test.py index 8d8b539f..c084ee7f 100644 --- a/python/tests/ci_macos_asan_test.py +++ b/python/tests/ci_macos_asan_test.py @@ -54,12 +54,22 @@ def _calls(call_names: tuple[str, ...], module_bazel: str) -> list[ast.Call]: def _argument(call: ast.Call, keyword: str, position: int) -> object | None: - """The argument given as keyword or at position, or None if absent.""" + """The argument given as keyword or at position, or None if absent. + + **kwargs (a keyword whose arg is None) may supply a missing argument + without naming it, so it resolves to _UNRESOLVED rather than to absent. + A *args at the requested position is an ast.Starred node, which + _literal() already resolves to _UNRESOLVED. (A *args before it could + shift later positions, but callers only read past position 0 once + position 0 has resolved to a literal.) + """ for kw in call.keywords: if kw.arg == keyword: return _literal(kw.value) if position < len(call.args): return _literal(call.args[position]) + if any(kw.arg is None for kw in call.keywords): + return _UNRESOLVED return None @@ -306,6 +316,32 @@ def test_has_apple_support_override_fails_closed_on_non_literal_module_name( with self.subTest(module_name=module_name): self.assertTrue(_has_apple_support_override(sample)) + def test_has_apple_support_override_fails_closed_on_keyword_unpacking( + self, + ) -> None: + """**kwargs may supply module_name, whose value can't be read back.""" + for unpacked in ( + '**{"module_name": "apple_support", "path": "../apple_support"}', + "**override_kwargs", + ): + sample = f"local_path_override({unpacked})" + with self.subTest(unpacked=unpacked): + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_fails_closed_on_positional_unpacking( + self, + ) -> None: + """*args may supply module_name positionally.""" + sample = 'local_path_override(*["apple_support"], path = "../x")' + self.assertTrue(_has_apple_support_override(sample)) + + def test_has_apple_support_override_ignores_unpacking_after_positional_module_name( + self, + ) -> None: + """module_name given positionally can't also come from **kwargs.""" + sample = 'local_path_override("rules_cc", **{"path": "../x"})' + self.assertFalse(_has_apple_support_override(sample)) + def test_has_apple_support_override_ignores_positional_string_in_list( self, ) -> None: