paulk-asert commented on PR #2996: URL: https://github.com/apache/groovy/pull/2996#issuecomment-6402791787
Thanks Daniel, a thorough pass. All addressed in the latest commit except the two scope items noted below. 1. **Console render.** Fixed. `AstNodeToScriptAdapter.visitShortTernaryExpression` now visits the true expression (the operand for both forms) and prints ` ?? ` or ` ?: ` according to `isNullCoalescing()`, so `a ?? b` round-trips as `a ?? b` rather than `a != null ?: b`. `AstNodeToScriptAdapterTest.testNullCoalescingOperator` locks it beside the Elvis case. 2. **Other consumers.** I went through every in-tree `visitShortTernaryExpression` override and `ElvisOperatorExpression` construction. Apart from the console adapter, none interprets truth semantics: `SecureASTCustomizer`, the macro matchers and `ScriptToTreeNodeAdapter` authorise, match or label by class, so they're right for both forms. Two additions all the same: `ElvisOperatorExpression.getText()` now gives `a ?: b` / `a ?? b` instead of the inherited `(a != null) ? a : b`, which is what error messages show; and `GeneralUtils.nullCoalesceX(base, otherwise)` sits beside `elvisX` so transform authors don't have to remember the three-arg constructor. I'd keep the single node class: codegen, the STC compound-assignment rewrite and `transformExpression` all share one path, and `instanceof ElvisOperatorExpression` plus the flag describes it honestly. 3. **STC coverage.** `NullCoalescingOperatorTest` now parameterises `@TypeChecked` as well as `@CompileStatic` (29 cases). `STCAssignmentTest` has four `??=` cases mirroring the `?=` ones from GROOVY-10419/10628: setter through `with`, a getter/setter pair, kept falsy `Integer`/`int`/`String` plus the assigned-when-null path, and the type mismatch. They run under `@TypeChecked` and again under static compilation via `AssignmentsStaticCompileTest`, and the setter case exercises the `ensureValidSetter` line Codecov flagged. `TypesTest` asserts `isAssignment` and `ofType` for `NULL_COALESCE_EQUAL`. 4. **`a??[0]`.** Agreed it needs calling out. I'll note it in the 7.0 release notes and on GROOVY-12442 rather than here. 5. **Property LHS once-evaluation.** You're right about the contract: `obj.prop ??= value` evaluates the receiver twice and the getter and setter once each, in all three modes. That's inherited from `?=` (verified on 6.0.0), so I've filed it separately as GROOVY-12515 covering both operators; the fix is the GROOVY-12099 temporary-slot treatment applied to `PropertyExpression`. In this PR, `testAssignToPropertyCallsAccessorsOnce` asserts the parts that are right (getter once, setter once) in all three modes. It also documents that, as for `?=`, the setter runs whether or not the fallback is used. 6. **Shared base node.** The class javadoc now says the boolean and true expressions share the base and that codegen evaluates it once. Worth noting the double visit isn't new: Elvis has always wrapped the same base in its `BooleanExpression`, so `CodeVisitorSupport` visited it twice before this change too. What's new is that the condition is a real `COMPARE_NOT_EQUAL` binary, hence the note. 7. **Mixed parser AST.** `ParserPositiveSyntaxTest` now has the trees for `a ?: b ?? c` and `a ?? b ?: c`, with `nullCoalescing` on the right node. Nits: `StaticTypeCheckingSupport` javadoc lists `NULL_COALESCE_EQUAL`, the `ensureValidSetter` comment covers both operators, and the `ElvisOperatorExpression` javadoc introduces `??` up front with its 7.0.0 since. -- 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]
