From 94c544d3faabaf496b6fe58afd6f5cad735b22b7 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 15:34:36 +0100 Subject: [PATCH 01/25] Bump toolchains_llvm to 1.11.2, dropping the #791 archive_override. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit toolchains_llvm 1.8.0 emitted an unused -stdlib=libc++ under -nostdinc++, breaking this project's Linux -Werror builds; the project carried an archive_override pinned to the unreleased merge commit of upstream PR #791 to work around it. That fix shipped in the 1.9.0 release (verified by diffing cc_toolchain_config.bzl at that tag against the pinned commit — identical), so pin to 1.11.2 directly and drop the override. Confirmed llvm_versions = "21.1.8" is still a catalogued archive at 1.11.2 (toolchain/distributions/github.jsonc), and that the extra_link_flags / extra_link_libs mechanism this project relies on for the macOS 27 SDK / Linux native-linker overrides is unchanged (cc_toolchain_config.bzl still appends them after the toolchain-default --ld-path, so "last wins" still holds). 1.11.0's new linker = {...} catalogue attribute is not adopted here -- it's a more idiomatic way to express the same override going forward, but switching to it is a separate, deliberate migration, not a side effect of this version bump. ci_msan_test's guard for the override is replaced with one that checks the bazel_dep version and that no archive/single_version/git_override targets toolchains_llvm, using the same full-call-body matching approach as the apple_support guard (see PR #421) instead of a "next line" regex. Verified on this macOS 27 / Xcode 27 host, which is exactly the scenario the original fix targets: - bazelisk test //... : 104/104 pass - bazelisk test --config=asan //library/tests/system/... : 22/22 pass - bazelisk test --config=tsan and --config=ubsan //library/tests/system:worker_context_reuse_test: pass - msan is Linux-only and can't run on this host; confirmed the Bazel config wiring test (ci_msan_test) passes, and that an unrelated --nobuild analysis probe of its toolchain fails identically before and after this bump (pre-existing macOS-host limitation, not a regression). Co-Authored-By: Claude Sonnet 5 --- MODULE.bazel | 17 +------- MODULE.bazel.lock | 10 +++-- docs/BUILD_SYSTEM.md | 5 --- python/tests/ci_msan_test.py | 77 ++++++++++++++++++++++++++++++------ 4 files changed, 72 insertions(+), 37 deletions(-) diff --git a/MODULE.bazel b/MODULE.bazel index 58c4496b..801ab14e 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -24,9 +24,7 @@ bazel_dep(name = "googletest", version = "1.17.0.bcr.2") bazel_dep(name = "pybind11_bazel", version = "3.0.1") bazel_dep(name = "rules_python", version = "2.0.1") # 1.8.0+ adds --features=msan and libcxx_url for an instrumented libc++ overlay. -# BCR 1.8.0 still emits unused `-stdlib=libc++` under `-nostdinc++`, which fails -# our Linux `-Werror` builds. Override to the #791 fix until BCR publishes >1.8.0. -bazel_dep(name = "toolchains_llvm", version = "1.8.0") +bazel_dep(name = "toolchains_llvm", version = "1.11.2") # Xcode CC toolchain for macOS AddressSanitizer builds (--config=asan). LLVM's ASAN # runtime hangs on current macOS; see .bazelrc build:asan_macos. @@ -70,19 +68,6 @@ archive_override( ], ) -# toolchains_llvm#791: drop redundant `-stdlib=libc++` (unused with `-nostdinc++`). -# Commit is the merge of that PR; drop this override when BCR ships a release -# that includes it. -archive_override( - module_name = "toolchains_llvm", - integrity = "sha256-spaFpVBa8JGs27s/9vk0Xb4gd8fJ9OFmnxaBYPeOt4o=", - strip_prefix = "toolchains_llvm-c3ac93f5c61cb78487765d2e81e7485ad3f8bf2c", - urls = [ - "https://github.com/bazel-contrib/toolchains_llvm/archive/c3ac93f5c61cb78487765d2e81e7485ad3f8bf2c.tar.gz", - "https://codeload.github.com/bazel-contrib/toolchains_llvm/tar.gz/c3ac93f5c61cb78487765d2e81e7485ad3f8bf2c", - ], -) - llvm = use_extension("@toolchains_llvm//toolchain/extensions:llvm.bzl", "llvm") llvm.toolchain( name = "llvm_toolchain", diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 5b18a4b7..87b96364 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -93,14 +93,14 @@ "https://bcr.bazel.build/modules/googletest/1.17.0.bcr.2/MODULE.bazel": "827f54f492a3ce549c940106d73de332c2b30cebd0c20c0bc5d786aba7f116cb", "https://bcr.bazel.build/modules/googletest/1.17.0.bcr.2/source.json": "3664514073a819992320ffbce5825e4238459df344d8b01748af2208f8d2e1eb", "https://bcr.bazel.build/modules/googletest/1.17.0/MODULE.bazel": "dbec758171594a705933a29fcf69293d2468c49ec1f2ebca65c36f504d72df46", - "https://bcr.bazel.build/modules/helly25_bzl/0.4.3/MODULE.bazel": "9c20052fd3f1fb767c48b78c1bbc46f501a4ca69d14558429d1234e68e449f68", - "https://bcr.bazel.build/modules/helly25_bzl/0.4.3/source.json": "e8c54d81e72633fb6f1d23c7e2d44e0b39b1caef2a256141136a2f63e6204b78", "https://bcr.bazel.build/modules/jq.bzl/0.1.0/MODULE.bazel": "2ce69b1af49952cd4121a9c3055faa679e748ce774c7f1fda9657f936cae902f", "https://bcr.bazel.build/modules/jq.bzl/0.1.0/source.json": "746bf13cac0860f091df5e4911d0c593971cd8796b5ad4e809b2f8e133eee3d5", "https://bcr.bazel.build/modules/jsoncpp/1.9.5/MODULE.bazel": "31271aedc59e815656f5736f282bb7509a97c7ecb43e927ac1a37966e0578075", "https://bcr.bazel.build/modules/jsoncpp/1.9.6/MODULE.bazel": "2f8d20d3b7d54143213c4dfc3d98225c42de7d666011528dc8fe91591e2e17b0", "https://bcr.bazel.build/modules/jsoncpp/1.9.6/source.json": "a04756d367a2126c3541682864ecec52f92cdee80a35735a3cb249ce015ca000", "https://bcr.bazel.build/modules/libpfm/4.11.0/MODULE.bazel": "45061ff025b301940f1e30d2c16bea596c25b176c8b6b3087e92615adbd52902", + "https://bcr.bazel.build/modules/mboworks_bzl/0.5.1/MODULE.bazel": "20c921aded4f4e00a6128dad47a843371c196ba8987474c11c8ba7b9da8c16f1", + "https://bcr.bazel.build/modules/mboworks_bzl/0.5.1/source.json": "3d9086bb203475113a16afb0ca0c9b97fdb37fc6e5a62b604b60ffc2450dc8c5", "https://bcr.bazel.build/modules/nlohmann_json/3.6.1/MODULE.bazel": "6f7b417dcc794d9add9e556673ad25cb3ba835224290f4f848f8e2db1e1fca74", "https://bcr.bazel.build/modules/nlohmann_json/3.6.1/source.json": "f448c6e8963fdfa7eb831457df83ad63d3d6355018f6574fb017e8169deb43a9", "https://bcr.bazel.build/modules/package_metadata/0.0.2/MODULE.bazel": "fb8d25550742674d63d7b250063d4580ca530499f045d70748b1b142081ebb92", @@ -268,6 +268,8 @@ "https://bcr.bazel.build/modules/tar.bzl/0.6.0/source.json": "4a620381df075a16cb3a7ed57bd1d05f7480222394c64a20fa51bdb636fda658", "https://bcr.bazel.build/modules/toml.bzl/0.3.0/MODULE.bazel": "5016e5dd1ad2200e119a4b28b2b3935e276c4b480f2fe3e952bea7eeba88f578", "https://bcr.bazel.build/modules/toml.bzl/0.3.0/source.json": "0cf7c878c419b37ddb55f3dd93dd7c0c409bd7c4efacb3da504e0748780b2fa9", + "https://bcr.bazel.build/modules/toolchains_llvm/1.11.2/MODULE.bazel": "9decd692dbc7e38000b498aca117d71529816331564e3b548dc9b1dced2bc28a", + "https://bcr.bazel.build/modules/toolchains_llvm/1.11.2/source.json": "fe4d32e7f6dfb1f537917627f49da7b93d72dbacdc7a01652243dbe90942ad2e", "https://bcr.bazel.build/modules/upb/0.0.0-20220923-a547704/MODULE.bazel": "7298990c00040a0e2f121f6c32544bab27d4452f80d9ce51349b1a28f3005c43", "https://bcr.bazel.build/modules/upb/0.0.0-20230516-61a97ef/MODULE.bazel": "c0df5e35ad55e264160417fd0875932ee3c9dda63d9fccace35ac62f45e1b6f9", "https://bcr.bazel.build/modules/yq.bzl/0.1.1/MODULE.bazel": "9039681f9bcb8958ee2c87ffc74bdafba9f4369096a2b5634b88abc0eaefa072", @@ -580,8 +582,8 @@ }, "@@toolchains_llvm+//toolchain/extensions:distributions.bzl%llvm_distributions": { "general": { - "bzlTransitiveDigest": "gCdXpBt3HBBc280OILOFheHmO7lXWXJYnPgYiTtyapI=", - "usagesDigest": "3B/1Qf38oum/lRpBvi3mnJwV6XMAQF+4//DH0B29rNA=", + "bzlTransitiveDigest": "RfYsFLwRh0rVY8ReiYgbOEyr84zuBs7mbcHq3dPY3jI=", + "usagesDigest": "O4NdxV580ttZjpnUuilLkOBjAswi7Jp5RxsiIZpKaio=", "recordedInputs": [], "generatedRepoSpecs": { "llvm_distributions_data": { diff --git a/docs/BUILD_SYSTEM.md b/docs/BUILD_SYSTEM.md index 93f063b2..907d667b 100644 --- a/docs/BUILD_SYSTEM.md +++ b/docs/BUILD_SYSTEM.md @@ -220,11 +220,6 @@ libc++ overlay (`libcxx_url` / `libcxx_sha256` in `MODULE.bazel`) built against the same Ubuntu 22.04 LLVM release as `MSAN_LLVM_VERSION`. Keep those pins aligned when bumping LLVM. Do not combine MSAN with ASAN/TSAN/UBSAN. -`MODULE.bazel` currently `archive_override`s `toolchains_llvm` past BCR 1.8.0 -to include [#791](https://github.com/bazel-contrib/toolchains_llvm/pull/791) -(drops unused `-stdlib=libc++` that breaks Linux `-Werror` builds). Drop the -override when BCR publishes a release that includes that fix. - **When upgrading LLVM or Xcode**, update all coupled paths together: 1. Bump `llvm_versions` in `MODULE.bazel` (and run diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 4ce8f2fe..a4a690df 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -20,6 +20,32 @@ def _repo_root(start: Path | None = None) -> Path: raise AssertionError("could not locate repository root from test file path") +def _toolchains_llvm_bazel_dep_version(module_bazel: str) -> str: + match = re.search( + r'bazel_dep\(\s*name\s*=\s*"toolchains_llvm"\s*,\s*version\s*=\s*"([^"]+)"', + module_bazel, + ) + if match is None: + raise AssertionError('expected bazel_dep(... name = "toolchains_llvm" ...)') + return match.group(1) + + +def _has_toolchains_llvm_override(module_bazel: str) -> bool: + """True if any override pins toolchains_llvm to a specific commit/archive. + + 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 "(". + """ + for call in re.finditer( + r"(?:archive_override|single_version_override|git_override)\(\s*([^)]*)\)", + module_bazel, + ): + if re.search(r'module_name\s*=\s*"toolchains_llvm"', call.group(1)): + return True + return False + + class TestMsanBazelConfig(unittest.TestCase): def test_bazelrc_defines_msan_config(self) -> None: bazelrc = (_repo_root() / ".bazelrc").read_text(encoding="utf-8") @@ -109,23 +135,50 @@ def test_module_defines_msan_toolchain_with_instrumented_libcxx(self) -> None: ) def test_module_pins_toolchains_llvm_past_unused_stdlib_fix(self) -> None: - """BCR 1.8.0 emits unused -stdlib=libc++; -Werror breaks Linux builds. + """BCR 1.8.0 emitted unused -stdlib=libc++, breaking Linux -Werror builds. - toolchains_llvm #791 drops the redundant flag. Until BCR ships a release - that includes it, MODULE.bazel must archive_override past that commit. + toolchains_llvm#791 dropped the redundant flag; it shipped in the 1.9.0 + release, so a plain bazel_dep replaces the archive_override this + project used to carry to pin a specific pre-release commit. """ module = (_repo_root() / "MODULE.bazel").read_text(encoding="utf-8") - self.assertRegex( - module, - r'archive_override\(\s*\n\s*module_name\s*=\s*"toolchains_llvm"', - "expected archive_override for toolchains_llvm until BCR > 1.8.0", + version = tuple( + int(p) for p in _toolchains_llvm_bazel_dep_version(module).split(".") ) - # Merge commit of bazel-contrib/toolchains_llvm#791. - self.assertIn( - "c3ac93f5c61cb78487765d2e81e7485ad3f8bf2c", - module, - "toolchains_llvm override must include the unused -stdlib=libc++ fix", + self.assertGreaterEqual( + version, + (1, 9, 0), + "toolchains_llvm must stay >= 1.9.0 (drops unused -stdlib=libc++, #791)", ) + self.assertFalse( + _has_toolchains_llvm_override(module), + "toolchains_llvm no longer needs an archive_override pin", + ) + + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: + sample = ( + 'archive_override(module_name = "toolchains_llvm", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_detects_reordered_args(self) -> None: + sample = """ +archive_override( + strip_prefix = "toolchains_llvm-abc123", + module_name = "toolchains_llvm", +) +""" + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: + sample = """ +single_version_override( + module_name = "apple_support", + version = "2.10.1", +) +""" + self.assertFalse(_has_toolchains_llvm_override(sample)) class TestMsanLinuxCi(unittest.TestCase): From 0c9ec92c67e2b7deab985d53be3bd449c2334402 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 16:09:59 +0100 Subject: [PATCH 02/25] Cover local_path_override and multiple_version_override in the toolchains_llvm guard. _has_toolchains_llvm_override()'s alternation only matched archive_override, single_version_override, and git_override -- Bazel's other two module override directives, local_path_override and multiple_version_override, silently returned False, so either could reintroduce an override without the guard noticing. Added both to the alternation and added a test for each; confirmed both new tests fail against the pre-fix regex (verified with a standalone repro) before adding the fix. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index a4a690df..17866cfe 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -38,7 +38,8 @@ def _has_toolchains_llvm_override(module_bazel: str) -> bool: after the opening "(". """ for call in re.finditer( - r"(?:archive_override|single_version_override|git_override)\(\s*([^)]*)\)", + 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*"toolchains_llvm"', call.group(1)): @@ -180,6 +181,26 @@ def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_local_path_override(self) -> None: + sample = """ +local_path_override( + module_name = "toolchains_llvm", + path = "../toolchains_llvm", +) +""" + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_detects_multiple_version_override( + self, + ) -> None: + sample = """ +multiple_version_override( + module_name = "toolchains_llvm", + versions = ["1.9.0", "1.11.2"], +) +""" + self.assertTrue(_has_toolchains_llvm_override(sample)) + class TestMsanLinuxCi(unittest.TestCase): def test_ci_linux_runs_msan_job(self) -> None: From ccf2a1f3ae2687f93a2dd153173e032f25ce02a8 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 16:40:59 +0100 Subject: [PATCH 03/25] Parse bazel_dep's toolchains_llvm version independently of argument order. _toolchains_llvm_bazel_dep_version() only matched "name" immediately followed by "version", so reordered or multiline bazel_dep(...) arguments would fail the guard even with the dependency correctly pinned. Matches the existing _rules_cc_bazel_dep_version() approach in ci_windows_cppopts_test.py: parse the whole call body and find both fields independently. Confirmed the new reordered-args test fails against the old pattern (standalone repro) before the fix. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 17866cfe..413c2948 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -21,13 +21,13 @@ def _repo_root(start: Path | None = None) -> Path: def _toolchains_llvm_bazel_dep_version(module_bazel: str) -> str: - match = re.search( - r'bazel_dep\(\s*name\s*=\s*"toolchains_llvm"\s*,\s*version\s*=\s*"([^"]+)"', - module_bazel, - ) - if match is None: - raise AssertionError('expected bazel_dep(... name = "toolchains_llvm" ...)') - return match.group(1) + for match in re.finditer(r"bazel_dep\(\s*([^)]*)\)", module_bazel): + body = match.group(1) + name = re.search(r'name\s*=\s*"toolchains_llvm"', body) + version = re.search(r'version\s*=\s*"([^"]+)"', body) + if name and version: + return version.group(1) + raise AssertionError('expected bazel_dep(... name = "toolchains_llvm" ...)') def _has_toolchains_llvm_override(module_bazel: str) -> bool: @@ -156,6 +156,12 @@ def test_module_pins_toolchains_llvm_past_unused_stdlib_fix(self) -> None: "toolchains_llvm no longer needs an archive_override pin", ) + def test_toolchains_llvm_bazel_dep_version_tolerates_reordered_args( + self, + ) -> None: + sample = 'bazel_dep(version = "1.11.2", name = "toolchains_llvm")' + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' From 1a817e87507d779e96fee58f235399280f86472b Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 17:17:26 +0100 Subject: [PATCH 04/25] Use a balanced-paren/string/comment-aware scanner for call bodies. The [^)]*)\) regex stops at the first ")" even when it appears inside a string literal, a "#" comment, or a nested call -- a leading comment like "# see upstream fix (closes #791)" before the real args would truncate the match and make _toolchains_llvm_bazel_dep_version()/ _has_toolchains_llvm_override() fail or return False despite the file being correct. _iter_call_bodies() replaces both regexes with a character scanner that tracks string/comment state and paren depth, so the body always spans to its true matching ")". 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_msan_test.py | 78 ++++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 8 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 413c2948..3d81dec0 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -20,9 +20,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 _toolchains_llvm_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*"toolchains_llvm"', body) version = re.search(r'version\s*=\s*"([^"]+)"', body) if name and version: @@ -37,12 +76,15 @@ def _has_toolchains_llvm_override(module_bazel: str) -> bool: order or formatting, rather than assuming module_name is the first line after the opening "(". """ - 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*"toolchains_llvm"', 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*"toolchains_llvm"', body): return True return False @@ -162,6 +204,17 @@ def test_toolchains_llvm_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "1.11.2", name = "toolchains_llvm")' self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_survives_an_earlier_paren( + self, + ) -> None: + """A ")" inside a leading comment must not truncate the call body.""" + sample = """bazel_dep( + # see upstream fix (closes #791) + version = "1.11.2", + name = "toolchains_llvm", +)""" + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' @@ -178,6 +231,15 @@ def test_has_toolchains_llvm_override_detects_reordered_args(self) -> None: """ self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_survives_an_earlier_paren(self) -> None: + """A ")" inside a leading comment must not truncate the call body.""" + sample = """archive_override( + # pin past #791 (unreleased fix) + module_name = "toolchains_llvm", + strip_prefix = "toolchains_llvm-abc123", +)""" + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: sample = """ single_version_override( From 58d3db74e5ec96de35a0cdf8e057e5885a61d4e9 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 17:37:22 +0100 Subject: [PATCH 05/25] Ignore commented-out MODULE.bazel calls in the call-body scanner. _iter_call_bodies() located each call with a plain regex search for "name(", which matches that text inside a "#" comment just as readily as live code -- 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_toolchains_llvm_override return a false positive. Rewritten as a single linear scan that tracks string/comment state continuously, so a call name is only recognized outside both. 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_msan_test.py | 101 +++++++++++++++++++++++++++-------- 1 file changed, 78 insertions(+), 23 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 3d81dec0..d2308225 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -21,39 +21,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. - - 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 ")". + """Yield the argument-list text of each top-level call to call_names. + + 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]) @@ -215,6 +253,14 @@ def test_toolchains_llvm_bazel_dep_version_survives_an_earlier_paren( )""" self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_ignores_commented_out_call( + self, + ) -> None: + sample = """# bazel_dep(name = "toolchains_llvm", version = "9.9.9") +bazel_dep(name = "toolchains_llvm", version = "1.11.2") +""" + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' @@ -240,6 +286,15 @@ def test_has_toolchains_llvm_override_survives_an_earlier_paren(self) -> None: )""" self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_commented_out_call( + self, + ) -> None: + sample = ( + '# archive_override(module_name = "toolchains_llvm", ' + 'strip_prefix = "x")\n' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: sample = """ single_version_override( From 8e10887f344b2cee92b0db427449ee5dac71c39d Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 18:14:29 +0100 Subject: [PATCH 06/25] Tolerate whitespace before "(" and mask comments out of call bodies. Mirrors the fix just applied to the identical apple_support guard in #424, confirmed here with the same standalone adversarial-input repros before fixing: - "archive_override (module_name = ...)" (space before the parenthesis, valid Starlark) matched zero calls -- a false negative. - The returned body text kept comment characters verbatim, so a commented-out "module_name = \"toolchains_llvm\"" inside a different module's override made it look like a toolchains_llvm override, and a commented-out stale version before the real one could win the version-guard's regex search. 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 kept verbatim). Added a regression test for each. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 101 ++++++++++++++++++++++++++++------- 1 file changed, 81 insertions(+), 20 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index d2308225..9d1eaef8 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -27,10 +27,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] = [] @@ -61,40 +65,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 @@ -261,6 +288,17 @@ def test_toolchains_llvm_bazel_dep_version_ignores_commented_out_call( """ self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_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 = "toolchains_llvm", + # version = "9.9.9" (stale) + version = "1.11.2", +)""" + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' @@ -304,6 +342,29 @@ def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_commented_module_name( + self, + ) -> None: + """A comment mentioning toolchains_llvm in an apple_support override + + is not itself a toolchains_llvm override. + """ + sample = """single_version_override( + # module_name = "toolchains_llvm" (old pin, no longer used) + module_name = "apple_support", + version = "2.10.1", +)""" + self.assertFalse(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_tolerates_whitespace_before_paren( + self, + ) -> None: + sample = ( + 'archive_override (module_name = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_local_path_override(self) -> None: sample = """ local_path_override( From d10df56d5ed425c822d4fe7c587c1e32eb18f865 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 18:21:03 +0100 Subject: [PATCH 07/25] Parse real keyword=value tokens instead of regex-searching masked body text. Masking "#" comments out of the returned body (the previous fix) still left a gap the review correctly identified: 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 = "toolchains_llvm", version = "9.9.9"'] would be misread as the toolchains_llvm dependency, and the same shape of string value fools the override check. Confirmed both with a standalone 13-case repro script before rewriting (both misfired; all 13 cases, including every prior regression case, pass with the fix). _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 -- so text inside a string literal can never be mistaken for a real top-level keyword argument, regardless of what it contains. _toolchains_llvm_bazel_dep_version() and _has_toolchains_llvm_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_msan_test.py | 151 ++++++++++++++++++++++++----------- 1 file changed, 103 insertions(+), 48 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 9d1eaef8..bea3ebec 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -20,24 +20,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 = "toolchains_llvm", ...']` — a + bazel_dep for a different module would be misread as the toolchains_llvm + 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 @@ -69,9 +83,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 @@ -81,65 +93,79 @@ 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 _toolchains_llvm_bazel_dep_version(module_bazel: str) -> str: - for body in _iter_call_bodies(("bazel_dep",), module_bazel): - name = re.search(r'name\s*=\s*"toolchains_llvm"', 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") == "toolchains_llvm" and "version" in kwargs: + return kwargs["version"] raise AssertionError('expected bazel_dep(... name = "toolchains_llvm" ...)') def _has_toolchains_llvm_override(module_bazel: str) -> bool: """True if any override pins toolchains_llvm to a specific commit/archive. - 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 "(". + Matches each override 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", @@ -148,8 +174,8 @@ def _has_toolchains_llvm_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*"toolchains_llvm"', body): + for kwargs in _iter_call_kwargs(override_names, module_bazel): + if kwargs.get("module_name") == "toolchains_llvm": return True return False @@ -299,6 +325,21 @@ def test_toolchains_llvm_bazel_dep_version_ignores_commented_out_version( )""" self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_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 = "toolchains_llvm", version = "9.9.9"'], +)""" + with self.assertRaises(AssertionError): + _toolchains_llvm_bazel_dep_version(sample) + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' @@ -356,6 +397,20 @@ def test_has_toolchains_llvm_override_ignores_commented_module_name( )""" self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "apple_support", + version = "2.10.1", + patch_cmds = ['the module_name = "toolchains_llvm" override is legacy'], +)""" + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_tolerates_whitespace_before_paren( self, ) -> None: From 252d8c15f4a5332b42111a5d82468db1ad2b3760 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 19:10:18 +0100 Subject: [PATCH 08/25] Handle triple-quoted strings and identifier boundaries correctly. Two gaps confirmed with standalone repros against the previous token-parsing rewrite before fixing: - The scanner toggled string state per single quote character, so a triple-quoted string wasn't kept opaque as one unit. It happened to survive simple cases by accident (an even number of embedded quotes before a fake call keeps the toggle parity aligned), but 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, letting a stale fake version win. - The call-name/keyword matcher used a bare substring search with no identifier-boundary check, so "legacy_bazel_dep(...)", "bazel_depfoo(...)", and even a qualified "extensions.bazel_dep(...)" were all misread as a real top-level bazel_dep call. _skip_string()/_skip_comment() replace the per-character in_string/ in_comment toggle with single jumps that treat a triple-quoted delimiter as one atomic unit (never scanning its content, including embedded quotes, as code) 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_msan_test.py | 181 ++++++++++++++++++++++------------- 1 file changed, 116 insertions(+), 65 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index bea3ebec..f8dad226 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -29,22 +29,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 = "toolchains_llvm", ...']` — a bazel_dep for a different module would be misread as the toolchains_llvm 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 @@ -54,40 +113,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 @@ -96,26 +139,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 @@ -125,7 +153,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) @@ -133,18 +161,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 @@ -340,6 +360,37 @@ def test_toolchains_llvm_bazel_dep_version_ignores_fake_assignment_in_string( with self.assertRaises(AssertionError): _toolchains_llvm_bazel_dep_version(sample) + def test_toolchains_llvm_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 = "toolchains_llvm", version = "9.9.9") +""" +bazel_dep(name = "toolchains_llvm", version = "1.11.2") +''' + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + + def test_toolchains_llvm_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 = "toolchains_llvm", version = "9.9.9")', + 'bazel_depfoo(name = "toolchains_llvm", version = "9.9.9")', + 'extensions.bazel_dep(name = "toolchains_llvm", version = "9.9.9")', + ): + with self.assertRaises(AssertionError): + _toolchains_llvm_bazel_dep_version(sample) + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' From a319902a9e10f421f2348fc8b9567f079c477472 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 19:43:34 +0100 Subject: [PATCH 09/25] Capture positional module_name/name arguments, not just keyword form. Mirrors the fix just applied to the identical apple_support guard in #424, confirmed here with the same standalone repro before fixing: Bazel's *_override APIs and bazel_dep all accept their first argument (module_name / name) positionally, e.g. archive_override("toolchains_llvm", urls = [...]), which _iter_call_kwargs() -- only capturing "identifier = value" pairs -- missed entirely. A bare top-level string 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 = ["toolchains_llvm"]) is correctly never mistaken for the call's own positional argument. 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_msan_test.py | 86 +++++++++++++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 6 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index f8dad226..994560d1 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -21,6 +21,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: @@ -108,6 +109,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]] = [] @@ -136,13 +145,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 == "(": @@ -153,7 +171,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) @@ -175,7 +209,8 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, def _toolchains_llvm_bazel_dep_version(module_bazel: str) -> str: for kwargs in _iter_call_kwargs(("bazel_dep",), module_bazel): - if kwargs.get("name") == "toolchains_llvm" and "version" in kwargs: + name = kwargs.get("name") or kwargs.get(_POSITIONAL_KEY) + if name == "toolchains_llvm" and "version" in kwargs: return kwargs["version"] raise AssertionError('expected bazel_dep(... name = "toolchains_llvm" ...)') @@ -183,9 +218,9 @@ def _toolchains_llvm_bazel_dep_version(module_bazel: str) -> str: def _has_toolchains_llvm_override(module_bazel: str) -> bool: """True if any override pins toolchains_llvm to a specific commit/archive. - Matches each override call's keyword args regardless of argument order - or formatting, rather than assuming module_name is the first line after - the opening "(". + Matches each override 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 "(". """ override_names = ( "archive_override", @@ -195,7 +230,8 @@ def _has_toolchains_llvm_override(module_bazel: str) -> bool: "single_version_override", ) for kwargs in _iter_call_kwargs(override_names, module_bazel): - if kwargs.get("module_name") == "toolchains_llvm": + module_name = kwargs.get("module_name") or kwargs.get(_POSITIONAL_KEY) + if module_name == "toolchains_llvm": return True return False @@ -315,6 +351,16 @@ def test_toolchains_llvm_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "1.11.2", name = "toolchains_llvm")' self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_accepts_positional_name( + self, + ) -> None: + """bazel_dep's name is positional too: bazel_dep("toolchains_llvm", + + version = ...) is valid Starlark. + """ + sample = 'bazel_dep("toolchains_llvm", version = "1.11.2")' + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_survives_an_earlier_paren( self, ) -> None: @@ -398,6 +444,34 @@ def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_positional_module_name( + self, + ) -> None: + """The override APIs accept module_name as their first positional + + argument; archive_override("toolchains_llvm", ...) is as real an + override as the keyword form. + """ + sample = ( + 'archive_override("toolchains_llvm", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "apple_support", + urls = ["toolchains_llvm"], +)""" + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_reordered_args(self) -> None: sample = """ archive_override( From 3de290bff9c803dcbeebd2e72fadbf169fe6052d Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 20:06:06 +0100 Subject: [PATCH 10/25] Extract triple-quoted string values instead of discarding them. Mirrors the fix just applied to the identical apple_support guard in #424, confirmed here with the same standalone repro before fixing: _skip_string() unconditionally returned None for triple-quoted literals, so a valid archive_override(module_name = \"\"\"toolchains_llvm\"\"\", ...) silently produced no module_name. _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 earlier opacity fix is unaffected. Removed the now- dead "if value is not None" checks in _iter_call_kwargs(). Added a dedicated regression test for the triple-quoted-module-name case. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 47 ++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 994560d1..ab471425 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -36,30 +36,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]) @@ -157,8 +163,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) @@ -196,8 +201,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() @@ -458,6 +462,19 @@ def test_has_toolchains_llvm_override_detects_positional_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = ( + 'archive_override(module_name = """toolchains_llvm""", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_positional_string_in_list( self, ) -> None: From 35dd67bd844d34922caae5c91de7e01db43ff4c6 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sat, 10 Oct 2026 21:28:01 +0100 Subject: [PATCH 11/25] Decode Starlark backslash escapes instead of stripping the backslash. Mirrors the fix just applied to the identical apple_support guard in #424, confirmed here with the same standalone repro before fixing: "\x61pple_support" (a valid Starlark hex escape for "apple_support") decoded to the wrong string "x61pple_support", letting an override written with an escaped module_name bypass the guard entirely. _decode_escape() handles Starlark's simple single-character escapes, up to 3 octal digits, and exactly two hex digits after \x, used by both branches of _skip_string(). Caught and fixed a stale "j += 2" left over from the pre-fix code in the ordinary-string branch while porting this -- it would have double-advanced past what _decode_escape() already consumed, corrupting any string containing an escape. Added a dedicated regression test for the hex-escape case. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 73 +++++++++++++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 6 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index ab471425..33255452 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -36,6 +36,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]. @@ -46,8 +94,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: @@ -56,8 +104,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) @@ -68,8 +116,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) @@ -475,6 +523,19 @@ def test_has_toolchains_llvm_override_detects_triple_quoted_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_decodes_hex_escape_in_module_name( + self, + ) -> None: + """\\x74 is a valid Starlark hex escape for "t"; the decoded value + + must still be compared, not the raw escape text. + """ + sample = ( + 'archive_override(module_name = "\\x74oolchains_llvm", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_positional_string_in_list( self, ) -> None: From 5bcd5843c01944562b64eddda1ec21a059039927 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 00:04:06 +0100 Subject: [PATCH 12/25] Decode Starlark \u/\U Unicode escapes in module_name values. Mirrors the same fix on #424: _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 "toolchains_llvm" therefore decoded to the wrong string and bypassed _has_toolchains_llvm_override(). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 48 +++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 33255452..4d0fe0fd 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -57,12 +57,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, "\\" @@ -74,6 +77,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 = "" @@ -536,6 +545,33 @@ def test_has_toolchains_llvm_override_decodes_hex_escape_in_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_decodes_unicode_escape_in_module_name( + self, + ) -> None: + """\\u0074 is a valid Starlark 4-hex-digit Unicode escape for "t"; + + the decoded value must still be compared, not the raw escape text. + """ + sample = ( + 'archive_override(module_name = "\\u0074oolchains_llvm", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_decodes_big_unicode_escape_in_module_name( + self, + ) -> None: + """\\U00000074 is a valid Starlark 8-hex-digit Unicode escape for + + "t"; the decoded value must still be compared, not the raw escape + text. + """ + sample = ( + 'archive_override(module_name = "\\U00000074oolchains_llvm", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_positional_string_in_list( self, ) -> None: From f28d7b95b04c263a8b740fad7c3a9761b74c2f1b Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 11:47:42 +0100 Subject: [PATCH 13/25] Fail closed on a non-literal module_name in override detection. Mirrors the same fix on #424: _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_toolchains_llvm_override() read it the same as "not toolchains_llvm" 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_msan_test.py | 99 +++++++++++++++++++++++++++++++++--- 1 file changed, 92 insertions(+), 7 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 4d0fe0fd..91f5b5d2 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -22,6 +22,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: @@ -135,6 +142,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. @@ -220,7 +239,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) @@ -255,12 +276,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 @@ -281,7 +323,13 @@ def _has_toolchains_llvm_override(module_bazel: str) -> bool: Matches each override 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 "(". + 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 toolchains_llvm" + would let a real override on a computed target slip past this guard + undetected. """ override_names = ( "archive_override", @@ -292,7 +340,7 @@ def _has_toolchains_llvm_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 == "toolchains_llvm": + if module_name == "toolchains_llvm" or module_name is _UNRESOLVED: return True return False @@ -572,6 +620,43 @@ def test_has_toolchains_llvm_override_decodes_big_unicode_escape_in_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 toolchains_llvm override) rather than silently treating + the override as targeting some other module. + """ + sample = 'archive_override(module_name = target, strip_prefix = "x")' + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 + ("toolchains_") would be captured by a naive literal-only parser, + which must not be mistaken for the whole, real value. + """ + sample = ( + 'archive_override(module_name = "toolchains_" + "llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_positional_string_in_list( self, ) -> None: From 2490fd958f9b88945a12c58bbcaa733f742b3083 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 21:34:43 +0100 Subject: [PATCH 14/25] 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 Mirrors the same fix on #424: _followed_by_concat() only recognized a directly adjacent "+", so module_name = "xtoolchains_llvm"[1:] (slicing), "toolchains_%s" % "llvm" (percent formatting), or a concatenation whose "+" follows an inline comment all still read as the bare literal fragment and could bypass _has_toolchains_llvm_override() despite resolving to toolchains_llvm. 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_msan_test.py | 80 ++++++++++++++++++++++++++++++------ 1 file changed, 67 insertions(+), 13 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 91f5b5d2..a93a18f5 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -142,16 +142,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: @@ -240,7 +252,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 @@ -280,9 +292,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 @@ -657,6 +669,48 @@ def test_has_toolchains_llvm_override_fails_closed_on_concatenated_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_" # fixes a typo below + + "llvm", + strip_prefix = "x", +)""" + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "xtoolchains_llvm"[1:], ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_fails_closed_on_percent_formatted_module_name( + self, + ) -> None: + """A "%"-formatted literal is a template, not the resolved value; + + "toolchains_%s" % "llvm" resolves to "toolchains_llvm" but the + literal "toolchains_%s" alone must not be mistaken for it. + """ + sample = ( + 'archive_override(module_name = "toolchains_%s" % "llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_positional_string_in_list( self, ) -> None: From c2cff83a49e4e22180f3c425710e5a1137763d84 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Sun, 11 Oct 2026 21:37:30 +0100 Subject: [PATCH 15/25] Fail closed on a parenthesized positional module_name expression. A top-level "(" that isn't preceded by "ident =" opens a parenthesized positional expression (e.g. the first argument written as ("toolchains_" + "llvm")) rather than a literal or a bare variable reference. _iter_call_kwargs() descended straight into it without ever recording _POSITIONAL_KEY, so _has_toolchains_llvm_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_msan_test.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index a93a18f5..1b14c7b3 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -259,6 +259,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 @@ -654,6 +662,21 @@ def test_has_toolchains_llvm_override_fails_closed_on_positional_variable_module sample = 'archive_override(target, strip_prefix = "x")' self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_fails_closed_on_parenthesized_positional_module_name( + self, + ) -> None: + """A parenthesized expression passed as the first positional + + argument (e.g. ("toolchains_" + "llvm")) 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(("toolchains_" + "llvm"), strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_fails_closed_on_concatenated_module_name( self, ) -> None: From 425e2fdb39a824d0ecc09d75a7771baac019c58c Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 19:16:10 +0100 Subject: [PATCH 16/25] Fail closed on list/dict positional module_name expressions. Same gap as the parenthesized-positional fix, for the "[" and "{" branches: a top-level list or dict expression opening the first positional argument (e.g. ["toolchains_llvm"][0]) was descended into without ever recording _POSITIONAL_KEY, so module_name read as absent and _has_toolchains_llvm_override() missed the override. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 1b14c7b3..0b818f73 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -275,6 +275,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 @@ -283,6 +289,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 @@ -677,6 +687,31 @@ def test_has_toolchains_llvm_override_fails_closed_on_parenthesized_positional_m ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_fails_closed_on_list_positional_module_name( + self, + ) -> None: + """A list expression passed as the first positional argument (e.g. + + ["toolchains_llvm"][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(["toolchains_llvm"][0], strip_prefix = "x")' + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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({"toolchains_llvm": 1}.keys()[0], ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_fails_closed_on_concatenated_module_name( self, ) -> None: From ed28e607bf9e14fd29a55fec9b99834ec64d8e41 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 19:31:56 +0100 Subject: [PATCH 17/25] Skip comments, not just whitespace, between "=" and a keyword's RHS. Mirrors the same fix on #424: 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_toolchains_llvm_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_msan_test.py | 67 +++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 0b818f73..63f55a53 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -142,18 +142,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: @@ -163,6 +158,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 ",)" @@ -306,7 +316,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] = ( @@ -530,6 +540,22 @@ def test_toolchains_llvm_bazel_dep_version_ignores_commented_out_version( )""" self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_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 = "toolchains_llvm", + version = + # pinned past a fix + "1.11.2", +)""" + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_ignores_fake_assignment_in_string( self, ) -> None: @@ -819,6 +845,23 @@ def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 a fix + "apple_support", + strip_prefix = "x", +)""" + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_commented_module_name( self, ) -> None: From 29c5d11559ddf1c69e7f62341bcb304aef499b6d Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 21:14:23 +0100 Subject: [PATCH 18/25] Recognize a parenthesized-callable-indirection call form. Mirrors the same fix on #424: 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 = \"toolchains_llvm\", ...)", 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_toolchains_llvm_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_msan_test.py | 58 ++++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 63f55a53..43f3cbdf 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -190,6 +190,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. @@ -244,6 +270,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 @@ -623,6 +653,34 @@ def test_has_toolchains_llvm_override_detects_positional_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'strip_prefix = "toolchains_llvm-abc123")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_triple_quoted_module_name( self, ) -> None: From 3dc70b222a2ae165a222d0fab2f14b50e28cc4f3 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 21:18:20 +0100 Subject: [PATCH 19/25] Decode backslash-newline line continuations instead of keeping both. _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_msan_test.py | 43 +++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 43f3cbdf..450fe42c 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -64,19 +64,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": @@ -734,6 +742,31 @@ def test_has_toolchains_llvm_override_decodes_big_unicode_escape_in_module_name( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_\\\n' + 'llvm", strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "toolchains_\\\r\n' + 'llvm", strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_fails_closed_on_keyword_variable_module_name( self, ) -> None: From dedcc23e34e0f6c9747fe26bf96456b9ceabee77 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Mon, 12 Oct 2026 22:02:51 +0100 Subject: [PATCH 20/25] Handle comments in grouped-call detection; recognize raw strings. Two fixes: 1. _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 = \"toolchains_llvm\", ...)", 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(). 2. Mirrored from #424: module_name = r"rules_cc" (a raw string literal, valid Starlark) was read as a bare identifier "r" followed by a separate quoted string, falling through to the bare-identifier "unresolved" path, so the guard incorrectly returned True (failed closed) 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; every other backslash, including one before a newline, is kept completely literal) and wired _skip_any_string() into the keyword-RHS and first-positional checks. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 145 ++++++++++++++++++++++++++++++++--- 1 file changed, 134 insertions(+), 11 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 450fe42c..811a1677 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -150,6 +150,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"toolchains_llvm" 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. @@ -207,18 +258,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 @@ -296,15 +349,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 @@ -355,8 +408,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) @@ -689,6 +745,21 @@ def test_has_toolchains_llvm_override_rejects_longer_identifier_in_parens( ) self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_llvm", strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_triple_quoted_module_name( self, ) -> None: @@ -936,6 +1007,58 @@ def test_has_toolchains_llvm_override_ignores_other_modules(self) -> None: """ self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_raw_string_for_other_module( + self, + ) -> None: + """r"apple_support" is a valid Starlark raw string literal; it must + + be recognized as the complete, resolved literal "apple_support" + 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"apple_support", strip_prefix = "x")' + self.assertFalse(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_detects_raw_string_module_name( + self, + ) -> None: + """A raw string literal is still a real override when it does + + name toolchains_llvm. + """ + sample = ( + 'archive_override(module_name = r"toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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"toolchains_llvm", strip_prefix = "x")' + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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"toolchains_\\"llvm" (an + embedded quote) is a different string from "toolchains_llvm" and + must not be mistaken for a toolchains_llvm override. + """ + sample = ( + 'archive_override(module_name = r"toolchains_\\"llvm", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_skips_comment_between_equals_and_value( self, ) -> None: From 993d99c669ec5357a3ef6db480e979d1f2d8b6c6 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Tue, 13 Oct 2026 12:30:49 +0100 Subject: [PATCH 21/25] Handle triple-quote delimiters in raw string literals. Mirrors the same fix on #424: _skip_raw_string() only handled a single-character delimiter, so module_name = r"""apple_support""" 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. Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 64 +++++++++++++++++++++++++++++++++--- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 811a1677..054f90ef 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -165,13 +165,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\"\"\"toolchains_llvm\"\"\" 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) @@ -594,6 +610,17 @@ def test_toolchains_llvm_bazel_dep_version_tolerates_reordered_args( sample = 'bazel_dep(version = "1.11.2", name = "toolchains_llvm")' self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_accepts_raw_triple_quoted_name( + self, + ) -> None: + """name = r\"\"\"toolchains_llvm\"\"\" 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"""toolchains_llvm""", version = "1.11.2")' + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_accepts_positional_name( self, ) -> None: @@ -1043,6 +1070,35 @@ def test_has_toolchains_llvm_override_detects_raw_string_positional_module_name( sample = 'archive_override(r"toolchains_llvm", strip_prefix = "x")' self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_raw_triple_quoted_for_other_module( + self, + ) -> None: + """r\"\"\"apple_support\"\"\" is a valid Starlark raw triple-quoted + + literal; it must resolve to the complete literal "apple_support" + 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"""apple_support""", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_detects_raw_triple_quoted_module_name( + self, + ) -> None: + """A raw triple-quoted literal is still a real override when it + + does name toolchains_llvm. + """ + sample = ( + 'archive_override(module_name = r"""toolchains_llvm""", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_decodes_escaped_quote_in_raw_string( self, ) -> None: From 8b90637b12135f638e1deb6fb9991e98b76f5ee3 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Thu, 15 Oct 2026 09:33:41 +0100 Subject: [PATCH 22/25] Unwrap nested grouped-callable parens; mirror two fixes from #424. Three fixes: 1. (#423's own flagged finding) _match_parenthesized_call() only handled exactly one layer of "(" name ")(" grouping, so "((archive_override))(module_name = \"toolchains_llvm\", ...)" -- valid Starlark, since grouping parens nest arbitrarily -- didn't match and let the override bypass the guard. Rewrote it to count an arbitrary number of leading "(" and require the same number of matching ")", rather than special-casing exactly one layer. 2. (mirrored from #424) _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. 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. 3. (mirrored from #424) bazel_dep allows both name and version positionally (bazel_dep("toolchains_llvm", "1.11.2")), but the scanner only ever recorded the first positional argument, so a positional version made _toolchains_llvm_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_msan_test.py | 278 +++++++++++++++++++++++++++-------- 1 file changed, 214 insertions(+), 64 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 054f90ef..6482cdee 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -31,6 +31,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 @@ -159,17 +177,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\"\"\"toolchains_llvm\"\"\" 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: @@ -177,9 +197,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) @@ -189,9 +218,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) @@ -268,28 +306,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 @@ -317,13 +363,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]] = [] @@ -366,10 +414,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 @@ -377,13 +426,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 @@ -394,10 +443,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 @@ -407,9 +455,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 @@ -446,13 +494,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 @@ -461,10 +506,24 @@ def _iter_call_kwargs(call_names: tuple[str, ...], text: str) -> list[dict[str, def _toolchains_llvm_bazel_dep_version(module_bazel: str) -> str: + """Return the pinned toolchains_llvm bazel_dep version. + + bazel_dep's first two parameters, name and version, are both + allowed positionally (bazel_dep("toolchains_llvm", "1.11.2")), 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 == "toolchains_llvm" and "version" in kwargs: - return kwargs["version"] + name = kwargs.get("name") + if name is None: + name = _positional(kwargs, 0) + if name == "toolchains_llvm": + 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 = "toolchains_llvm" ...)') @@ -489,7 +548,9 @@ def _has_toolchains_llvm_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 == "toolchains_llvm" or module_name is _UNRESOLVED: return True return False @@ -631,6 +692,17 @@ def test_toolchains_llvm_bazel_dep_version_accepts_positional_name( sample = 'bazel_dep("toolchains_llvm", version = "1.11.2")' self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_accepts_positional_version( + self, + ) -> None: + """version is also allowed positionally, as bazel_dep's second + + argument: bazel_dep("toolchains_llvm", "1.11.2") is valid + Starlark, not just the keyword form. + """ + sample = 'bazel_dep("toolchains_llvm", "1.11.2")' + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_survives_an_earlier_paren( self, ) -> None: @@ -787,6 +859,53 @@ def test_has_toolchains_llvm_override_detects_parenthesized_call_with_comment( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_detects_triply_parenthesized_call( + self, + ) -> None: + """Same as the double-layer case, but with three layers.""" + sample = ( + '(((archive_override)))(module_name = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_override_ignores_doubly_parenthesized_other_module( + self, + ) -> None: + sample = ( + '((archive_override))(module_name = "apple_support", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_triple_quoted_module_name( self, ) -> None: @@ -1115,6 +1234,37 @@ def test_has_toolchains_llvm_override_decodes_escaped_quote_in_raw_string( ) self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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"toolchains_llvm\\\\", ' + 'module_name = "rules_cc")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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"toolchains_llvm\\\\\\"x", ' + 'strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_skips_comment_between_equals_and_value( self, ) -> None: From e1dfdbf204ff43e4abbb88e3577c0cd6670989ce Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Thu, 15 Oct 2026 13:11:33 +0100 Subject: [PATCH 23/25] Skip backslash-newline continuations; mirror two fixes from #424. #423's own finding: _skip_ws() only recognized the bare whitespace characters " \t\r\n", not a backslash-newline line continuation. A valid Starlark call like `archive_override\` + newline + `(...)` was therefore never recognized -- the backslash after the call name wasn't whitespace, so the "(" lookup failed and _has_toolchains_llvm_override silently returned False even when module_name = "toolchains_llvm", letting a real override slip past the regression guard. Fixed by having _skip_ws() treat a backslash immediately followed by LF or CRLF as whitespace too, for both line ending forms. Mirrored from #424 (same parser): - The outer top-level scan recognized only ordinary string literals (checking for a leading quote character, then _skip_string()), so a raw string like `r"C:\U12345678\emsdk"` outside any recognized call had its "r" prefix treated as an unrelated bare identifier and its body fed to the ordinary escape decoder, which raises on sequences that are only valid because of the "r" prefix. Fixed by using the same _is_raw_string_start()/_skip_any_string() dispatch already used inside a call body. - 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 "#", so the keyword was misread as a bare positional identifier -- failing closed and incorrectly tripping the guard for an unrelated module's override, and raising AssertionError out of _toolchains_llvm_bazel_dep_version when the misread identifier was a real bazel_dep's "name". Fixed by using _skip_ws_and_comments() there instead, matching how comments are already skipped between "=" and the right-hand side. All three reproduced against throwaway scripts before the fix and added as permanent regression tests; full //python/... suite green (24/24). Co-Authored-By: Claude Sonnet 5 --- python/tests/ci_msan_test.py | 108 +++++++++++++++++++++++++++++++++-- 1 file changed, 103 insertions(+), 5 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 6482cdee..9344f173 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -50,8 +50,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 @@ -382,8 +400,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 @@ -469,7 +487,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 ( @@ -749,6 +767,24 @@ def test_toolchains_llvm_bazel_dep_version_skips_comment_after_equals( )""" self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_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 a fix + = "toolchains_llvm", + version = "1.11.2", +)""" + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + def test_toolchains_llvm_bazel_dep_version_ignores_fake_assignment_in_string( self, ) -> None: @@ -1265,6 +1301,25 @@ def test_has_toolchains_llvm_override_continues_raw_string_at_odd_backslash_run( ) self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "apple_support", strip_prefix = "x")' + ) + self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_skips_comment_between_equals_and_value( self, ) -> None: @@ -1282,6 +1337,25 @@ def test_has_toolchains_llvm_override_skips_comment_between_equals_and_value( )""" self.assertFalse(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_ignores_commented_module_name( self, ) -> None: @@ -1319,6 +1393,30 @@ def test_has_toolchains_llvm_override_tolerates_whitespace_before_paren( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'strip_prefix = "x")' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_local_path_override(self) -> None: sample = """ local_path_override( From 4dc017ed17f9286794367ba407615ce20706bf71 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Fri, 16 Oct 2026 10:15:41 +0100 Subject: [PATCH 24/25] Mirror #424's comment-before-"(" and nested-override-call fixes. Both reported by Copilot review on #424 (same parser): - 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 = "toolchains_llvm", ...))`, 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. Both 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_msan_test.py | 313 ++++++++++++++++++++++------------- 1 file changed, 194 insertions(+), 119 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 9344f173..8c83bb2f 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -357,8 +357,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 @@ -376,10 +530,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 — @@ -403,123 +557,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 @@ -1417,6 +1459,39 @@ def test_has_toolchains_llvm_override_detects_call_after_crlf_continuation( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_llvm", path = "../toolchains_llvm"))' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "apple_support", ' + 'patches = single_version_override(module_name = "toolchains_llvm", ' + 'version = "1.11.2") or [])' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_local_path_override(self) -> None: sample = """ local_path_override( From 1452404c134b5cd27a34f55e9928d80d6d7ddcd8 Mon Sep 17 00:00:00 2001 From: Martin Nygren Date: Fri, 16 Oct 2026 14:16:57 +0100 Subject: [PATCH 25/25] Mirror #424: exclude spaced attribute calls; classify arguments once. Same shared parser, same two defects Copilot reported on #424: - "ext. bazel_dep(...)" / "ext. archive_override(...)" (or a continuation or comment after the ".") was read as a real directive, since only the character immediately before the name was checked. Both scans now consume the attribute name after any "." (_skip_attribute); a "." inside a comment is never seen. - Tokens inside a keyword's right-hand side could be recorded as positional arguments, so `repo_name = "toolchains_llvm_" + "9.9.9"` supplied a fake version to a versionless bazel_dep. The body scan now tracks top-level "," boundaries and classifies each argument once, by its first token. Co-Authored-By: Claude Opus 5.5 --- python/tests/ci_msan_test.py | 206 ++++++++++++++++++++++++----------- 1 file changed, 142 insertions(+), 64 deletions(-) diff --git a/python/tests/ci_msan_test.py b/python/tests/ci_msan_test.py index 8c83bb2f..bda51b39 100644 --- a/python/tests/ci_msan_test.py +++ b/python/tests/ci_msan_test.py @@ -357,6 +357,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]: @@ -387,10 +403,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 = "toolchains_llvm_" + "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. @@ -403,15 +427,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, @@ -419,62 +453,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 "\"'" @@ -493,15 +476,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 @@ -557,6 +567,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 @@ -873,6 +886,45 @@ def test_toolchains_llvm_bazel_dep_version_rejects_identifier_substring( with self.assertRaises(AssertionError): _toolchains_llvm_bazel_dep_version(sample) + def test_toolchains_llvm_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 = "toolchains_llvm", version = "9.9.9")\n' + 'bazel_dep(name = "toolchains_llvm", version = "1.9.0")' + ) + with self.subTest(qualifier=qualifier): + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.9.0") + + def test_toolchains_llvm_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 = "toolchains_llvm", version = "1.11.2")' + self.assertEqual(_toolchains_llvm_bazel_dep_version(sample), "1.11.2") + + def test_toolchains_llvm_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 = "toolchains_llvm", dev_dependency = False, ' + 'repo_name = "toolchains_llvm_" + "9.9.9")' + ) + with self.assertRaises(AssertionError): + _toolchains_llvm_bazel_dep_version(sample) + def test_has_toolchains_llvm_override_detects_same_line_args(self) -> None: sample = ( 'archive_override(module_name = "toolchains_llvm", ' @@ -1492,6 +1544,32 @@ def test_has_toolchains_llvm_override_detects_nested_override_call( ) self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_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 = "toolchains_llvm", ' + 'urls = ["x"])' + ) + with self.subTest(qualifier=qualifier): + self.assertFalse(_has_toolchains_llvm_override(sample)) + + def test_has_toolchains_llvm_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 = "toolchains_llvm", urls = ["x"])' + ) + self.assertTrue(_has_toolchains_llvm_override(sample)) + def test_has_toolchains_llvm_override_detects_local_path_override(self) -> None: sample = """ local_path_override(