aglinxinyuan opened a new pull request, #7873:
URL: https://github.com/apache/texera/pull/7873
### What changes were proposed in this PR?
`EncodableInspector.safeAccessed` hopped from an accessor symbol to
`accessed` unconditionally. A trait member has no backing field of its own, so
its accessor's `accessed` is `NoSymbol` — and that is exactly where scalac
leaves the marker for a trait `val`/`var`, since `@EncodableStringAnnotation`
targets `METHOD` as well as `FIELD`. The hop threw the annotation away, and the
failure direction is the dangerous one:
```
Before: trait T { @EncodableStringAnnotation val s: String }
pyb"print(${t.s})" -> PyLiteralStringRenderer -> emitted raw,
no diagnostic
After: -> EncodableStringRenderer -> decode(...) at
run time
```
Nothing warned about it at either end.
`BoundaryValidator.validateCompileTime` runs only for arguments the inspector
classified Encodable (`PythonTemplateBuilder.scala:358`), so a trait member
spliced into an unsafe position skipped the compile-time boundary check as well.
The hop is now conditional — fall back to the accessor when there is no
field to hop to:
```scala
case accessor: TermSymbol if accessor.isAccessor =>
val field = accessor.accessed
if (field == null || field == NoSymbol) accessor else field
case _ => sym
```
Measured one shape per row, with a throwaway probe macro that dumped the
symbol and type state at expansion time (`@Enc` abbreviates
`@EncodableStringAnnotation`):
| Declaration | `accessed` | Marker sits on | Before | After |
| --- | --- | --- | --- | --- |
| `trait T { @Enc val s: String }` | `NoSymbol` | the accessor | raw |
**encoded** |
| `trait T { @Enc val s: String = "x" }` | `NoSymbol` | the accessor | raw |
**encoded** |
| `trait T { @Enc var s: String = "x" }` | `NoSymbol` | the accessor | raw |
**encoded** |
| `class C { @Enc val s: String = "x" }` | the field | the field | encoded |
unchanged |
| `case class H(@Enc ui: String)` | the field | the ctor param | raw |
unchanged |
| `abstract class C { @Enc val s: String }` | `NoSymbol` | — | raw | raw |
Two rows deliberately do not move. The case-class one is the documented
meta-annotation rule: without a meta-annotation the marker stays on the
constructor parameter, and `@(EncodableStringAnnotation @field)` is the shape
that reaches the field. The abstract-class one is out of reach from here —
scalac keeps the marker on neither the accessor (its `annotations` is empty
once its info is forced) nor any field, so the macro never sees it. Type
position works there, `val s: String @EncodableStringAnnotation`, which is what
the `EncodableString` alias expands to anyway; the spec asserts that working
shape rather than cementing the hole with a negative test.
`safeAccessed`'s second case, `case methodAccessor: MethodSymbol if
methodAccessor.isAccessor`, goes at the same time. `MethodSymbol` is a subtype
of `TermSymbol` in scala-reflect, so anything that reached it had already
matched the first case: it could never fire, and it was not a trait path in
disguise.
### Any related issues, documentation, discussions?
Stacked on #7864, which added the probe macro these tests use and
deliberately left this path unpinned so that fixing it would not have to fight
a test that had cemented it. Until that merges, the diff shown against `main`
includes its spec commit — only the second commit belongs to this PR.
#7864 also recorded `methodReturnHasAnn` as unpinnable without a production
seam. That turned out to be wrong for a reason worth writing down; it is pinned
here, and the stale comment is corrected.
### How was this PR tested?
Five tests added to `EncodableInspectorSpec`, all driven through #7864's
probe macro so they read the classifier's answers directly instead of inferring
them from compile-error text:
| Test | Pins |
| --- | --- |
| `a trait's marked val is Encodable even though its accessor has no backing
field` | the defect itself, over three fixtures: abstract `val`, concrete
`val`, `var` |
| `a marked trait val is lowered to an EncodableStringRenderer` | what the
classification is *for* — `wrapArg` must reach priority 2, not the raw-literal
default |
| `an unmarked trait val stays a plain literal` | the fallback reads the
accessor's own annotations, and reads them *selectively*: a second fixture puts
a foreign `@(ZzAnnG @getter)` annotation there |
| `an abstract class's abstract val needs the marker in type position` | the
working shape for the row the fallback cannot reach |
| `a def whose inline-annotated result type is stripped at the call site is
Encodable` | `methodReturnHasAnn` |
Written before the fix. Mutation testing on the touched code — 4 mutants, 4
killed:
| Mutant | Killed by |
| --- | --- |
| revert `safeAccessed` to the unconditional hop (the defect) | the two
trait-member tests |
| never hop to the field, `case accessor ... => accessor` | the pre-existing
`@(EncodableStringAnnotation @field)` test |
| symbol scan `annotations.exists(annIsEncodableString)` ->
`annotations.nonEmpty` | `an unmarked trait val stays a plain literal` |
| `case m: MethodSymbol => typeHasEncodableString(...)` -> `case _:
MethodSymbol => false` | `an abstract class's abstract val ...` and `a def
whose inline-annotated result type ...` |
The last row is the gap #7864 recorded. The trait-member fixture is not what
closes it — a trait `val` is detected through the symbol path, not the
signature path. A different measurement is: `EncodableString` is a type
*alias*, so on `def ui: EncodableString` the call-site tree's own type
dealiases back to the annotated type and the last disjunct answers true on its
own, which is why the arm looked untestable. An *inline* `def ui: String
@EncodableStringAnnotation` behaves differently — scalac strips the annotation
off the call-site tree, leaving `m.typeSignature.finalResultType` the only
place the marker survives.
```bash
sbt "PyBuilder/clean" "PyBuilder/test"
```
`Tests: succeeded 189, failed 0` across 5 suites, 184 before.
```bash
sbt "PyBuilder/scalafmtCheckAll" "PyBuilder/scalafixAll --check"
```
Clean.
### 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]
