This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-8405-bda3400261d38e674086f3b44cfcae169ccc55e5 in repository https://gitbox.apache.org/repos/asf/texera.git
commit ec3a9dd3ca9c361cbd7f339d8bc78669024e84e8 Author: Xinyuan Lin <[email protected]> AuthorDate: Wed Sep 9 06:04:25 2026 +0000 test(pybuilder): make the bad-neighbour boundary sweep actually compile its snippets (#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) --------- Signed-off-by: Xinyuan Lin <[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> --- .../pybuilder/PythonTemplateBuilderSpec.scala | 215 ++++++++++++++++----- 1 file changed, 165 insertions(+), 50 deletions(-) diff --git a/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala b/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala index 727f5cf14c..71810f9316 100644 --- a/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala +++ b/common/pybuilder/src/test/scala/org/apache/texera/amber/pybuilder/PythonTemplateBuilderSpec.scala @@ -31,7 +31,7 @@ import java.nio.charset.StandardCharsets import java.util.Base64 import scala.annotation.meta.field import scala.reflect.runtime.currentMirror -import scala.tools.reflect.ToolBox +import scala.tools.reflect.{ToolBox, ToolBoxError} class PythonTemplateBuilderSpec extends AnyFunSuite { @@ -41,26 +41,100 @@ class PythonTemplateBuilderSpec extends AnyFunSuite { private def decodeExpr(text: String): String = PythonTemplateBuilder.wrapWithPythonDecoderExpr(base64Of(text)) - // Toolbox helpers: used to assert runtime exceptions without checking error strings. + // ------------------------------------------------------------------------ + // ToolBox harness for the exhaustive boundary sweeps further down. + // + // A `pyb` snippet can only be *compiled* (never `eval`-run) through a ToolBox: the expansion + // calls the `private[amber]` `fromInterpolated`, which the ToolBox's synthetic `__wrapper` + // package cannot access. Compilation is enough, because the macro fully expands (running + // `BoundaryValidator.validateCompileTime`) before that access error surfaces. So every snippet + // fails, and the two outcomes are told apart by the captured `ToolBoxError` *message*: + // + // - an abort, whose message carries the specific boundary reason, versus + // - a benign expansion whose only failure is the `fromInterpolated` access error. + // + // The snippet must be a *block*, never a `package` clause: the ToolBox cannot wrap a PackageDef + // into its synthetic wrapper method at all and dies with a bare + // `java.lang.AssertionError: assertion failed: method wrapper` during typer, for any input + // whatsoever. This mirrors `BoundaryValidatorSpec`; see its header for the same reasoning. + // ------------------------------------------------------------------------ private lazy val tb: ToolBox[scala.reflect.runtime.universe.type] = currentMirror.mkToolBox() - private def inPybuilderPkg(code: String): String = - s"""package org.apache.texera.amber.pybuilder { - | - |$code - | - |}""".stripMargin - - private def assertToolboxDoesNotCompile(code: String): Unit = { - intercept[Throwable] { - // compile only (don’t run); macro expansion happens during compilation - tb.compile(tb.parse(inPybuilderPkg(code))) - } - () - } - // Unicode escapes in *generated* Scala source: must be written as "\\uXXXX" in this test file. - private def scalaUnicodeEscape(ch: Char): String = - f"\\\\u${ch.toInt}%04X" + private val toolboxHeader = + """import org.apache.texera.amber.pybuilder.PythonTemplateBuilder._ + |import org.apache.texera.amber.pybuilder.PyStringTypes._""".stripMargin + + /** Marker present in every `BoundaryValidator` compile-time abort message. */ + private val boundaryMarker = "@EncodableStringAnnotation argument #" + + /** The benign outcome: the validator did not abort, only the private access failed. */ + private val benignMarker = "fromInterpolated" + + /** Compile a self-contained `pyb` snippet as a *block*; it always fails, so return the message. */ + private def macroError(body: String): String = + intercept[ToolBoxError] { + tb.compile(tb.parse(s"{\n$toolboxHeader\n$body\n}")) + }.getMessage + + private def oneLine(message: String): String = + message.linesIterator.map(_.trim).filter(_.nonEmpty).mkString(" | ").take(240) + + /** + * `"""` spelled through a normal literal. + * + * The generated snippets below wrap the Python template in a triple-quoted Scala string so the + * neighbour character can be embedded verbatim. `\"` is *not* an escape inside a triple-quoted + * string, so the delimiter cannot be written inline in the `s"""..."""` builders below. + */ + private val tripleQuote = "\"\"\"" + + /** Literal `${ui}` for the generated snippet - a plain literal, not an interpolation. */ + private val uiSplice = "${ui}" + + /** Wrap one Python template line as a `pyb` snippet with a direct `EncodableString` argument. */ + private def pybSnippet(pythonTemplate: String): String = + s"""val ui: EncodableString = "x" + |pyb$tripleQuote$pythonTemplate$tripleQuote""".stripMargin + + /** + * The neighbour character, doubled, as it must appear in Scala source. + * + * Doubling matters for the two quote characters: `validateCompileTime` runs the unclosed-quote + * rule *before* the neighbour rules, so a lone `'` or `"` in the prefix would abort with the + * "inside a quoted Python string literal" reason instead of the neighbour reason. A pair leaves + * the prefix quote-balanced, so the neighbour rule is what fires. For every other character the + * doubling is a no-op. `$` is the one character needing escaping, since it would otherwise open + * an interpolation in the generated snippet. + */ + private def neighborPad(ch: Char): String = { + val asSource = if (ch == '$') "$$" else ch.toString + asSource + asSource + } + + /** `pyb"""pre<ch><ch>${ui} post"""` - the character is the arg's immediate left neighbour. */ + private def leftAdjacentSnippet(ch: Char): String = + pybSnippet(s"pre${neighborPad(ch)}$uiSplice post") + + /** `pyb"""pre ${ui}<ch><ch> post"""` - the character is the arg's immediate right neighbour. */ + private def rightAdjacentSnippet(ch: Char): String = + pybSnippet(s"pre $uiSplice${neighborPad(ch)} post") + + /** + * The `isBadNeighbor` set over printable ASCII, spelled out rather than derived. + * + * Deriving the sweep's input from `PythonLexerUtils.isBadNeighbor` would make it + * self-referential: shrinking the predicate would silently shrink the sweep instead of failing + * it. `PythonLexerUtilsSpec` only *samples* the predicate (`'`, `"`, `a`, `Z`, `0`, `_`, plus + * two negatives), so it too stays green when a single character is dropped from the set. The + * two sweeps below pin the whole set from the outside - shrinking it reds the bad sweep, + * growing it reds the safe sweep. + */ + private val badNeighborChars: Seq[Char] = + "\"'0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz".toSeq + + /** Printable ASCII (33..126, so no whitespace) that is *not* in [[badNeighborChars]]. */ + private val safeNeighborChars: Seq[Char] = + (33 to 126).map(_.toChar).filterNot(badNeighborChars.contains) // ======================================================================== // Rendering basics (plain vs encoded) @@ -435,38 +509,79 @@ class PythonTemplateBuilderSpec extends AnyFunSuite { } test("all isBadNeighbor characters reject direct UI adjacency at compile time (left + right)") { - val candidates = (33 to 126).map(_.toChar) // printable ASCII, avoids whitespace - val badChars = candidates.filter(PythonLexerUtils.isBadNeighbor) - - // This is intentionally exhaustive over the implementation-defined "bad neighbor" set. - // We assert only compile success/failure, not the specific error message. - badChars.zipWithIndex.foreach { - case (ch, i) => - val esc = scalaUnicodeEscape(ch) - - val leftAdj = - s""" - |import org.apache.texera.amber.pybuilder.PythonTemplateBuilder._ - |import org.apache.texera.amber.pybuilder.PyStringTypes._ - |object UiBadLeft_$i { - | val ui: EncodableString = "x" - | val b = pyb\"\"\"pre$esc${'$'}{ui}post\"\"\" - |} - |""".stripMargin - - val rightAdj = - s""" - |import org.apache.texera.amber.pybuilder.PythonTemplateBuilder._ - |import org.apache.texera.amber.pybuilder.PyStringTypes._ - |object UiBadRight_$i { - | val ui: EncodableString = "x" - | val b = pyb\"\"\"pre${'$'}{ui}$esc post\"\"\" - |} - |""".stripMargin - - assertToolboxDoesNotCompile(leftAdj) - assertToolboxDoesNotCompile(rightAdj) + assert(badNeighborChars.size == 65, "the sweep's character set changed unexpectedly") + + val mismatches = List.newBuilder[String] + var checked = 0 + + badNeighborChars.foreach { ch => + Seq( + ("left", leftAdjacentSnippet(ch), s"must not be immediately adjacent to '$ch' on the left"), + ( + "right", + rightAdjacentSnippet(ch), + s"must not be immediately adjacent to '$ch' on the right" + ) + ).foreach { + case (side, snippet, expectedReason) => + checked += 1 + val message = macroError(snippet) + if (!message.contains(boundaryMarker) || !message.contains(expectedReason)) { + mismatches += s"$side [$ch] (U+${"%04X".format(ch.toInt)}): ${oneLine(message)}" + } + } + } + + assert(checked == 130, "the sweep did not run every case") + val failures = mismatches.result() + assert( + failures.isEmpty, + s"${failures.size} of $checked adjacency cases did not abort with the neighbor reason:\n" + + failures.mkString("\n") + ) + } + + test("no safe-neighbour character aborts direct UI adjacency (the sweep discriminates)") { + // The counterpart of the sweep above: without this, weakening the neighbour rule to "always + // abort" would leave that sweep green. `#` on the left is excluded because the comment rule + // legitimately fires first there; the test below pins that case separately. + assert(safeNeighborChars.size == 29, "the sweep's character set changed unexpectedly") + + val aborts = List.newBuilder[String] + var checked = 0 + + safeNeighborChars.foreach { ch => + val sides = + if (ch == '#') Seq("right" -> rightAdjacentSnippet(ch)) + else Seq("left" -> leftAdjacentSnippet(ch), "right" -> rightAdjacentSnippet(ch)) + + sides.foreach { + case (side, snippet) => + checked += 1 + val message = macroError(snippet) + if (message.contains(boundaryMarker) || !message.contains(benignMarker)) { + aborts += s"$side [$ch] (U+${"%04X".format(ch.toInt)}): ${oneLine(message)}" + } + } } + + assert(checked == 57, "the sweep did not run every case") + val failures = aborts.result() + assert( + failures.isEmpty, + s"${failures.size} of $checked safe-neighbour cases were not benign:\n" + + failures.mkString("\n") + ) + } + + test("'#' as a left neighbour aborts for the comment rule, not the neighbour rule") { + val message = macroError(leftAdjacentSnippet('#')) + assert(message.contains(boundaryMarker), oneLine(message)) + assert( + message.contains("appears after a '#' comment marker on the same line."), + oneLine(message) + ) + assert(!message.contains("must not be immediately adjacent"), oneLine(message)) } // ========================================================================
