paulk-asert commented on PR #3068:
URL: https://github.com/apache/groovy/pull/3068#issuecomment-6600664880
I agree something is needed here beyond documentation:
`MethodPointerExpression.getExpression()` hands out `THIS_EXPRESSION` for a
null receiver and `transformExpression` bakes it in, so after any
`ClassCodeExpressionTransformer` pass the compiler's own AST carries the shared
node, and groovy-contracts uses it in generated code too. On master 7 of the 16
cases in `Groovy12441` fail, plus one the test doesn't cover yet: `this::tag`
inside a closure fails with "Failed to find method 'tag()' for the type:
C$_run_closure1". This PR fixes that one as well, so it's worth adding.
I also tried the other obvious fix, making SC resolve an untyped `this` in a
closure to the enclosing class (`StaticTypesTypeChooser` +
`StaticInvocationWriter`, 14 lines). It passes all 16 cases but breaks 8
existing tests (Groovy7996, 9063, 9288, 9293, `testMapPropertyAccess10`,
Memoized record serialization), because classgen itself creates untyped `this`
nodes inside closure classes that really do mean the closure. So typing each
use before classgen, as done here, is the right layer.
On Jochen's point about the amount of visitor code: the same four hook
points work with a `ClassCodeExpressionTransformer` subclass whose only
override is `transform()` (fresh node for either sentinel; descend into
closures/lambdas as its Javadoc shows), plus a small `CodeVisitorSupport`
presence check so members without a shared node are never rebuilt. That's about
100 lines including the header, and gives the same result:
STC/SC/bugs/gls/transform/typing suites green (13,323 tests), all 18 cases
passing. It also removes the `getText()` special case:
`MethodPointerExpression.transformExpression` never routes a truly-null
receiver through `transform()`, so implicit pointers stay implicit on their
own. (As written, the `startsWith("::")` branch is dead code:
`MethodReferenceExpression.getText()` goes through `getExpression()`, which
already substitutes the shared node, so an implicit `::m` would currently be
rewritten to an explicit `this::m`.) The trade-off is that `transformExpre
ssion` rebuilds the composite nodes of the affected member rather than
mutating in place; metadata and positions are copied, as
`StaticCompilationTransformer` already relies on. With that, the rewriter's own
unit test could shrink to the `bugs` test.
Happy to share the patch if useful. Separately, a Javadoc on
`THIS_EXPRESSION`/`SUPER_EXPRESSION` pointing transform authors at
`varX("this")` would address Björn's request, either here or as a follow-up.
--
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]
