pharo-vm icon indicating copy to clipboard operation
pharo-vm copied to clipboard

Block argument inlining may cause undesired side effects

Open guillep opened this issue 3 years ago • 1 comments

See testBlockValueAsArgumentWithNonLeafArgumentAndMultipleUse

Here 3 foo: 4 is executed once and used twice as i

([ :i | 1 foo: i. i = 4 ] value: (3 foo: 4)) ifTrue: [ a := b ]

=>

Yet here it is evaluated twice.

if ((foo(1, foo(3, 4)), (foo(3, 4)) == 4)) {
	a = b;
}

If foo has a side effect, this may be dangerous.

guillep avatar Jun 13 '22 15:06 guillep

The current method TStatementListNode>>bindVariablesIn: and related globally replace variable nodes with copies of the provided nodes. Therefore, this cause duplication if a parameter of a block is used more than once.

A related and more general issue is that also the order of execution is not respected, causing possible issue if there are side effects (like modification of global state like the heap).

eg [ :a | x foo. a ] value: y bar will be transpiled to foo(x); bar(y) whereas in pharo, y bar should be evaluated before x foo (I assume).

A possible solution could be to store automatically the arguments value: in temporary variables. The issue is that I do not think there is a facility in slang to store a node in a made-up temporary variable. Especially since T AST variables seem to need C types and unfortunately this information is unknown on most T nodes.

Another solution could be to complain if a block parameter who is binded to a complex T node (a send node for instance). (And is used more than once if we do not care about the general evaluation order issue). Such an error will force the user to create a temporary variable themself in their pharo (slang) code, with the correct type pragma when needed,

privat avatar Oct 05 '22 09:10 privat