[
https://issues.apache.org/jira/browse/GROOVY-12515?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12515:
-------------------------------
Description:
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.
{code:groovy}
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
{code}
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/Groovy12515 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 ??=.
was:
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 ??=.
> 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
> Priority: Major
>
> 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.
> {code:groovy}
> 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
> {code}
> 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/Groovy12515 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)
