Paul King created GROOVY-12515:
----------------------------------
Summary: Elvis-assignment on a property evaluates the receiver
twice (a.b.c ?= d, a.b.c ??= d)
Key: GROOVY-12515
URL: https://issues.apache.org/jira/browse/GROOVY-12515
Project: Groovy
Issue Type: Bug
Components: bytecode
Affects Versions: 6.0.0
Reporter: Paul King
Description
A compound Elvis-assignment whose left-hand side is a property with a
non-trivial receiver evaluates that receiver twice: once for the read in the
Elvis test and once more for the store. The property getter and setter each run
once. The same happens dynamically, under @TypeChecked and under @CompileStatic.
class Bean {
def prop
}
class Holder {
Bean bean = new Bean()
int receiverEvals = 0
Bean getBean() { receiverEvals++; bean }
}
def h = new Holder()
h.bean.prop ?= 'x'
assert h.receiverEvals == 1 // fails: 2
Expected: the receiver (h.bean) is evaluated once, as the left-hand side of an
assignment is in Java and as a[i] ?= b has evaluated its receiver and index
since GROOVY-12099. A receiver that is not referentially transparent (a factory
call, a cursor, a side-effecting getter) otherwise reads from one object and
writes to another.
Actual: BinaryExpressionHelper.evaluateElvisEqual rewrites lhs ?= rhs to lhs =
(lhs ?: rhs) and hands it to evaluateEqual, which evaluates the property's
object expression for the store after the Elvis expression has already
evaluated it for the read. GROOVY-12099 added a temporary-slot path for
non-safe subscripts only; property targets still take the generic path.
??= (GROOVY-12442) is built on the same rewrite, so it inherits the behaviour.
The fix should cover both operators.
Fix sketch
Extend the GROOVY-12099 treatment in evaluateElvisEqual to a PropertyExpression
left-hand side: store the object expression in a temporary (storeInTemporary),
build a PropertyExpression over the resulting VariableSlotLoader, and use it
for both the Elvis read and the store. Safe (?.) and spread-safe property
targets keep their existing behaviour, as safe subscripts do. An implicit-this
receiver needs no temporary.
Tests
A bugs/Groovy<NNNNN> test in the shape of Groovy12099: receiver evaluated once
when the value is kept and when the fallback is used, non-idempotent receiver,
left-to-right order, for ?= and ??=, dynamically and under @CompileStatic.
NullCoalescingOperatorTest.testAssignToPropertyCallsAccessorsOnce already
asserts getter-once and setter-once for ??=.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
