aglinxinyuan opened a new pull request, #8405:
URL: https://github.com/apache/texera/pull/8405
### What changes were proposed in this PR?
`PythonTemplateBuilderSpec`'s exhaustive bad-neighbour test passed
vacuously. Its private helper wrapped every snippet in a package clause before
handing it to a runtime `ToolBox`:
```scala
private def inPybuilderPkg(code: String): String =
s"""package org.apache.texera.amber.pybuilder {
|$code
|}""".stripMargin
private def assertToolboxDoesNotCompile(code: String): Unit = {
intercept[Throwable] { tb.compile(tb.parse(inPybuilderPkg(code))) }
()
}
```
A `ToolBox` cannot wrap a `PackageDef` into its synthetic `__wrapper`
method, so `tb.compile` dies during typer with a bare
`java.lang.AssertionError` for **any** input. Note the type: not
`ToolBoxError`, which is why `intercept[Throwable]` was needed for the helper
to pass.
I ran the helper against inputs that obviously must not be rejected. Every
one "does not compile":
| input fed to the helper | `tb.parse` | `tb.compile` |
|---|---|---|
| `""` (the empty string) | OK -> `PackageDef` | `java.lang.AssertionError:
assertion failed: method wrapper` |
| `object Trivial { val x = 1 }` | OK -> `PackageDef` | same |
| a correct `pyb"foo $ui bar"` (whitespace neighbours) | OK -> `PackageDef`
| same |
| `,` as a left neighbour — a *good* neighbour | OK -> `PackageDef` | same |
| syntactically broken garbage | throws `ToolBoxError` | (not reached) |
The measured message body is `assertion failed: / method wrapper / while
compiling: <no file> / during phase: typer / library version: version 2.13.18 /
compiler version: version 2.13.18 / reconstructed args: / last tree to typer:
...`. The compilation dies wrapping the tree, before macro expansion.
`tb.parse` on the same input succeeds, so the failure is entirely in
`tb.compile`. Compiling the identical body as a *block* instead of a package
clause reaches the macro and aborts properly, which is what the rewrite relies
on.
The helper was used at two call sites, inside the test that iterates the
`isBadNeighbor` subset of printable ASCII and asserts left- and right-adjacency
per character. All 130 of those assertions passed without the `pyb` macro ever
expanding.
**Before -> after:**
```
before: assertToolboxDoesNotCompile(<package block>) -> AssertionError,
always, for every input
after: macroError(<block>) -> ToolBoxError whose MESSAGE says which rule
fired
```
The rewrite uses the technique `BoundaryValidatorSpec` already documents in
its header: compile a *block*, not a package; the snippet always fails (the
expansion calls the `private[amber]` `fromInterpolated`, which the ToolBox's
`__wrapper` package cannot reach), but the macro fully expands first, so the
two outcomes are distinguishable by message — a `validateCompileTime` abort
carrying the specific boundary reason, versus a benign expansion whose only
failure is the `fromInterpolated` access error.
Concretely:
- Each of the 65 bad-neighbour characters is asserted twice — once as the
left neighbour, once as the right — to abort with the abort marker *and* its
own templated reason, e.g. ``must not be immediately adjacent to 'z' on the
left``. 130 real macro expansions.
- New test, the discriminating direction: the 29 safe-neighbour characters
must produce the *benign* outcome (57 cases; `#` on the left is excluded and
pinned separately). Without this, weakening the neighbour rule to "always
abort" would leave the first sweep green.
- New test: `#` as a left neighbour aborts for the comment rule, not the
neighbour rule.
- The character sets are spelled out rather than derived from
`PythonLexerUtils.isBadNeighbor`. Deriving them was a second, subtler vacuity:
shrinking the predicate would silently shrink the sweep's input instead of
failing it.
- Removed: `inPybuilderPkg`, `assertToolboxDoesNotCompile`, and
`scalaUnicodeEscape`, which had no other user. The escape helper was broken on
its own terms too: it emitted `\\u0041` — **two** backslashes — into the
generated source, nothing rewrites that inside the generated triple-quoted
literal, and the abort message proves what the macro actually saw: the left
neighbour was the digit `1`, never `A`. So the old sweep fed the same wrong
neighbour for all 65 characters. The new snippets embed the raw character
instead.
**What this PR does not do:** it touches no production code and adds no
tests to any other area. Measured from the JUnit XML, the file goes from 65 to
67 test cases: 1 test rewritten, 2 added, 0 removed and 0 renamed, so **64**
tests, their names and the file's structure are untouched. The file contains
**nine** `assertDoesNotCompile` tests using ScalaTest's own macro (not the
broken helper), **four** of them single-case adjacency tests; those nine are
unchanged, and three of the four adjacency ones demonstrably fire under
Mutation C below.
### Any related issues, documentation, discussions?
Closes #8401
### How was this PR tested?
Baseline on `1cbe857007`, `PyBuilder/test`: **184 tests, 5 suites, 0
failures**. After: **186 tests, 5 suites, 0 failures**. Comparing test-case
identities from `common/pybuilder/target/test-reports/TEST-*.xml` rather than
counts: 0 removed, 0 renamed, and the 2 additions are exactly the new tests
named above. Every suite reports `failures="0"` on both sides.
`PyBuilder/scalafmtCheck`, `PyBuilder/Test/scalafmtCheck` and
`PyBuilder/scalafixAll --check` all pass.
Non-vacuity, both directions, measured:
| check | result |
|---|---|
| bad-neighbour sweep | 130 of 130 cases abort with the expected reason
*and* the expected character |
| safe-neighbour sweep | 57 of 57 cases benign (no abort marker,
`fromInterpolated` present) |
| `#` left neighbour | aborts with the comment reason, not a neighbour
reason |
| real ToolBox compilations | 187 (130 + 57), each one a genuine macro
expansion |
Both sweeps carry loop-ran guards (`assert(checked == 130)` /
`assert(checked == 57)`) and set-size guards (`assert(size == 65)` /
`assert(size == 29)`), and report every mismatching case rather than failing
fast, so a regression names the characters.
Mutation A — `PythonLexerUtils.isBadNeighbor` changed to `... ||
(isIdentChar(c) && c != 'z')`:
```
rewritten test: RED - 1 test failed, "2 of 130 adjacency cases did not abort
with the neighbour reason":
left [z] (U+007A): ... method fromInterpolated ... cannot be accessed ...
right [z] (U+007A): ... method fromInterpolated ... cannot be accessed ...
old test, same mutation: GREEN - PyBuilder/test = 184 succeeded, 0 failed, 5
suites
```
That contrast is the clearest evidence here: under a mutation that removes
one character from the bad-neighbour set, the old test and every other suite in
the module stayed green; the rewritten sweep names the exact character, both
sides, and quotes the benign message as the reason. `PythonLexerUtilsSpec` does
not catch it either — it only *samples* the predicate (`'`, `"`, `a`, `Z`, `0`,
`_`, plus two negatives) and never tests `z`. The rewritten sweep catches it
only because the character set is no longer derived from the predicate.
Mutation B — the right-neighbour abort in
`BoundaryValidator.validateCompileTime` disabled (`if (false &&
isBadNeighbor(rightNeighbor))`):
```
rewritten test: RED - "65 of 130 adjacency cases" (exactly the right-side
arm; the 65 left-side cases still pass)
```
Mutation C — *both* compile-time neighbour arms disabled:
```
rewritten test: RED - "130 of 130 adjacency cases"
also RED: 3 pre-existing single-case tests -
"UI glued to identifier on the left does not compile"
"UI glued to identifier on the right does not compile"
"UI glued to a quote on the right does not compile"
total: 4 failed, 63 succeeded
```
The fourth adjacency test ("PyString (EncodableString) glued to identifier
on the left") stays green under Mutation C — it is pinned through a different
path, not the compile-time neighbour arms.
All three mutations were reverted by copying back pre-mutation file copies,
never `git checkout`/`git restore`. `git diff 1cbe857007 -- '*/src/main/*'` is
empty on the committed branch.
**Corrections after review.** Three claims in an earlier draft of this
description were wrong and are fixed above; recording them rather than editing
them away:
| earlier claim | measured |
|---|---|
| "the other 63 tests ... untouched" | 64 (65 -> 67 cases, 1 rewritten, 2
added, 0 removed) |
| "Two pre-existing single-case boundary tests" | 9 `assertDoesNotCompile`
tests, 4 of them adjacency; "two" was the count that happened to fire under
Mutation B, not a property of the file |
| the escape helper "fed `A` ... as six literal characters" | it emitted
`\\u0041`, two backslashes; the macro's neighbour was the digit `1` |
A fourth: an earlier draft put the sweep runtime at "~4.2 s". Wall-clock for
ToolBox work is not stable enough to quote — three runs on the same machine
measured the two sweeps at 4.02 s, 4.90 s and 5.70 s — so the figure is dropped
in favour of the compilation count, which is exact.
One thing worth recording, because it looks like a bug and is not: `pyb"pre
${ui}\" post"` — a `"` written as a Scala escape — is *not* rejected.
`StringContext.parts` for a custom interpolator are raw, so both the validator
and `fromInterpolated` see `\` as the neighbour, and `fromInterpolated` is
documented as taking raw parts and does not call `processEscapes`. The
validator and the renderer agree, so there is nothing to fix. The new snippets
sidestep it by using triple-quoted Scala literals, where the character is
verbatim.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
